~protesilaos/modus-themes

6 2

Unconditional setting of vc-annotate-background-mode

Details
Message ID
<875ylfxkgi.fsf@posteo.net>
DKIM signature
missing
Download raw message
Hi,

I just recently noticed that modus-themes were setting
vc-annotate-background-mode to nil, even if I configured it to be t (by
custom.el is loaded before loading modus).  Is there a particular reason
for doing so?  Personally, I find the default a lot more intuitive to
reason about what code is old and what is new.  It is easy to fix, but
one might consider a theme changing a user option like this to be
invasive.
Details
Message ID
<87a6ar18vi.fsf@protesilaos.com>
In-Reply-To
<875ylfxkgi.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
Patch: +0 -23
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 05 Jun 2022 14:18:21 +0000
>
>
> Hi,

Hello Philip,

> I just recently noticed that modus-themes were setting
> vc-annotate-background-mode to nil, even if I configured it to be t (by
> custom.el is loaded before loading modus).  Is there a particular reason
> for doing so?  Personally, I find the default a lot more intuitive to
> reason about what code is old and what is new.  It is easy to fix, but
> one might consider a theme changing a user option like this to be
> invasive.

It is invasive and I want to remove it.  The reason I never touched it
is because no-one ever provided any feedback about it.

I am now thinking about removing all those variables:

 modus-themes.el | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/modus-themes.el b/modus-themes.el
index f068e4a..860f03e 100644
--- a/modus-themes.el
+++ b/modus-themes.el
@@ -7504,29 +7504,6 @@ ;;;; mini-modeline
;;;; pdf-tools
    `(pdf-view-midnight-colors
      '(,fg-main . ,bg-dim))
;;;; vc-annotate (C-x v g)
    `(vc-annotate-background nil)
    `(vc-annotate-background-mode nil)
    `(vc-annotate-color-map
      '((20 . ,red)
        (40 . ,magenta)
        (60 . ,magenta-alt)
        (80 . ,red-alt)
        (100 . ,yellow)
        (120 . ,yellow-alt)
        (140 . ,fg-special-warm)
        (160 . ,fg-special-mild)
        (180 . ,green)
        (200 . ,green-alt)
        (220 . ,cyan-alt-other)
        (240 . ,cyan-alt)
        (260 . ,cyan)
        (280 . ,fg-special-cold)
        (300 . ,blue)
        (320 . ,blue-alt)
        (340 . ,blue-alt-other)
        (360 . ,magenta-alt-other)))
    `(vc-annotate-very-old-color nil)
;;;; wid-edit
    `(widget-link-prefix ,(if (memq 'all-buttons modus-themes-box-buttons)
                              " "

What do you think?  They don't do the right thing and things don't work
properly anyway.  I will update the documentation instead.

An aside here: I remember in previous versions of Emacs where the
vc-annotate-color-map could be controlled by the themes by setting it
this way, but this is not possible in the last few months.  The return
value I get is the default:

   vc-annotate-color-map
   ;; => ((20 . "#FF3F3F") (40 . "#FF6C3F") (60 . "#FF993F") (80
   ;; . "#FFC63F") (100 . "#FFF33F") (120 . "#DDFF3F") (140 . "#B0FF3F")
   ;; (160 . "#83FF3F") (180 . "#56FF3F") (200 . "#3FFF56") (220
   ;; . "#3FFF83") (240 . "#3FFFB0") (260 . "#3FFFDD") (280 . "#3FF3FF")
   ;; (300 . "#3FC6FF") (320 . "#3F99FF") (340 . "#3F6CFF") (360
   ;; . "#3F3FFF"))

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<875ylfw39j.fsf@posteo.net>
In-Reply-To
<87a6ar18vi.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Protesilaos Stavrou <info@protesilaos.com> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sun, 05 Jun 2022 14:18:21 +0000
>>
>>
>> Hi,
>
> Hello Philip,
>
>> I just recently noticed that modus-themes were setting
>> vc-annotate-background-mode to nil, even if I configured it to be t (by
>> custom.el is loaded before loading modus).  Is there a particular reason
>> for doing so?  Personally, I find the default a lot more intuitive to
>> reason about what code is old and what is new.  It is easy to fix, but
>> one might consider a theme changing a user option like this to be
>> invasive.
>
> It is invasive and I want to remove it.  The reason I never touched it
> is because no-one ever provided any feedback about it.

I love being the first one to do so :).

> I am now thinking about removing all those variables:
>
>  modus-themes.el | 23 -----------------------
>  1 file changed, 23 deletions(-)
>
> diff --git a/modus-themes.el b/modus-themes.el
> index f068e4a..860f03e 100644
> --- a/modus-themes.el
> +++ b/modus-themes.el
> @@ -7504,29 +7504,6 @@ ;;;; mini-modeline
>  ;;;; pdf-tools
>      `(pdf-view-midnight-colors
>        '(,fg-main . ,bg-dim))
> -;;;; vc-annotate (C-x v g)
> -    `(vc-annotate-background nil)

Why is this option set to what already was the default value?

> -    `(vc-annotate-background-mode nil)
> -    `(vc-annotate-color-map
> -      '((20 . ,red)
> -        (40 . ,magenta)
> -        (60 . ,magenta-alt)
> -        (80 . ,red-alt)
> -        (100 . ,yellow)
> -        (120 . ,yellow-alt)
> -        (140 . ,fg-special-warm)
> -        (160 . ,fg-special-mild)
> -        (180 . ,green)
> -        (200 . ,green-alt)
> -        (220 . ,cyan-alt-other)
> -        (240 . ,cyan-alt)
> -        (260 . ,cyan)
> -        (280 . ,fg-special-cold)
> -        (300 . ,blue)
> -        (320 . ,blue-alt)
> -        (340 . ,blue-alt-other)
> -        (360 . ,magenta-alt-other)))

I think keeping `vc-annotate-color-map' would be just fine.  If you
remove it the default colors are used, and while it works, it wouldn't
match the colour scheme.

> -    `(vc-annotate-very-old-color nil)

It might be interesting if this could be set to some sensible value, but
as far as I see it has to be a colour spec.  Can an entry in
modus-themes-custom-variables depend on the specific Modus theme

>  ;;;; wid-edit
>      `(widget-link-prefix ,(if (memq 'all-buttons modus-themes-box-buttons)
>                                " "
>
> What do you think?  They don't do the right thing and things don't work
> properly anyway.  I will update the documentation instead.
>
> An aside here: I remember in previous versions of Emacs where the
> vc-annotate-color-map could be controlled by the themes by setting it
> this way, but this is not possible in the last few months.  The return
> value I get is the default:
>
>     vc-annotate-color-map
>     ;; => ((20 . "#FF3F3F") (40 . "#FF6C3F") (60 . "#FF993F") (80
>     ;; . "#FFC63F") (100 . "#FFF33F") (120 . "#DDFF3F") (140 . "#B0FF3F")
>     ;; (160 . "#83FF3F") (180 . "#56FF3F") (200 . "#3FFF56") (220
>     ;; . "#3FFF83") (240 . "#3FFFB0") (260 . "#3FFFDD") (280 . "#3FF3FF")
>     ;; (300 . "#3FC6FF") (320 . "#3F99FF") (340 . "#3F6CFF") (360
>     ;; . "#3F3FFF"))
Details
Message ID
<877d5v15b1.fsf@protesilaos.com>
In-Reply-To
<875ylfw39j.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 05 Jun 2022 15:15:04 +0000
>
>> It is invasive and I want to remove it.  The reason I never touched it
>> is because no-one ever provided any feedback about it.
>
> I love being the first one to do so :).

And thanks for doing it!

>> I am now thinking about removing all those variables:
>>
>>  modus-themes.el | 23 -----------------------
>>  1 file changed, 23 deletions(-)
>>
>> diff --git a/modus-themes.el b/modus-themes.el
>> index f068e4a..860f03e 100644
>> --- a/modus-themes.el
>> +++ b/modus-themes.el
>> @@ -7504,29 +7504,6 @@ ;;;; mini-modeline
>>  ;;;; pdf-tools
>>      `(pdf-view-midnight-colors
>>        '(,fg-main . ,bg-dim))
>> -;;;; vc-annotate (C-x v g)
>> -    `(vc-annotate-background nil)
>
> Why is this option set to what already was the default value?

The idea was to prevent the user from setting it.  As for why: because
it is not possible to know if the colour the user picks will lead to
accessible combinations.

For the record, I also prefer backgrounds for diffs and related.

>> -    `(vc-annotate-background-mode nil)
>> -    `(vc-annotate-color-map
>> -      '((20 . ,red)
>> -        (40 . ,magenta)
>> -        (60 . ,magenta-alt)
>> -        (80 . ,red-alt)
>> -        (100 . ,yellow)
>> -        (120 . ,yellow-alt)
>> -        (140 . ,fg-special-warm)
>> -        (160 . ,fg-special-mild)
>> -        (180 . ,green)
>> -        (200 . ,green-alt)
>> -        (220 . ,cyan-alt-other)
>> -        (240 . ,cyan-alt)
>> -        (260 . ,cyan)
>> -        (280 . ,fg-special-cold)
>> -        (300 . ,blue)
>> -        (320 . ,blue-alt)
>> -        (340 . ,blue-alt-other)
>> -        (360 . ,magenta-alt-other)))
>
> I think keeping `vc-annotate-color-map' would be just fine.  If you
> remove it the default colors are used, and while it works, it wouldn't
> match the colour scheme.

The problem is that it does not work properly.  If I keep those values
and the user changes vc-annotate-background-mode to t, then they will
get unusable colours.  That mode takes those colours and applies them to
the background.  With modus-operandi, this results in intense
backgrounds with black text---virtually unreadable.

Also, I have noticed that these user options generally don't behave as
expected.  For example, I am using the following to experiment
with/without backgrounds and with switching between modus-operandi and
modus-vivendi.  For some reason, modus-operandi now always uses
backgrounds while modus-vivendi doesn't (vc-annotate-background-mode is
nil)...  I know that I need to re-run M-x vc-annotate for changes to
take effect.

    (setq vc-annotate-background-mode nil)

    (defun my-modus-themes-vc-annotate ()
      ;; Actual values are for demo purposes
      (modus-themes-with-colors
        (if vc-annotate-background-mode
            (setq vc-annotate-background bg-alt
                  vc-annotate-color-map
                  `((20 .  ,red-intense-bg)
                    (40 .  ,red-subtle-bg)
                    (60 .  ,red-refine-bg)
                    (80 .  ,yellow-intense-bg)
                    (100 . ,yellow-subtle-bg)
                    (120 . ,yellow-refine-bg)
                    (140 . ,magenta-intense-bg)
                    (160 . ,magenta-subtle-bg)
                    (180 . ,magenta-refine-bg)
                    (200 . ,cyan-intense-bg)
                    (220 . ,cyan-subtle-bg)
                    (240 . ,cyan-refine-bg)
                    (260 . ,green-intense-bg)
                    (280 . ,green-subtle-bg)
                    (300 . ,green-refine-bg)
                    (320 . ,blue-intense-bg)
                    (340 . ,blue-subtle-bg)
                    (360 . ,blue-refine-bg)))
          (setq vc-annotate-background nil
                vc-annotate-color-map
                `((20 . ,red)
                  (40 . ,magenta)
                  (60 . ,magenta-alt)
                  (80 . ,red-alt)
                  (100 . ,yellow)
                  (120 . ,yellow-alt)
                  (140 . ,fg-special-warm)
                  (160 . ,fg-special-mild)
                  (180 . ,green)
                  (200 . ,green-alt)
                  (220 . ,cyan-alt-other)
                  (240 . ,cyan-alt)
                  (260 . ,cyan)
                  (280 . ,fg-special-cold)
                  (300 . ,blue)
                  (320 . ,blue-alt)
                  (340 . ,blue-alt-other)
                  (360 . ,magenta-alt-other))))))

    (add-hook 'modus-themes-after-load-theme-hook #'my-modus-themes-vc-annotate)

>> -    `(vc-annotate-very-old-color nil)
>
> It might be interesting if this could be set to some sensible value, but
> as far as I see it has to be a colour spec.  Can an entry in
> modus-themes-custom-variables depend on the specific Modus theme

In principle, yes.  Though we could apply an appropriate gray background
since the rest of the colour space is occupied.

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<87ee03ully.fsf@posteo.net>
In-Reply-To
<877d5v15b1.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Protesilaos Stavrou <info@protesilaos.com> writes:

>>> -    `(vc-annotate-background-mode nil)
>>> -    `(vc-annotate-color-map
>>> -      '((20 . ,red)
>>> -        (40 . ,magenta)
>>> -        (60 . ,magenta-alt)
>>> -        (80 . ,red-alt)
>>> -        (100 . ,yellow)
>>> -        (120 . ,yellow-alt)
>>> -        (140 . ,fg-special-warm)
>>> -        (160 . ,fg-special-mild)
>>> -        (180 . ,green)
>>> -        (200 . ,green-alt)
>>> -        (220 . ,cyan-alt-other)
>>> -        (240 . ,cyan-alt)
>>> -        (260 . ,cyan)
>>> -        (280 . ,fg-special-cold)
>>> -        (300 . ,blue)
>>> -        (320 . ,blue-alt)
>>> -        (340 . ,blue-alt-other)
>>> -        (360 . ,magenta-alt-other)))
>>
>> I think keeping `vc-annotate-color-map' would be just fine.  If you
>> remove it the default colors are used, and while it works, it wouldn't
>> match the colour scheme.
>
> The problem is that it does not work properly.  If I keep those values
> and the user changes vc-annotate-background-mode to t, then they will
> get unusable colours.  That mode takes those colours and applies them to
> the background.  With modus-operandi, this results in intense
> backgrounds with black text---virtually unreadable.

This could be solved in the future by allowing vc-annotate-color-map to
map numbers to faces.  But of course it would take a while until this
feature would trickle down to everyday users.

> Also, I have noticed that these user options generally don't behave as
> expected.  For example, I am using the following to experiment
> with/without backgrounds and with switching between modus-operandi and
> modus-vivendi.  For some reason, modus-operandi now always uses
> backgrounds while modus-vivendi doesn't (vc-annotate-background-mode is
> nil)...  I know that I need to re-run M-x vc-annotate for changes to
> take effect.

Hmm, that is peculiar.  In that case it might make sense to apply the
changes you propose for now.

>     (setq vc-annotate-background-mode nil)
>
>     (defun my-modus-themes-vc-annotate ()
>       ;; Actual values are for demo purposes
>       (modus-themes-with-colors
>         (if vc-annotate-background-mode
>             (setq vc-annotate-background bg-alt
>                   vc-annotate-color-map
>                   `((20 .  ,red-intense-bg)
>                     (40 .  ,red-subtle-bg)
>                     (60 .  ,red-refine-bg)
>                     (80 .  ,yellow-intense-bg)
>                     (100 . ,yellow-subtle-bg)
>                     (120 . ,yellow-refine-bg)
>                     (140 . ,magenta-intense-bg)
>                     (160 . ,magenta-subtle-bg)
>                     (180 . ,magenta-refine-bg)
>                     (200 . ,cyan-intense-bg)
>                     (220 . ,cyan-subtle-bg)
>                     (240 . ,cyan-refine-bg)
>                     (260 . ,green-intense-bg)
>                     (280 . ,green-subtle-bg)
>                     (300 . ,green-refine-bg)
>                     (320 . ,blue-intense-bg)
>                     (340 . ,blue-subtle-bg)
>                     (360 . ,blue-refine-bg)))
>           (setq vc-annotate-background nil
>                 vc-annotate-color-map
>                 `((20 . ,red)
>                   (40 . ,magenta)
>                   (60 . ,magenta-alt)
>                   (80 . ,red-alt)
>                   (100 . ,yellow)
>                   (120 . ,yellow-alt)
>                   (140 . ,fg-special-warm)
>                   (160 . ,fg-special-mild)
>                   (180 . ,green)
>                   (200 . ,green-alt)
>                   (220 . ,cyan-alt-other)
>                   (240 . ,cyan-alt)
>                   (260 . ,cyan)
>                   (280 . ,fg-special-cold)
>                   (300 . ,blue)
>                   (320 . ,blue-alt)
>                   (340 . ,blue-alt-other)
>                   (360 . ,magenta-alt-other))))))
>
>     (add-hook 'modus-themes-after-load-theme-hook #'my-modus-themes-vc-annotate)
>
>>> -    `(vc-annotate-very-old-color nil)
>>
>> It might be interesting if this could be set to some sensible value, but
>> as far as I see it has to be a colour spec.  Can an entry in
>> modus-themes-custom-variables depend on the specific Modus theme
>
> In principle, yes.  Though we could apply an appropriate gray background
> since the rest of the colour space is occupied.
Details
Message ID
<874k0z116j.fsf@protesilaos.com>
In-Reply-To
<87ee03ully.fsf@posteo.net> (view parent)
DKIM signature
missing
Download raw message
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 05 Jun 2022 16:21:45 +0000
>
> Hmm, that is peculiar.  In that case it might make sense to apply the
> changes you propose for now.

Done in commit 6f2b048.  I elaborated in the commit message.  Thank you!

By the way, this was in the modus-themes.org (now removed):

    ** Note on vc-annotate-background-mode
    :properties:
    :custom_id: h:5095cbd1-e17a-419c-93e8-951c186362a3
    :end:

    Due to the unique way ~vc-annotate~ ({{{kbd(C-x v g)}}}) applies colors, support
    for its background mode (~vc-annotate-background-mode~) is disabled at the
    theme level.

    Normally, such a drastic measure should not belong in a theme: assuming
    the user's preferences is bad practice.  However, it has been deemed
    necessary in the interest of preserving color contrast accessibility
    while still supporting a useful built-in tool.

    If there actually is a way to avoid such a course of action, without
    prejudice to the accessibility standard of this project, then please
    report as much or send patches ([[#h:9c3cd842-14b7-44d7-84b2-a5c8bc3fc3b1][Contributing]]).

-- 
Protesilaos Stavrou
https://protesilaos.com
Details
Message ID
<87r143vv91.fsf@posteo.net>
In-Reply-To
<874k0z116j.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
Protesilaos Stavrou <info@protesilaos.com> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sun, 05 Jun 2022 16:21:45 +0000
>>
>> Hmm, that is peculiar.  In that case it might make sense to apply the
>> changes you propose for now.
>
> Done in commit 6f2b048.  I elaborated in the commit message.  Thank you!

Great, thank you!
Reply to thread Export thread (mbox)