Subject: Re: bug#51404: Support system dark mode on Windows 10

From: Eli Zaretskii <[email-masked]>
Date: Tue, 26 Oct 2021 17:01:51 +0300

> From: Vince Salvino <[email-masked]>
> 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.