bug#45068: [PATCH] 28.0.50; Update Modus themes 1.0.2 (backward-incompatible)

[Protesilaos Stavrou <info@protesilaos.com>]

On 2021-03-04, 19:06 -0300, Mauro Aranda <maurooaranda@gmail.com> wrote:

> Protesilaos Stavrou <info@protesilaos.com> writes:
>
>> On 2021-03-04, 13:53 -0300, Mauro Aranda <maurooaranda@gmail.com> wrote:
>>
>>>> ;;;###autoload
>>>> (when (and (boundp 'custom-theme-load-path) load-file-name)
>>>>   (add-to-list 'custom-theme-load-path
>>>>                (file-name-as-directory (file-name-directory load-file-name))))
>>>
>>> A nit: I think this code should avoid adding the value of
>>> custom-theme-directory or the built-in theme directory name to
>>> custom-theme-load-path , if `custom-theme-directory' (for the former) or
>>> t (for the latter) are already present in custom-theme-load-path.  In
>>> particular, a theme distributed with Emacs should at least check for t,
>>> to avoid a repeated entry.
>>>
>>> I've noticed that the leuven theme has a similar code as well: I think
>>> that is a (really minor) bug.
>>
>> I have removed that form altogether.  It makes sense for packages but
>> here they are safe themes.  Is that okay, or have I misunderstood
>> something?
>
> Sounds OK to me; for themes that are only distributed with Emacs, it
> doesn't seem to be needed.

That is my impression as well.

> But if you plan to keep distributing them as packages via ELPA, then
> it might make sense to keep it.  I don't know what's the plan, so I
> can't say for sure if the form should stay or not.

My original plan was to update the themes in emacs.git and then figure
out what needs to be done for elpa.git to treat them as ":core" packages
instead of ":external".

So I had this and would have used a similar technique for the
above-quoted code:

    (if (and (>= emacs-major-version 28)
             (functionp 'require-theme))
          (require-theme 'modus-themes)
        (require 'modus-themes))

But that produced a major bug of not loading the desired theme in
certain setups.[1] I suspect it is because 'require' needs to be at the
top level?  Not sure...  Maybe there is some clean way to fix that,
though I would need more time to research and test it; a time frame that
I cannot estimate right now.

[1]: <https://gitlab.com/protesilaos/modus-themes/-/issues/162>.

So I prefer to use files that 100% work in emacs.git and then I will
treat elpa.git separately.  Using all those untested conditional clauses
will give me trouble.  Perhaps the themes in elpa.git should not be
":core" after all?  Keeping them as ":external", though updated to the
newest release, seems like the most reliable path forward.

Ultimately this means more work for me, though I prefer to not have to
deal with packaging-related bugs (notwithstanding the fact that I need
to ask for someone else to push changes for me in Emacs/ELPA and I would
rather not bother them).

>> Thanks again!
>
> Thanks to you!

I appreciate your contributions ("your" singular and plural).  For me
this is all part of a learning process and am happy to be part of a
community that (i) tolerates my errors and (ii) helps me learn through
them without making any discounts on technical requirements.

-- 
Protesilaos Stavrou
protesilaos.com