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: support `systemPreferences.isDarkMode()` on Windows #19217

Merged
merged 2 commits into from Jul 15, 2019

Conversation

@deermichel
Copy link

deermichel commented Jul 12, 2019

Description of Change

Closes #18696. Closes #15316. Adds support for the systemPreferences.isDarkMode() API on Windows using Chromium's NativeTheme API.

Outdated (not-actually-used) approach

Whether the system uses dark or light theme is derived from following registry key:

HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Themes\Personalize\AppsUseLightTheme 
  | value 0  == dark
  | value 1+ == light

cc @MarshallOfSound @erickzhao @codebytere

Checklist

Release Notes

Notes: Added support for systemPreferences.isDarkMode() API on Windows.

@deermichel deermichel requested a review from erickzhao Jul 12, 2019
@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Jul 12, 2019

I want to reiterate on this again #18666 (comment), why aren't we reusing the api from ui::NativeTheme ?

/cc @codebytere @MarshallOfSound

@deermichel

This comment has been minimized.

Copy link
Author

deermichel commented Jul 12, 2019

Huh... probs bc that would've been too easy 😄. I'll take a closer look at it tmw - looks promising... and even offers an event emitter for theme changes.

Maybe we can get Linux support as well and upstream it to Chromium?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Jul 12, 2019

Yeah macOS is the issue there. We could use that api just for Windows support though?

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Jul 12, 2019

@codebytere I don't see the reason to patch even if thats the case we could return ui::NativeTheme result for if (@available(macOS 10.14, *)) { else return result from our implementation..

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Jul 12, 2019

oops my comment somehow deleted but in response to the above, yeah it might just be fine to gate it on mac and use for windows in totality

@electron-cation electron-cation bot removed the new-pr 🌱 label Jul 13, 2019
@deermichel deermichel force-pushed the intern/win-darkmode branch from 494b9b1 to 06cbf62 Jul 13, 2019
@zcbenz
zcbenz approved these changes Jul 15, 2019
@deermichel

This comment has been minimized.

Copy link
Author

deermichel commented Jul 15, 2019

Before merging, should I also add a dark-mode-changed (or sth like that) event or better do so in an additional PR? (And i might migrate the other methods to NativeTheme as well)

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Jul 15, 2019

@deermichel If we do do a first-party dark-mode-changed event we should probs do it for macOS as well and that requires some consideration as we support < 10.14. Let's land isDarkMode for windows and think about the event separately

@deermichel

This comment has been minimized.

Copy link
Author

deermichel commented Jul 15, 2019

You're right, I totally forgot about macOS. Then this PR is ready to get merged 👍

@codebytere codebytere merged commit da672a3 into master Jul 15, 2019
10 of 11 checks passed
10 of 11 checks passed
build-linux Workflow: build-linux
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 15, 2019

Release Notes Persisted

Added support for systemPreferences.isDarkMode() API on Windows.

@codebytere codebytere deleted the intern/win-darkmode branch Jul 15, 2019
@caesar

This comment has been minimized.

Copy link
Contributor

caesar commented Jul 20, 2019

Will this make it into Electron 6.0.0?

@hacdias hacdias mentioned this pull request Aug 13, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.