Find/Replace overlay: consistent handling of incremental base location (#1913 #1914)

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,15 +145,16 @@ 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 {
 			incrementalBaseLocation = new Point(0, 0);
 		}
 	}
 
-	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 <code>true</code> 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;

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 <code>setIncrementalSearch(true)</code>. 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();
+
 }

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() {

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) {

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

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

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

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