On 2021-03-04, 18:41 +0000, "Basil L. Contovounesios" <contovob@tcd.ie> wrote:

> Protesilaos Stavrou <info@protesilaos.com> writes:
>
>> As noted earlier, please find attached the patch that upgrades the
>> themes to their newest version 1.2.0.  I tested it with Basil's latest
>> patch for 'require-theme', though it still works with the variant of
>> that function currently in trunk.
>
> Thanks, they're looking good!  Just a couple of nits from me below.

Thank you Basil and Mauro!  The attached patch addresses your feedback.

>> +Before you load a theme, it is necessary to require the main library:
>>  
>>  #+begin_src emacs-lisp
>> +(require 'modus-themes)
>>  #+end_src
>
> Is this always true?  Because M-x load-theme RET
> modus-{operandi,vivendi} RET works fine here with your patch, and users
> can set variables even before they're defined - defvar and defcustom
> only change the symbol's value if it's unbound.
>
> Maybe this node should clarify which users/setups/use-cases this applies
> to?

I updated the language to disambiguate the use-cases.  If you think it
needs further work, I will rewrite it.

> Similarly with other suggestions below like this one:
>
>      (require 'modus-themes)
>      (require 'modus-operandi-theme)
>      (require 'modus-vivendi-theme)
>
> which surely won't work with the built-in themes.

Appended a comment to the first form noting that it is not needed for
the built-in themes.  Replaced the latter two forms with:

(load-theme 'modus-operandi t t)
(load-theme 'modus-vivendi t t)

>> ++ Ideas and user feedback :: Aaron Jensen, Adam Spiers, Adrian Manea,
>> +  Alex Griffin, Alex Peitsinis, Alexey Shmalko, Alok Singh, Anders
>> +  Johansson, André Alexandre Gomes, Arif Rezai, Basil L. Contovounesios,
>                                                           ^^
> Hopefully Org's Texinfo export can one day be taught that this is not
> the end of a sentence ;).

Ah yes, I recall noticing that!  Can we circumvent it somehow?  Perhaps
by omitting the space?

>> +(require 'cl-lib)
>
> In theory this should be wrapped in eval-when-compile, but it doesn't
> make a difference for now since built-in themes are not byte-compiled.

Wrapped it in eval-when-compile.

>> +(deftheme modus-vivendi
>> +  "Accessible and customizable light theme (WCAG AAA standard).
>                                   ^^^^^
>                                   dark

Fixed!

[ Answer only if it is easy: how do you draw those ^^^ below the text? ]


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?

Thanks again!

-- 
Protesilaos Stavrou
protesilaos.com