Find/replace overlay: move action management to command class#4185
Draft
HeikoKlare wants to merge 1 commit into
Draft
Find/replace overlay: move action management to command class#4185HeikoKlare wants to merge 1 commit into
HeikoKlare wants to merge 1 commit into
Conversation
The management of actions for the find/replace overlay, the registration of key shortcuts and the initialization of according tooltips with the shortcuts is currently part of the FindReplaceOverlay itself. In addition, the tooltip texts are static throughout the overlay lifecycle and show conflicting shortcuts, such as Enter for both the "search forward" and "replace" button, even though only either of them is registered, depending on which of the input fields has focus. With this change, the responsibility for the action management with the activation and deactivation of their key bindings is moved to the FindReplaceOverlayCommandSupport. This prepares for the provision of the actions as Eclipse handlers in order to allow rebinding shortcuts. Contributes to eclipse-platform#2015
There was a problem hiding this comment.
Pull request overview
Moves find/replace overlay action/shortcut management out of FindReplaceOverlay into FindReplaceOverlayCommandSupport, enabling focus-dependent activation/deactivation and tooltip shortcut hints that reflect the currently active bindings (preparatory step towards proper Eclipse command handlers and rebindable shortcuts).
Changes:
- Centralize overlay action registration and per-control shortcut registration in
FindReplaceOverlayCommandSupport, and activate/deactivate bindings based on whether the search or replace text field has focus. - Add a shortcut-hint listener mechanism to
FindReplaceOverlayActionand drive tooltip updates from activation/deactivation events. - Update
AccessibleToolItemto maintain a base tooltip and dynamically append/remove shortcut hints.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayCommandSupport.java | Adds action lists + control registration and focus-based activation/deactivation logic. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayAction.java | Introduces shortcut-hint listeners and activation/deactivation notifications. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java | Wires overlay UI to command support for registering actions/shortcuts and focus events. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItem.java | Updates tooltip handling to support dynamic shortcut hint display. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+61
| String tooltipWithHin = baseToolTipText; | ||
| if (hint != null && !hint.isEmpty()) { | ||
| tooltipWithHin += " (" + hint + ")"; //$NON-NLS-1$//$NON-NLS-2$ | ||
| } | ||
| toolItem.setToolTipText(tooltipWithHin); |
Comment on lines
+79
to
+81
| void deactivateKeyBinding() { | ||
| shortcutHintListeners.forEach(listener -> listener.accept(null)); // $NON-NLS-1$ | ||
| } |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The management of actions for the find/replace overlay, the registration of key shortcuts and the initialization of according tooltips with the shortcuts is currently part of the FindReplaceOverlay itself. In addition, the tooltip texts are static throughout the overlay lifecycle and show conflicting shortcuts, such as Enter for both the "search forward" and "replace" button, even though only either of them is registered, depending on which of the input fields has focus.
With this change, the responsibility for the action management with the activation and deactivation of their key bindings is moved to the FindReplaceOverlayCommandSupport. This prepares for the provision of the actions as Eclipse handlers in order to allow rebinding shortcuts.
Contributes to:
Example
Before
Shortcuts for replace actions are shown even if search input field has focus and shortcuts are bound to search actions.
After
Shortcuts for replace actions are not shown when search input field has focus and shortcuts are bound to search actions.
Towards proper Eclipse key binding handlers
Having a dedicated class own all command infrastructure is a prerequisite for incrementally replacing the current reflection-based workaround with proper Eclipse command-framework handlers for the overlay actions. This change prepares that future migration. It follows: