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 https://github.com/eclipse-platform/eclipse.platform.swt/issues/1447

Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/2099

Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/2246

Issues to be Resolved Before Merging

• Test failures on Linux need to be investigated

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 #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[bot]]

Test Results

 1 818 files  ±0   1 818 suites  ±0   1h 41m 40s :stopwatch: + 8m 34s  7 708 tests ±0   7 480 :white_check_mark: +1  228 :zzz: ±0  0 :x:  - 1  24 285 runs  ±0  23 538 :white_check_mark: +1  747 :zzz: ±0  0 :x:  - 1 

Results for commit 32db531d. ± Comparison against base commit 4e0740d7.

:recycle: 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

[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: https://github.com/eclipse-platform/eclipse.platform.ui/issues/2093#issue-2411252154)? 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.