Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Refactors the search results to be able to use paging in Replace All #7059

Closed
wants to merge 1 commit into from

Conversation

TomMalbran
Copy link
Contributor

I moved most of the code that handles the Search Results bottom panel display into a new SearchResults class, and I used it in FindInFiles and ReplaceAll. This made it possible to have pagination in ReplaceAll.

I included the files sorting here too, but using it in slightly different way.

@marcelgerber
Copy link
Contributor

Currently in Replace All, when you tick the Toggle All checkbox, all results on all pages get toggled.
It might be nice to make a way to just toggle the current pages' results.

And another cool thing would be to have an info text like 2999 of 3000 results will be replaced (counts the checkboxes).

@TomMalbran
Copy link
Contributor Author

@SAplayer Nice ideas, but no idea where to place all the stuff, the panel title would end up with too much text if we add all that.

@TomMalbran
Copy link
Contributor Author

Instead of doing this it might be a lot easier to implement Replace in Files as Find in Files search where in the results we include checkboxes to select what to replace and an input and button to do the replace. After that is done, Replace All, can be done as a Replace in a single File. We would still need to increase the number of results from a single file.

@JeffryBooher
Copy link
Contributor

@larz0 and @njx any thoughts?
I find it a little strange that you click "Replace All" then you have more buttons and checkboxes to click later.

@TomMalbran
Copy link
Contributor Author

I really like how it works now, since it lets you select what you want to replace and doesn't do it blindly. You can also see what will be replaced.

@larz0
Copy link
Member

larz0 commented Mar 4, 2014

I like how it works now too; I actually think it's a feature.

@JeffryBooher
Copy link
Contributor

I think one of the contributing factors to my unease is feature discoverability. I wasn't sure how to test your stuff and I thought I was just going to start blindly replacing all occurrences when I clicked the button.

@TomMalbran
Copy link
Contributor Author

Replace All works in the same was as is does in master. The only difference is that this can show over 300 results and using pagination.

Anyway, i don't think this is the right solution to add pagination to replace all. A better solution would be to just use find in files to do the search, and just add a replace button into that panel, when we want to also replace. That would also work with replace in files. Eventually, replace all can work as replace in files using a single file as the scope.

@JeffryBooher
Copy link
Contributor

@TomMalbran can you merge master into this branch? This one looks a little stale.

@TomMalbran
Copy link
Contributor Author

@JeffryBooher I think I am just going to close it. This got too complicated and there will be a better solution by implementing Replace in Files, using the Find in Files search (and just slightly changing the panel), and then implementing Replace All, using Replace in Files with one file as the scope.

I know how to do the UI for Replace in Files, but not sure how to handle the actual replace.

@TomMalbran TomMalbran closed this Mar 18, 2014
@njx
Copy link

njx commented Apr 30, 2014

@TomMalbran I finished the bulk of the Find Bar refactoring - you can see it in nj/unify-find-ui. I started trying to do a test-merge from tom/search-results into that branch, but it looks like this branch was from before the exclusions filters were added to FindInFiles. So it might be a little easier if we merge master into this branch first before trying to merge it into my branch.

If you have time to do that in the next couple days, that would be cool. I'm going to start thinking about the underlying replace-in-files functionality and maybe temporarily implementing it without the result list UI just to test it out. Once that seems to be working we could hook it up to the unified result list UI.

@njx
Copy link

njx commented Apr 30, 2014

(BTW, if you don't have time to merge master into this branch before I'm ready for it, I can do it myself.)

@TomMalbran
Copy link
Contributor Author

I should have time on Thursday and Friday, or tomorrow depending on work. Go ahead and merge it, if you need it before I get to work on it.

@njx
Copy link

njx commented Apr 30, 2014

Cool, I'll let you know either way.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants