Highlighting problem when using the dark theme on Windows eclipse-platform/eclipse.platform.swt#811

[Christopher-Hermann]

Highlighting problem when using the dark theme on Windows eclipse-platform/eclipse.platform.swt#811

JFace viewer are using OS selection color to highlight the selected item. On some OS this is not accessible. In particular when the eclipse is used with dark theme and the OS is used with light theme, this can cause really bad UI/UX experience. With this change, the selection color can be changed via color preference in the settings of eclipse.

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

Example
Selection color on Windows Server in dark theme looks really bad:

Testing
On start up of eclipse, the selection colors in column viewers (table and tree viewer) should look like before.
On Windows, the dark theme should predefine the selection colors to fix the bad coloring on some Windows platforms.

In the preferences (Preferences > General > Appearance > Colors and Fonts), there are 4 new properties for the coloring of the viewer selection:

• Basic > Selection foreground color for cell viewer
• Basic > Selection background color for cell viewer
• Basic > Selection foreground color for cell viewer without focus
• Basic > Selection background color for cell viewer without focus
  Changing these colors should affect the coloring of the selection in viewers. The coloring should immediately be applied as soon as the apply button is confirmed. The reset button should reset the coloring to the OS specific highlight coloring, expect for Windows dark theme, where the predefined colors should be restored.

To test the in different kind of viewers, import project viewerSelectionDemo.zip and execute the command "Open Viewer Selection Demo". There a bunch of different viewers with different selection colors are rendered to test the coloring for focused/not focused viewers.

Open Questions/Issues
This will have impact on coloring for all eclipse installations on all OSs and themes. The default coloring of the OS is overwritten in any case, even if it is not needed. Maybe we should only overwrite the selection color if this is enabled/requested by the user via a preference setting?

[github-actions]

Test Results

 1 815 files  +  605   1 815 suites  +605   1h 30m 30s ⏱️ + 28m 34s
 7 664 tests +    5   7 436 ✅ +    8  228 💤  -   3  0 ❌ ±0 
24 153 runs  +8 061  23 404 ✅ +7 825  749 💤 +236  0 ❌ ±0 

Results for commit fb52307. ± Comparison against base commit 934ccc6.

♻️ This comment has been updated with latest results.

[fedejeanne]

@Christopher-Hermann thank you for looking into this! I personally use the dark theme and am sometimes annoyed that it doesn't highlight stuff properly (at least on Windows).

I tested the viewer (on Windows 10) and I see this:

I like it!

Would it be possible to somehow also fix the auto-completion dialog though?

Or maybe I am just testing this the wrong way?

Here are the GIFs in raw format: gifs.zip

[Christopher-Hermann]

Would it be possible to somehow also fix the auto-completion dialog though?

I just looked into this exact issue today, see #1688. I hope I can finish the work on this the next weeks.

[Christopher-Hermann]

Not sure about the design of TableOwnerDrawSupport and the sub class CompletionTableDrawSupport. Maybe someone has an better idea how to overwrite the selection color in CompletionTableDrawSupport.

[Christopher-Hermann]

I would appreciate some assistance on deciding how to proceed with this PR.

Summary
Four new color options are now available in the preferences to style table and tree selections. Users can even customize these from the preferences dialog for a more personalized user experience:

This offers a significant UI/UX enhancement, especially on Windows, where the default OS coloring resulted in a poor user experience:

With these new color preferences, table and tree viewer selection colors remain consistent across all platforms, and can be tailored to individual user preferences. However, changing the selection color for every Eclipse installation might result into problems on certain platforms.

For instance, screenshots taken from Lars Vogels' tutorials on Linux Ubuntu show the default selection color to be orange:

Moreover, MacOS users can specify custom selection colors in the OS (Akzentfarbe), which is currently recognized by Eclipse:

Open Questions

• Can we universally alter the default selection color across all operating systems?
• Would it be better to make the selection coloring optional, perhaps by including an additional setting within the preferences?
• Is it possible to consider OS selection coloring when initially launching Eclipse, while still providing users with the option to modify the coloring in the preferences?

[fedejeanne]

Open Questions

* Can we universally alter the default selection color across all operating systems?

* Would it be better to make the selection coloring optional, perhaps by including an additional setting within the preferences?

* Is it possible to consider OS selection coloring when initially launching Eclipse, while still providing users with the option to modify the coloring in the preferences?

I don't know how the following could be achieved but my personal favorite would be:

• Provide a new button to let the 4 new configurations use the "system color" (similar to Use System Font)
• Use the system color by default: I would expect that Mac and Linux still look good when using the dark theme (also, the green "Akzentfarbe" should be used in Mac)
• (Optionally) Override the system color for Windows when using the dark theme (if the decision on this is blocking the PR, just postpone it to another PR)

IMO letting the users fix the coloring problem on their own by actively changing it would be good enough for this PR.

My two cents on the issue :-)

[HeikoKlare]

First of all, I really like and appreciate this contribution, as this dark theme highlighting problem on Windows is quite annoying. So thank you for working on that, @Christopher-Hermann!

I have to admit that I had no time yet to look into the proposed solution, so I can only give input in terms of "solution-independent expectations". Maybe they can help you in some way; otherwise feel free to ignore them:

• From a pure user experience point of view, I would expect that on Linux and Mac everything stays as it is and on Windows the dark theme by default uses the new colors (as there are no native colors to be reasonably used).
• From a more technical point of view, I would expect that by default the native colors are used and they can optionall be overwriting via preferences. On Windows, the theme could then overwrite these values by default. I am not sure whether that's possible with the preferences mechanism.

[Christopher-Hermann]

• From a more technical point of view, I would expect that by default the native colors are used and they can optionall be overwriting via preferences. On Windows, the theme could then overwrite these values by default. I am not sure whether that's possible with the preferences mechanism.

Yes, I also prefer this approach. I will have a look if something like this can be done within the preferences. I can imagine that taking the OS colors on the initial eclipse startup can also cause troubles when the OS color settings are changed in a later point in time. But I will have a look.

• (Optionally) Override the system color for Windows when using the dark theme (if the decision on this is blocking the PR, just postpone it to another PR)

If the preference thing is not possible (or at least not with manageable effort) I will go for this approach to provide a quick fix at least for windows users. I think that's where the problem is most annoying. An then have a look on a separate PR.

[Christopher-Hermann]

I achieved the desired behavior, at least for MacOS it's working fine:

• System highlight color is set as viewer selection color on the startup of eclipse.
• The color can be overwritten by the user via the color preferences.
• For dark theme on Windows, the system colors are overwritten by the theme settings.

It would be nice if someone can test the latest changes on other platforms. As soon as testing looks good I will have a look if unit tests can be added.

[fedejeanne]

@Christopher-Hermann could you please describe how you tested and what is the expected behavior?

Please explain which menus/preferences should be changed in the "Testing" section of this PR for future reference. I can test in Linux if it's ready in the next couple of hours. Thank you.

[Christopher-Hermann]

Please explain which menus/preferences should be changed in the "Testing" section of this PR for future reference. I can test in Linux if it's ready in the next couple of hours. Thank you.

You can use the ZIP file provided under Testing here: #1690 (comment)
There are a bunch of Viewers rendered where the selection coloring with and without the fix can be compared.

Furthermore, the new selection color should be applied in all standard eclipse views using column viewer, for example project explorer, outline, etc.

On Linux, the behavior should be:

• Every thing is colored as it was without the change (in light and dark theme)
• Going to Preferences > General > Appearance > Colors and Fonts and changing the color settings for the selection (Baisc > Selection background color for cell viewer, etc.)
• After changing the color settings, the viewers from the test ZIP and all Eclipse views should adapt the selection color accordingly

[fedejeanne]

@Christopher-Hermann thank you for the details.

Changes look good in Linux and the colors can be changed. I also briefly tested in Windows and it looks OK.

Please remember to:

• Edit the Testing section of this PR
• Mention the 4 added configurations: where are they and what are their names?
• Mention the expected state of the buttons according to the platform (Windows overrides the defaults so the state of the "Restore" button is different than on the other 2 platforms)
• Mention a "quickly noticeable" change once you hit the "Apply" button (you should even see it in the preferences dialog).

The reason I insist on this is because it should be easy to see how to test and what to expect in which specific platform for newcomers. It would also give me and other reviewers an idea of how you tested and we could expand on that idea and test for stuff that you might have overlooked.

My findings

I also found that, in Linux:

• The system values for "Selection background color for cell viewer without focus" (white) and "Selection foreground color for cell viewer without focus" (gray) don't work well together (look at the 2nd and 4th columns in the screenshot below)
• Some borders are still painted on the right columns when working with cell selection (look at the 4th and 5th column of the screenshot below)

[Christopher-Hermann]

• The system values for "Selection background color for cell viewer without focus" (white) and "Selection foreground color for cell viewer without focus" (gray) don't work well together (look at the 2nd and 4th columns in the screenshot below)

I noticed the same (not as bad as on Linux, but it could still be better) for MacOS. Maybe we need to define a color for the non focus selection. This shouldn't be affected by the OS highlight color anyway.

[Christopher-Hermann]

• Some borders are still painted on the right columns when working with cell selection (look at the 4th and 5th column of the screenshot below)

Not related to this change, I opened another issue: #1878. You can assign the issue to me, I will look into it later.

[BeckerWdf]

Hopefully not a regression, but I don't see this error in the workspace this morning.

So should we simply rebase this PR and see if a new build is fine again?

[fedejeanne]

Hopefully not a regression, but I don't see this error in the workspace this morning.

So should we simply rebase this PR and see if a new build is fine again?

@BeckerWdf I just rebased, hopefully the error will be gone and the tests (in Mac) will run too

[merks]

Yes, rebasing is a good idea.

[Christopher-Hermann]

Rebase is not working. But the error is not reliable reproducible, sometime the Windows, sometime the Mac and sometime the Linux build is failing.

[BeckerWdf]

failing test is unrelated and documented in #926

[BeckerWdf]

@fedejeanne: Are you ok with merging this PR?

[fedejeanne]

Thank you @Christopher-Hermann for this, it looks really good!
I'm sorry for the delay, I'm swamped.

I took a final look into it and noticed that you exported some packages in org.eclipse.jface.text.tests, which I assume was necessary because of Mockito? In any case, I removed the exports and the tests ran just fine on my machine so I tooked the liberty of pushing my changes into your PR so you don't need to wait for me again.

All checks pass so this one is ready to go ✅

Thank you again and thank you @BeckerWdf for your input too!

[jukzi]

please have a look at test failures: #1955

[jukzi]

And may this relate too? eclipse-platform/eclipse.platform.swt#1275

[BeckerWdf]

please have a look at test failures: #1955

Are these tests not executed on the PR's verification build?

[jukzi]

Are these tests not executed on the PR's verification build?

I don't know. Sometimes the tests behave just different in Integration Builds or on OSs/java versions not tested on Ci.

[BeckerWdf]

Are these tests not executed on the PR's verification build?

I don't know. Sometimes the tests behave just different in Integration Builds or on OSs/java versions not tested on Ci.

So if that's the case that's really sad. Once should have confidence that everything is ok if the verification builds are green.

[merks]

See #1955 (comment) for why this is broken.

I.e., code should not assume any specific color key is registered if that plugin itself did not register that color key. I'm not sure if JFace is supposed to work standalone too, but in that case, you can't ever assume some color is registered if it's done via a plugin.xml.

[iloveeclipse]

This caused #1956 regression too.

[Phillipus]

I'm not sure if JFace is supposed to work standalone too,

Yes, it is. See https://wiki.eclipse.org/JFace#Using_JFace_outside_the_Eclipse_platform

[Acru]

Is this still being worked on? Sad to see it was reverted due to side effects, rather than being improved.

I've been looking for a fix for this, where the default highlighting in my setup is such that it is nearly impossible to tell when something is highlighted, as it is nearly the same color as the background.

Edit: I'm also having the same problem with enabled toolbar buttons actually, eg. with the perspective toolbar I can't tell which perspective is selected, though this pull doesn't help with that...

[Christopher-Hermann]

It's still on my TODOs list but unfortunately I don't really have time at the moment for this topic.
The regressions/side effects were only related to Linus OS and it's really complex for me to setup, test and fix this on Linux.

[Acru]

That is too bad.

In the mean time I have been trying to figure out where the toolbar button toggle highlighting is actually being done, though it seems that the highlighting is somewhere separate from the toolitems in org.eclipse.swt.widgets?

[Acru]

Unless I am missing something, it looks like toolbar item highlighting is also entirely provided by the OS's implementation and theme coloring, and I did see there is an outstanding issue for windows 11 as well, in that regard; eclipse-platform/eclipse.platform.swt#217 (comment)