+ * The preference value is of type Boolean
+ *
@@ -784,6 +796,9 @@ private AbstractDecoratedTextEditorPreferenceConstants() { public static void initializeDefaultValues(IPreferenceStore store) { store.setDefault(AbstractDecoratedTextEditorPreferenceConstants.EDITOR_USE_FIND_REPLACE_OVERLAY, true); store.setDefault(AbstractDecoratedTextEditorPreferenceConstants.EDITOR_FIND_REPLACE_OVERLAY_AT_BOTTOM, false); + store.setDefault( + AbstractDecoratedTextEditorPreferenceConstants.EDITOR_RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS, + false); store.setDefault(AbstractDecoratedTextEditorPreferenceConstants.USE_ANNOTATIONS_PREFERENCE_PAGE, false); store.setDefault(AbstractDecoratedTextEditorPreferenceConstants.USE_QUICK_DIFF_PREFERENCE_PAGE, false); 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 017a4664f9f..93773160202 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 @@ -40,6 +40,8 @@ import org.eclipse.swt.widgets.ToolItem; import org.eclipse.swt.widgets.Widget; +import org.eclipse.core.runtime.Assert; + import org.eclipse.jface.dialogs.IDialogSettings; import org.eclipse.jface.fieldassist.ControlDecoration; import org.eclipse.jface.fieldassist.TextContentAdapter; @@ -201,12 +203,17 @@ void install() { } public FindReplaceOverlay(IWorkbenchPart part, IFindReplaceTarget target) { + Assert.isLegal(canOpenInPart(part), "the overlay cannot be opened in the given part"); //$NON-NLS-1$ targetPart = part; commandSupport = new FindReplaceOverlayCommandSupport(targetPart); targetControl = getTargetControl(part); findReplaceLogic = createFindReplaceLogic(target); createContainerAndSearchControls(); containerControl.addDisposeListener(__ -> commandSupport.dispose()); + // The overlay is a sibling of the target control, and in parts whose parent + // composite outlives that control it would otherwise stay behind as a widget + // nobody disposes, holding listeners on a disposed target. + targetControl.addDisposeListener(__ -> containerControl.dispose()); customFocusOrder.install(); updateReplaceVisibility(false); containerControl.setVisible(false); @@ -214,6 +221,20 @@ public FindReplaceOverlay(IWorkbenchPart part, IFindReplaceTarget target) { IAbstractTextEditorHelpContextIds.FIND_REPLACE_OVERLAY); } + /** + * Tells whether the overlay can be opened in the given part, which is the + * precondition of + * {@link #FindReplaceOverlay(IWorkbenchPart, IFindReplaceTarget)}. It requires a + * text control to place itself on and to take its colors from, which a part + * showing its text through something else cannot provide. + * + * @param part the part to open the overlay in, may be {@code null} + * @return whether the overlay can be opened in the given part + */ + public static boolean canOpenInPart(IWorkbenchPart part) { + return getTargetControl(part) != null; + } + private static StyledText getTargetControl(IWorkbenchPart targetPart) { if (targetPart == null) { return null; diff --git a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceAction.java b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceAction.java index 0a170c1f0ef..44cc9822226 100644 --- a/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceAction.java +++ b/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceAction.java @@ -66,10 +66,22 @@ public class FindReplaceAction extends ResourceAction implements IUpdate { private static final String FIND_REPLACE_OVERLAY_AT_BOTTOM = "findReplaceOverlayAtBottom"; //$NON-NLS-1$ + private static final String RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS = "restrictFindReplaceOverlayToTextEditors"; //$NON-NLS-1$ + + // Whether the overlay can be opened in the part is only known to the overlay + // itself, whether it shall be is a matter of configuration. private boolean shouldUseOverlay() { + if (!FindReplaceOverlay.canOpenInPart(fWorkbenchPart)) { + return false; + } IPreferencesService preferences = Platform.getPreferencesService(); - boolean overlayPreference = preferences.getBoolean(INSTANCE_SCOPE_NODE_NAME, USE_FIND_REPLACE_OVERLAY, true, null); - return overlayPreference && fWorkbenchPart instanceof StatusTextEditor; + if (!preferences.getBoolean(INSTANCE_SCOPE_NODE_NAME, USE_FIND_REPLACE_OVERLAY, true, null)) { + return false; + } + // Restores the former behavior of offering the overlay for text editors only. + boolean restrictedToTextEditors = preferences.getBoolean(INSTANCE_SCOPE_NODE_NAME, + RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS, false, null); + return !restrictedToTextEditors || fWorkbenchPart instanceof StatusTextEditor; } private static boolean shouldPositionOverlayOnTop() { @@ -82,7 +94,9 @@ private static boolean shouldPositionOverlayOnTop() { if (this.overlay == null) { return; } - if (event.getKey().equals(USE_FIND_REPLACE_OVERLAY)) { + if (event.getKey().equals(USE_FIND_REPLACE_OVERLAY) + || (event.getKey().equals(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS) && !shouldUseOverlay())) { + // The next invocation decides anew, so the open overlay must not outlive the change. this.overlay.close(); } else if (event.getKey().equals(FIND_REPLACE_OVERLAY_AT_BOTTOM)) { this.overlay.setPositionToTop(shouldPositionOverlayOnTop()); @@ -419,13 +433,19 @@ private void showDialog() { private void showOverlayInEditor() { if (overlay == null) { overlay = new FindReplaceOverlay(fWorkbenchPart, fTarget); + // The overlay goes down with the control it is placed on, which may happen + // while the part is still alive, so the next invocation has to build a new one + // instead of reusing disposed controls. + overlay.getContainerControl().addDisposeListener(__ -> { + removeDialogPreferenceListener(); + overlay = null; + }); } overlay.open(); overlay.setPositionToTop(shouldPositionOverlayOnTop()); hookDialogPreferenceListener(); - overlay.getContainerControl().addDisposeListener(__ -> removeDialogPreferenceListener()); } @Override diff --git a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayInEditorTest.java b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayInEditorTest.java index 9c435992538..047d633272a 100644 --- a/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayInEditorTest.java +++ b/tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayInEditorTest.java @@ -14,6 +14,7 @@ import static org.eclipse.ui.internal.findandreplace.FindReplaceTestUtil.processPendingEvents; import static org.eclipse.ui.internal.findandreplace.FindReplaceTestUtil.runEventQueue; import static org.eclipse.ui.internal.findandreplace.FindReplaceTestUtil.waitForFocus; +import static org.eclipse.ui.internal.findandreplace.overlay.OverlayAccess.RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS_PREFERENCE; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -33,6 +34,9 @@ import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Text; +import org.eclipse.core.runtime.preferences.IEclipsePreferences; +import org.eclipse.core.runtime.preferences.InstanceScope; + import org.eclipse.jface.bindings.TriggerSequence; import org.eclipse.jface.bindings.keys.KeySequence; import org.eclipse.jface.bindings.keys.KeyStroke; @@ -81,8 +85,12 @@ public class FindReplaceOverlayInEditorTest { private static final String REPLACE_FIELD = "replaceInput"; //$NON-NLS-1$ + private static final String INSTANCE_SCOPE_NODE_NAME = "org.eclipse.ui.editors"; //$NON-NLS-1$ + private StatusTextEditor editor; + private FindReplaceAction findReplaceAction; + private Text searchField; private String testName; @@ -97,8 +105,10 @@ void openEditorWithOverlay(TestInfo testInfo) throws PartInitException { // Opening through the action rather than through its internals also asserts // that an editor of this kind gets the overlay rather than the dialog. - new FindReplaceAction(ResourceBundle.getBundle("org.eclipse.ui.texteditor.ConstructedEditorMessages"), //$NON-NLS-1$ - "Editor.FindReplace.", editor).run(); //$NON-NLS-1$ + findReplaceAction = new FindReplaceAction( + ResourceBundle.getBundle("org.eclipse.ui.texteditor.ConstructedEditorMessages"), //$NON-NLS-1$ + "Editor.FindReplace.", editor); //$NON-NLS-1$ + findReplaceAction.run(); searchField = focusedInputField(SEARCH_FIELD); } @@ -132,6 +142,7 @@ static void closeWelcomePage() { @AfterEach void closeEditor() { + findReplaceAction = null; searchField = null; if (editor != null) { editor.getSite().getPage().closeEditor(editor, false); @@ -139,6 +150,27 @@ void closeEditor() { } } + /** + * User can restrict the overlay to text editors in case they have trouble with it elsewhere, so + * an editor must keep getting the overlay rather than the dialog once they do. + *
+ * The preference is meant as a temporary fallback for the overlay's generalization to parts + * other than text editors. Remove this test together with it. + */ + @Test + public void testTextEditorKeepsOverlayWhenItIsRestrictedToTextEditors() { + IEclipsePreferences preferences= InstanceScope.INSTANCE.getNode(INSTANCE_SCOPE_NODE_NAME); + boolean wasRestricted= preferences.getBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS_PREFERENCE, false); + try { + // Restricting it closes the overlay that is open, so the action decides anew. + preferences.putBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS_PREFERENCE, true); + processPendingEvents(); + assertTrue(searchField.isVisible(), "the overlay should stay open"); //$NON-NLS-1$ + } finally { + preferences.putBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS_PREFERENCE, wasRestricted); + } + } + /** * A key typed into an input field belongs to that field: it must never be * carried out on the editor's document instead. 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 9fc245decce..e72dd5baf26 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 @@ -14,13 +14,17 @@ package org.eclipse.ui.internal.findandreplace.overlay; import static org.eclipse.ui.internal.findandreplace.FindReplaceTestUtil.waitForFocus; +import static org.eclipse.ui.internal.findandreplace.overlay.OverlayAccess.RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS_PREFERENCE; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; +import java.util.Arrays; import java.util.ResourceBundle; +import java.util.Set; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -28,6 +32,8 @@ import org.eclipse.swt.SWT; import org.eclipse.swt.graphics.Point; import org.eclipse.swt.graphics.Rectangle; +import org.eclipse.swt.widgets.Display; +import org.eclipse.swt.widgets.Shell; import org.eclipse.core.runtime.preferences.IEclipsePreferences; import org.eclipse.core.runtime.preferences.InstanceScope; @@ -89,6 +95,22 @@ private static IWorkbenchPart createHostPart(TextViewer viewer) { return part; } + @Test + public void testOverlayCanOnlyBeOpenedInPartExposingTextViewerWidget() { + openTextViewer("line"); + + assertTrue(FindReplaceOverlay.canOpenInPart(createHostPart(getTextViewer()))); + assertFalse(FindReplaceOverlay.canOpenInPart(Mockito.mock(IWorkbenchPart.class))); + assertFalse(FindReplaceOverlay.canOpenInPart(null)); + } + + @Test + public void testOverlayRejectsPartItCannotBeOpenedIn() { + IWorkbenchPart partWithoutTextViewer= Mockito.mock(IWorkbenchPart.class); + + assertThrows(IllegalArgumentException.class, () -> new FindReplaceOverlay(partWithoutTextViewer, null)); + } + @Test public void testShortcutHintReflectsFocusedInputField() { initializeTextViewerWithFindReplaceUI("line"); @@ -242,6 +264,52 @@ public void testDisableOverlayViaPreference() { } } + @Test + public void testOverlayIsOpenedByActionForPartExposingTextViewer() { + // The part hosting the viewer is no text editor, so this also covers that the + // action goes by what the part exposes rather than by its type. + initializeTextViewerWithFindReplaceUI(""); + getDialog().close(); + assertFalse(getDialog().isShown()); + + getFindReplaceAction().run(); + + assertTrue(getDialog().isShown(), "the action should have opened the overlay"); + } + + /** + * User can restrict the overlay to text editors in case they have trouble with it elsewhere, so + * we need to ensure that for an editor that is not a StatusTextEditor, the dialog is opened. + *
+ * The preference is meant as a temporary fallback for the overlay's generalization to parts
+ * other than text editors. Remove this test together with it.
+ */
+ @Test
+ public void testOverlayIsNotOpenedForNonTextEditorWhenRestrictedToTextEditors() {
+ IEclipsePreferences preferences= InstanceScope.INSTANCE.getNode(INSTANCE_SCOPE_NODE_NAME);
+ boolean wasRestricted= preferences.getBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS_PREFERENCE, false);
+ Display display= Display.getCurrent();
+ Set