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

Previous Next

Full log

Message #259 received at 57639 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Eli Zaretskii <eliz <at> gnu.org>,
 Protesilaos Stavrou <info <at> protesilaos.com>, 57639 <at> debbugs.gnu.org
Subject: Re: bug#57639: [PATCH] Add new command 'toggle-theme'
Date: Thu, 13 Oct 2022 17:46:55 -0300

Hi Philip,

Philip Kaludercic <philipk <at> posteo.net> writes:

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

I think this added sentence is not clear.

Also, no documentation for these special properties for toggling themes?

> +(defun theme-list-variants (theme &rest list)
> +  "Return a list of theme variants for THEME.
> +If the optional argument LIST is not given, "

This docstring is incomplete.

> +  (let* ((properties (get theme 'theme-properties))
> +         (family (plist-get properties :family)))
> +    (seq-filter
> +     (lambda (variant)
> +       (and (eq (plist-get (get variant 'theme-properties) :family)
> +                family)
> +            (not (eq variant theme))))
> +     (or list (custom-available-themes)))))
> +
> +(defun theme-choose-variant (&optional no-confirm no-enable)
> +  "Prompt to switch from the current theme to one of its a variants.

I'd say: "Command to switch..."

> +  (let ((active-color-schemes
> +         (seq-filter
> +          (lambda (theme)
> +            ;; FIXME: As most themes currently do not have a `:kind'
> +            ;; tag, it is assumed that a theme is a color scheme by
> +            ;; default.  This should be reconsidered in the future.
> +            (memq (plist-get (get theme 'theme-properties) :kind)
> +                  '(color-scheme nil)))

I think that theme writers who care about this functionality will add
:kind and :family to the themes, and those who don't won't bother with
that.  So I don't really see the point in supporting (:kind nil).

> +          custom-enabled-themes)))
> +    (cond
> +     ((length= active-color-schemes 0)
> +      (user-error "No theme is active, cannot toggle"))

This message will be confusing when there are themes whose :kind is not
color-scheme...

> +     ((length> active-color-schemes 1)
> +      (user-error "More than one theme active, cannot unambiguously 
toggle")))
> +    (let* ((theme (car active-color-schemes))
> +           (family (plist-get (get theme 'theme-properties) :family)))
> +      (unless family
> +        (error "Theme `%s' does not have any known variants" theme))

This will pretty much always error with themes that don't really care
about toggling (see above).  Could you tell more about what is the
benefit of supporting (:kind nil)?

> --- a/lisp/emacs-lisp/loaddefs-gen.el
> +++ b/lisp/emacs-lisp/loaddefs-gen.el
> @@ -283,6 +283,12 @@ loaddefs-generate--make-autoload
>             ,@(when-let ((safe (plist-get props :safe)))
>                 `((put ',varname 'safe-local-variable ,safe))))))
>
> +     ;; Extract theme properties

Full stop missing.

> +     ((eq car 'deftheme)
> +      (let* ((name (car-safe (cdr-safe form)))
> +         (props (nthcdr 3 form)))
> +    `(put ',name 'theme-properties (list ,@props))))

In the Autoload section of the Elisp Manual, we have this:
"The forms which are not copied verbatim are the following:..."

Shouldn't deftheme be added to that list as well?

Previous Next

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