[replace-across-files] Unify Find Bars across FindReplace/FindInFiles#7705
Conversation
…e apparently unused bits.
…t into a new FindBar component. There are still a few places in FindInFiles where it's monkey-patching directly into the FindBar's ModalBar. Hoping to eliminate those in future.
There was a problem hiding this comment.
Instead of creating a jQuery object each time we need them, maybe is better to create all the jQuery selectors once on FindBar.open and save them as properties of FindBar, since these will not change on a single modal bar. Notice that we are creating the same objects several times in the code.
There was a problem hiding this comment.
Are these prefs guaranteed to always be initialized to strict boolean values? If they might be undefined or somesuch initially, need to wrap this in Boolean(...getViewState(...)) (because the jQuery toggleClass() API sucks :-/).
There was a problem hiding this comment.
@TomMalbran Personally I'm not too worried about the slight inefficiency of repeating the jQ query. We do that a lot in other parts of the code too -- if it's not on a critical perf path then it seems probably simpler to leave it as-is here.
There was a problem hiding this comment.
Fixed the toggleClass() params (using !!). Leaving the jQuery calls as is.
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
There was a problem hiding this comment.
Couldn't this just be:
this.$("#find-what, #replace-with, ...").prop("disabled", ...)
|
@njx Done reviewing. Sorry for the long list of comments! I feel like a good half of them or more are about the many added null checks, which collectively make me worried there's some timing complexity here that I'm missing... |
|
I'll move on to looking at #7809 in the meantime |
|
The null checks are probably just out of an abundance of caution. I don't remember any specific reason for adding them, but I'll take a closer look. (It's possible that some of them were already there in some other guise and I just preserved them.) |
|
Actually - you might be right that it was a concern over timing - that the find bar might be closed (and then nulled out) at some unexpected time. But in some of these cases you're right that that doesn't seem possible (e.g. event handlers). |
Add Replace in... to context menus
[replace-across-files] Replace in Files
|
Merging into the main replace-all branch. |
[replace-across-files] Unify Find Bars across FindReplace/FindInFiles
…oResults() and parseQuery() cleanup
This gets rid of a lot of the redundant find bar code across single-file and multi-file find. There are still a few bits in FindInFiles that directly monkeypatch into the FindBar's ModalBar (see the TODOs in FindInFiles), but I think we can clean that up later if we want.
The first two commits are fairly semantic, and it's probably worth looking at them individually (the big one is the second one) - I added some comments in the individual commits, but if you want I can move them to the main PR diffs.
Note that this PR is a PR into the
nj/replace-across-filesbranch, so this can be merged when it's reviewed (it'll just go into that long-running branch).