unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Vince Salvino <salvino@coderedcorp.com>
Cc: 51404@debbugs.gnu.org
Subject: bug#51404: Support system dark mode on Windows 10
Date: Tue, 26 Oct 2021 17:01:51 +0300	[thread overview]
Message-ID: <834k93zxyo.fsf@gnu.org> (raw)
In-Reply-To: <CH2PR12MB4231758A937F6FB9F33291CFA5849@CH2PR12MB4231.namprd12.prod.outlook.com> (message from Vince Salvino on Tue, 26 Oct 2021 04:46:27 +0000)

> 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.





  reply	other threads:[~2021-10-26 14:01 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-26  4:46 bug#51404: Support system dark mode on Windows 10 Vince Salvino
2021-10-26 14:01 ` Eli Zaretskii [this message]
2021-10-26 16:18   ` Eli Zaretskii
2021-10-26 16:49   ` Vince Salvino
2021-10-26 17:05     ` Eli Zaretskii
2021-10-26 18:20       ` Vince Salvino
2021-10-27 21:41         ` Vince Salvino
2021-10-28  7:15           ` Eli Zaretskii
2021-10-30 10:34             ` Eli Zaretskii
2021-10-30 17:13               ` Vince Salvino
2021-10-30 17:39                 ` Eli Zaretskii
2021-11-11  5:36                   ` bug#47291: " Lars Ingebrigtsen
2021-11-11  7:51                     ` Eli Zaretskii
2021-11-11 12:15                       ` Lars Ingebrigtsen
2021-11-11 15:08                         ` Eli Zaretskii
2021-11-12  3:00                           ` Lars Ingebrigtsen
2021-11-12  6:19                             ` Eli Zaretskii
2022-01-14  6:00 ` Vince Salvino
2022-01-23  0:00 ` Vince Salvino
2022-01-29  3:34   ` bug#51404: " Vince Salvino
2022-01-29  8:40     ` bug#51404: " Eli Zaretskii
2022-01-29 20:27       ` Vince Salvino

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=834k93zxyo.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=51404@debbugs.gnu.org \
    --cc=salvino@coderedcorp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).