[replace-across-files] Replace in Files #7809
Conversation
Also fixed a bug and dealt with some nonobvious asynchrony in one of the existing tests.
…add confirmation dialog. Clean up some unnecessary runs() in tests.
Logically, this is a single operation, not a set of merged adjacent operations, and we wouldn't want a second replaceAll done immediately afterward to merge with it (though it would be difficult to get into that case).
* hitting Enter in Find field sets focus to Replace field * hitting Enter in Replace field starts the search
…ed by a Replace in Files operation
… in Replace in Files unit tests
…. Clone results so they don't get updated during the replace.
* Broke out query/results state into a model object that behaves similarly between Replace All and Find/Replace in Files * Got rid of separate ReplaceAllResults, unifying code into SearchResultsView
| return FindUtils.performReplacements(results, replaceText, options).always(function () { | ||
| // For UI integration testing only | ||
| exports._replaceDone = true; | ||
| }); |
There was a problem hiding this comment.
Is this wrapper really needed? It doesn't seem to add any value vs. people just calling performReplacements() directly
There was a problem hiding this comment.
It's an artifact of an intermediate step in the refactoring - originally performReplacements() was called from single-file Replace All as well as Replace in Files, but now that Replace All just delegates to Replace in Files, it doesn't need to be separate anymore. Let's do that cleanup in a later step after this lands so as not to mess up the diffs too much for now.
|
@njx Done reviewing everything but the unit tests. Will try to get to that tomorrow... I'm loving how much more cleanly everything is factored now! Would be cool to see FindInFiles become even more stateless & non-singletonized in the future, maybe turning into a generic TextSearchEngine class or some such... but that's for another day. Still haven't had time to actually test out the functionality at runtime btw -- also hoping to squeeze in some of that tomorrow... |
Add Replace in... to context menus
|
Merging into the main replace-all branch. |
[replace-across-files] Replace in Files
* Got rid of unnecessary partial in search-summary-paging.html * Refactored change notifications in Document to _notifyDocumentChange() method, and fix up similar logic in SpecRunnerUtils.createMockActiveDocument() * Moved makeDialogFileList from StringUtils to FileUtils * Renamed "file/item/index" to "fileIndex/itemIndex/matchIndex" for clarity in SearchResultsView * Return empty array from _getSearchMatches instead of null * Properly debounce document changes when updating search results view * Fix up file rename handling in search results view to use indexOf() instead of match() * Rename "doReplaceAll" event on FindBar to "replaceAll" * Return a promise from FindInFilesUI.searchAndShowResults() * Replace calls to _.filter() and _.map() with native JS calls * Lots of comment/JSDoc improvements
This is all the work for Replace in Files after the find bar unification (which is in #7705). There were probably a few changes to the find bar after that PR, but the work was fairly separable (and some of the code moved around after that), so it's probably worth reviewing that one first/separately anyway.
High-level points:
FindInFileswas refactored into multiple pieces:FindInFilesis now just the cross-file search and replace engine code, with no UI logic. (The replace code is actually primarily in FindUtils, because I thought I was going to need it in two places, but it turns out I don't because single-file Replace All went away - see below.) FindInFiles also takes care of listening for document/file change events and updating the SearchModel incrementally. (Note, however, that those updates only happen in Find mode. In Replace mode, we shut the panel immediately whenever a change happens to any document that was found by the initial search, to avoid complexity with updating the checkbox state, etc. We have a Trello card for more sophisticated update behavior during Replace.)FindBarhandles the modal bar UI for both single-file Find/Replace and Find/Replace In Files (that work was done in [replace-across-files] Unify Find Bars across FindReplace/FindInFiles #7705).SearchModelencapsulates the query and results state for Find In Files and Replace In Files.SearchResultsViewmanages the results panel for Find In Files and Replace In Files, including paging and (in replace mode) the checkboxes for choosing what to replace. It listens to SearchModel for updates to the results. This is mostly from work done by @TomMalbran to unify the old single-file Replace All panel with the Find in Files panel.FindInFilesUIis the controller module that sews all the above together and actually handles the menu commands, etc.Replace Allis no longer has its code for running the search/replace; it just delegates to FindInFilesUI, passing the current file as the scope. Also, we took the opportunity to rename it fromAll...toBatch...since we've heard people be confused by the original naming.FindInFilesUI.searchAndShowResults()with the query/scope/filter info (as requested in expose doSearch for extensions #7445). If you wanted, you could even do a completely headless search or replace by calling directly intoFindInFiles.doSearchInScope().I'll add comments throughout the diffs on what has changed.