Repository navigation
Find/Replace overlay: offer it in any part that can host it - #4509
HeikoKlare merged 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Non-editor target disposal can leave overlay resources registered, and action-selection behavior lacks direct test coverage.
2 open findings
What changed in this PR
Generalizes Find/Replace overlay eligibility to parts exposing a text viewer, preparing support for the linked Console change.
Changes:
- Moves host capability checks into the overlay and enforces its constructor precondition.
- Adds an editor-only fallback preference and preference-page controls.
- Tests host eligibility, constructor rejection, and preference-triggered closure.
| File | Description |
|---|---|
| tests/org.eclipse.ui.workbench.texteditor.tests/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlayTest.java | Adds eligibility and preference tests. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceAction.java | Separates capability checks from preference-based selection. |
| bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java | Defines and enforces host eligibility. |
| bundles/org.eclipse.ui.editors/src/org/eclipse/ui/texteditor/AbstractDecoratedTextEditorPreferenceConstants.java | Defines the restriction preference and default. |
| bundles/org.eclipse.ui.editors/src/org/eclipse/ui/internal/editors/text/TextEditorMessages.properties | Adds the preference label. |
| bundles/org.eclipse.ui.editors/src/org/eclipse/ui/internal/editors/text/TextEditorMessages.java | Exposes the localized label. |
| bundles/org.eclipse.ui.editors/src/org/eclipse/ui/internal/editors/text/TextEditorDefaultsPreferencePage.java | Adds the dependent restriction checkbox. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4852a74 to
5a7c5ba
Compare
r-mennig
left a comment
r-mennig
left a comment
There was a problem hiding this comment.
I've tested this for the TextConsolePage using the changes described in eclipse-platform/eclipse.platform#3029. The RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS preference gets evaluated correctly and if set to false forces the TextConsolePage to show the find/replace dialog instead of the overlay.
The logic looks sound to me and I've only found a (potential) minor issue and missing test case.
| getFindReplaceAction().run(); | ||
|
|
||
| assertTrue(getDialog().isShown(), "the action should have opened the overlay"); | ||
| } |
There was a problem hiding this comment.
There is a test case missing here: ensuring that the dialog (instead of the overlay) is opened for generic text viewers if RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS is true. FindReplaceOverlayInEditorTest.testTextEditorKeepsOverlayWhenItIsRestrictedToTextEditors only covers the text editor case.
| } | |
| @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"); | |
| } | |
| @Test | |
| public void testOverlayIsNotOpenedForNonTextEditorWhenRestrictedToTextEditors() { | |
| IEclipsePreferences preferences= InstanceScope.INSTANCE.getNode(INSTANCE_SCOPE_NODE_NAME); | |
| boolean wasRestricted= preferences.getBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS, false); | |
| try { | |
| preferences.putBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS, true); | |
| initializeTextViewerWithFindReplaceUI(""); | |
| getDialog().close(); | |
| assertFalse(getDialog().isShown()); | |
| getFindReplaceAction().run(); | |
| assertFalse(getDialog().isShown(), "the action should have opened the dialog (so no overlay should be shown)"); | |
| } finally { | |
| preferences.putBoolean(RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS, wasRestricted); | |
| } | |
| } |
There was a problem hiding this comment.
I tried to only add minimal tests for this fallback functionality, as it is supposed to be temporary and will eventually be removed anyway. But you are of course right that as long as the functionality is there, it's behavior should be tested. What is particularly helpful with this test is that it will start failing once the fallback is removed, so that the developer becomes aware that tests have to be removed as well. The other test would have simply keps succeeding.
I have factored out the constant for the preference name, shared between both tests, so that the joint removal of those tests becomes more obvious.
That's definitely an improvement, thank you!
| } | ||
| 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)) { |
There was a problem hiding this comment.
Minor: Couldn't the overlay be kept open on a RESTRICT_FIND_REPLACE_OVERLAY_TO_TEXT_EDITORS change if the workbench part is a StatusTextEditor?
There was a problem hiding this comment.
Such a preference change will happen very seldom, so it would probably not be that problematic if an overlay is unnecessarily closed on a change. But I see that it can easily be added via checking for shouldUseOverlay(), so I have simply extended the check accordingly. Having another explicit check for StatusTextEditor is something I would like to avoid to keep the logic expressing the fallback constraints local to one method.
7886e8c to
b685ff5
Compare
Whether the Find/Replace overlay or the dialog is used was decided by checking the part for a concrete editor type, while the overlay itself requires something else: a control showing the part's text that it can place itself on and take its colors from. The two conditions could disagree in both directions, offering the overlay for an editor whose text widget does not exist and withholding it from other parts that present their text through a text viewer. Both halves of that decision are therefore separated and moved to where the knowledge for them resides. Whether the overlay *can* be opened in a part is answered by the overlay, which is the only place that knows what it needs, and its constructor enforces that condition as a precondition rather than failing somewhere inside the construction. Whether it *shall* be used is a matter of configuration and stays with the action that chooses between overlay and dialog. Reaching beyond text editors also brings parts whose composite outlives the control the overlay is placed on, where the overlay would be left behind as a widget nobody disposes, holding listeners on a disposed target and failing once anything asks it to close. Its lifetime is therefore tied to that control rather than to the widget tree it happens to hang in, and the action forgets an overlay whose controls are gone instead of reusing them. Reaching more parts than before carries the risk that the overlay behaves badly in one of them, which users would have no way to escape other than giving up the overlay altogether. A new preference, next to the ones controlling whether the overlay is used at all and where it is placed, restricts it back to text editors and thereby to the previous behavior. It defaults to the unrestricted behavior, so that the overlay is offered wherever it works and the preference serves as a fallback for those who need it. Assisted-by: Claude Opus 5 <noreply@anthropic.com>
b685ff5 to
2986b94
Compare
|
While a preference is good, I wonder if there should be a |
That's definitely something one can think about as an additional future functionality. Our current vision would be to have the overlay enabled for text editors in general (unless someone opts out from it overall) and to provide an extension mechanism for other views to configurable integraty the overlay into them as well. |
The the title / description is misleading. It talks about *parts what is an E4 concept (where we often use tags to disable things e.g. min/max) and editor is an E3 concern with no direct counterpart in E4 only available to the compatibility mode. |
|
Not sure if I fully understand the concern: the change is not specific to E3 or E4 at all. The parts we are talking about are the technical |
|
Merging this to have it in M2 for testing. We can still apply refinements and enhancement as follow-ups. |
Whether the Find/Replace overlay or the dialog is used was decided by checking the part for a concrete editor type, while the overlay itself requires something else: a control showing the part's text that it can place itself on and take its colors from. The two conditions could disagree in both directions, offering the overlay for an editor whose text widget does not exist and withholding it from other parts that present their text through a text viewer.
Both halves of that decision are therefore separated and moved to where the knowledge for them resides. Whether the overlay can be opened in a part is answered by the overlay, which is the only place that knows what it needs, and its constructor enforces that condition as a precondition rather than failing somewhere inside the construction. Whether it shall be used is a matter of configuration and stays with the action that chooses between overlay and dialog.
Effect on existing parts
None that I am aware of. There is no known adaptation to
ITextViewerthat this change would newly cover, so the set of parts offering the overlay stays as it is for now. The change is a generalization that takes effect as parts start exposing their text viewer.Find/Replace overlay in the Console view
lets the console page answer
ITextViewer, which it can do without any work because its viewer is aSourceVieweralready. Neither change reaches the Console view on its own: that one adds an adapter nothing asks for yet, and this one finds no console to offer the overlay to. Together they bring the overlay to the Console view, the first part beyond text editors to get it.Preference to restrict the overlay to text editors
Reaching more parts than before carries the risk that the overlay behaves badly in one of them, in a layout or a part this was not tried in. Without a way out, users hitting such a problem could only give up the overlay altogether, including in the editors where it works well today. The new preference, placed next to the existing ones for whether the overlay is used at all and where it is positioned, restores the former behavior of offering it in text editors only.
It defaults to the unrestricted behavior, so the generalization is what users get, and the preference serves purely as a fallback for those who need it. It is deliberately a narrow escape hatch rather than a configuration knob with a future: should the generalization prove sound in practice, the preference can be removed again in any later release without taking anything away from anyone, since its default already describes the behavior that would then remain.
🤖 Generated with Claude Code