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

From: Philip Kaludercic <[email-masked]>
Date: Wed, 19 Oct 2022 07:20:52 +0000

Stefan Monnier <[email-masked]> writes:

>>> > I think you'd basically put something like the following into
>>> > `loaddefs-generate--emacs-batch' --
>>> >
>>> > (let ((lisp-mode-autoload-regexp
>>> >         "^;;;###\\(\\(noexist\\)-\\)?\\(theme-autoload\\)"))
>
> Hmm... `lisp-mode-autoload-regexp` is defined as a `defconst`.  If we're
> going to set it to some other value, we should change its definition to
> a `defvar`, no?
>
> Also, this code should come with a comment explaining why we're doing
> this silly dance (it took me a while to go from that code to here).
> [ I don't understand the "noexist" thingy, BTW.  Is that intended to be
>   a regexp that will never match?  Should it use `regexp-unmatchable`
>   then?  Or why not just "^;;;###\\(?3:theme-autoload\\)"?  ]
>
>>> >   (loaddefs-generate
>>> >     (expand-file-name "../etc/themes/" lisp-directory)
>>> >     (expand-file-name "theme-loaddefs.el" lisp-directory)))
>
> Note that if one of those ;;;###theme-autoload cookies is placed in
> front of a function definition, it will still result in a broken:
>
>     (autoload 'FUNCTIONNAME "etc/themes/FILENAME" ...)
>
> which fails because "etc/themes/FILENAME" isn't found in `load-path`.

Oh, that sounds bad!

>>> > Then you have to teach `loaddefs-generate--make-autoload' to generate
>>> > the correct forms as a result of the `deftheme' -- which should be some
>>> > `put's.
>>> 
>>> OK, it looks like this worked.  Just to recap, these are the proposed
>>> changes:
>>
>> What, if anything, does this mean for themes that aren't bundled with
>> Emacs?
>>
>> Also, this needs a NEWS entry.
>
> I see this got the following entry:
>
>     ** Themes have special autoload cookies.
>     All build-in themes are scraped for ;;;###theme-autoload cookies that
>     are loaded along with the regular auto-loaded code.
>
> but I can't see any good reason why Emacs users should care about that.
> It seems like a purely internal hack (and given the restrictions on
> what can be autoloaded this way, we probably shouldn't advertise it too
> loudly, unless accompanied with appropriate warnings).

Hmm, I believe I agree in retrospect that this doesn't really help anyone.

>         Stefan