From d4128ab48f69abf5a7a9952f2f214538febd54aa Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Sun, 5 Jul 2026 13:46:47 +0200 Subject: [PATCH] Find/Replace overlay: decouple toggle button state from action execution Previously, checkbox tool items in the Find/Replace overlay managed their own visual state by reading or flipping the SWT ToolItem selection inside the action lambda. This coupled the UI state update to the execution path of the action, making the action responsible for two unrelated concerns: toggling a search option in the model and keeping a widget in sync with that state. FindReplaceLogic now exposes an addSearchOptionChangedListener mechanism that notifies observers whenever a search option is activated or deactivated. AccessibleToolItemBuilder gains withSearchOption and withInvertedSearchOption methods that bind a ToolItem's selection state directly to a SearchOptions value: the initial state is read from the logic at build time, and subsequent changes are pushed to the item automatically via the observer. The action operations for checkbox buttons are reduced to pure model commands and no longer touch widget state at all. Tests for the new addSearchOptionChangedListener API are added to FindReplaceLogicTest, and a dedicated overlay test verifies the correct inversion of the searchInSelection button relative to the GLOBAL option through both transitions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../findandreplace/FindReplaceLogic.java | 21 +++++++ .../findandreplace/IFindReplaceLogic.java | 18 ++++++ .../overlay/AccessibleToolBar.java | 7 +-- .../overlay/AccessibleToolItem.java | 20 +----- .../overlay/AccessibleToolItemBuilder.java | 44 ++++++++++++- .../overlay/FindReplaceOverlay.java | 31 +++++---- .../findandreplace/FindReplaceLogicTest.java | 63 +++++++++++++++++++ .../findandreplace/overlay/OverlayAccess.java | 32 +++++----- 8 files changed, 182 insertions(+), 54 deletions(-) diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java index c2d33effce8..c81dd909b82 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java @@ -15,10 +15,15 @@ package org.eclipse.ui.internal.findandreplace; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; @@ -58,6 +63,20 @@ public class FindReplaceLogic implements IFindReplaceLogic { private String findString = ""; //$NON-NLS-1$ private String replaceString = ""; //$NON-NLS-1$ + private final Map>> searchOptionListeners = new HashMap<>(); + + @Override + public void addSearchOptionChangedListener(SearchOptions option, Consumer listener) { + Objects.requireNonNull(option); + Objects.requireNonNull(listener); + searchOptionListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener); + } + + private void notifySearchOptionChangedListeners(SearchOptions option, boolean newState) { + List> listeners = searchOptionListeners.getOrDefault(option, Collections.emptyList()); + listeners.forEach(l -> l.accept(newState)); + } + @Override public void setFindString(String findString) { this.findString = Objects.requireNonNull(findString); @@ -91,6 +110,7 @@ public void activate(SearchOptions searchOption) { default: break; } + notifySearchOptionChangedListeners(searchOption, true); } @Override @@ -106,6 +126,7 @@ public void deactivate(SearchOptions searchOption) { if (searchOption == SearchOptions.FORWARD && shouldInitIncrementalBaseLocation()) { resetIncrementalBaseLocation(); } + notifySearchOptionChangedListeners(searchOption, false); } @Override diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java index 1d7af642f4d..9ccd97d83f0 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java @@ -13,6 +13,8 @@ *******************************************************************************/ package org.eclipse.ui.internal.findandreplace; +import java.util.function.Consumer; + import org.eclipse.jface.text.IFindReplaceTarget; import org.eclipse.ui.internal.findandreplace.status.IFindReplaceStatus; @@ -58,6 +60,22 @@ public interface IFindReplaceLogic { */ public void deactivate(SearchOptions searchOption); + /** + * Registers a listener that is notified whenever the given search option is + * activated or deactivated. The listener is called with {@code true} when the + * option becomes active and {@code false} when it becomes inactive. + * + *

+ * Listeners are only notified when the state actually changes — repeated calls + * to {@link #activate} or {@link #deactivate} for an already-active/inactive + * option do not trigger a notification. + *

+ * + * @param option the search option to observe + * @param listener the listener to register + */ + public void addSearchOptionChangedListener(SearchOptions option, Consumer listener); + /** * @param searchOption option * @return whether the option is active diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolBar.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolBar.java index 5c3cc32fd02..8f56bc33533 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolBar.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolBar.java @@ -28,9 +28,7 @@ * This class wraps the ToolBar to make it possible to use tabulator-keys to * navigate between the buttons of a ToolBar. For this, we simulate a singular * ToolBar by putting each ToolItem into it's own ToolBar and composing them - * into a Composite. Since the "Enter" keypress could not previously trigger - * activation behavior, we listen for it manually and send according events if - * necessary. + * into a Composite. */ class AccessibleToolBar extends Composite { @@ -45,8 +43,7 @@ public AccessibleToolBar(Composite parent) { } /** - * Creates a ToolItem handled by this ToolBar and returns it. Will add a - * KeyListener which will handle presses of "Enter". + * Creates a ToolItem handled by this ToolBar and returns it. * * @param styleBits the StyleBits to apply to the created ToolItem * @return a newly created ToolItem diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItem.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItem.java index d2490d8c5c9..38d744db1e4 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItem.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItem.java @@ -33,16 +33,6 @@ class AccessibleToolItem { ToolBar toolbar = new ToolBar(parent, SWT.FLAT | SWT.HORIZONTAL); GridDataFactory.fillDefaults().grab(true, true).align(SWT.CENTER, SWT.CENTER).applyTo(toolbar); toolItem = new ToolItem(toolbar, styleBits); - addToolItemTraverseListener(toolbar); - } - - private void addToolItemTraverseListener(ToolBar parent) { - parent.addTraverseListener(e -> { - if (e.keyCode == SWT.CR || e.keyCode == SWT.KEYPAD_CR) { - action.execute(); - e.doit = false; - } - }); } ToolItem getToolItem() { @@ -62,15 +52,7 @@ void setToolTipText(String text) { } void setOperation(Runnable operation, List shortcuts) { - boolean isCheckbox = (toolItem.getStyle() & SWT.CHECK) != 0; - if (isCheckbox) { - action = new FindReplaceOverlayAction(() -> { - toolItem.setSelection(!toolItem.getSelection()); - operation.run(); - }); - } else { - action = new FindReplaceOverlayAction(operation); - } + action = new FindReplaceOverlayAction(operation); action.addShortcuts(shortcuts); setToolTipText(toolItem.getToolTipText()); toolItem.addSelectionListener(SelectionListener.widgetSelectedAdapter(__ -> operation.run())); diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItemBuilder.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItemBuilder.java index da7e784d51a..b378940c8fa 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItemBuilder.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItemBuilder.java @@ -23,6 +23,9 @@ import org.eclipse.jface.bindings.keys.KeyStroke; +import org.eclipse.ui.internal.findandreplace.IFindReplaceLogic; +import org.eclipse.ui.internal.findandreplace.SearchOptions; + /** * Builder for ToolItems for {@link AccessibleToolBar}. */ @@ -33,6 +36,9 @@ class AccessibleToolItemBuilder { private String toolTipText; private List shortcuts = Collections.emptyList(); private Runnable operation; + private SearchOptions searchOption; + private IFindReplaceLogic findReplaceLogic; + private boolean invertSearchOption; public AccessibleToolItemBuilder(AccessibleToolBar accessibleToolBar) { this.accessibleToolBar = Objects.requireNonNull(accessibleToolBar); @@ -63,6 +69,31 @@ public AccessibleToolItemBuilder withOperation(Runnable newOperation) { return this; } + /** + * Binds a {@link SearchOptions} value to this item. When built, the item's + * selection state is initialized from the logic's current state and kept in + * sync automatically whenever the option changes. + */ + public AccessibleToolItemBuilder withSearchOption(SearchOptions option, IFindReplaceLogic logic) { + this.searchOption = option; + this.findReplaceLogic = logic; + this.invertSearchOption = false; + return this; + } + + /** + * Like {@link #withSearchOption(SearchOptions, IFindReplaceLogic)} but inverts + * the mapping: the item is selected when the option is inactive. + * Useful for options like {@link SearchOptions#GLOBAL} where a "search in + * selection" button should be selected when searching globally is turned off. + */ + public AccessibleToolItemBuilder withInvertedSearchOption(SearchOptions option, IFindReplaceLogic logic) { + this.searchOption = option; + this.findReplaceLogic = logic; + this.invertSearchOption = true; + return this; + } + public ToolItem build() { AccessibleToolItem accessibleToolItem = accessibleToolBar.createToolItem(styleBits); if (image != null) { @@ -74,7 +105,16 @@ public ToolItem build() { if (operation != null) { accessibleToolItem.setOperation(operation, shortcuts); } - - return accessibleToolItem.getToolItem(); + ToolItem toolItem = accessibleToolItem.getToolItem(); + if (searchOption != null) { + boolean initial = findReplaceLogic.isActive(searchOption); + toolItem.setSelection(invertSearchOption ? !initial : initial); + findReplaceLogic.addSearchOptionChangedListener(searchOption, state -> { + if (!toolItem.isDisposed()) { + toolItem.setSelection(invertSearchOption ? !state : state); + } + }); + } + return toolItem; } } diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java index 9f15fc62022..fad56abd371 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java @@ -611,11 +611,12 @@ private void createAreaSearchButton() { .withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_SEARCH_IN_AREA)) .withToolTipText(FindReplaceMessages.FindReplaceOverlay_searchInSelectionButton_toolTip) .withOperation(() -> { - activateInFindReplacerIf(SearchOptions.GLOBAL, !searchInSelectionButton.getSelection()); + activateInFindReplacerIf(SearchOptions.GLOBAL, !findReplaceLogic.isActive(SearchOptions.GLOBAL)); updateIncrementalSearch(); }) - .withShortcuts(KeyboardShortcuts.OPTION_SEARCH_IN_SELECTION).build(); - searchInSelectionButton.setSelection(!findReplaceLogic.isActive(SearchOptions.GLOBAL)); + .withShortcuts(KeyboardShortcuts.OPTION_SEARCH_IN_SELECTION) + .withInvertedSearchOption(SearchOptions.GLOBAL, findReplaceLogic) + .build(); } private void createRegexSearchButton() { @@ -623,13 +624,15 @@ private void createRegexSearchButton() { .withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_FIND_REGEX)) .withToolTipText(FindReplaceMessages.FindReplaceOverlay_regexSearchButton_toolTip) .withOperation(() -> { - activateInFindReplacerIf(SearchOptions.REGEX, regexSearchButton.getSelection()); + activateInFindReplacerIf(SearchOptions.REGEX, !findReplaceLogic.isActive(SearchOptions.REGEX)); wholeWordSearchButton.setEnabled(findReplaceLogic.isAvailable(SearchOptions.WHOLE_WORD)); updateIncrementalSearch(); updateContentAssistAvailability(); decorate(); - }).withShortcuts(KeyboardShortcuts.OPTION_REGEX).build(); - regexSearchButton.setSelection(findReplaceLogic.isActive(SearchOptions.REGEX)); + }) + .withShortcuts(KeyboardShortcuts.OPTION_REGEX) + .withSearchOption(SearchOptions.REGEX, findReplaceLogic) + .build(); } private void createCaseSensitiveButton() { @@ -637,10 +640,12 @@ private void createCaseSensitiveButton() { .withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_CASE_SENSITIVE)) .withToolTipText(FindReplaceMessages.FindReplaceOverlay_caseSensitiveButton_toolTip) .withOperation(() -> { - activateInFindReplacerIf(SearchOptions.CASE_SENSITIVE, caseSensitiveSearchButton.getSelection()); + activateInFindReplacerIf(SearchOptions.CASE_SENSITIVE, !findReplaceLogic.isActive(SearchOptions.CASE_SENSITIVE)); updateIncrementalSearch(); - }).withShortcuts(KeyboardShortcuts.OPTION_CASE_SENSITIVE).build(); - caseSensitiveSearchButton.setSelection(findReplaceLogic.isActive(SearchOptions.CASE_SENSITIVE)); + }) + .withShortcuts(KeyboardShortcuts.OPTION_CASE_SENSITIVE) + .withSearchOption(SearchOptions.CASE_SENSITIVE, findReplaceLogic) + .build(); } private void createWholeWordsButton() { @@ -648,10 +653,12 @@ private void createWholeWordsButton() { .withImage(FindReplaceOverlayImages.get(FindReplaceOverlayImages.KEY_WHOLE_WORD)) .withToolTipText(FindReplaceMessages.FindReplaceOverlay_wholeWordsButton_toolTip) .withOperation(() -> { - activateInFindReplacerIf(SearchOptions.WHOLE_WORD, wholeWordSearchButton.getSelection()); + activateInFindReplacerIf(SearchOptions.WHOLE_WORD, !findReplaceLogic.isActive(SearchOptions.WHOLE_WORD)); updateIncrementalSearch(); - }).withShortcuts(KeyboardShortcuts.OPTION_WHOLE_WORD).build(); - wholeWordSearchButton.setSelection(findReplaceLogic.isActive(SearchOptions.WHOLE_WORD)); + }) + .withShortcuts(KeyboardShortcuts.OPTION_WHOLE_WORD) + .withSearchOption(SearchOptions.WHOLE_WORD, findReplaceLogic) + .build(); } private void createReplaceTools() { diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java index 0bd4c1ed9c3..94b1e805d25 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java @@ -31,6 +31,8 @@ import static org.mockito.Mockito.when; import static org.mockito.Mockito.withSettings; +import java.util.ArrayList; +import java.util.List; import java.util.function.Predicate; import org.junit.jupiter.api.AfterEach; @@ -968,6 +970,67 @@ public void testIncrementalSearchBackwardNoUpdateIfAlreadyOnWord() { assertThat(findReplaceLogic.getTarget().getSelection(), is(new Point(5, 5))); } + @Test + public void testSearchOptionListenerCalledOnActivation() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + + findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); + + assertThat(receivedValues, is(List.of(true))); + } + + @Test + public void testSearchOptionListenerCalledOnDeactivation() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + + findReplaceLogic.deactivate(SearchOptions.CASE_SENSITIVE); + + assertThat(receivedValues, is(List.of(false))); + } + + @Test + public void testSearchOptionListenerNotCalledWhenAlreadyActive() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + + findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); + + assertTrue(receivedValues.isEmpty()); + } + + @Test + public void testSearchOptionListenerNotCalledWhenAlreadyInactive() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + // CASE_SENSITIVE is inactive by default + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + + findReplaceLogic.deactivate(SearchOptions.CASE_SENSITIVE); + + assertTrue(receivedValues.isEmpty()); + } + + @Test + public void testMultipleListenersForSameOptionAllNotified() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + List received1= new ArrayList<>(); + List received2= new ArrayList<>(); + findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, received1::add); + findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, received2::add); + + findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); + + assertThat(received1, is(List.of(true))); + assertThat(received2, is(List.of(true))); + } + private void expectStatusEmpty(IFindReplaceLogic findReplaceLogic) { assertThat(findReplaceLogic.getStatus(), instanceOf(NoStatus.class)); } diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/OverlayAccess.java b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/OverlayAccess.java index 16de0671b77..c8fc1769901 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/OverlayAccess.java +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/OverlayAccess.java @@ -92,7 +92,7 @@ private Composite getContainerControl() { } private void restoreInitialConfiguration() { - find.setText(""); + setFindText(""); select(SearchOptions.GLOBAL); unselect(SearchOptions.REGEX); unselect(SearchOptions.CASE_SENSITIVE); @@ -113,28 +113,24 @@ public void close() { @Override public void select(SearchOptions option) { - ToolItem button= getButtonForSearchOption(option); - if (button == null) { + if (getSelectedOptions().contains(option)) { return; } - button.setSelection(true); - if (option == SearchOptions.GLOBAL) { - button.setSelection(false); + ToolItem button= getButtonForSearchOption(option); + if (button != null) { + button.notifyListeners(SWT.Selection, null); } - button.notifyListeners(SWT.Selection, null); } @Override public void unselect(SearchOptions option) { - ToolItem button= getButtonForSearchOption(option); - if (button == null) { + if (!getSelectedOptions().contains(option)) { return; } - button.setSelection(false); - if (option == SearchOptions.GLOBAL) { - button.setSelection(true); + ToolItem button= getButtonForSearchOption(option); + if (button != null) { + button.notifyListeners(SWT.Selection, null); } - button.notifyListeners(SWT.Selection, null); } @Override @@ -208,11 +204,15 @@ private Set getSelectedOptions() { private Predicate isOptionSelected() { return option -> { ToolItem buttonForSearchOption= getButtonForSearchOption(option); + if (buttonForSearchOption == null) { + return false; + } if (option == SearchOptions.GLOBAL) { - return !buttonForSearchOption.getSelection();// The "Global" option is mapped to a button that - // selects whether to search in the selection, thus inverting the semantic + // The "Global" option is mapped to a button that selects whether to search in + // the selection, thus inverting the semantic + return !buttonForSearchOption.getSelection(); } - return buttonForSearchOption != null && buttonForSearchOption.getSelection(); + return buttonForSearchOption.getSelection(); }; }