feat: honor nativeTheme.themeSource = 'dark' before creating BrowserWindow on Windows
#25373
Conversation
There was a problem hiding this comment.
This is a tricky one because it "fixes" a thing that I would argue isn't expected behavior. On Windows there are maybe 3 apps that do this and all of them are windows built-in. The private API this utilizes to do this is super hacky, apparently depends on exact versions / builds of Windows and in theory could break at any time.
If there isn't a public API for this maybe we should ask for one? It doesn't feel right adding this amount of private API complexity to do a thing that by most definitions isn't expected behavior (and speaking for one app in particular we'd want to opt out of)
|
@MarshallOfSound it looks like we'll be using undocumented API if we want this to work in the short term. I agree this is a lot of code for a small amount of functionality -- but at the same time, I think having this feature work is still worthwhile. What if I shave down all the code to only the minimum subset that we need, and then we see how large that patch is? |
|
@ckerr I'm really not a fan of this much private API usage. If this were to land against my recommendations the minimum requirements would be:
|
|
If this feature were to land, would it allow apps to forcefully apply dark mode? As opposed to applying it only when the user has chosen that OS theme. Aside from the issue of private API usage, this would be a nice addition for apps which are primarily dark in aesthetics, but aren't using a frameless window. |
I've removed everything not necessary to fix the titlebar issue reported in #23479. This revised patch only loads enough API to determine how to see if dark mode is supported, if it's allowed, and how to set it for newly-created windows. dark_mode.cc is now about 60% smaller despite having new comments to address the next point --
I've tried to eliminate confusing code where possible, added explanatory comments, limited variables' scope, and moved variables to be declared just before use. Looking up function pointers from the dlls is still verbose -- you have to declare the function type, then declare a variable, and then do the lookup -- but I've added some template helper code to make it more readable.
I'm not sure this is a good idea -- it doesn't seem productive to require users to opt-in to a bugfix? This fixes an existing code in our dark mode: nativeTheme.themeSource = 'dark';
new BrowserWindow(...)
// windows' titlebar will not be dark |
Yes it would. The use case is literally setting |
nativeTheme.themeSource = 'dark' before creating BrowserWindow on Windows
c4b9e8e to
8e96c90
Compare
fcca81c to
d432f76
Compare
mas CI failures are unrelated |
|
Release Notes Persisted
|
|
I was unable to backport this PR to "10-x-y" cleanly; |
|
I was unable to backport this PR to "11-x-y" cleanly; |
|
Can we have this in production now without the flag? |
Description of Change
Fixes #23479.
With this patch, setting
nativeTheme.themeSource = 'dark'or'light'before creating the window is honored and the window will be rendered with the correct colors when shown.As a side-effect of this change, the setting themeSource dynamically now works on Linux too.
There is still a wart: Changing themeSource dynamically when the window is already shown isn't perfect: the property is set, but the window still has to be poked before the change takes effect, e.g. changing focus or minimizing/restoring the window. The usual programmatic ways of forcing a window refresh don't seem to work here; perhaps this is my error but let's see about getting guidance for the right recipe to use.
This PR adds a lot of scaffolding to pull in the undocumented Win32 dark mode APIs. Much of this ground is covered by https://github.com/ysc3839/win32-darkmode, which is license-compatible (MIT).
TODO:
win\dark_mode.ccis really big. Audit it to see if any can be removedCC @MarshallOfSound @electron/wg-releases
Checklist
npm testpassesRelease Notes
Notes: Improved dark mode support on Windows.