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