From c087338a47d3431a33e6dc9846b010295a261c52 Mon Sep 17 00:00:00 2001 From: Maximilian Wittmer Date: Mon, 3 Jun 2024 13:33:07 +0200 Subject: [PATCH] Find/Replace overlay: consistent handling of incremental base location Increment the incremental base location consistently, remove method for "performIncrementalSearch" as it only called "performSearch" and lead to semantic confusion while calling "performSearch". - "SearchOptions.REGEX" will now correctly update the incremental base location once it is disabled. - Added a method for updating the incremental base location manually from outside fixes #1914 fixes #1913 --- .../findandreplace/FindReplaceLogic.java | 67 ++++++------------- .../findandreplace/IFindReplaceLogic.java | 27 +++----- .../overlay/FindReplaceOverlay.java | 20 ++++-- .../FindReplaceOverlayFirstTimePopup.java | 1 - .../ui/texteditor/FindReplaceDialog.java | 7 +- .../findandreplace/FindReplaceLogicTest.java | 52 ++++++++++++++ .../overlay/FindReplaceOverlayTest.java | 4 ++ .../tests/FindReplaceDialogTest.java | 15 +++++ 8 files changed, 122 insertions(+), 71 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 1d46d9cd698..afbf665c530 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 @@ -66,8 +66,8 @@ public void activate(SearchOptions searchOption) { break; case FORWARD: case INCREMENTAL: - if (shouldInitIncrementalBaseLocation()) { - initIncrementalBaseLocation(); + if (shouldResetIncrementalBaseLocation()) { + resetIncrementalBaseLocation(); } break; // $CASES-OMITTED$ @@ -82,12 +82,16 @@ public void deactivate(SearchOptions searchOption) { return; } + if (searchOption == SearchOptions.REGEX) { + resetIncrementalBaseLocation(); + } + if (searchOption == SearchOptions.GLOBAL) { initializeSearchScope(); } - if (searchOption == SearchOptions.FORWARD && shouldInitIncrementalBaseLocation()) { - initIncrementalBaseLocation(); + if (searchOption == SearchOptions.FORWARD && shouldResetIncrementalBaseLocation()) { + resetIncrementalBaseLocation(); } } @@ -141,7 +145,8 @@ public boolean isRegExSearchAvailableAndActive() { * Initializes the anchor used as starting point for incremental searching. * */ - private void initIncrementalBaseLocation() { + @Override + public void resetIncrementalBaseLocation() { if (target != null && isActive(SearchOptions.INCREMENTAL) && !isRegExSearchAvailableAndActive()) { incrementalBaseLocation = target.getSelection(); } else { @@ -149,7 +154,7 @@ private void initIncrementalBaseLocation() { } } - public boolean shouldInitIncrementalBaseLocation() { + public boolean shouldResetIncrementalBaseLocation() { return isActive(SearchOptions.INCREMENTAL) && !isActive(SearchOptions.REGEX); } @@ -158,8 +163,8 @@ public boolean shouldInitIncrementalBaseLocation() { * selected lines. */ private void initializeSearchScope() { - if (shouldInitIncrementalBaseLocation()) { - initIncrementalBaseLocation(); + if (shouldResetIncrementalBaseLocation()) { + resetIncrementalBaseLocation(); } if (target == null || !(target instanceof IFindReplaceTargetExtension)) { @@ -329,28 +334,16 @@ private boolean replaceSelection(String replaceString) { } @Override - public boolean performSearch(String searchString) { - return performSearch(shouldInitIncrementalBaseLocation(), searchString); - } + public boolean performSearch(String findString) { + resetStatus(); - /** - * Locates the user's findString in the text of the target. - * - * @param mustInitIncrementalBaseLocation true if base location - * must be initialized - * @param findString the String to search for - * @return Whether the string was found in the target - */ - private boolean performSearch(boolean mustInitIncrementalBaseLocation, String findString) { - if (mustInitIncrementalBaseLocation) { - initIncrementalBaseLocation(); + if (isActive(SearchOptions.INCREMENTAL) && !isIncrementalSearchAvailable()) { + return false; // Do nothing if search options are not compatible } - resetStatus(); boolean somethingFound = false; if (findString != null && !findString.isEmpty()) { - try { somethingFound = findNext(findString); } catch (PatternSyntaxException ex) { @@ -359,6 +352,7 @@ private boolean performSearch(boolean mustInitIncrementalBaseLocation, String fi // we don't keep state in this dialog } } + return somethingFound; } @@ -367,8 +361,7 @@ private boolean performSearch(boolean mustInitIncrementalBaseLocation, String fi * Returns the number of replacements that occur. * * @param findString the string to search for - * @param replaceString the replacement string - * expression + * @param replaceString the replacement string expression * @return the number of occurrences * */ @@ -604,7 +597,7 @@ public void updateTarget(IFindReplaceTarget newTarget, boolean canEditTarget) { } } - initIncrementalBaseLocation(); + resetIncrementalBaseLocation(); } @Override @@ -656,26 +649,6 @@ private void statusLineMessage(String message) { statusLineMessage(false, message); } - @Override - public void performIncrementalSearch(String searchString) { - resetStatus(); - - if (isActive(SearchOptions.INCREMENTAL) && isIncrementalSearchAvailable()) { - if (searchString.equals("") && target != null) { //$NON-NLS-1$ - // empty selection at base location - int offset = incrementalBaseLocation.x; - - if (isActive(SearchOptions.FORWARD)) { - offset = offset + incrementalBaseLocation.y; - } - - findAndSelect(offset, ""); //$NON-NLS-1$ - } else { - performSearch(false, searchString); - } - } - } - @Override public IFindReplaceTarget getTarget() { return target; 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 12d3bf672bb..c7c25699f54 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 @@ -70,21 +70,6 @@ public interface IFindReplaceLogic { */ public boolean isIncrementalSearchAvailable(); - /** - * Updates the search result after the Text was Modified. Used in combination - * with setIncrementalSearch(true). This method specifically allows - * for "search-as-you-type" - * - * "Search-as-you-type" is not compatible with RegEx-search. This will - * initialize the base-location for search (if not initialized already) but will - * not update it, meaning that incrementally searching the same string twice in - * a row will always yield the same result, unless the Base location was - * modified (eg., by performing "find next") - * - * @param searchString the String that is to be searched - */ - public void performIncrementalSearch(String searchString); - /** * Replaces all occurrences of the user's findString with the replace string. * Indicate to the user the number of replacements that occur. @@ -102,7 +87,11 @@ public interface IFindReplaceLogic { public void performSelectAll(String findString); /** - * Locates the user's findString in the target + * Locates the user's findString in the target. If incremental search is + * activated, the search will be performed starting from an incremental search + * position, which can be reset using {@link #resetIncrementalBaseLocation()}. + * If incremental search is activated and RegEx search is activated, nothing + * happens. * * @param searchString the String to search for * @return Whether the string was found in the target @@ -174,4 +163,10 @@ public interface IFindReplaceLogic { */ public boolean isWholeWordSearchAvailable(String findString); + /** + * Initializes the anchor used as starting point for incremental searching. + * Subsequent incremental searches will start from the current selection. + */ + void resetIncrementalBaseLocation(); + } \ No newline at end of file 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 b7d737c403a..9997366ccc9 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 @@ -606,11 +606,19 @@ private void createSearchBar() { @Override public void focusGained(FocusEvent e) { - // we want to update the base-location of where we start incremental search - // to the currently selected position in the target - // when coming back into the dialog - findReplaceLogic.deactivate(SearchOptions.INCREMENTAL); + findReplaceLogic.resetIncrementalBaseLocation(); findReplaceLogic.activate(SearchOptions.INCREMENTAL); + if (getFindString().isEmpty()) { + return; + } + updateSelectionWithIncrementalSearch(); + } + + private void updateSelectionWithIncrementalSearch() { + boolean foundSearchString = findReplaceLogic.performSearch(getFindString()); + if (foundSearchString) { + searchBar.setSelection(0, getFindString().length()); + } } @Override @@ -629,7 +637,7 @@ private void updateIncrementalSearch() { if (findReplaceLogic.getTarget() instanceof IFindReplaceTargetExtension targetExtension) { targetExtension.setSelection(targetExtension.getLineSelection().x, 0); } - findReplaceLogic.performIncrementalSearch(getFindString()); + findReplaceLogic.performSearch(getFindString()); evaluateFindReplaceStatus(); } @@ -855,8 +863,8 @@ private void performSearch(boolean forward) { activateInFindReplacerIf(SearchOptions.FORWARD, forward); findReplaceLogic.deactivate(SearchOptions.INCREMENTAL); findReplaceLogic.performSearch(getFindString()); - activateInFindReplacerIf(SearchOptions.FORWARD, oldForwardSearchSetting); findReplaceLogic.activate(SearchOptions.INCREMENTAL); + activateInFindReplacerIf(SearchOptions.FORWARD, oldForwardSearchSetting); } private void updateFromTargetSelection() { diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayFirstTimePopup.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayFirstTimePopup.java index 5b8a924b9a9..f2be579e9ac 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayFirstTimePopup.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayFirstTimePopup.java @@ -96,7 +96,6 @@ public static void displayPopupIfNotAlreadyShown(Shell shellToUse) { true) .delay(POPUP_VANISH_TIME.toMillis()).open(); } - } private static Control createFirstTimeNotification(Composite composite) { diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java index 442f7f558d3..e7ec0f682f2 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java @@ -142,7 +142,9 @@ public void modifyText(ModifyEvent e) { return; } - findReplaceLogic.performIncrementalSearch(getFindString()); + if (findReplaceLogic.isActive(SearchOptions.INCREMENTAL)) { + findReplaceLogic.performSearch(getFindString()); + } evaluateFindReplaceStatus(); updateButtonState(!findReplaceLogic.isActive(SearchOptions.INCREMENTAL)); @@ -292,9 +294,12 @@ public void widgetSelected(SelectionEvent e) { setupFindReplaceLogic(); boolean eventRequiresInverseSearchDirection = (e.stateMask & SWT.MODIFIER_MASK) == SWT.SHIFT; boolean forwardSearchActivated = findReplaceLogic.isActive(SearchOptions.FORWARD); + boolean incrementalSearchActivated = findReplaceLogic.isActive(SearchOptions.INCREMENTAL); activateInFindReplaceLogicIf(SearchOptions.FORWARD, eventRequiresInverseSearchDirection != forwardSearchActivated); + findReplaceLogic.deactivate(SearchOptions.INCREMENTAL); boolean somethingFound = findReplaceLogic.performSearch(getFindString()); + activateInFindReplaceLogicIf(SearchOptions.INCREMENTAL, incrementalSearchActivated); activateInFindReplaceLogicIf(SearchOptions.FORWARD, forwardSearchActivated); writeSelection(); 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 7cf83a10ac6..78b7c501a57 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 @@ -662,6 +662,58 @@ private void expectStatusEmpty(IFindReplaceLogic findReplaceLogic) { assertThat(findReplaceLogic.getStatus(), instanceOf(NoStatus.class)); } + /** + * Test for https://github.com/eclipse-platform/eclipse.platform.ui/issues/1914 + */ + @Test + public void testActivateRegexInIncrementalIncrementalBaseLocationUpdated() { + TextViewer textViewer= setupTextViewer("Test Test Test Test"); + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer); + findReplaceLogic.activate(SearchOptions.INCREMENTAL); + findReplaceLogic.activate(SearchOptions.REGEX); + findReplaceLogic.activate(SearchOptions.FORWARD); + + findReplaceLogic.deactivate(SearchOptions.INCREMENTAL); + findReplaceLogic.performSearch("Test"); + findReplaceLogic.activate(SearchOptions.INCREMENTAL); + + assertThat(findReplaceLogic.getTarget().getSelection().x, is(0)); + assertThat(findReplaceLogic.getTarget().getSelection().y, is(4)); + + findReplaceLogic.deactivate(SearchOptions.INCREMENTAL); + findReplaceLogic.performSearch("Test"); + findReplaceLogic.activate(SearchOptions.INCREMENTAL); + + assertThat(findReplaceLogic.getTarget().getSelection().x, is(5)); + assertThat(findReplaceLogic.getTarget().getSelection().y, is(4)); + + findReplaceLogic.deactivate(SearchOptions.REGEX); + findReplaceLogic.performSearch("Test"); + + assertThat(findReplaceLogic.getTarget().getSelection().x, is(10)); + assertThat(findReplaceLogic.getTarget().getSelection().y, is(4)); + } + + @Test + public void testPerformIncrementalSearch() { + TextViewer textViewer= setupTextViewer("Test Test Test Test"); + IFindReplaceLogic findReplaceLogic= setupFindReplaceLogicObject(textViewer); + findReplaceLogic.activate(SearchOptions.INCREMENTAL); + findReplaceLogic.activate(SearchOptions.FORWARD); + + findReplaceLogic.performSearch("Test"); + assertThat(findReplaceLogic.getTarget().getSelection().x, is(0)); + assertThat(findReplaceLogic.getTarget().getSelection().y, is(4)); + + findReplaceLogic.performSearch("Test"); // incremental search is idempotent + assertThat(findReplaceLogic.getTarget().getSelection().x, is(0)); + assertThat(findReplaceLogic.getTarget().getSelection().y, is(4)); + + findReplaceLogic.performSearch(""); // this clears the incremental search, but the "old search" still remains active + assertThat(findReplaceLogic.getTarget().getSelection().x, is(0)); + assertThat(findReplaceLogic.getTarget().getSelection().y, is(4)); + } + private void expectStatusIsCode(IFindReplaceLogic findReplaceLogic, FindStatus.StatusCode code) { assertThat(findReplaceLogic.getStatus(), instanceOf(FindStatus.class)); assertThat(((FindStatus) findReplaceLogic.getStatus()).getMessageCode(), equalTo(code)); diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java index 3eadef64ef3..e317ec0255c 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java @@ -154,13 +154,17 @@ public void testSearchBackwardsWithRegEx() { dialog.select(SearchOptions.REGEX); dialog.setFindText("text"); // with RegEx enabled, there is no incremental search! dialog.pressSearch(true); + assertThat(dialog.getTarget().getSelection().x, is(0)); assertThat(dialog.getTarget().getSelection().y, is(4)); dialog.pressSearch(true); assertThat(dialog.getTarget().getSelection().x, is("text ".length())); + assertThat(dialog.getTarget().getSelection().y, is(4)); dialog.pressSearch(true); assertThat(dialog.getTarget().getSelection().x, is("text text ".length())); + assertThat(dialog.getTarget().getSelection().y, is(4)); dialog.pressSearch(false); assertThat(dialog.getTarget().getSelection().x, is("text ".length())); + assertThat(dialog.getTarget().getSelection().y, is(4)); } } diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/workbench/texteditor/tests/FindReplaceDialogTest.java b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/workbench/texteditor/tests/FindReplaceDialogTest.java index 03c9d04166b..9355f0bca05 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/workbench/texteditor/tests/FindReplaceDialogTest.java +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/workbench/texteditor/tests/FindReplaceDialogTest.java @@ -231,4 +231,19 @@ public void testFindWithWholeWordEnabledWithMultipleWordsNotIncremental() { assertEquals(0, (target.getSelection()).x); assertEquals(dialog.getFindText().length(), (target.getSelection()).y); } + + @Test + public void testFindNextDespiteIncrementalEnabled() { + initializeTextViewerWithFindReplaceUI("test test test test"); + DialogAccess dialog= getDialog(); + dialog.select(SearchOptions.INCREMENTAL); + + dialog.setFindText("test"); + assertThat(dialog.getTarget().getSelection().x, is(0)); + assertThat(dialog.getTarget().getSelection().y, is(4)); + + dialog.simulateEnterInFindInputField(false); + assertThat(dialog.getTarget().getSelection().x, is(5)); + assertThat(dialog.getTarget().getSelection().y, is(4)); + } }