-
Notifications
You must be signed in to change notification settings - Fork 228
Remove high contrast theme #3095 #3138
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
Remove high contrast theme #3095 #3138
Conversation
|
cc @vogella |
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
|
Wasn't there some user asking to keep it? I can't find the comment on original issue, but I believe there was one. |
|
I'll draft it until we decide how to proceed. Open questions: do we disable theming when to OS is set to use High Contrast? ATM only Windows was capable of reporting if High Contrast was enabled, but I know for a fact that Ubuntu has a High Contrast option too and maybe even Mac does? If that is correct then we need to adapt the implementations of |
|
+1 for disabling theming if high contrast is enabled. This looks the best |
Currently only working for Windows since Linux and Mac do not report about high contrast mode being enabled/disabled. Contributes to eclipse-platform#3095
|
Commit e3f02aa is an attempt to update the preferences dialog accordingly (disabling theming) when high contrast is activated. Considerations:
Feedback is welcome (the PR remains drafted though). |
You might want to share a screenshot here to get more feedback. |
|
Adding some support for Linux based on (oversimplified) themeName.contains("HighContrast") should be fairly trivial and I can do it needed. |
... but we want to remove the high contrast theme. How would |
It will check Gtk theme name(https://wiki.archlinux.org/title/GTK) not Eclipse theme name. If the Gtk theme name contains HighContrast SWT Display.isHighContrast will return true, otherwise it will return false. |
🤯 In that case, you could also check for "ContrastHigh" since there is one for Mate that's called "ContrastHighInverse" (and there might be more like that?) |
|
eclipse-platform/eclipse.platform.swt#2372 - Gtk implementation |
Thank you! One thing that just occurred to me is: does the mechanism to notify the user about a change in the "High Contrast" also work on Linux? On Windows, when I activate the High Contrast setting (at OS level) I get this dialog asking me to restart:
Which comes from here: Lines 318 to 333 in 4a2727c
Does this also currently work on Linux, @akurtakov ? If not, it might be worth considering adding (in another PR). |
|
Detection of "high contrast" theme on startup works just fine on Linux. Runtime theme change doesn't seem to "fire" settings event on Gtk. That would have to be fixed on SWT side but it is not a blocker for this PR. |
It works on Linux now with Latest I-build. IMO the PR is good to be merged. |
I didn't have time to look into Mac yet and I got no answer from other Mac users. Maybe we could wait until M1? |
|
@Phillipus maybe you can provide some Mac feedback here? |
Isn't High Contrast a Windows only theme? I don't see it on Mac or Linux. |
|
OK, it's not on Mac. |
|
Let's merge in this case |
Ok. FTR the check failure seems unrelated: ... but I see that the Code Analysis check started running again. I'll wait. |
|
Same as before, the check failure is unrelated. Merging. |
|
The nightly tests look good so I think it's safe to say: Adiós, high contrast theme. One less outdated feature to maintain 👏 . Thank you all for your contribution! IMO this is worthy of an N&N entry. Does anyone want to do the honors? :-) |




Fixes #3095
How to test
Expected outcome