From 2524500dc1986d49ffb60c4e753d880ffeafa1b0 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 6 Jan 2025 15:13:52 +0100 Subject: [PATCH] FindReplaceOverlay: improve error indication for find and replace inputs The find and replace input strings of the FindReplaceOverlay are temporarily colored red to indicate failures of find or replace operations, such as no matches being found for the input string or regular expressions being invalid. The behavior is currently inconsistent and partly unexpected. For example: - The field to be colored is selected by the one having focus rather than the one conforming to the current operation; so a failing replace operation will color the search input field if that one has focus - Whenever the replace input field shall be colored, the search input field is also colored - When a find operation following a successful replace fails (because there are not more matches), the replace field is erroneously colored With this change, the error feedback via coloring the find and replace input strings is streamlined based on the two following goals: - Only one of the input fields is colored at one point in time - Which of the fields is colored depends on whether a find or a replace operation failed - The coloring is cleared iff either any of the two input fields loses focus or another find/replace operation was performed --- .../overlay/FindReplaceOverlay.java | 73 ++++++++++--------- 1 file changed, 39 insertions(+), 34 deletions(-) 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 ff9e0ccbcc2..aa728bf0626 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 @@ -72,7 +72,6 @@ import org.eclipse.ui.internal.findandreplace.FindReplaceMessages; import org.eclipse.ui.internal.findandreplace.HistoryStore; import org.eclipse.ui.internal.findandreplace.SearchOptions; -import org.eclipse.ui.internal.findandreplace.status.IFindReplaceStatus; import org.eclipse.ui.internal.texteditor.TextEditorPlugin; import org.eclipse.ui.part.MultiPageEditorSite; @@ -149,6 +148,8 @@ private final class KeyboardShortcuts { private Color widgetBackgroundColor; private Color overlayBackgroundColor; private Color normalTextForegroundColor; + private Color errorTextForegroundColor; + private boolean positionAtTop = true; private ControlDecoration searchBarDecoration; private ContentAssistCommandAdapter contentAssistSearchField, contentAssistReplaceField; @@ -336,7 +337,7 @@ public Composite getContainerControl() { private void performReplaceAll() { BusyIndicator.showWhile(containerControl.getShell() != null ? containerControl.getShell().getDisplay() : Display.getCurrent(), findReplaceLogic::performReplaceAll); - evaluateFindReplaceStatus(); + evaluateStatusAfterReplace(); replaceBar.storeHistory(); searchBar.storeHistory(); } @@ -460,7 +461,7 @@ private void createContainerAndSearchControls(Composite parent) { if (insertedInTargetParent()) { parent = parent.getParent(); } - retrieveBackgroundColor(); + retrieveColors(); createMainContainer(parent); initializeSearchShortcutHandlers(); @@ -479,7 +480,7 @@ private void initializeSearchShortcutHandlers() { * would otherwise inherit non-fitting custom colors from the containing * StyledText. */ - private void retrieveBackgroundColor() { + private void retrieveColors() { if (targetPart instanceof StatusTextEditor textEditor) { Control targetWidget = textEditor.getAdapter(ITextViewer.class).getTextWidget(); widgetBackgroundColor = targetWidget.getBackground(); @@ -492,6 +493,7 @@ private void retrieveBackgroundColor() { textBarForRetrievingTheRightColor.dispose(); } overlayBackgroundColor = retrieveDefaultCompositeBackground(); + errorTextForegroundColor = JFaceColors.getErrorText(targetControl.getShell().getDisplay()); } private Color retrieveDefaultCompositeBackground() { @@ -652,8 +654,6 @@ private void createWholeWordsButton() { } private void createReplaceTools() { - Color warningColor = JFaceColors.getErrorText(containerControl.getShell().getDisplay()); - replaceTools = new AccessibleToolBar(replaceContainer); replaceTools.createToolItem(SWT.SEPARATOR); @@ -664,7 +664,7 @@ private void createReplaceTools() { .withToolTipText(FindReplaceMessages.FindReplaceOverlay_replaceButton_toolTip) .withOperation(() -> { if (getFindString().isEmpty()) { - showUserFeedback(warningColor, true); + applyErrorColor(replaceBar); return; } performSingleReplace(); @@ -675,7 +675,7 @@ private void createReplaceTools() { .withToolTipText(FindReplaceMessages.FindReplaceOverlay_replaceAllButton_toolTip) .withOperation(() -> { if (getFindString().isEmpty()) { - showUserFeedback(warningColor, true); + applyErrorColor(replaceBar); return; } performReplaceAll(); @@ -703,9 +703,8 @@ private void createSearchBar() { searchBar.selectAll(); searchBar.addModifyListener(e -> { wholeWordSearchButton.setEnabled(findReplaceLogic.isAvailable(SearchOptions.WHOLE_WORD)); - - showUserFeedback(normalTextForegroundColor, true); updateIncrementalSearch(); + decorate(); }); searchBar.addFocusListener(new FocusListener() { @Override @@ -714,21 +713,18 @@ public void focusGained(FocusEvent e) { } @Override public void focusLost(FocusEvent e) { - showUserFeedback(normalTextForegroundColor, false); + resetErrorColoring(); } }); searchBar.addFocusListener(targetActionActivationHandling); searchBar.setMessage(FindReplaceMessages.FindReplaceOverlay_searchBar_message); contentAssistSearchField = createContentAssistField(searchBar, true); - searchBar.addModifyListener(Event -> { - decorate(); - }); searchBar.setTabList(null); } private void updateIncrementalSearch() { findReplaceLogic.setFindString(searchBar.getText()); - evaluateFindReplaceStatus(); + evaluateStatusAfterFind(); } private void createReplaceBar() { @@ -742,12 +738,10 @@ private void createReplaceBar() { replaceBar.setMessage(FindReplaceMessages.FindReplaceOverlay_replaceBar_message); replaceBar.addModifyListener(e -> { findReplaceLogic.setReplaceString(replaceBar.getText()); + resetErrorColoring(); }); replaceBar.addFocusListener(targetActionActivationHandling); - replaceBar.addFocusListener(FocusListener.focusLostAdapter(e -> { - replaceBar.setForeground(normalTextForegroundColor); - searchBar.setForeground(normalTextForegroundColor); - })); + replaceBar.addFocusListener(FocusListener.focusLostAdapter(e -> resetErrorColoring())); contentAssistReplaceField = createContentAssistField(replaceBar, false); } @@ -978,8 +972,13 @@ private String getFindString() { } private void performSingleReplace() { - findReplaceLogic.performReplaceAndFind(); - evaluateFindReplaceStatus(); + if (findReplaceLogic.performSelectAndReplace()) { + findReplaceLogic.performSearch(); + evaluateStatusAfterFind(); + } else { + evaluateStatusAfterReplace(); + } + replaceBar.storeHistory(); searchBar.storeHistory(); } @@ -989,7 +988,7 @@ private void performSearch(boolean forward) { activateInFindReplacerIf(SearchOptions.FORWARD, forward); findReplaceLogic.performSearch(); activateInFindReplacerIf(SearchOptions.FORWARD, oldForwardSearchSetting); - evaluateFindReplaceStatus(); + evaluateStatusAfterFind(); searchBar.storeHistory(); } @@ -1008,22 +1007,28 @@ private void updateFromTargetSelection() { searchBar.setSelection(0, searchBar.getText().length()); } - private void evaluateFindReplaceStatus() { - Color warningColor = JFaceColors.getErrorText(containerControl.getShell().getDisplay()); - IFindReplaceStatus status = findReplaceLogic.getStatus(); + private void evaluateStatusAfterFind() { + resetErrorColoring(); + if (!findReplaceLogic.getStatus().wasSuccessful()) { + applyErrorColor(searchBar); + } + } - if (!status.wasSuccessful()) { - boolean colorReplaceBar = okayToUse(replaceBar) && replaceBar.isFocusControl(); - showUserFeedback(warningColor, colorReplaceBar); - } else { - showUserFeedback(normalTextForegroundColor, false); + private void evaluateStatusAfterReplace() { + resetErrorColoring(); + if (!findReplaceLogic.getStatus().wasSuccessful()) { + applyErrorColor(replaceBar); } } - private void showUserFeedback(Color feedbackColor, boolean colorReplaceBar) { - searchBar.setForeground(feedbackColor); - if (colorReplaceBar && okayToUse(replaceBar)) { - replaceBar.setForeground(feedbackColor); + private void applyErrorColor(HistoryTextWrapper inputField) { + inputField.setForeground(errorTextForegroundColor); + } + + private void resetErrorColoring() { + searchBar.setForeground(normalTextForegroundColor); + if (okayToUse(replaceBar)) { + replaceBar.setForeground(normalTextForegroundColor); } }