bug#57639: [PATCH] Add new command 'toggle-theme'

[Eli Zaretskii]

> Cc: 57639@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 17 Sep 2022 18:13:39 +0000
> 
> +@findex theme-choose-variant
> +  Some themes have variants (most often these are light and dark
> +pairs).  You can switch between these by typing @kbd{M-x
> +theme-choose-variant}.  Note that this only works if only one theme is
> +active.  If a theme has only one alternative, it will toggle
> +automatically.  If there are more of them, it will query which one to
> +use.

This description is confusing, I needed to read it several times
before I understood what you were trying to say.  The main problem is
that the "Note" sentence doesn't belong, and it interrupts the logic
of the description.  Here's my suggestion for saying it more clearly:

  Some themes have variants (most often just two: light and dark).
  You can switch to another variant with @kbd{M-x
  theme-choose-variant}.  If the currently active theme has only one
  other variant, it will be selected; if there are more variants, the
  command will prompt you which one to switch to.

  Note that @code{theme-choose-variant} only works if a single theme
  is active.

(Btw, what happens if more than one theme is active and the user
invokes theme-choose-variant? should this be described?)

> +Themes*} buffer.  The remaining arguments @var{properties} is used
> +pass a property list with theme attributes.                ^^^^^^^

"are used", in plural.

> +  :brightness 'light

Should we use :background-mode instead of :brightness, for consistency
with frame-background-mode?

> +(defun theme-choose-variant (&optional no-confirm no-enable)
> +  "Toggle the current active theme by enabling its dual pair.

"Toggle ... by enabling"?  "Dual pair"?  Can this sentence be
rephrased to be more clear?

> +The current theme will be immediately disabled before the dual
> +theme has been enabled.

Likewise here: "dual theme" doesn't explain itself, and seems to be
inaccurate, given the description in the manual.

>                          If THEME is not active an error will be
> +raised.

Passive tense alert!

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

Something's missing here or wrong with the punctuation?

> +   ((length> custom-enabled-themes 1)
> +    (user-error "More than one theme active, cannot unambiguously toggle")))

Wouldn't it be better to prompt for the theme in this case?

> +  (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?

Thanks.