-
Notifications
You must be signed in to change notification settings - Fork 150
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
Highlighting problem when using the dark theme on Windows eclipse-platform/eclipse.platform.swt#811 #1690
base: master
Are you sure you want to change the base?
Conversation
Test Results 1 809 files - 3 1 809 suites - 3 1h 41m 39s ⏱️ + 9m 15s Results for commit 2014747. ± Comparison against base commit af7cdc3. This pull request removes 3 and adds 5 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
including merge of PR eclipse-platform#1690
@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 |
I just looked into this exact issue today, see #1688. I hope I can finish the work on this the next weeks. |
fbe1bf5
to
0b528bf
Compare
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. |
I don't know how the following could be achieved but my personal favorite would be:
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 :-) |
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:
|
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.
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. |
I achieved the desired behavior, at least for MacOS it's working fine:
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. |
294d1e3
to
9a6987b
Compare
@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. |
....eclipse.jface.text/src/org/eclipse/jface/text/contentassist/CompletionTableDrawSupport.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/ColumnViewerSelectionColorListener.java
Show resolved
Hide resolved
You can use the ZIP file provided under Testing here: #1690 (comment) 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:
|
@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:
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 findingsI also found that, in Linux:
|
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. |
Not related to this change, I opened another issue: #1878. You can assign the issue to me, I will look into it later. |
94a47f9
to
3f84898
Compare
Would be good to have some unit tests for the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a peek at the latest changes in b9ac801 and I have some minor comments.
I didn't test the changes yet, I will try to come back to this later this week.
net.bytebuddy.byte-buddy;bundle-version="1.14.13", | ||
net.bytebuddy.byte-buddy-agent;bundle-version="1.14.13" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. byte-buddy is needed for Mockito, Mockito is needed to mock the static class GC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I thought it should be automatically added since I didn't see any other references to byte-buddy in the WS (but several references to Mockito).
The tests are failing (invalid access). Can you please look into that?
....tests/src/org/eclipse/jface/internal/text/contentassist/CompletionTableDrawSupportTest.java
Show resolved
Hide resolved
....eclipse.jface.text.tests/src/org/eclipse/jface/internal/text/TableOwnerDrawSupportTest.java
Show resolved
Hide resolved
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: 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 eclipse-platform/eclipse.platform.swt@43cf295. |
The test is failing because the it tries to access a protected method (the constructor 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 eclipse-platform/.github#203, namely either plug-in-internal tests (as done in eclipse-platform/eclipse.platform.swt#1203 for SWT) or test fragments. Maybe in this case it could be easier to instantiate the |
…tform/eclipse.platform.swt#811 JFace viewer are using OS selection color to highlight the selected item. On some OS this is not accessible. With this change, the selection color can be changed via color preference in the settings of eclipse. Fixes eclipse-platform/eclipse.platform.swt#811
The system selection/highlight color is set as default color for the viewer selection in Eclipse. This color setting can be overwritten by the user via the color preferences.
Overwrite the default selection color for dark theme on Windows since the system color on Windows is hard to read.
581c087
to
0846e48
Compare
Done, thanks for your help. There are still some build failure and a test failure. It seems to be related to: |
Let's wait with version number increases once the freeze period is over and development for the next releases starts. |
In addition to the errors due to required version increments and API tools failures, there also seem to be some compilation problems:
Maybe those are worth to fix before the next development cycle starts. |
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:
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?