From 81cbd7d917acb6cd9049435a188c15323ea5faf3 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Thu, 9 Jul 2026 23:43:46 +0200 Subject: [PATCH] Find/replace: centralize UI state notifications in FindReplaceLogic Previously, the overlay managed button enablement imperatively: toggle lambdas and search-bar listeners scattered setEnabled() calls across FindReplaceOverlay, replicating logic that FindReplaceLogic already encapsulates internally. FindReplaceLogic now exposes two notification channels - addSearchOptionActivationChangedListener and addSearchOptionAvailabilityChangedListener - so that UI components can subscribe to state changes rather than polling after each mutation. AccessibleToolItemBuilder registers both listeners for every button bound to a SearchOption, removing the need for the overlay to know which widgets to update when logic state changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../findandreplace/FindReplaceLogic.java | 51 ++++++++++-- .../findandreplace/IFindReplaceLogic.java | 20 ++++- .../overlay/AccessibleToolItemBuilder.java | 20 +++-- .../overlay/FindReplaceOverlay.java | 8 +- .../findandreplace/FindReplaceLogicTest.java | 78 +++++++++++++++++-- 5 files changed, 153 insertions(+), 24 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 c81dd909b82..57cb6b2f15e 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 @@ -16,6 +16,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.EnumMap; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -63,26 +64,56 @@ public class FindReplaceLogic implements IFindReplaceLogic { private String findString = ""; //$NON-NLS-1$ private String replaceString = ""; //$NON-NLS-1$ - private final Map>> searchOptionListeners = new HashMap<>(); + private final Map>> activationChangedListeners = new HashMap<>(); + private final Map>> availabilityChangedListeners = new HashMap<>(); @Override - public void addSearchOptionChangedListener(SearchOptions option, Consumer listener) { + public void addSearchOptionActivationChangedListener(SearchOptions option, Consumer listener) { Objects.requireNonNull(option); Objects.requireNonNull(listener); - searchOptionListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener); + activationChangedListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener); } - private void notifySearchOptionChangedListeners(SearchOptions option, boolean newState) { - List> listeners = searchOptionListeners.getOrDefault(option, Collections.emptyList()); + @Override + public void addSearchOptionAvailabilityChangedListener(SearchOptions option, Consumer listener) { + Objects.requireNonNull(option); + Objects.requireNonNull(listener); + availabilityChangedListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener); + } + + private void notifyActivationChangedListeners(SearchOptions option, boolean newState) { + List> listeners = activationChangedListeners.getOrDefault(option, Collections.emptyList()); listeners.forEach(l -> l.accept(newState)); } + private void notifyAvailabilityChangedListeners(SearchOptions option, boolean newAvailability) { + List> listeners = availabilityChangedListeners.getOrDefault(option, Collections.emptyList()); + listeners.forEach(l -> l.accept(newAvailability)); + } + + private Map captureAvailabilities() { + Map snapshot = new EnumMap<>(SearchOptions.class); + availabilityChangedListeners.keySet().forEach(option -> snapshot.put(option, isAvailable(option))); + return snapshot; + } + + private void notifyAvailabilityChangedIfNeeded(Map oldAvailabilities) { + oldAvailabilities.forEach((option, oldAvailability) -> { + boolean newAvailability = isAvailable(option); + if (oldAvailability != newAvailability) { + notifyAvailabilityChangedListeners(option, newAvailability); + } + }); + } + @Override public void setFindString(String findString) { + Map oldAvailabilities = captureAvailabilities(); this.findString = Objects.requireNonNull(findString); if (isAvailableAndActive(SearchOptions.INCREMENTAL)) { performSearch(true); } + notifyAvailabilityChangedIfNeeded(oldAvailabilities); } @Override @@ -92,6 +123,7 @@ public void setReplaceString(String replaceString) { @Override public void activate(SearchOptions searchOption) { + Map oldAvailabilities = captureAvailabilities(); if (!searchOptions.add(searchOption)) { return; } @@ -110,11 +142,13 @@ public void activate(SearchOptions searchOption) { default: break; } - notifySearchOptionChangedListeners(searchOption, true); + notifyActivationChangedListeners(searchOption, true); + notifyAvailabilityChangedIfNeeded(oldAvailabilities); } @Override public void deactivate(SearchOptions searchOption) { + Map oldAvailabilities = captureAvailabilities(); if (!searchOptions.remove(searchOption)) { return; } @@ -126,7 +160,8 @@ public void deactivate(SearchOptions searchOption) { if (searchOption == SearchOptions.FORWARD && shouldInitIncrementalBaseLocation()) { resetIncrementalBaseLocation(); } - notifySearchOptionChangedListeners(searchOption, false); + notifyActivationChangedListeners(searchOption, false); + notifyAvailabilityChangedIfNeeded(oldAvailabilities); } @Override @@ -613,6 +648,7 @@ public void updateTarget(IFindReplaceTarget newTarget, boolean canEditTarget) { this.isTargetEditable = canEditTarget; if (this.target != newTarget) { + Map oldAvailabilities = captureAvailabilities(); if (this.target instanceof IFindReplaceTargetExtension) { ((IFindReplaceTargetExtension) this.target).endSession(); } @@ -625,6 +661,7 @@ public void updateTarget(IFindReplaceTarget newTarget, boolean canEditTarget) { activate(SearchOptions.GLOBAL); } + notifyAvailabilityChangedIfNeeded(oldAvailabilities); } resetIncrementalBaseLocation(); 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 9ccd97d83f0..9dd2408890c 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 @@ -74,7 +74,25 @@ public interface IFindReplaceLogic { * @param option the search option to observe * @param listener the listener to register */ - public void addSearchOptionChangedListener(SearchOptions option, Consumer listener); + public void addSearchOptionActivationChangedListener(SearchOptions option, Consumer listener); + + /** + * Registers a listener that is notified whenever the availability of the given + * search option changes. The listener is called with {@code true} when the + * option becomes available and {@code false} when it becomes unavailable. + * + *

+ * Availability is determined by {@link #isAvailable(SearchOptions)} and depends + * on the state of the target and the compatibility of active options (e.g. + * {@link SearchOptions#WHOLE_WORD} is unavailable when + * {@link SearchOptions#REGEX} is active or the search string is not a single + * word). Listeners are only notified when availability actually changes. + *

+ * + * @param option the search option to observe + * @param listener the listener to register + */ + public void addSearchOptionAvailabilityChangedListener(SearchOptions option, Consumer listener); /** * @param searchOption option 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 84793ffec62..a7078045fdd 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 @@ -61,8 +61,9 @@ public AccessibleToolItemBuilder withAction(FindReplaceOverlayAction newAction) /** * 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. + * selection state is initialized from the logic's current activation state and + * kept in sync automatically. The item's enabled state is also initialized from + * and kept in sync with the option's availability. */ public AccessibleToolItemBuilder withSearchOption(SearchOptions option, IFindReplaceLogic logic) { this.searchOption = option; @@ -73,9 +74,10 @@ public AccessibleToolItemBuilder withSearchOption(SearchOptions option, IFindRep /** * 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. + * the selection 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; @@ -99,11 +101,17 @@ public ToolItem build() { if (searchOption != null) { boolean initial = findReplaceLogic.isActive(searchOption); toolItem.setSelection(invertSearchOption ? !initial : initial); - findReplaceLogic.addSearchOptionChangedListener(searchOption, state -> { + findReplaceLogic.addSearchOptionActivationChangedListener(searchOption, state -> { if (!toolItem.isDisposed()) { toolItem.setSelection(invertSearchOption ? !state : state); } }); + toolItem.setEnabled(findReplaceLogic.isAvailable(searchOption)); + findReplaceLogic.addSearchOptionAvailabilityChangedListener(searchOption, available -> { + if (!toolItem.isDisposed()) { + toolItem.setEnabled(available); + } + }); } 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 cb2a3440ae2..d0242a4bc59 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 @@ -646,10 +646,7 @@ private void createAreaSearchButton() { private void createRegexSearchButton() { FindReplaceOverlayAction regexAction = new FindReplaceOverlayAction(() -> { activateInFindReplacerIf(SearchOptions.REGEX, !findReplaceLogic.isActive(SearchOptions.REGEX)); - wholeWordSearchButton.setEnabled(findReplaceLogic.isAvailable(SearchOptions.WHOLE_WORD)); updateIncrementalSearch(); - updateContentAssistAvailability(); - decorate(); }); regexAction.addShortcuts(KeyboardShortcuts.OPTION_REGEX); commonActions.add(regexAction); @@ -658,6 +655,10 @@ private void createRegexSearchButton() { .withToolTipText(FindReplaceMessages.FindReplaceOverlay_regexSearchButton_toolTip) .withSearchOption(SearchOptions.REGEX, findReplaceLogic) .withAction(regexAction).build(); + findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.REGEX, activated -> { + updateContentAssistAvailability(); + decorate(); + }); } private void createCaseSensitiveButton() { @@ -744,7 +745,6 @@ private void createSearchBar() { searchBar.selectAll(); searchBar.addModifyListener(e -> { updateIncrementalSearch(); - wholeWordSearchButton.setEnabled(findReplaceLogic.isAvailable(SearchOptions.WHOLE_WORD)); decorate(); }); searchBar.addFocusListener(new FocusListener() { 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 463b81d9f6c..1862f80e442 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 @@ -987,7 +987,7 @@ public void testIncrementalSearchBackwardNoUpdateIfAlreadyOnWord() { public void testSearchOptionListenerCalledOnActivation() { IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); List receivedValues= new ArrayList<>(); - findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); @@ -999,7 +999,7 @@ public void testSearchOptionListenerCalledOnDeactivation() { IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); List receivedValues= new ArrayList<>(); - findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); findReplaceLogic.deactivate(SearchOptions.CASE_SENSITIVE); @@ -1011,7 +1011,7 @@ public void testSearchOptionListenerNotCalledWhenAlreadyActive() { IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); List receivedValues= new ArrayList<>(); - findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); + findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); @@ -1023,7 +1023,7 @@ 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.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add); findReplaceLogic.deactivate(SearchOptions.CASE_SENSITIVE); @@ -1035,8 +1035,8 @@ 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.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, received1::add); + findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, received2::add); findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE); @@ -1044,6 +1044,72 @@ public void testMultipleListenersForSameOptionAllNotified() { assertThat(received2, is(List.of(true))); } + @Test + public void testAvailabilityListenerCalledWhenRegexActivationMakesWholeWordUnavailable() { + TextViewer textViewer= setupTextViewer(""); + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer); + findReplaceLogic.setFindString("word"); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add); + + findReplaceLogic.activate(SearchOptions.REGEX); + + assertThat(receivedValues, is(List.of(false))); + } + + @Test + public void testAvailabilityListenerCalledWhenRegexDeactivationMakesWholeWordAvailable() { + TextViewer textViewer= setupTextViewer(""); + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer); + findReplaceLogic.setFindString("word"); + findReplaceLogic.activate(SearchOptions.REGEX); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add); + + findReplaceLogic.deactivate(SearchOptions.REGEX); + + assertThat(receivedValues, is(List.of(true))); + } + + @Test + public void testAvailabilityListenerCalledWhenFindStringBecomesNonWord() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + findReplaceLogic.setFindString("word"); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add); + + findReplaceLogic.setFindString("hello world"); + + assertThat(receivedValues, is(List.of(false))); + } + + @Test + public void testAvailabilityListenerCalledWhenFindStringBecomesWord() { + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null); + findReplaceLogic.setFindString("hello world"); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add); + + findReplaceLogic.setFindString("word"); + + assertThat(receivedValues, is(List.of(true))); + } + + @Test + public void testAvailabilityListenerNotCalledWhenAvailabilityUnchanged() { + TextViewer textViewer= setupTextViewer(""); + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer); + findReplaceLogic.setFindString("hello world"); + List receivedValues= new ArrayList<>(); + findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add); + + // WHOLE_WORD is already unavailable due to non-word string; activating REGEX + // does not change its availability + findReplaceLogic.activate(SearchOptions.REGEX); + + assertTrue(receivedValues.isEmpty()); + } + private void expectStatusEmpty(IFindReplaceLogic findReplaceLogic) { assertThat(findReplaceLogic.getStatus(), instanceOf(NoStatus.class)); }