Find/replace overlay: replace shell with integrated composite #2099

[HeikoKlare]

❗ This is work in progress

The FindReplaceOverlay is currently realized as a separate shell (more precisely, a JFace Dialog), which is placed at a proper position on top of the workbench shell. This has some drawback:

• It has to manually adapt to movements of the parent shell or the target part/widget
• It has to manually hide and show depending on visibility changes of the target part/widget
• It does not follow events of the target immediately, i.e., movements are always some milliseconds behind, minimize/maximize operations/animations are not synchronous etc.
• It does not locate properly when the platform uses Wayland, as manual shell positioning is not possible there.

This change replaces the dialog-based implementation of the FindReplaceOverlay with an in-place composite-based implementation. A composite is created in the target widget and placed relative to this composite. In consequence, the overlay automatically follows all move, resize, hide/show operations of the target widget.

Fixes eclipse-platform/eclipse.platform.swt#1447

Fixes #2099

WiP

This is work in progress. At least the following is not working:

• Cursor: the cursor is shown according to the contents of the target widget, not according to the widgets in the overlay. I.e., on a button still the text cursor is shown.
• Shortcuts: shortcuts do currently not work as they are processed by the target widget and do not reach the overlay. Considering Find/Replace Overlay: Allow rebinding shortcuts #2015, we may directly implement the shortcut as actions that can be reconfigured.

Despite these problems that may require some additional effort, the overall design and integration with this approach makes more sense to be, as the overlay is really "integrated" into the target widget, as it is supposed to be. It gets rid of all the additional functionality to manually update position, size and visibility state according to changes of the target widget.

How it looks

This is how it looks on Ubuntu using Wayland:

[github-actions]

Test Results

 1 814 files   -  1   1 814 suites   - 1   1h 34m 30s ⏱️ - 4m 12s
 7 699 tests ± 0   7 463 ✅  -  8  233 💤 +5  3 ❌ +3 
24 213 runs   - 45  23 461 ✅  - 48  749 💤 ±0  3 ❌ +3 

For more details on these failures, see this check.

Results for commit b6cf9bb. ± Comparison against base commit a88f3c0.

This pull request skips 5 tests.

org.eclipse.ui.genericeditor.tests.HoverTest ‑ testEnabledWhenHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testMultipleHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testProblemHover
org.eclipse.ui.genericeditor.tests.HoverTest ‑ testSingleHover
org.eclipse.ui.genericeditor.tests.ShowInformationTest ‑ testInformationControl

♻️ This comment has been updated with latest results.

[Wittmaxi]

There are a few extra pixels on the right side

[Wittmaxi]

looks like a very contructive commits which fixes multiple shortcomings of the current Find/Replace Overlay

[Wittmaxi]

Why use this kind of composite? What do we fix here?

[HeikoKlare]

It fixes that the CSS engine repeatedly overwrites the background color of composite with one defined by the theme. This is definitely not a perfect way to deal with the necessity to use some other component's background color here, but it's the only workaround I found so far. I am open for a better solution.

Note that this does not mean that the component is not properly themed: it just takes the themed background color of the shell instead of the one of the overlay's outer composite.

[Wittmaxi]

I am really not familiar (at all) with the CSS engine, but maybe we can add some CSS-tag to the Find/Replace overlay for custom styling? Drawback: all other stylesheets will need to style the overlay.

Anyway, this makes sense to me now and I believe that this is an okay workaround

[HeikoKlare]

I think we need to check again how the styling behaves in the overlay in general. Currently, the widgets inherit styling from the outer composite (usually the the composite of the editor's StyledText) except for the outermost composite within the overlay, which would otherwise simply be black in dark theme (i.e., have the same color as the editor, thus without any border). But I am not sure what happens if the composite was something else, i.e., if the colors will still fit then. Note that the proposed solution gets rid of all the existing custom color applications in the current implementation. It only needs to style two widgets instead of all the others.

We should definitely have another look at this topic.

[akurtakov]

This looks very promising. What is holding it off ?

[HeikoKlare]

This looks very promising. What is holding it off ?

It's the key bindings. Most of them (e.g., those for all the search options) are currently not working in this proposal. The reason is that now that the overlay is not a separate dialog with its own key input handling, the active workbench part (i.e., the target text editor of the overlay) now processes key events. The two consequences are that (1) the find/replace key bindings do not work (at least most of them) as the input event won't even reach the overlay and (2) the usual text editor key bindings are also evaluated when the overlay has focus (e.g., you can comment out the current text editor line while the overlay has focus with the according shortcut). Both behaviors are uninteded.
I am not yet sure how to properly solve this. I thought about adding some separate scope for the overlay, but I am not sure if it is even possible to have completely independent key binding scopes within the same workbench part. Maybe it is not sufficient to only integrate the overlay at the SWT layer (with just some composite at the right place) but also requires some integration in the workbench models (comparable to the proposal here: #2093 (comment))? If anyone with more knowledge in the key binding concept knows a/the way to properly handle this I would very much appreciate input on this.