Sync theia dark/light theme with electron nativeTheme setting #15037
Conversation
* Sync theia dark/light theme setting with electron nativeTheme setting * This is to have native tooltips matching to the dark/light setting of the current theme Fixes eclipse-theia#14075 Signed-off-by: Florian Richter <[email protected]>
There was a problem hiding this comment.
Thanks for the changes!
Apart from the minor improvements in the comment the code looks good to me 👍
For me this however does not seem to work. If i change the Theme in the Theia example app i always get the dark os theme. However, i should note, that i also get the dark theme on current master, even when i change my local theme to light.
Do you have any idea, where this might come from? Are you aware of any special settings in this regard that you have active? Which OS did you use to test?
packages/core/src/common/theme.ts
Outdated
| @@ -32,6 +32,10 @@ export function isHighContrast(scheme: ThemeType): boolean { | |||
| return scheme === 'hc' || scheme === 'hcLight'; | |||
| } | |||
|
|
|||
| export function isLightOrDark(type: ThemeType): 'light' | 'dark' { | |||
There was a problem hiding this comment.
Can we create a type for the 'light' | 'dark'? We use it quite a lot and it is originally a subtype of the ThemeType.
Additionally, the name isLightOrDark implies a boolean function imho. Can we either change this into:
- isLightTheme (which returns true for
hcLightandlight) (or the other way around, but i believe this makes the method more readable). - getThemeMode(), if we choose this variant i believe we should wrap
type === 'hc' || type === 'dark'in () to make it easier to understand.
There was a problem hiding this comment.
Thank you for your review. As suggested, I created a type ThemeMode and renamed the helper method to getThemeMode
Signed-off-by: Florian Richter <[email protected]>
|
Thank you for your review. Originally, I only tested it on Windows 11 and there it works. |
|
I investigated this further. I tried the dark mode electron fiddle example on Fedora and Ubuntu and the theming of native elements like tooltips and the titlebar didn't change. This only seems to be implemented on Windows and MacOS. |
|
Thank you for the code improvements and the investigation. I agree, this is still an improvement even though it does not work on Linux. I believe we should track this in the issue, that the Linux implementation for this is still missing. Could you update the ticket accordingly, once this PR is merged. Btw, the titlebar changes the color for me, when i use the electron native bar, so at least that seems to work 🤷 @cdamus would you have time to test this change on Mac? If you have any questions about what there is to test just let me know. |
|
Seems to work on macOS @sgraband . 👍
|
There was a problem hiding this comment.
Thanks for checking @cdamus! This LGTM then 👍
@mvtec-richter Could you update the ticket to refelect that this is currently not working for Linux?
Thank you very much for the review. |
|
It would be good to also add this to #14075 linking to this PR and documenting there that a fix for linux is still required to close the issue. Thank you! I will merge this as soon as the tests succeed. |
What it does
This will sync the theia dark/light theme setting with electron nativeTheme setting.
This is to have native tooltips matching to the dark/light setting of the current theme.
This only works on Windows and MacOS in the electron application. Changing nativeTheme on Linux is not implemented in electron yet. Changing themes in the browser is not possible. This would still require to migrate tooltips to the HoverService or something similar.
Relates to #14075
How to test
Previously, the tooltip background color followed the system default. With this PR, the background color will align with theme currently selected in theia.
Follow-ups
Breaking changes
Attribution
Contributed by MVTec Software GmbH
Review checklist
Reminder for reviewers