all messages for Emacs-related lists mirrored at yhetil.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: Sat, 29 Jan 2022 10:40:01 +0200	[thread overview]
Message-ID: <83r18raqam.fsf@gnu.org> (raw)
In-Reply-To: <CH2PR12MB4231B48AB2971B11106E3E9FA5239@CH2PR12MB4231.namprd12.prod.outlook.com> (message from Vince Salvino on Sat, 29 Jan 2022 03:34:32 +0000)

> From: Vince Salvino <salvino@coderedcorp.com>
> CC: Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 29 Jan 2022 03:34:32 +0000
> 
> Update: I improved the previous patch to use a linked list to track the window handles during runtime, and am reasonably happy with it. If this looks good please go ahead and install the attached patch 0002 to master. Thanks!

Thanks.  A few comments below, mostly about minor stylistic issues.

> From a8c2f353372d8f015538804e17682e72e40af222 Mon Sep 17 00:00:00 2001
> From: Vince Salvino <salvino@coderedcorp.com>
> Date: Fri, 28 Jan 2022 22:25:13 -0500
> Subject: [PATCH] Support MS-Windows light/dark mode theme change during
>  runtime. (Bug#51404)

Please provide a ChangeLog-style description of changes (see
CONTRIBUTE for the details of the format we prefer).

> -/* If the OS is set to use dark mode.  */
> +/* If the OS supports light/dark mode. */
                                        ^^
Our style is to leave 2 spaces after the final period of the comment
(here and elsewhere in your patch).

> +/* Simple linked list to track window handles during runtime so they
> +   can be updated if the Windows light/dark mode theme is changed. */
> +struct HWND_NODE
> +{
> +  HWND hwnd;
> +  struct HWND_NODE *next;
> +};
> +struct HWND_NODE *g_hwnd_root;

I see where you add windows to the  list, but I don't see where you
remove deleted windows from the list.  Does that mean the list will
always grow indefinitely through an Emacs session, even if windows are
deleted?

>  /* Applies the Windows system theme (light or dark) to the window
> -   handle HWND.  */
> +   handle HWND. `track` should generally be TRUE to keep a reference
                 ^^
Two spaces between sentences there.  Also, our style of quoting in
comments is 'like this', not MD-style `like this`.

> +      /* After applying the theme, add the HWND to our global list so
> +	 it can be changed later if the OS light/dark mode theme is
> +	 changed. */
> +      if(track)
> +	{
> +	  struct HWND_NODE *curr = malloc(sizeof(struct HWND_NODE));

Please use xmalloc, not malloc, to allocate memory.

Also, our style is to leave one space between a function name and the
opening parenthesis that follows it.

> +	      /* Loop through all known HWNDs and apply theme */
                                                            ^^
Each comment should end with a period (and 2 spaces).

> +	      struct HWND_NODE *curr = g_hwnd_root;
> +	      while ( curr != NULL )
                     ^^^^^^^^^^^^^^
Our style is NOT to leave a space after the opening parenthesis and
before the closing parenthesis.





  reply	other threads:[~2022-01-29  8:40 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
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     ` Eli Zaretskii [this message]
2022-01-29 20:27       ` bug#51404: " 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

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

  git send-email \
    --in-reply-to=83r18raqam.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 external index

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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.