Find/Replace overlay: decouple toggle button state from action execution

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<SearchOptions, List<Consumer<Boolean>>> searchOptionListeners = new HashMap<>();
+
+	@Override
+	public void addSearchOptionChangedListener(SearchOptions option, Consumer<Boolean> listener) {
+		Objects.requireNonNull(option);
+		Objects.requireNonNull(listener);
+		searchOptionListeners.computeIfAbsent(option, k -> new CopyOnWriteArrayList<>()).add(listener);
+	}
+
+	private void notifySearchOptionChangedListeners(SearchOptions option, boolean newState) {
+		List<Consumer<Boolean>> 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

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.
+	 *
+	 * <p>
+	 * 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.
+	 * </p>
+	 *
+	 * @param option   the search option to observe
+	 * @param listener the listener to register
+	 */
+	public void addSearchOptionChangedListener(SearchOptions option, Consumer<Boolean> listener);
+
 	/**
 	 * @param searchOption option
 	 * @return whether the option is active

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

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<KeyStroke> 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()));

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<KeyStroke> 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 <em>inactive</em>.
+	 * 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;
 	}
 }

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

@@ -611,47 +611,54 @@ 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() {
 		regexSearchButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.CHECK)
 				.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() {
 		caseSensitiveSearchButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.CHECK)
 				.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() {
 		wholeWordSearchButton = new AccessibleToolItemBuilder(searchTools).withStyleBits(SWT.CHECK)
 				.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() {

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<Boolean> 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<Boolean> 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<Boolean> 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<Boolean> 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<Boolean> received1= new ArrayList<>();
+		List<Boolean> 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));
 	}