Skip to content

Commit

Permalink
Correct incremental and inverted search behavior in FindReplaceDialog
Browse files Browse the repository at this point in the history 
* Makes incremental search consider the current selection if still
matching instead of moving to the next match
* Makes Shift+Enter invert search direction even if backwards search is
enabled and adds regression test for that behavior
* Avoids that search for whole words stays enabled when the checkbox
becomes disabled and adds a regression test for that behavior
* Improves naming of IStatus#isError() to IStatus#isInputValid()
* Renames identifiers containing "FindReplacer" to "FindReplaceLogic"
  • Loading branch information
@HeikoKlare @vogella
HeikoKlare authored and vogella committed Jan 15, 2024
1 parent 5498b3e commit 4a2074e
Show file tree
Hide file tree
Showing 10 changed files with 133 additions and 94 deletions.
21 changes: 13 additions & 8 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 @@ -58,7 +58,9 @@ public class FindReplaceLogic implements IFindReplaceLogic {

@Override
public void activate(SearchOptions searchOption) {
searchOptions.add(searchOption);
if (!searchOptions.add(searchOption)) {
return;
}

switch (searchOption) {
case GLOBAL:
Expand All @@ -78,7 +80,10 @@ public void activate(SearchOptions searchOption) {

@Override
public void deactivate(SearchOptions searchOption) {
searchOptions.remove(searchOption);
if (!searchOptions.remove(searchOption)) {
return;
}

if (searchOption == SearchOptions.GLOBAL) {
useSelectedLines(true);
}
Expand Down Expand Up @@ -241,7 +246,7 @@ public void run() {
String msg = NLSUtility.format(FindReplaceMessages.FindReplace_Status_noMatchWithValue_label,
findString);
statusLineMessage(false, msg);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH, false);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH);
}
} catch (PatternSyntaxException ex) {
status = new InvalidRegExStatus(ex);
Expand Down Expand Up @@ -286,7 +291,7 @@ public void run() {
String msg = NLSUtility.format(FindReplaceMessages.FindReplace_Status_noMatchWithValue_label,
findString);
statusLineMessage(false, msg);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH, false);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH);
}
} catch (PatternSyntaxException ex) {
status = new InvalidRegExStatus(ex);
Expand All @@ -310,7 +315,7 @@ private boolean prepareTargetForEditing() {
if (target instanceof IFindReplaceTargetExtension2) {
IFindReplaceTargetExtension2 extension = (IFindReplaceTargetExtension2) target;
if (!extension.validateTargetState()) {
status = new FindStatus(FindStatus.StatusCode.READONLY, true);
status = new FindStatus(FindStatus.StatusCode.READONLY);
return false;
}
}
Expand Down Expand Up @@ -490,10 +495,10 @@ private int findIndex(String findString, int startPosition) {

if (isActive(SearchOptions.WRAP)) {
statusLineMessage(FindReplaceMessages.FindReplace_Status_wrapped_label);
status = new FindStatus(FindStatus.StatusCode.WRAPPED, false);
status = new FindStatus(FindStatus.StatusCode.WRAPPED);
index = findAndSelect(-1, findString);
} else {
status = new FindStatus(FindStatus.StatusCode.NO_MATCH, false);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH);
}
}
return index;
Expand Down Expand Up @@ -564,7 +569,7 @@ private boolean findNext(String findString, boolean forwardSearch) {
if (index == -1) {
String msg = NLSUtility.format(FindReplaceMessages.FindReplace_Status_noMatchWithValue_label, findString);
statusLineMessage(false, msg);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH, false);
status = new FindStatus(FindStatus.StatusCode.NO_MATCH);
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions ...workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/status/FindAllStatus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ public <T> T accept(IStatusVisitor<T> visitor) {
}

@Override
public boolean isError() {
return false;
public boolean isInputValid() {
return true;
}

}
8 changes: 3 additions & 5 deletions ...ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/status/FindStatus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,9 @@ public enum StatusCode {
}

private StatusCode messageCode;
private boolean isError;

public FindStatus(StatusCode errorCode, boolean isError) {
public FindStatus(StatusCode errorCode) {
this.messageCode = errorCode;
this.isError = isError;
}

public StatusCode getMessageCode() {
Expand All @@ -38,8 +36,8 @@ public <T> T accept(IStatusVisitor<T> visitor) {
}

@Override
public boolean isError() {
return this.isError;
public boolean isInputValid() {
return messageCode != StatusCode.READONLY;
}

}
7 changes: 3 additions & 4 deletions ...se.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/status/IStatus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ public interface IStatus {
public <T> T accept(IStatusVisitor<T> visitor);

/**
* Whether the status represents an Error in the Find/Replace-Operation.
*
* @return the error state.
* {@return whether the input is valid, e.g., that the find string is valid and
* that the target is writable on replace operations}
*/
public boolean isError();
public boolean isInputValid();
}
4 changes: 2 additions & 2 deletions ...ench.texteditor/src/org/eclipse/ui/internal/findandreplace/status/InvalidRegExStatus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ public <T> T accept(IStatusVisitor<T> visitor) {
}

@Override
public boolean isError() {
return true;
public boolean isInputValid() {
return false;
}

}
4 changes: 2 additions & 2 deletions ...e.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/status/NoStatus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ public <T> T accept(IStatusVisitor<T> visitor) {
}

@Override
public boolean isError() {
return false;
public boolean isInputValid() {
return true;
}

}
4 changes: 2 additions & 2 deletions ...kbench.texteditor/src/org/eclipse/ui/internal/findandreplace/status/ReplaceAllStatus.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ public <T> T accept(IStatusVisitor<T> visitor) {
}

@Override
public boolean isError() {
return false;
public boolean isInputValid() {
return true;
}

}
72 changes: 37 additions & 35 deletions .../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 @@ -145,7 +145,7 @@ public void modifyText(ModifyEvent e) {
}

findReplaceLogic.performIncrementalSearch(getFindString());
evaluateFindReplacerStatus(false);
evaluateFindReplaceStatus(false);

updateButtonState(!findReplaceLogic.isActive(SearchOptions.INCREMENTAL));
}
Expand Down Expand Up @@ -290,18 +290,18 @@ private Composite createButtonSection(Composite parent) {
new SelectionAdapter() {
@Override
public void widgetSelected(SelectionEvent e) {
setupFindReplacer();
boolean eventRequiresBackwardSearch = (e.stateMask & SWT.MODIFIER_MASK) == SWT.SHIFT;
boolean oldSearchDirection = findReplaceLogic.isActive(SearchOptions.FORWARD);
activateInFindReplacerIf(SearchOptions.FORWARD,
!eventRequiresBackwardSearch && oldSearchDirection);
setupFindReplaceLogic();
boolean eventRequiresInverseSearchDirection = (e.stateMask & SWT.MODIFIER_MASK) == SWT.SHIFT;
boolean forwardSearchActivated = findReplaceLogic.isActive(SearchOptions.FORWARD);
activateInFindReplaceLogicIf(SearchOptions.FORWARD,
eventRequiresInverseSearchDirection != forwardSearchActivated);
boolean somethingFound = findReplaceLogic.performSearch(getFindString());
activateInFindReplacerIf(SearchOptions.FORWARD, oldSearchDirection);
activateInFindReplaceLogicIf(SearchOptions.FORWARD, forwardSearchActivated);

writeSelection();
updateButtonState(!somethingFound);
updateFindHistory();
evaluateFindReplacerStatus();
evaluateFindReplaceStatus();
}
});
setGridData(fFindNextButton, SWT.FILL, true, SWT.FILL, false);
Expand All @@ -314,7 +314,7 @@ public void widgetSelected(SelectionEvent e) {
writeSelection();
updateButtonState();
updateFindAndReplaceHistory();
evaluateFindReplacerStatus();
evaluateFindReplaceStatus();
}
});
setGridData(fSelectAllButton, SWT.FILL, true, SWT.FILL, false);
Expand All @@ -330,7 +330,7 @@ public void widgetSelected(SelectionEvent e) {
}
updateButtonState();
updateFindAndReplaceHistory();
evaluateFindReplacerStatus();
evaluateFindReplaceStatus();
}
});
setGridData(fReplaceFindButton, SWT.FILL, false, SWT.FILL, false);
Expand All @@ -344,7 +344,7 @@ public void widgetSelected(SelectionEvent e) {
}
updateButtonState();
updateFindAndReplaceHistory();
evaluateFindReplacerStatus();
evaluateFindReplaceStatus();
}
});
setGridData(fReplaceSelectionButton, SWT.FILL, false, SWT.FILL, false);
Expand All @@ -358,7 +358,7 @@ public void widgetSelected(SelectionEvent e) {
writeSelection();
updateButtonState();
updateFindAndReplaceHistory();
evaluateFindReplacerStatus();
evaluateFindReplaceStatus();
}
});
setGridData(fReplaceAllButton, SWT.FILL, true, SWT.FILL, false);
Expand Down Expand Up @@ -506,7 +506,7 @@ private Composite createDirectionGroup(Composite parent) {
SelectionListener selectionListener = new SelectionListener() {
@Override
public void widgetSelected(SelectionEvent e) {
activateInFindReplacerIf(SearchOptions.FORWARD, fForwardRadioButton.getSelection());
activateInFindReplaceLogicIf(SearchOptions.FORWARD, fForwardRadioButton.getSelection());
}

@Override
Expand All @@ -528,7 +528,7 @@ public void widgetDefaultSelected(SelectionEvent e) {
backwardRadioButton.addSelectionListener(selectionListener);
storeButtonWithMnemonicInMap(backwardRadioButton);

activateInFindReplacerIf(SearchOptions.FORWARD, true); // search forward by default
activateInFindReplaceLogicIf(SearchOptions.FORWARD, true); // search forward by default
backwardRadioButton.setSelection(!findReplaceLogic.isActive(SearchOptions.FORWARD));
fForwardRadioButton.setSelection(findReplaceLogic.isActive(SearchOptions.FORWARD));

Expand Down Expand Up @@ -671,7 +671,7 @@ private Composite createOptionsGroup(Composite parent) {
SelectionListener selectionListener = new SelectionListener() {
@Override
public void widgetSelected(SelectionEvent e) {
setupFindReplacer();
setupFindReplaceLogic();
storeSettings();
}

Expand Down Expand Up @@ -708,7 +708,7 @@ public void widgetDefaultSelected(SelectionEvent e) {
fIncrementalCheckBox.addSelectionListener(new SelectionListener() {
@Override
public void widgetSelected(SelectionEvent e) {
setupFindReplacer();
setupFindReplaceLogic();
storeSettings();
}

Expand All @@ -728,7 +728,7 @@ public void widgetDefaultSelected(SelectionEvent e) {
public void widgetSelected(SelectionEvent e) {
boolean newState = fIsRegExCheckBox.getSelection();
fIncrementalCheckBox.setEnabled(!newState);
setupFindReplacer();
setupFindReplaceLogic();
storeSettings();
updateButtonState();
setContentAssistsEnablement(newState);
Expand Down Expand Up @@ -1019,7 +1019,7 @@ private void updateButtonState() {
* @since 3.0
*/
private void updateButtonState(boolean disableReplace) {
setupFindReplacer();
setupFindReplaceLogic();
if (okToUse(getShell()) && okToUse(fFindNextButton)) {

boolean hasActiveSelection = false;
Expand Down Expand Up @@ -1243,21 +1243,23 @@ protected int getDialogBoundsStrategy() {
private void readConfiguration() {
IDialogSettings s = getDialogSettings();

activateInFindReplacerIf(SearchOptions.WRAP, s.get("wrap") == null || s.getBoolean("wrap")); //$NON-NLS-1$ //$NON-NLS-2$
activateInFindReplacerIf(SearchOptions.CASE_SENSITIVE, s.getBoolean("casesensitive")); //$NON-NLS-1$
activateInFindReplacerIf(SearchOptions.WHOLE_WORD, s.getBoolean("wholeword")); //$NON-NLS-1$
activateInFindReplacerIf(SearchOptions.INCREMENTAL, s.getBoolean("incremental")); //$NON-NLS-1$
activateInFindReplacerIf(SearchOptions.REGEX, s.getBoolean("isRegEx")); //$NON-NLS-1$
activateInFindReplaceLogicIf(SearchOptions.WRAP, s.get("wrap") == null || s.getBoolean("wrap")); //$NON-NLS-1$ //$NON-NLS-2$
activateInFindReplaceLogicIf(SearchOptions.CASE_SENSITIVE, s.getBoolean("casesensitive")); //$NON-NLS-1$
activateInFindReplaceLogicIf(SearchOptions.WHOLE_WORD, s.getBoolean("wholeword")); //$NON-NLS-1$
activateInFindReplaceLogicIf(SearchOptions.INCREMENTAL, s.getBoolean("incremental")); //$NON-NLS-1$
activateInFindReplaceLogicIf(SearchOptions.REGEX, s.getBoolean("isRegEx")); //$NON-NLS-1$

}

private void setupFindReplacer() {
activateInFindReplacerIf(SearchOptions.WRAP, fWrapCheckBox.getSelection());
activateInFindReplacerIf(SearchOptions.FORWARD, fForwardRadioButton.getSelection());
activateInFindReplacerIf(SearchOptions.CASE_SENSITIVE, fCaseCheckBox.getSelection());
activateInFindReplacerIf(SearchOptions.WHOLE_WORD, fWholeWordCheckBox.getSelection());
activateInFindReplacerIf(SearchOptions.INCREMENTAL, fIncrementalCheckBox.getSelection());
activateInFindReplacerIf(SearchOptions.REGEX, fIsRegExCheckBox.getSelection());
private void setupFindReplaceLogic() {
activateInFindReplaceLogicIf(SearchOptions.WRAP, fWrapCheckBox.getSelection());
activateInFindReplaceLogicIf(SearchOptions.FORWARD, fForwardRadioButton.getSelection());
activateInFindReplaceLogicIf(SearchOptions.CASE_SENSITIVE, fCaseCheckBox.getSelection());
activateInFindReplaceLogicIf(SearchOptions.REGEX, fIsRegExCheckBox.getSelection());
activateInFindReplaceLogicIf(SearchOptions.WHOLE_WORD,
fWholeWordCheckBox.getEnabled() && fWholeWordCheckBox.getSelection());
activateInFindReplaceLogicIf(SearchOptions.INCREMENTAL,
fIncrementalCheckBox.getEnabled() && fIncrementalCheckBox.getSelection());
}

/**
Expand All @@ -1279,30 +1281,30 @@ private void writeConfiguration() {
replaceHistory.add(replaceString);
}

private void activateInFindReplacerIf(SearchOptions option, boolean shouldActivate) {
private void activateInFindReplaceLogicIf(SearchOptions option, boolean shouldActivate) {
if (shouldActivate) {
findReplaceLogic.activate(option);
} else {
findReplaceLogic.deactivate(option);
}
}

private void evaluateFindReplacerStatus() {
evaluateFindReplacerStatus(true);
private void evaluateFindReplaceStatus() {
evaluateFindReplaceStatus(true);
}

/**
* Evaluate the status of the FindReplaceLogic object.
*
* @param allowBeep Whether the evaluation should beep on some codes.
*/
private void evaluateFindReplacerStatus(boolean allowBeep) {
private void evaluateFindReplaceStatus(boolean allowBeep) {
IStatus status = findReplaceLogic.getStatus();

String dialogMessage = status.accept(new FindReplaceLogicMessageGenerator());
fStatusLabel.setText(dialogMessage);
fStatusLabel.setForeground(null);
if (status.isError()) {
if (!status.isInputValid()) {
fStatusLabel.setForeground(JFaceColors.getErrorText(fStatusLabel.getDisplay()));
}

Expand Down
4 changes: 0 additions & 4 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 @@ -97,7 +97,6 @@ public void testPerformReplaceAllForwards() {
* @param textViewer textviewer-object that contains the contents on which findReplaceLogic
* operates
*/
@SuppressWarnings("boxing")
private void performReplaceAllBaseTestcases(IFindReplaceLogic findReplaceLogic, TextViewer textViewer) {
Display display= parentShell.getDisplay();
textViewer.setDocument(new Document("aaaa"));
Expand Down Expand Up @@ -215,7 +214,6 @@ public void testPerformSelectAndReplaceBackward() {
}


@SuppressWarnings("boxing")
@Test
public void testPerformReplaceAndFind() {
TextViewer textViewer= setupTextViewer("Hello<replace>World<replace>!");
Expand Down Expand Up @@ -293,13 +291,11 @@ private void expectStatusIsCode(IFindReplaceLogic findReplaceLogic, FindStatus.S
assertThat(((FindStatus) findReplaceLogic.getStatus()).getMessageCode(), equalTo(code));
}

@SuppressWarnings("boxing")
private void expectStatusIsReplaceAllWithCount(IFindReplaceLogic findReplaceLogic, int count) {
assertThat(findReplaceLogic.getStatus(), instanceOf(ReplaceAllStatus.class));
assertThat(((ReplaceAllStatus) findReplaceLogic.getStatus()).getReplaceCount(), equalTo(count));
}

@SuppressWarnings("boxing")
private void expectStatusIsFindAllWithCount(IFindReplaceLogic findReplaceLogic, int count) {
assertThat(findReplaceLogic.getStatus(), instanceOf(FindAllStatus.class));
assertThat(((FindAllStatus) findReplaceLogic.getStatus()).getSelectCount(), equalTo(count));
Expand Down
Loading

0 comments on commit 4a2074e

Please sign in to comment.