-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Find/Replace] 'Find Next' (Ctrl+K) command broken with the overlay #2285
Comments
I was hoping to be the first to report this but, sadly, no, you got in before me 😢 Unless there's another key combination that can do the job, I'm going to have to revert to not using the overlay, even though it looks nice. |
Thank you for reporting this. I agree on the expectation that those actions should behave according to the last search operation (no matter whether it was in the dialog or overlay). Still, this might be a bit tricker to resolve (properly). Having a static value for the search pattern worked with the existing dialog, as there was only one for the whole workbench. Now that there is one overlay per editor, we need to ensure that the context is properly considered. Maybe it is easy to fix the behavior but I am not sure whether it may also require design change to not have it work "by accident" (as the last search operation always implicitly belongs to the editor/view of interest). |
Either way, a lot of these "quick search" command reimplement logic which was abstracted into the FindReplaceLogic class. (Ctrl+I, Ctrl+J, Ctrl+K, ...). It would be a clear improvement in code quality to make these commands re-use the logic which is now very well tested and controlled |
The search string for CTRL+K is not updated from the find/replace overlay. In your screencast, you set the search string via double-clicking the exact same search string as entered into the overlay, so it will be searched via CTRL+K. If you don't do that (just switch focus from the overlay to the editor), it will not behave as expected: |
@HeikoKlare when refactoring the search history, I changed the name of the dialog-settings entry to "searchhistory" to fit the name to match our notion of "searching" in the overlay. The FindNextAction still uses "findhistory" as search key. This is the problem and we should make sure all three have a similar name |
Great, can you provide a PR with a fix? |
@HeikoKlare I am currently working on this |
The history of the FindNextAction is now synchronized with the search history of either the FindReplaceOverlay or of the FindReplaceDialog (whichever is active currently). fixes eclipse-platform#2285
When the find/replace overlay is enabled, the 'Find Next (Ctrl+K)' (
org.eclipse.ui.edit.findNext
) command has no effect.When the find/replace overlay is disabled, it works again.
When then re-enabling the overlay, Ctrl+K still works on the 'old' search pattern that was entered in the legacy dialog.
The findNext action
org.eclipse.ui.texteditor.FindNextAction
works onorg.eclipse.ui.texteditor.FindNextAction.getDialogSettings()
which usesIDialogSettings settings = PlatformUI.getDialogSettingsProvider(FrameworkUtil.getBundle(FindNextAction.class)).getDialogSettings();
Apparently this dialog setting is not written anymore when using the new overlay.
I guess the overlay should also write to those dialog settings to keep the search histories in sync.
The text was updated successfully, but these errors were encountered: