Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Sep 7, 2024

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

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 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:
Screenshot from 2024-09-07 16-42-40

Copy link
Contributor

github-actions bot commented Sep 7, 2024

Test Results

 1 815 files  ±0   1 815 suites  ±0   1h 33m 47s ⏱️ + 2m 32s
 7 704 tests ±0   7 476 ✅ +1  228 💤 ±0  0 ❌  - 1 
24 273 runs  ±0  23 526 ✅ +1  747 💤 ±0  0 ❌  - 1 

Results for commit 088d3e6. ± Comparison against base commit 73b5314.

♻️ This comment has been updated with latest results.

@Wittmaxi
Copy link
Contributor

There are a few extra pixels on the right side
grafik

@Wittmaxi
Copy link
Contributor

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

private ContentAssistCommandAdapter contentAssistSearchField, contentAssistReplaceField;

private class FixColorComposite extends Composite {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

This looks very promising. What is holding it off ?

@HeikoKlare
Copy link
Contributor Author

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
Copy link
Contributor Author

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 HeikoKlare marked this pull request as ready for review October 2, 2024 07:38
@HeikoKlare HeikoKlare force-pushed the findreplace-inplace-composite branch 8 times, most recently from b8ac6a5 to bd45ba0 Compare October 3, 2024 10:38
@HeikoKlare HeikoKlare force-pushed the findreplace-inplace-composite branch 2 times, most recently from 875bed5 to 6eabf71 Compare October 3, 2024 13:48
…-platform#2099

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 eclipse-platform#2099

Fixes eclipse-platform#2246
@HeikoKlare HeikoKlare force-pushed the findreplace-inplace-composite branch from 6eabf71 to 088d3e6 Compare October 3, 2024 14:32
@HeikoKlare
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants