Skip to content

Commit

Permalink
Find/Replace overlay: consistent handling of incremental base location
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@Wittmaxi @HeikoKlare
Wittmaxi authored and HeikoKlare committed Jul 5, 2024
1 parent ecfde49 commit c087338
Show file tree
Hide file tree
Showing 8 changed files with 122 additions and 71 deletions.
67 changes: 20 additions & 47 deletions ....ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogic.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ public void activate(SearchOptions searchOption) {
break;
case FORWARD:
case INCREMENTAL:
if (shouldInitIncrementalBaseLocation()) {
initIncrementalBaseLocation();
if (shouldResetIncrementalBaseLocation()) {
resetIncrementalBaseLocation();
}
break;
// $CASES-OMITTED$
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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);
}

Expand All @@ -158,8 +163,8 @@ public boolean shouldInitIncrementalBaseLocation() {
* selected lines.
*/
private void initializeSearchScope() {
if (shouldInitIncrementalBaseLocation()) {
initIncrementalBaseLocation();
if (shouldResetIncrementalBaseLocation()) {
resetIncrementalBaseLocation();
}

if (target == null || !(target instanceof IFindReplaceTargetExtension)) {
Expand Down Expand Up @@ -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) {
Expand All @@ -359,6 +352,7 @@ private boolean performSearch(boolean mustInitIncrementalBaseLocation, String fi
// we don't keep state in this dialog
}
}

return somethingFound;
}

Expand All @@ -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
*
*/
Expand Down Expand Up @@ -604,7 +597,7 @@ public void updateTarget(IFindReplaceTarget newTarget, boolean canEditTarget) {
}
}

initIncrementalBaseLocation();
resetIncrementalBaseLocation();
}

@Override
Expand Down Expand Up @@ -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;
Expand Down
27 changes: 11 additions & 16 deletions ...ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/IFindReplaceLogic.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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();

}
20 changes: 14 additions & 6 deletions ...nch.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
}

Expand Down Expand Up @@ -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() {
Expand Down
1 change: 0 additions & 1 deletion .../src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayFirstTimePopup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ public static void displayPopupIfNotAlreadyShown(Shell shellToUse) {
true)
.delay(POPUP_VANISH_TIME.toMillis()).open();
}

}

private static Control createFirstTimeNotification(Composite composite) {
Expand Down
7 changes: 6 additions & 1 deletion .../org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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();
Expand Down
52 changes: 52 additions & 0 deletions ...nch.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/FindReplaceLogicTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
4 changes: 4 additions & 0 deletions ...itor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

}
15 changes: 15 additions & 0 deletions ...texteditor.tests/src/org/eclipse/ui/workbench/texteditor/tests/FindReplaceDialogTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

0 comments on commit c087338

Please sign in to comment.