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

[Philip Kaludercic <philipk@posteo.net>]

Mauro Aranda <maurooaranda@gmail.com> writes:

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

Currently no.

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

Fixed, thanks.

>> +  (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..."

Do you think it is necessary to point out that it is a command?

>> +  (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).

:kind nil will probably not occur in practice, it is just that
`plist-get' will return nil if no :kind is specified.

>> +          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...

How come?  Or do you think that we should explicitly clarify that
`theme-choose-variant' is just for color-schemes?

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

I guess you are right in saying that nobody will set :family without
setting :kind... But that won't change anything here, because what you
describe is intended (a theme that has no variants, cannot be toggled.)

>> --- 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.

Noted.

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

Good point, will do.