Skip to content

Commit 1d5a79b

Browse files
HeikoKlareCopilot
HeikoKlare
and
Copilot
committed
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. As a related fix, isAvailable(WHOLE_WORD) incorrectly depended on isAvailableAndActive(REGEX) instead of isActive(REGEX), causing the whole-word button to stop reacting to regex toggling on targets without regex support. The condition is corrected to depend solely on whether regex mode is active. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 84ad5a4 commit 1d5a79b

5 files changed

Lines changed: 155 additions & 25 deletions

File tree

‎bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java‎

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -63,26 +63,48 @@ public class FindReplaceLogic implements IFindReplaceLogic {
6363
private String findString = ""; //$NON-NLS-1$
6464
private String replaceString = ""; //$NON-NLS-1$
6565

66-
private final Map<SearchOptions, List<Consumer<Boolean>>> searchOptionListeners = new HashMap<>();
66+
private final Map<SearchOptions, List<Consumer<Boolean>>> activationChangedListeners = new HashMap<>();
67+
private final Map<SearchOptions, List<Consumer<Boolean>>> availabilityChangedListeners = new HashMap<>();
6768

6869
@Override
69-
public void addSearchOptionChangedListener(SearchOptions option, Consumer<Boolean> listener) {
70+
public void addSearchOptionActivationChangedListener(SearchOptions option, Consumer<Boolean> listener) {
7071
Objects.requireNonNull(option);
7172
Objects.requireNonNull(listener);
72-
searchOptionListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener);
73+
activationChangedListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener);
7374
}
7475

75-
private void notifySearchOptionChangedListeners(SearchOptions option, boolean newState) {
76-
List<Consumer<Boolean>> listeners = searchOptionListeners.getOrDefault(option, Collections.emptyList());
76+
@Override
77+
public void addSearchOptionAvailabilityChangedListener(SearchOptions option, Consumer<Boolean> listener) {
78+
Objects.requireNonNull(option);
79+
Objects.requireNonNull(listener);
80+
availabilityChangedListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener);
81+
}
82+
83+
private void notifyActivationChangedListeners(SearchOptions option, boolean newState) {
84+
List<Consumer<Boolean>> listeners = activationChangedListeners.getOrDefault(option, Collections.emptyList());
7785
listeners.forEach(l -> l.accept(newState));
7886
}
7987

88+
private void notifyAvailabilityChangedListeners(SearchOptions option, boolean newAvailability) {
89+
List<Consumer<Boolean>> listeners = availabilityChangedListeners.getOrDefault(option, Collections.emptyList());
90+
listeners.forEach(l -> l.accept(newAvailability));
91+
}
92+
93+
private void notifyAvailabilityChangedIfNeeded(boolean oldWholeWordAvailability) {
94+
boolean newAvailability = isAvailable(SearchOptions.WHOLE_WORD);
95+
if (oldWholeWordAvailability != newAvailability) {
96+
notifyAvailabilityChangedListeners(SearchOptions.WHOLE_WORD, newAvailability);
97+
}
98+
}
99+
80100
@Override
81101
public void setFindString(String findString) {
102+
boolean oldWholeWordAvailability = isAvailable(SearchOptions.WHOLE_WORD);
82103
this.findString = Objects.requireNonNull(findString);
83104
if (isAvailableAndActive(SearchOptions.INCREMENTAL)) {
84105
performSearch(true);
85106
}
107+
notifyAvailabilityChangedIfNeeded(oldWholeWordAvailability);
86108
}
87109

88110
@Override
@@ -92,6 +114,7 @@ public void setReplaceString(String replaceString) {
92114

93115
@Override
94116
public void activate(SearchOptions searchOption) {
117+
boolean oldWholeWordAvailability = isAvailable(SearchOptions.WHOLE_WORD);
95118
if (!searchOptions.add(searchOption)) {
96119
return;
97120
}
@@ -110,11 +133,13 @@ public void activate(SearchOptions searchOption) {
110133
default:
111134
break;
112135
}
113-
notifySearchOptionChangedListeners(searchOption, true);
136+
notifyActivationChangedListeners(searchOption, true);
137+
notifyAvailabilityChangedIfNeeded(oldWholeWordAvailability);
114138
}
115139

116140
@Override
117141
public void deactivate(SearchOptions searchOption) {
142+
boolean oldWholeWordAvailability = isAvailable(SearchOptions.WHOLE_WORD);
118143
if (!searchOptions.remove(searchOption)) {
119144
return;
120145
}
@@ -126,7 +151,8 @@ public void deactivate(SearchOptions searchOption) {
126151
if (searchOption == SearchOptions.FORWARD && shouldInitIncrementalBaseLocation()) {
127152
resetIncrementalBaseLocation();
128153
}
129-
notifySearchOptionChangedListeners(searchOption, false);
154+
notifyActivationChangedListeners(searchOption, false);
155+
notifyAvailabilityChangedIfNeeded(oldWholeWordAvailability);
130156
}
131157

132158
@Override
@@ -156,7 +182,7 @@ public boolean isAvailable(SearchOptions searchOption) {
156182
case REGEX:
157183
return isTargetSupportingRegEx;
158184
case WHOLE_WORD:
159-
return !isAvailableAndActive(SearchOptions.REGEX) && isWord(findString);
185+
return !isActive(SearchOptions.REGEX) && isWord(findString);
160186
case INCREMENTAL:
161187
case CASE_SENSITIVE:
162188
case FORWARD:

‎bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,25 @@ public interface IFindReplaceLogic {
7474
* @param option the search option to observe
7575
* @param listener the listener to register
7676
*/
77-
public void addSearchOptionChangedListener(SearchOptions option, Consumer<Boolean> listener);
77+
public void addSearchOptionActivationChangedListener(SearchOptions option, Consumer<Boolean> listener);
78+
79+
/**
80+
* Registers a listener that is notified whenever the availability of the given
81+
* search option changes. The listener is called with {@code true} when the
82+
* option becomes available and {@code false} when it becomes unavailable.
83+
*
84+
* <p>
85+
* Availability is determined by {@link #isAvailable(SearchOptions)} and depends
86+
* on the state of the target and the compatibility of active options (e.g.
87+
* {@link SearchOptions#WHOLE_WORD} is unavailable when
88+
* {@link SearchOptions#REGEX} is active or the search string is not a single
89+
* word). Listeners are only notified when availability actually changes.
90+
* </p>
91+
*
92+
* @param option the search option to observe
93+
* @param listener the listener to register
94+
*/
95+
public void addSearchOptionAvailabilityChangedListener(SearchOptions option, Consumer<Boolean> listener);
7896

7997
/**
8098
* @param searchOption option

‎bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/AccessibleToolItemBuilder.java‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,9 @@ public AccessibleToolItemBuilder withAction(FindReplaceOverlayAction newAction)
6161

6262
/**
6363
* Binds a {@link SearchOptions} value to this item. When built, the item's
64-
* selection state is initialized from the logic's current state and kept in
65-
* sync automatically whenever the option changes.
64+
* selection state is initialized from the logic's current activation state and
65+
* kept in sync automatically. The item's enabled state is also initialized from
66+
* and kept in sync with the option's availability.
6667
*/
6768
public AccessibleToolItemBuilder withSearchOption(SearchOptions option, IFindReplaceLogic logic) {
6869
this.searchOption = option;
@@ -73,9 +74,10 @@ public AccessibleToolItemBuilder withSearchOption(SearchOptions option, IFindRep
7374

7475
/**
7576
* Like {@link #withSearchOption(SearchOptions, IFindReplaceLogic)} but inverts
76-
* the mapping: the item is selected when the option is <em>inactive</em>.
77-
* Useful for options like {@link SearchOptions#GLOBAL} where a "search in
78-
* selection" button should be selected when searching globally is turned off.
77+
* the selection mapping: the item is selected when the option is
78+
* <em>inactive</em>. Useful for options like {@link SearchOptions#GLOBAL} where
79+
* a "search in selection" button should be selected when searching globally is
80+
* turned off.
7981
*/
8082
public AccessibleToolItemBuilder withInvertedSearchOption(SearchOptions option, IFindReplaceLogic logic) {
8183
this.searchOption = option;
@@ -99,11 +101,17 @@ public ToolItem build() {
99101
if (searchOption != null) {
100102
boolean initial = findReplaceLogic.isActive(searchOption);
101103
toolItem.setSelection(invertSearchOption ? !initial : initial);
102-
findReplaceLogic.addSearchOptionChangedListener(searchOption, state -> {
104+
findReplaceLogic.addSearchOptionActivationChangedListener(searchOption, state -> {
103105
if (!toolItem.isDisposed()) {
104106
toolItem.setSelection(invertSearchOption ? !state : state);
105107
}
106108
});
109+
toolItem.setEnabled(findReplaceLogic.isAvailable(searchOption));
110+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(searchOption, available -> {
111+
if (!toolItem.isDisposed()) {
112+
toolItem.setEnabled(available);
113+
}
114+
});
107115
}
108116
return toolItem;
109117
}

‎bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -646,10 +646,7 @@ private void createAreaSearchButton() {
646646
private void createRegexSearchButton() {
647647
FindReplaceOverlayAction regexAction = new FindReplaceOverlayAction(() -> {
648648
activateInFindReplacerIf(SearchOptions.REGEX, !findReplaceLogic.isActive(SearchOptions.REGEX));
649-
wholeWordSearchButton.setEnabled(findReplaceLogic.isAvailable(SearchOptions.WHOLE_WORD));
650649
updateIncrementalSearch();
651-
updateContentAssistAvailability();
652-
decorate();
653650
});
654651
regexAction.addShortcuts(KeyboardShortcuts.OPTION_REGEX);
655652
commonActions.add(regexAction);
@@ -658,6 +655,10 @@ private void createRegexSearchButton() {
658655
.withToolTipText(FindReplaceMessages.FindReplaceOverlay_regexSearchButton_toolTip)
659656
.withSearchOption(SearchOptions.REGEX, findReplaceLogic)
660657
.withAction(regexAction).build();
658+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.REGEX, activated -> {
659+
updateContentAssistAvailability();
660+
decorate();
661+
});
661662
}
662663

663664
private void createCaseSensitiveButton() {
@@ -744,7 +745,6 @@ private void createSearchBar() {
744745
searchBar.selectAll();
745746
searchBar.addModifyListener(e -> {
746747
updateIncrementalSearch();
747-
wholeWordSearchButton.setEnabled(findReplaceLogic.isAvailable(SearchOptions.WHOLE_WORD));
748748
decorate();
749749
});
750750
searchBar.addFocusListener(new FocusListener() {

‎tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java‎

Lines changed: 84 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -974,7 +974,7 @@ public void testIncrementalSearchBackwardNoUpdateIfAlreadyOnWord() {
974974
public void testSearchOptionListenerCalledOnActivation() {
975975
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
976976
List<Boolean> receivedValues= new ArrayList<>();
977-
findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
977+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
978978

979979
findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE);
980980

@@ -986,7 +986,7 @@ public void testSearchOptionListenerCalledOnDeactivation() {
986986
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
987987
findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE);
988988
List<Boolean> receivedValues= new ArrayList<>();
989-
findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
989+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
990990

991991
findReplaceLogic.deactivate(SearchOptions.CASE_SENSITIVE);
992992

@@ -998,7 +998,7 @@ public void testSearchOptionListenerNotCalledWhenAlreadyActive() {
998998
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
999999
findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE);
10001000
List<Boolean> receivedValues= new ArrayList<>();
1001-
findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
1001+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
10021002

10031003
findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE);
10041004

@@ -1010,7 +1010,7 @@ public void testSearchOptionListenerNotCalledWhenAlreadyInactive() {
10101010
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
10111011
// CASE_SENSITIVE is inactive by default
10121012
List<Boolean> receivedValues= new ArrayList<>();
1013-
findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
1013+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, receivedValues::add);
10141014

10151015
findReplaceLogic.deactivate(SearchOptions.CASE_SENSITIVE);
10161016

@@ -1022,15 +1022,93 @@ public void testMultipleListenersForSameOptionAllNotified() {
10221022
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
10231023
List<Boolean> received1= new ArrayList<>();
10241024
List<Boolean> received2= new ArrayList<>();
1025-
findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, received1::add);
1026-
findReplaceLogic.addSearchOptionChangedListener(SearchOptions.CASE_SENSITIVE, received2::add);
1025+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, received1::add);
1026+
findReplaceLogic.addSearchOptionActivationChangedListener(SearchOptions.CASE_SENSITIVE, received2::add);
10271027

10281028
findReplaceLogic.activate(SearchOptions.CASE_SENSITIVE);
10291029

10301030
assertThat(received1, is(List.of(true)));
10311031
assertThat(received2, is(List.of(true)));
10321032
}
10331033

1034+
@Test
1035+
public void testAvailabilityListenerCalledWhenRegexActivationMakesWholeWordUnavailable() {
1036+
TextViewer textViewer= setupTextViewer("");
1037+
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer);
1038+
findReplaceLogic.setFindString("word");
1039+
List<Boolean> receivedValues= new ArrayList<>();
1040+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add);
1041+
1042+
findReplaceLogic.activate(SearchOptions.REGEX);
1043+
1044+
assertThat(receivedValues, is(List.of(false)));
1045+
}
1046+
1047+
@Test
1048+
public void testAvailabilityListenerCalledWhenRegexDeactivationMakesWholeWordAvailable() {
1049+
TextViewer textViewer= setupTextViewer("");
1050+
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer);
1051+
findReplaceLogic.setFindString("word");
1052+
findReplaceLogic.activate(SearchOptions.REGEX);
1053+
List<Boolean> receivedValues= new ArrayList<>();
1054+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add);
1055+
1056+
findReplaceLogic.deactivate(SearchOptions.REGEX);
1057+
1058+
assertThat(receivedValues, is(List.of(true)));
1059+
}
1060+
1061+
@Test
1062+
public void testAvailabilityListenerCalledWhenRegexActivationMakesWholeWordUnavailableEvenWithoutRegexSupport() {
1063+
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
1064+
findReplaceLogic.setFindString("word");
1065+
List<Boolean> receivedValues= new ArrayList<>();
1066+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add);
1067+
1068+
findReplaceLogic.activate(SearchOptions.REGEX);
1069+
1070+
assertThat(receivedValues, is(List.of(false)));
1071+
}
1072+
1073+
@Test
1074+
public void testAvailabilityListenerCalledWhenFindStringBecomesNonWord() {
1075+
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
1076+
findReplaceLogic.setFindString("word");
1077+
List<Boolean> receivedValues= new ArrayList<>();
1078+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add);
1079+
1080+
findReplaceLogic.setFindString("hello world");
1081+
1082+
assertThat(receivedValues, is(List.of(false)));
1083+
}
1084+
1085+
@Test
1086+
public void testAvailabilityListenerCalledWhenFindStringBecomesWord() {
1087+
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(null);
1088+
findReplaceLogic.setFindString("hello world");
1089+
List<Boolean> receivedValues= new ArrayList<>();
1090+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add);
1091+
1092+
findReplaceLogic.setFindString("word");
1093+
1094+
assertThat(receivedValues, is(List.of(true)));
1095+
}
1096+
1097+
@Test
1098+
public void testAvailabilityListenerNotCalledWhenAvailabilityUnchanged() {
1099+
TextViewer textViewer= setupTextViewer("");
1100+
IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer);
1101+
findReplaceLogic.setFindString("hello world");
1102+
List<Boolean> receivedValues= new ArrayList<>();
1103+
findReplaceLogic.addSearchOptionAvailabilityChangedListener(SearchOptions.WHOLE_WORD, receivedValues::add);
1104+
1105+
// WHOLE_WORD is already unavailable due to non-word string; activating REGEX
1106+
// does not change its availability
1107+
findReplaceLogic.activate(SearchOptions.REGEX);
1108+
1109+
assertTrue(receivedValues.isEmpty());
1110+
}
1111+
10341112
private void expectStatusEmpty(IFindReplaceLogic findReplaceLogic) {
10351113
assertThat(findReplaceLogic.getStatus(), instanceOf(NoStatus.class));
10361114
}

0 commit comments

Comments
 (0)