GNU bug report logs - #57639
[PATCH] Add new command 'toggle-theme'

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: larsi <at> gnus.org, 57639 <at> debbugs.gnu.org
Subject: bug#57639: [PATCH] Add new command 'toggle-theme'
Date: Sun, 18 Sep 2022 09:53:53 +0300

> From: Philip Kaludercic <philipk <at> posteo.net>
> Cc: larsi <at> gnus.org,  57639 <at> debbugs.gnu.org
> Date: Sat, 17 Sep 2022 21:33:18 +0000
> 
> >> +  (let* ((theme (car custom-enabled-themes))
> >> +         (family (plist-get (get theme 'theme-properties) :family)))
> >> +    (unless family
> >> +      (error "`%s' is not part of a family" theme))
> >
> > "Family"? this terminology was never mentioned in the manual or the
> > doc string.  How about
> >
> >   Theme `%s' does not have any variants
> >
> > instead?
> 
> Strictly speaking that error message would be wrong at this point,
> because we cannot say if a theme has no variants if it is not part of a
> family.  This is because variants of a theme are all those that are part
> of the same family.  I think it would be better to clarify this in the
> documentation.

But the documentation doesn't explain what it means for a theme to be
part of a family.  Specifically, how does one tell, by looking at a
theme, whether it is or isn't part of a family?

An alternative for what I suggested above is to say

  Theme `%s' does not have any known variants

> +(defun theme-choose-variant (&optional no-confirm no-enable)
> +  "Prompt to switch from the current theme to a variant.
                                              ^^^^^^^^^^^^
"to one of its variants" is more clear.

> +Themes only have variants if they are part of a family of themes.

This should explain what it means to be part of a family, otherwise
this sentence is not helpful.

> +The current theme will be immediately disabled before variant is
> +enabled.

The "immediately" part should probably be removed, as it doesn't seem
to convey anything of importance, does it?  Come to think of it, what
exactly does this sentence try to say? isn't it obvious that the
current theme will be disabled and then the variant will be enabled?

>          In case the current theme has only one variant, it will
> +be toggled without prompting.

"toggled" is wrong here.  I suggest

  If the current theme has only one variant, switch to that variant
  without prompting, otherwise prompt for the variant to select.

>                                 For NO-CONFIRM and NO-ENABLE, see
> +`load-theme'."

I suggest to say this the other way around:

   See `load-theme' for the meaning of NO-CONFIRM and NO-ENABLE.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.