bug#51404: Support system dark mode on Windows 10

[Vince Salvino]

> > +#define DARK_MODE_APP_NAME L"DarkMode_Explorer"

> Can we make this exposed to Lisp, rather than hard-coded?  Hard-coding a 
> specific application for a theme sounds un-Emacsy.  People could want to 
> experiment with other apps.

Given that this is not so much a preference, as an undocumented magic string in 
Win32, I think anyone who wants to play with this is going to require knowledge 
of C and gdb to experiment, to risk causing erratic and unknown behavior. So I 
would be inclined to keep it in C.


> > +#ifndef DWMWA_USE_IMMERSIVE_DARK_MODE #define 
> > +DWMWA_USE_IMMERSIVE_DARK_MODE 20

> Why not use 19 and 20, depending on the Windows build number, and thus expand 
> the applicability of the feature?

I can add support for 19, but do not have the ability to test it on those 
specific Win10 builds to confirm if it actually works as expected. If someone 
is able to test on a version of Windows 10 older than 2004, then I will 
include. Erring on the side of stability for now.


> +/* Applies the Windows system theme (light or dark) to a window 
> +handle. */ static void w32_applytheme(HWND hwnd) {
> +  if (w32_darkmode) {
> +    /* Set window theme to that of a built-in Windows app (Explorer)
> +       because it has dark scroll bars and other UI elements. */

> Likewise here: it should be able to control this behavior by a user option.  
> We cannot assume that every Emacs user will automatically want to follow the 
> system theme.

I agree this would be a "nice to have", but the current functionality is 
in-line with behavior on other systems (GTK, macOS, etc. i.e. the application 
has no say in window decorations which are controlled by the window manager). 
If we did add an elisp setting it should default to the registry value at 
runtime. I also have no idea how to create an elisp setting and read it in C. 
Examples or contributions to this patch would be helpful.


> > +    /* Set the titlebar to system dark mode. */
> > +    if (DwmSetWindowAttribute_fn) {
> > +      DwmSetWindowAttribute_fn
> > +   (hwnd,
> > +    DWMWA_USE_IMMERSIVE_DARK_MODE,
> > +    &w32_darkmode,
> > +    sizeof(w32_darkmode));
> > +    }

> Does it make sense to call DwmSetWindowAttribute if we couldn't call 
> SetWindowTheme?  I know that such a situation shouldn't normally happen, but 
> what if it does?  If we need both calls, the second call should be 
> conditioned by SetWindowTheme_fn as well.

There is no harm in calling one without the other. SetWindowTheme sets things 
like scrollbars. DwmSetWindowAttribute specifically sets the titlebar. My 
original proof-of-concept only had DwmSetWindowAttribute and worked fine.

I will make the other requested changes, i.e. registry helper, style guide, and 
NEWS; and submit an updated patch.


Vince Salvino

-----Original Message-----
From: Eli Zaretskii <eliz@gnu.org> 
Sent: Tuesday, October 26, 2021 10:02 AM
To: Vince Salvino <salvino@coderedcorp.com>
Cc: 51404@debbugs.gnu.org
Subject: Re: bug#51404: Support system dark mode on Windows 10

> From: Vince Salvino <salvino@coderedcorp.com>
> Date: Tue, 26 Oct 2021 04:46:27 +0000
> 
> Attached is the patch. Additional info available here: 
> https://github.com/vsalvino/emacs

Thanks.  I have some comments and questions below, but in any case these 
changes are large enough to require copyright assignment from you.  If you'd be 
willing to start the legal paperwork at this time, I will send you the form to 
fill with the appropriate instructions.

>  LPBYTE
>  w32_get_resource (const char *key, LPDWORD lpdwtype)
> +{
> +  return w32_query_registry(REG_ROOT, key, lpdwtype); }
> +
> +/* Enables reading any key/name from the Windows Registry */ LPBYTE 
> +w32_query_registry (const char *root, const char *key, LPDWORD 
> +lpdwtype)

I'd prefer that you simply add an extra argument to the existing 
w32_get_resource, and adjust its single caller to pass REG_ROOT there.

> +/*
> +  Internal/undocumented constants for Windows Dark mode.
> +  See: https://github.com/microsoft/WindowsAppSDK/issues/41
> +*/

Please follow our style for comments, both single-line and multi-line.

> +#define DARK_MODE_APP_NAME L"DarkMode_Explorer"

Can we make this exposed to Lisp, rather than hard-coded?  Hard-coding a 
specific application for a theme sounds un-Emacsy.  People could want to 
experiment with other apps.

> +#ifndef DWMWA_USE_IMMERSIVE_DARK_MODE #define 
> +DWMWA_USE_IMMERSIVE_DARK_MODE 20

Why not use 19 and 20, depending on the Windows build number, and thus expand 
the applicability of the feature?

> +/* Applies the Windows system theme (light or dark) to a window 
> +handle. */ static void w32_applytheme(HWND hwnd) {
> +  if (w32_darkmode) {
> +    /* Set window theme to that of a built-in Windows app (Explorer)
> +       because it has dark scroll bars and other UI elements. */

Likewise here: it should be able to control this behavior by a user option.  We 
cannot assume that every Emacs user will automatically want to follow the 
system theme.

> +    if(SetWindowTheme_fn) {
> +      SetWindowTheme_fn(hwnd, DARK_MODE_APP_NAME, NULL);
> +    }

Please follow our style of using braces in C code.

> +    /* Set the titlebar to system dark mode. */
> +    if (DwmSetWindowAttribute_fn) {
> +      DwmSetWindowAttribute_fn
> +     (hwnd,
> +      DWMWA_USE_IMMERSIVE_DARK_MODE,
> +      &w32_darkmode,
> +      sizeof(w32_darkmode));
> +    }

Does it make sense to call DwmSetWindowAttribute if we couldn't call 
SetWindowTheme?  I know that such a situation shouldn't normally happen, but 
what if it does?  If we need both calls, the second call should be conditioned 
by SetWindowTheme_fn as well.

Last, but not least: this feature should be called out in NEWS and preferably 
also described in the "MS-Windows" Appendix in the Emacs manual.

Thanks again for working on this.