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

feat: add new nativeTheme API #19656

Merged
merged 5 commits into from
Aug 14, 2019
Merged

feat: add new nativeTheme API #19656

merged 5 commits into from
Aug 14, 2019

Conversation

MarshallOfSound
Copy link
Member

This adds a new nativeTheme API which extracts APIs from systemPreferences (which was becoming a bit of a dumping ground) and consolidates them all to be backed by Chromium's NativeTheme module which is better implemented, more resilient and means we don't have to maintain the theme code 👍

It also allows us to easily implement the updated event.

Notes: Added new nativeTheme API to read and respond to changes in the OS's theme and color scheme

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 6, 2019
@MarshallOfSound MarshallOfSound added target/7-0-x semver/minor backwards-compatible functionality labels Aug 6, 2019
@deermichel
Copy link
Contributor

Now we're talking 🎉

@deermichel deermichel self-requested a review August 6, 2019 23:12
Copy link
Contributor

@deermichel deermichel left a comment

Choose a reason for hiding this comment

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

Lots of nits 😆 - super sorry. Additionally:

  • Did you miss module-keys.js or isn't it used anymore?
  • nativeTheme.shouldUseInvertedColorScheme doesn't work on macOS for me (always false), but update event gets fired when I toggle it
  • Afaik, the nativeTheme methods on Linux aren't implemented and default to some value - we should state that
  • Oh and we have some fake tests for the sysPref methods - do we wanna move them to a new nativeThemeSpec?

docs/api/native-theme.md Outdated Show resolved Hide resolved
docs/api/native-theme.md Outdated Show resolved Hide resolved
docs/api/native-theme.md Show resolved Hide resolved
docs/api/system-preferences.md Outdated Show resolved Hide resolved
docs/api/system-preferences.md Outdated Show resolved Hide resolved
shell/common/api/atom_api_native_theme.cc Show resolved Hide resolved
shell/common/api/atom_api_native_theme.h Outdated Show resolved Hide resolved
shell/common/api/atom_api_native_theme.h Outdated Show resolved Hide resolved
shell/common/api/atom_api_native_theme.cc Show resolved Hide resolved
shell/common/api/atom_api_native_theme.h Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 7, 2019
@MarshallOfSound
Copy link
Member Author

Did you miss module-keys.js or isn't it used anymore?

module-keys.js is only used for browser APIs. These APIs are in common.

nativeTheme.shouldUseInvertedColorScheme doesn't work on macOS for me (always false), but update event gets fired when I toggle it

This API is only implemented on Windows inside Chromium, it's a one liner to implement on macOS so I'll do that here as well.

Afaik, the nativeTheme methods on Linux aren't implemented and default to some value - we should state that

Potentially, will have to review which APIs are implemented for linux 👍

Oh and we have some fake tests for the sysPref methods - do we wanna move them to a new nativeThemeSpec?

New tests should be added here that the APIs "work"

docs/api/native-theme.md Outdated Show resolved Hide resolved
Copy link
Contributor

@deermichel deermichel left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@MarshallOfSound MarshallOfSound merged commit efa1818 into master Aug 14, 2019
@release-clerk
Copy link

release-clerk bot commented Aug 14, 2019

Release Notes Persisted

Added new nativeTheme API to read and respond to changes in the OS's theme and color scheme

@MarshallOfSound MarshallOfSound deleted the native-theme branch August 14, 2019 20:42
@trop trop bot mentioned this pull request Aug 14, 2019
@trop
Copy link
Contributor

trop bot commented Aug 14, 2019

I have automatically backported this PR to "7-0-x", please check out #19758

@sofianguy sofianguy added this to 7.0.0-beta.3 in 7.2.x Aug 15, 2019
@damienallen
Copy link

Just updated today and didn't even know this was coming, awesome 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor backwards-compatible functionality
Projects
No open projects
7.2.x
Fixed in 7.0.0-beta.3
Development

Successfully merging this pull request may close these issues.

None yet

4 participants