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

[Mauro Aranda <maurooaranda@gmail.com>]

Hi Philip,

Philip Kaludercic <philipk@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?