Skip to content
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

Sync theia dark/light theme with electron nativeTheme setting #15037

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mvtec-richter
Copy link
Contributor

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.

Fixes #14075

How to test

  • Start electron application
  • Hover any native tooltip:
    • editor tab title
    • filename in explorer
    • title of problem view

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

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Contributed by MVTec Software GmbH

Review checklist

Reminder for reviewers

* 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]>
@tsmaeder tsmaeder requested a review from sgraband February 26, 2025 08:22
Copy link
Contributor

@sgraband sgraband left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -32,6 +32,10 @@ export function isHighContrast(scheme: ThemeType): boolean {
return scheme === 'hc' || scheme === 'hcLight';
}

export function isLightOrDark(type: ThemeType): 'light' | 'dark' {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hcLight and light) (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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

Render tooltips Theia theme aware
2 participants