Find/Replace overlay: unify action responsibility in FindReplaceOverlayAction#4169
Open
HeikoKlare wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the Find/Replace overlay’s toolbar buttons to use explicit FindReplaceOverlayAction objects as the single source of truth for both the action operation and its shortcut bindings, enabling shortcut registration directly on the input controls without traversing widget hierarchies.
Changes:
- Introduces explicit
FindReplaceOverlayActioninstances for overlay operations and groups them into typed action lists (commonActions,searchActions,replaceActions). - Registers shortcut handlers on the search/replace input controls by iterating those action lists directly.
- Updates the accessible toolbar/item builder APIs to accept an action object (
withAction(...)) instead of separate operation/shortcut parameters.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/HistoryTextWrapper.java | Builds the history dropdown tool item using an explicit FindReplaceOverlayAction. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayAction.java | Exposes an unmodifiable view of shortcuts via getShortcuts(). |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java | Creates explicit actions, stores them in lists, and registers shortcuts on input controls via those lists. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItemBuilder.java | Switches builder configuration from operation/shortcuts to a single action (withAction). |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItem.java | Stores a FindReplaceOverlayAction and executes it from selection events. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolBar.java | Removes shortcut-registration delegation now that shortcut registration is handled at the overlay level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ayAction Each overlay button previously had its operation and keyboard shortcut hint managed separately through the builder (withOperation / withShortcuts). This meant the relationship between what a button does and what shortcut triggers it was spread across the call site. A FindReplaceOverlayAction is now created before building the tool item and passed as a single entity via withAction(), making the action the sole source of truth for both the operation and its shortcut bindings. Shortcut registration on input fields iterates the typed action lists (commonActions, searchActions, replaceActions) directly rather than delegating through the toolbar controls, and AccessibleToolBar no longer needs a registerActionShortcutsAtControl method. AccessibleToolItem's action field is initialized as null rather than a no-op dummy; setToolTipText falls back to the plain text for items that receive no action. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
HeikoKlare
force-pushed
the
findreplace-overlay-action-responsibility
branch
from
7ff9294 to
9775290
Compare
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.
Motivation
Each overlay button previously had its operation and its keyboard shortcut hints defined as two independent values in the builder, with no single object representing the concept of an "action". As a consequence, registering those shortcuts on additional controls (the search and replace input fields) required reaching into the toolbar widget hierarchy — the overlay asked each toolbar to register shortcuts at a given control, the toolbar delegated to each tool item, and the tool item held an internal action that was not otherwise accessible. This made it impossible to reason in one place about which actions are active on which controls.
This is part of the ongoing effort to separate UI setup from controller logic in the Find/Replace overlay, tracked in issue #1912.
Change
A
FindReplaceOverlayActionis now created explicitly before building the corresponding tool item and passed to the builder as a single entity — the sole source of truth for both the operation and its shortcut bindings. The overlay maintains typed action lists (commonActions,searchActions,replaceActions) and registers shortcuts on input fields by iterating them directly, without involving the widget hierarchy at all.Towards proper Eclipse key binding handlers
Having actions as explicit, named objects with well-defined shortcut lists is a prerequisite for replacing the current SWT-level keyboard listener approach with proper Eclipse command-framework handlers. Such handlers would let the platform's key binding infrastructure dispatch shortcuts in a context-aware way. This change prepares that future migration. It follows:
This change was created with the help of GitHub Copilot.