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

Test Results

 1 815 files  +  605   1 815 suites  +605   1h 30m 30s :stopwatch: + 28m 34s  7 664 tests +    5   7 436 :white_check_mark: +    8  228 :zzz:  -   3  0 :x: ±0  24 153 runs  +8 061  23 404 :white_check_mark: +7 825  749 :zzz: +236  0 :x: ±0 

Results for commit fb52307f. ± Comparison against base commit 934ccc66.

:recycle: 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: https://github.com/eclipse-platform/eclipse.platform.ui/pull/1690#issue-2138265188 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: https://github.com/eclipse-platform/eclipse.platform.ui/issues/1878. You can assign the issue to me, I will look into it later.

[BeckerWdf]

Would be good to have some unit tests for the changes. Also it would be nice if somebody on windows could test this PR.

[Christopher-Hermann]

For the failing unit test I will need some help. On my local machine I fixed the test by adding the packages as export package: org.eclipse.jface.internal.text.contentassist;x-internal:=true, in bundles/org.eclipse.jface.text/META-INF/MANIFEST.MF org.eclipse.jface.internal.text.contentassist, as export package in tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF

Not sure if something else needs to be done to fix this in the GitHub run

[alexanderKononov-sap]

Thank you Christopher for taking on this task! I have just tested it under Windows 11, and it works as expected.

• New preferences take effect immediately after pressing "Apply"
• Reset to default works
• In all views that I tested, the selection had new colors that I set
• Also in the code completion pop-over

The only thing I have noticed, is that in your test view, the 4th and 5th trees look the same. I assume that this is a copy-paste error in the test view, but if not, please check it out.

[fedejeanne]

For the failing unit test I will need some help. On my local machine I fixed the test by adding the packages as export package: org.eclipse.jface.internal.text.contentassist;x-internal:=true, in bundles/org.eclipse.jface.text/META-INF/MANIFEST.MF org.eclipse.jface.internal.text.contentassist, as export package in tests/org.eclipse.jface.text.tests/META-INF/MANIFEST.MF

Not sure if something else needs to be done to fix this in the GitHub run

Maybe something similar to the problem we faced before, @HeikoKlare ? I mean the one fixed by https://github.com/eclipse-platform/eclipse.platform.swt/commit/43cf2953c7fe32ed4ef24c4c14c4cdee9c2fd1cd.

[HeikoKlare]

The test is failing because the it tries to access a protected method (the constructor TableOwnerDrawSupport(Table)) without having access to it. The reason is that the test is placed in a test plug-in, which (in an OSGi environment) uses a different class loader than the to-be-tested plug-in, so that even though test class and tested class are placed in the same package they cannot access protected or package-visible methods of each other (even though one might assume that when considering ordinary Java visibility behavior).

If the to-be-tested functionality is not part of plug-in's public API but should still be tested in isolation (in terms of a proper unit test), you need to use one of the options discussed in https://github.com/eclipse-platform/.github/discussions/203, namely either plug-in-internal tests (as done in https://github.com/eclipse-platform/eclipse.platform.swt/pull/1203 for SWT) or test fragments.

Maybe in this case it could be easier to instantiate the TableOwnerDrawSupport via public API (i.e., via TableOwnerDrawSupport::install(Table)) instead of providing a more complicated test setup?

[Christopher-Hermann]

Maybe in this case it could be easier to instantiate the TableOwnerDrawSupport via public API (i.e., via TableOwnerDrawSupport::install(Table)) instead of providing a more complicated test setup?

Done, thanks for your help.

There are still some build failure and a test failure. It seems to be related to: org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.8-SNAPSHOT:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.ui.themes: Only qualifier changed for (org.eclipse.ui.themes/1.2.2400.v20240522-0941). Expected to have bigger x.y.z than what is available in baseline (1.2.2400.v20240213-1133) I will check on that

[BeckerWdf]

Let's wait with version number increases once the freeze period is over and development for the next releases starts.

[HeikoKlare]

In addition to the errors due to required version increments and API tools failures, there also seem to be some compilation problems:

Error:  Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:4.0.8-SNAPSHOT:compile (default-compile) on project org.eclipse.jface.text.tests: Compilation failure: Compilation failure: 
Error:  /home/runner/work/eclipse.platform.ui/eclipse.platform.ui/tests/org.eclipse.jface.text.tests/src/org/eclipse/jface/internal/text/contentassist/CompletionTableDrawSupportTest.java:[37] 
Error:  	CompletionTableDrawSupport.install(table);
Error:  	^^^^^^^^^^^^^^^^^^^^^^^^^^
Error:  Access restriction: The type 'CompletionTableDrawSupport' is not API (restriction on classpath entry '/home/runner/work/eclipse.platform.ui/eclipse.platform.ui/bundles/org.eclipse.jface.text/target/org.eclipse.jface.text-3.25.100-SNAPSHOT.jar')
[...]

Maybe those are worth to fix before the next development cycle starts.

[merks]

There are 16 commits in this PR. That seems like 15 too many…

[Christopher-Hermann]

There are 16 commits in this PR. That seems like 15 too many…

It's called iterative development, should be no problem with the squash and merge function. Since I'm not that experienced with the Eclipse build pipelines I need to trail and error...

[merks]

Personally I prefer to see one commit that’s amended

https://github.com/eclipse-platform/.github/blob/main/CONTRIBUTING.md#creating-a-pull-request

At some point there will need to be one commit with a good commit description.

[Christopher-Hermann]

I always try to avoid using amend and force push, at least when collaborating with others. But for this PR your are right, I think there are no others working on this PR

[Christopher-Hermann]

But back to work, do you have any suggestion how to increase the bundle version? org.eclipse.ui.themes 1.2.2400 -> 1.2.2500? org.eclipse.jface 3.34.0 -> 3.35.0? Not sure if there are any other versions to increase.

[HeikoKlare]

But back to work, do you have any suggestion how to increase the bundle version? org.eclipse.ui.themes 1.2.2400 -> 1.2.2500? org.eclipse.jface 3.34.0 -> 3.35.0? Not sure if there are any other versions to increase.

Taking a quick look at the changes, I don't see any additions to public API, so I would only expect micro version bumps for all affected bundles to be required:

• org.eclipse.jface.text
• org.eclipse.ui.themes
• org.eclipse.ui.workbench
• org.eclipse.ui
• org.eclipse.jface: this one already has a micro version bump for the new development cycle. But you already applied a minor version bump here, so I guess that was somehow necessary?

Some of the bumps might become obsolete rather soon once other PRs affecting those bundles are merged in the new development cycle.

[BeckerWdf]

This branch has conflicts that must be resolved

[iloveeclipse]

@Christopher-Hermann : please squash all commits to one and provide appropriate commit message.