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

[HeikoKlare]

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. With this change, when the overlay has focus, general shortcuts for the editor (such as opening types/resources but also undo/redo) still work, while content specific shortcut (e.g., comment out current line) are disabled.

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

Fixes #2099

Fixes #2246

Known (Accepted) Issues / Drawbacks

• Proper handling of shortcuts requires the disablement of key bindings for the target text editor in order to (1) react to shortcuts at all and (2) in order to not execute unintended commands in the editor, such as commenting out a line while the overlay has focus. The implementation to achieve this is adapted from the breadcrumb implementation, which is kind of hacky but currently the only way I see to do this.
• Two of the shortcuts had to be changed due to conflicts with other shortcuts that can now be accessed from within the overlay. On the contrary, all shortcuts registered for editor, such as opening types or resources, are now accessible from within the overlay, which may be considered a benefit.
• For the other composite of the overlay, custom color handling had to be implemented, because it will usually be embedded into a StatusTextEditor containing a StyledText that uses a custom way of coloring as well, which is then inherited by the overlay. In order to have a consistent look, the colors of the outer composite of the overlay have to be retrieved and set in a custom way.

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 818 files  ±0   1 818 suites  ±0   1h 41m 40s ⏱️ + 8m 34s
 7 708 tests ±0   7 480 ✅ +1  228 💤 ±0  0 ❌  - 1 
24 285 runs  ±0  23 538 ✅ +1  747 💤 ±0  0 ❌  - 1 

Results for commit 32db531. ± Comparison against base commit 4e0740d.

♻️ 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.

[HeikoKlare]

This change of replacing the embedding of the overlay using dialog with an inplace composite seems to work almost fine now. I have documented some known drawbacks/issues (which I would consider acceptable for now) in the original post. In my opinion, the benefits (working on Linux/Wayland, resolution of several known issues) outweights them.

There are some test failures on Linux I will have a look into once I am at a Linux system again. After that, I would like to give this a try to finally have the overlay working on Linux, and apply further incremental improvements if necessary afterwards. Looking forward to your opinions on this, in particular @Wittmaxi.

[HeikoKlare]

Test failures have been fixed and manual tests on all platforms (Windows, MacOS, Linux with/without Wayland) looked promising. Just found that undo/redo shortcuts now also work as discussed in some request for the overlay (i.e., they now apply to the editor instead of the search/replace input field). E.g., if you perform a replace and want to undo it, you can do that right away with the according shortcut without the need to switch focus to the editor before.

I plan to keep this open for feedback until next Wednesday, 9th October (and then merge), as I won't be able to respond to potential needs for follow-up actions before. In case someone wants to have this merged earlier, feel free to take over and do so.