[replace-across-files] Unify the Search Results Panel FindInFiles/ReplaceAll #7727
Conversation
| @@ -191,524 +149,6 @@ define(function (require, exports, module) { | |||
| } | |||
| } | |||
|
|
|||
| /** Remove listeners that were tracking potential search result changes */ | |||
| function _removeListeners() { | |||
There was a problem hiding this comment.
This is now in FindInFiles.prototype.removeListeners()
There was a problem hiding this comment.
You mean FindInFilesResults.prototype, right?
|
Thanks, I'll take a look soon. I'll update replace-across-files to current master (it actually doesn't have anything other than master in it yet). |
|
FYI, I've gotten to the point where I'm about ready to look at hooking this up. The nj/replace-on-disk branch has the replace functionality starting to work (without the checkbox panel); you can do Find > Replace in Files, enter a query and a replace string, and click Replace All..., and it will replace everything in the project. (I'm not yet sure what should happen if you hit Enter in the Find box without entering a replace string.) So the next step is to bring up the checkbox panel so the user can choose what to replace. I'll take a look at what you've got here tomorrow and see if I can wrap my head around it enough to just hook it up to what I've already got. |
|
One side note: it looks like we never added unit tests for exercising the Replace All UI with the checkboxes (there's one test that checks all the checkboxes, I think, but nothing that tests individual checkbox checking), or tests for paging in Find in Files. Since we're doing a major refactoring here, it seems like it would be prudent to add some of these. |
| * @param {(string|RegExp)} replaceWhat Query that will be passed into CodeMirror Cursor to search for results. | ||
| * @param {string} replaceWith String that should be used to replace chosen results. | ||
| */ | ||
| ReplaceAllResults.prototype.showReplaceAll = function (editor, replaceWhat, replaceWith) { |
There was a problem hiding this comment.
The name of this method would probably clearer as something like findAllAndShowResults.
There was a problem hiding this comment.
Actually, I think it should be split into 2 methods: one that does the search and one that shows the results. This will be needed for the 2 different search algorithms.
There was a problem hiding this comment.
Yup. It seems like we could use this same code for Find All and Select (which I wrote independently - didn't think to look at this code). That code currently uses the existing FindReplace infrastructure (_getNextMatch, etc.) instead of just using a separate search cursor directly like this code does, which seems simpler; I don't think there's any particular reason it needed to use the higher-level FindReplace stuff.
There was a problem hiding this comment.
Now that I see it more in detail, we should move the searching bit of the code into it's own method which can be called from this function, but would make it easier to replace it.
|
General comment: this UI is about the level of complexity where I start actually wanting an MVC framework, or React, or something :) I wonder if it would be worth considering that. /cc @dangoor |
|
OK, I've eyeballed most of the code, and I think it's looking good overall. I haven't accounted for all the differences yet, but I did comb through most of the old and new Find in Files/SearchResults code. To my eye, it basically looks like:
Does that sound about right? So, it seems like in order to review it for correctness, the primary thing is for me to understand the changes to the Replace All functionality to understand how it works with the new display logic. I'll probably do that in more detail tomorrow just to see if I spot anything before starting to figure out how to integrate it with my code. |
|
I don't think that this is too complex. I find the Extension Manager to have more layers of complexity. That is basically what I did. I made Replace All use the display of Find in Files. And while I was doing it I ended moving the results to the new class, so that I wouldn't need to pass it as a parameter all the time. Since now the results are no longer a semi-global variable I had to make more functions from Find in Files methods of the results class. Moving forwards, we need to incorporate more of the functions used for searching into SearchResults, so that we can eventually move the update code from FindInFilesResults into SearchResults. Right now the update functions use several of the global find functions from Find in Files. This is required so that the Replace in Files Results auto update. In Replace All you might only need to change the |
|
@TomMalbran The Extension Manager is definitely the level of complexity that it would benefit from React. I've got a partly written blog post about my experiment with React. Maybe I can get that up today. |
|
Yeah, this isn't terribly complex (and not as complex as Extension Manager). It's just that manually synchronizing UI state with model state via explicit event handlers, incremental update, etc. feels so 2010 :) Not specific to this feature - it's just that Brackets UI is growing and the more complex aspects of it could benefit from something less ad hoc. |
|
Regarding search functions, I think ideally I'd actually like to factor the search functionality itself out of the results UI, to have a better model/view separation. The search functionality used by a particular client could be passed in as a callback of some sort. This is a fairly minor point, though, since the search functionality isn't that complicated. |
|
Anyway in which is easy for the Search Result to search is fine. it could easily be a new class which is passed to the Search Results. Right now, Since I knew that you were changing the search part, I didn't do any changes there. |
|
Yup. I can look at all that when I start trying to merge it into my stuff. |
|
FYI, I added a couple of unit tests in my current branch (before merging your stuff) for replace all checkboxes and Find in Files result paging - you can see them in the last couple commits in https://github.com/adobe/brackets/commits/nj/replace-on-disk. That'll at least give us a basic sanity check once I merge your refactoring into my branch. We can add more later (e.g. one for the combination of checkboxes and paging). |
|
OK, I'm going to go ahead and manually merge your branch into my nj/replace-on-disk branch (not the nj/replace-across-files branch that this is currently targeted at - I want to keep the nj/unify-find-ui PR separate for now so Peter F can review it, and merge that into the nj/replace-across-files branch first). |
|
@TomMalbran - This is now merged into nj/replace-on-disk. It's not hooked up to the Replace in Files flow yet, but the existing functionality is still working and passes the new unit tests I wrote for checkboxes and paging. Thanks so much for doing all this work - it's a huge help. I'll let you know when it's hooked up to Replace in Files so you can check it out. Closing this PR. Let's keep the branch around for reference for a little while until I'm sure I didn't make any errors while merging :) But the merge wasn't that bad so I think it's ok. |
|
@TomMalbran I just integrated it with Replace in Files and everything is working, with checkboxes and paging! You can try it out in the nj/replace-on-disk branch - note that you should click the "Replace All..." button instead of hitting Enter - I need to work out some UI kinks there. Hooking up the result list was very straightforward. I think my next step is going to refactor the search-related functionality back out, as I don't think it belongs in the result list class - that should be a pure UI class. I think if I do that, I can mostly or totally get rid of the subclasses of SearchResults and unify everything. Also, if I do it right, I can get rid of some of the large diffs from master (by moving some of the search functionality in FindInFiles back to its original locations), which will make the overall PR easier to review. |
|
@njx That is awesome!! I'll check it later. If we can easily hookup the Searching with the Results UI, that would be great. The problem is that one searches and the other one uses the results. As I said before, maybe the Results UI could have a reference to the search class to get the results. The old tests used the results object for testing. Now that those live inside a class, maybe we could expose the instances of the search classes for testing? |
|
@TomMalbran I took a slightly different tack for the refactoring than what you might have been thinking...hopefully it makes sense. I decided the first thing was to move the query/results state into a separate model object, rather than having it be a mix of module variables in FindInFiles and stuff in the view class. This makes it so that we can push changes into the model object from the various search functions, and have the view update the same way no matter what kind of search was done initially. With this, I was able to get rid of the ReplaceAllResults class entirely, unifying the specific summary logic into the main SearchResultsView and capturing other differences in the model. A small bit of it moved back into the main FindReplace code. I was also able to get rid of some special-case code in FindInFilesResults. Now pretty much all that's left in there is the updating code, and it doesn't refer to the view at all - it just updates the model class. I'm thinking, though, that there isn't much benefit to making the updating code work for single-file Replace All. The reason is that even if we were able to incrementally update the results list, it would be difficult to know how to preserve the checkbox state anyway - because we wouldn't easily be able to tell how the set of results after the change maps to the set of results before the change. (For small edits like typing, we could probably do it, but if there were a large edit, like a paste, or if the file changed on disk, we wouldn't be able to reliably map them.) So I think keeping the existing behavior for Replace All in a single file - just closing the panel when the document changes - is probably correct for now. Given that, I'm tempted to basically get rid of the FindInFilesResults class (since there's nothing view-specific in it anymore), and just move the updating code back into the main FindInFiles module. That will reduce the number of diffs from master. Also, it just occurred to me that I could basically get rid of the special-case Replace All code entirely and basically just have it call Replace in Files on the current document. That would get rid of some duplicate code paths. I should look into that next. |
|
Hm, wow. If I do make Replace All just delegate to Replace in Files, that actually gets rid of the separate panel entirely, so we really end up with just one panel instance after all :) But it was still worth all the refactoring in order to have the various UI and model bits more cleanly separated out, instead of tangled up in global module state in FindInFiles, I think. |
That is kind of similar to what I was thinking. But since I was thinking of having the Replace in Files autoupdate, that would require to have the Search in memory and Search in files functions in the model, and probably some of the filters/scope functions.
Makes sense since the main differences came from the search and listeners that could be moved to the Search results.
Maybe it might make sense to move the update code to the model (if the search was there too). Maybe not for the brackets but for extensions (#7445). If more of the code is in this Classes it will be easy to expose them for others to use them.
I am not sure about this, maybe there is a benefit to have it for Replace in Files (which will be used in Replace All since it should use Replace in Files). But keeping the states of the checkboxes will be in an issue in the case of deleting and readding a result. But in the case of pasting or adding new code it might not be an issue. Currently the Update code, updates on the lines that changed. it basically searches again over those lines, since it was easier than searching over the actual changed ranges and update the columns when required. So there might be cases where we might remove and readd the same result, and maybe that will be a problem.
I'll give it a better look tomorrow. I noticed that #7809 has several extra commits, so it is showing a lot more changed files. Do you think that you could update
That is the reason why I closed this PR. After doing all this, I figure that Replace in Files could just use the Find in Files panel and Replace All can rely on Replace in Files. So I was thinking that by just adding a few extra conditions to the current panel we could just add the checkboxes and the the replace button and not have to do any mayor change. |
FWIW, the use case in #7445 should be handled already, since I exposed a public function from FindInFiles that lets you kick off a search. (From what @zaggino did, it looks like he didn't really need a pure search-only function - he actually wanted it to bring up the results panel UI, etc.) But yeah, if we really only have one "search all" engine now that works for both the single- and multi-file cases, we could probably move all that into the model class, and have FindInFiles really just be a controller that hooks the Find Bar up to the engine and the results panel. Then we could break up the unit tests a little bit if we wanted to (or at least write more fine-grained unit tests just on the model itself). I'll take a look at that after I get Replace All hooked up to Find in Files. (It's working for me in the actual app, but I can't seem to fix the Replace All unit tests to work with this for some reason.)
I see...if we actually know exactly which lines changed, then the checkboxes outside the affected portion should be able to retain their state. It would only be the ones inside the edited area that would be affected. I guess I was thinking that it's not really super likely that someone would intentionally make document changes before committing a replace operation, so the extra complexity might not be worthwhile for now.
Yes, that's because
Ah, I didn't get that :) It didn't really become obvious to me that they could be the same until after I did Replace in Files and made it so that it would replace in-memory for files that were already open. So yeah, I think that's where we ended up...but I think it was fine that we did some refactoring along the way. |
|
(Actually, rather than moving the engine code into the model, it's probably more modular to just have it be yet another separate object that just updates the model.) |
|
(In fact, that's basically what the remaining code in FindInFilesResults is doing at this point. I could just rename it to "FindInFilesEngine", move a few more bits from the FindInFiles module into it, and then move it into a separate module. Then the existing FindInFiles will really just be about managing the find bar and filtering UI.) |
|
BTW, there's one more bit of complexity with doing incremental update of the results in the Replace in Files case...In order to do replacements in files on disk, I store offsets from the beginning of the text in addition to the {line, ch} positions that are already being calculated, because I don't want the hit of recalculating offsets from the {line, ch} positions when doing the replacements. (I guess you could argue that we're already taking that hit when doing the initial search, but I didn't want to add to it.) If we wanted to incrementally update the result list for changes within a file, then we'd need to update those offsets too, which would be more complexity. (Probably at that point we'd just always do everything in terms of {line, ch} positions and take the hit of recalculating the offsets, I guess.) Anyway, as a first cut, we discussed just making it so if anything in a given file changed while the Replace in Files results panel is open, we'd just redo the search in the whole file (with a warning to the user that they'd be losing their checkbox state for that file), as opposed to trying to incrementally update within the file. |
|
One thought about the offsets. Since the results are per line, could you About the autoupdate, it already only searches over the changed/added lines |
|
Yup, I could split the file, but wanted to avoid the overhead of splitting/joining the whole file when there might only be a few replacements in it. I don't really know how big the performance difference is, though (might do a jsperf to see). For now, I think I want to avoid dealing with within-file incremental update for replace results, since I'm fairly paranoid about replace offsets or checkbox state getting out of sync - the potential impact of a bug in that code is higher with replace than with simple searches, since it could silently corrupt documents, and conversely the benefit doesn't seem that high since I don't think people editing documents in the middle of a replace operation is going to be common. For now, I'll just warn the user and invalidate all the results for a file if it changes, and re-do the search. After the main implementation is stable, I can go back and look at adding unit tests for incremental update of replace results if time permits. |
@njx I just merged with master, and fixed several issues. I wasn't able to move all of the update code to the global search panel, since in this PR i just merged the display of the results and not the actual searches. We will need to move some of the search code into the results classes before we are able to move the auto update functions to the Search Results class.
Your branch is a bit behind master, so there are a few extra commits here. I guess that might not be much of a problem since this PR just mainly touches FindInFiles, and FindReplace, plus a few tempaltes and adds a new SearchResults file. But I just wanted to create a new PR.