Hello Prot!

This is a followup of sorts to my previous thread back in May:

Subject: Composing modus-themes-diffs with modus-themes-deuteranopia
Message ID: <878rqt4jhm.fsf@gmail.com> 
Date: Sun, 22 May 2022 18:31:17 +0200

(Something about Sunday afternoons, it seems!)

I'm still very much happy with how Ediff looks with the improvements you
made then.  One thing I did not pay a lot of attention to at the time
though are refinements in Magit diff buffers: would you agree that the
contrast between

* magit-diff-removed-highlight (the background for removed lines on the
  hunk at point),
* diff-refine-removed (the background for refined bits in these lines),

has taken a turn for the worse?

I'm attaching:

- modus-magit-refine-2.3.0.png: an example magit-revision buffer with
  Modus 2.3.0,

- modus-magit-refine-2.4.1.png: the same buffer, with Modus 2.4.1,

- magit-refine.el: an Elisp script to set up these example sessions[1],
  where in addition to the magit-revision buffer, I dump various
  statistics in a *Colors* buffer[2], and display the Git revision in
  the echo area for good measure,

- Colors-2.3.0 and Colors-2.4.1, the content of these buffers for both
  Modus versions.

I find it harder to see the refined bits with the ≥2.4 vivendi palette.
It's not a big deal since I often just drop into Ediff anyway and the
contrast between ediff-{current,fine}-diff-A is all right, but this has
been nagging me for the past couple of months[3].

So far I haven't thought too hard about the best way to improve
things[4], yet decided to report this now in case you have the Muse of
Emacs Themes on speed dial and she deigns impart further bits of her
ineffable colory wisdom to you.

I'd totally understand if you are getting weary of playing whack-a-mole
with diff colors though, and I can come back once I have a more concrete
proposal in the form of a patch, if you prefer.

As always, thanks a lot for all the hard work!


[1] Invoked like so:

    for e in ~/src/emacs/modus-2.{3,4}.*
    do
        HOME=$(mktemp -d) ${e}/src/emacs -Q -l magit-refine.el &
    done

    With ~/src/emacs/modus-x.y.z corresponding to a worktree of
    emacs.git set to the commit that updated the themes to x.y.z.

[2] RGB, and HSL hue, in both operandi and vivendi.

[3] It doesn't help that I spend a lot of time looking at Magit diffs,
    at $DAYJOB and elsewhere.

[4] FWIW, comparing vivendi and operandi, it seems to me that the
    operandi backgrounds are closer to #fff than the vivendi backgrounds
    are to #000; maybe diff-removed and magit-diff-removed-highlight
    could be darkened?

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Sun, 28 Aug 2022 18:58:51 +0200
>
> Hello Prot!

Hello Kevin and thank you for doing this!

> [... 8 lines elided]

> I'm still very much happy with how Ediff looks with the improvements you
> made then.  One thing I did not pay a lot of attention to at the time
> though are refinements in Magit diff buffers: would you agree that the
> contrast between
>
> * magit-diff-removed-highlight (the background for removed lines on the
>   hunk at point),
> * diff-refine-removed (the background for refined bits in these lines),
>
> has taken a turn for the worse?

Was this part of the change we had discussed and implemented as commit
11c701a (2022-05-23?

Because I can only find 0853c27 from 2021-12-02 as the last time those
colour values were edited:

    commit 0853c27b53c82ae1f760dcc81af2d4d9d326005f
    Author: Protesilaos Stavrou <info@protesilaos.com>
    Date:   Thu Dec 2 08:02:48 2021 +0200

        Refine modus-vivendi diff BGs for deuteranopia

        These tweaks improve the distinction between red and yellow in contexts
        where they appear together (e.g. smerge-mode).  Basically, yellow will
        look more bright as a yellow, while red appears as brown.

        The blue is toned down a bit to look consistent with the others.

     modus-themes.el | 8 ++++----
     1 file changed, 4 insertions(+), 4 deletions(-)

> [... 14 lines elided]

> I find it harder to see the refined bits with the ≥2.4 vivendi palette.
> It's not a big deal since I often just drop into Ediff anyway and the
> contrast between ediff-{current,fine}-diff-A is all right, but this has
> been nagging me for the past couple of months[3].

We need to improve this situation.  Though per the above commit, the
change needs to account for all the colours.

> So far I haven't thought too hard about the best way to improve
> things[4], yet decided to report this now in case you have the Muse of
> Emacs Themes on speed dial and she deigns impart further bits of her
> ineffable colory wisdom to you.

Haha, that's brilliant! 🤣

> I'd totally understand if you are getting weary of playing whack-a-mole
> with diff colors though, and I can come back once I have a more concrete
> proposal in the form of a patch, if you prefer.

No, don't worry about that.  I just want the best possible outcome and
have no problem iterating on this.  If you do have a patch, please send
it.  Otherwise I can try something on my end and share with you for
discussion.

> As always, thanks a lot for all the hard work!

You are welcome!

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com

Protesilaos Stavrou <info@protesilaos.com> writes:

>> I'm still very much happy with how Ediff looks with the improvements you
>> made then.  One thing I did not pay a lot of attention to at the time
>> though are refinements in Magit diff buffers: would you agree that the
>> contrast between
>>
>> * magit-diff-removed-highlight (the background for removed lines on the
>>   hunk at point),
>> * diff-refine-removed (the background for refined bits in these lines),
>>
>> has taken a turn for the worse?
>
> Was this part of the change we had discussed and implemented as commit
> 11c701a (2022-05-23?
>
> Because I can only find 0853c27 from 2021-12-02 as the last time those
> colour values were edited:

I think so.  I think what happened is that enabling deuteranopia colors
used to make modus-themes--diff disregard altbg:

>    (if modus-themes-deuteranopia
> -      (list :background (or deuteranbg mainbg) :foreground (or deuteranfg mainfg))
> +      (pcase modus-themes-diffs
> +        ('desaturated (list :background (or deualtbg altbg) :foreground (or deualtfg altfg)))
> +        ('bg-only (list :background (or deualtbg altbg) :foreground (if bg-only-fg (or deualtfg altfg) 'unspecified)))
> +        (_ (list :background (or deubg mainbg) :foreground (or deufg mainfg))))

So before that commit I never saw the altbg colors, despite setting
modus-themes-diffs to 'bg-only; running my repro script without
deuteranopia enabled, I see that the problem does show up in 2.3.0 as
well.

So IIUC under 'bg-only or 'desaturated, the contrast of
modus-themes-diff-refine-removed over modus-themes-diff-focus-removed
could be improved.  Currently, we have:

|                                  | mainbg                 | altbg                 |
|----------------------------------+------------------------+-----------------------|
| modus-themes-diff-refine-removed | bg-diff-refine-removed | bg-diff-focus-removed |
| modus-themes-diff-focus-removed  | bg-diff-focus-removed  | bg-diff-removed       |
| modus-themes-diff-removed        | bg-diff-focus-removed  | red-nuanced-bg        |
| magit-diff-removed               | bg-diff-removed        | red-nuanced-bg        |

So sort of a sliding scale[1], which can be visualized with:

(list-colors-display
 (seq-map 'modus-themes-color
          '(red-nuanced-bg
            bg-diff-removed
            bg-diff-focus-removed
            bg-diff-refine-removed)))

Before suggesting any concrete tweaks, I'd like to step back and spell
out a list of "constraints":

(a) Refinements (modus-themes-diff-refine-removed) should stand out in
    both diff-mode hunks and Magit hunks (modus-themes-diff-removed and
    modus-themes-diff-focus-removed, respectively).

(b) Focused hunks (modus-themes-diff-focus-removed) should stand out vs
    other hunks (magit-diff-removed), but not necessarily by a lot IMO
    (magit-diff-context-highlight already does a good job at showing
    which hunk is selected).

(c) diff-mode backgrounds (modus-themes-diff-removed and
    modus-themes-diff-refine-removed) should have altbgs that are dark
    enough to keep a good contrast with whatever foreground syntactic
    font-locking throws at us.

>> So far I haven't thought too hard about the best way to improve
>> things[4], yet decided to report this now in case you have the Muse of
>> Emacs Themes on speed dial and she deigns impart further bits of her
>> ineffable colory wisdom to you.
>
> Haha, that's brilliant! 🤣

Well, from where I stand you two seem to have a great working
relationship 😉 (just peeked at the ef-themes so far, but those look
quite nice!)

>> I'd totally understand if you are getting weary of playing whack-a-mole
>> with diff colors though, and I can come back once I have a more concrete
>> proposal in the form of a patch, if you prefer.
>
> No, don't worry about that.  I just want the best possible outcome and
> have no problem iterating on this.  If you do have a patch, please send
> it.  Otherwise I can try something on my end and share with you for
> discussion.

No patch yet; I need to mull this over some more (e.g. see footnote [1]:
I don't think I realize the full impact of changing red-nuanced-bg; in
general I'm unsure whether this problem can/should be fixed just by
changing the current color values, or if we ought to introduce new
members in modus-themes-*-colors).


Thanks for your time 🙏


[1] Idle musing: wonder if it'd make sense to try to get these colors
    closer in terms of hue, and instead play with their brightness:

    (seq-map
     (lambda (c)
       (insert
        (format
         "%s %s\n"
         (string-join
          (seq-map (lambda (val) (format "%.03f" val))
                   (apply 'color-rgb-to-hsl (color-name-to-rgb (modus-themes-color c))))
          " ")
         c)))
     '(red-nuanced-bg
       bg-diff-removed
       bg-diff-focus-removed
       bg-diff-refine-removed))
    ⇒
    0.939 0.760 0.098 red-nuanced-bg
    0.024 0.620 0.155 bg-diff-removed
    0.933 0.684 0.186 bg-diff-focus-removed
    0.000 0.538 0.339 bg-diff-refine-removed

    So when considering altbgs and refinements in Magit diffs, the
    problem is that bg-diff-focus-removed does not stand out much vs
    bg-diff-removed: I think we can see that in how close the S and L
    values are for these two, despite their hue being "so" different…
    although take that with a generous pinch of salt, I'm just playing
    around here and have no formal background in color theory 😕

    Idle musing #2: I quite like the "cooler", more "magenta" colors on
    this sample; I think I wouldn't be mad with this scale:

    (list-colors-display
     (seq-map
      (lambda (hsl)
        (apply 'color-rgb-to-hex
               (apply 'color-hsl-to-rgb hsl)))
      (list '(0.939 0.760 0.050)
            '(0.939 0.760 0.075)
            '(0.939 0.760 0.120)
            '(0.939 0.760 0.160))))

    Although I realize we probably can't just replace the current colors
    with these, since e.g. red-nuanced-bg is re-used in other, non-diff
    contexts?

    Also for now I haven't actually checked whether this satisfies all
    the "constraints" I enumerated, nor have I checked how consistent
    this looks vs "added" faces (regular and deuteranopia) 🤦

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Mon, 29 Aug 2022 09:44:09 +0200
>

> [... 29 lines elided]

> So before that commit I never saw the altbg colors, despite setting
> modus-themes-diffs to 'bg-only; running my repro script without
> deuteranopia enabled, I see that the problem does show up in 2.3.0 as
> well.

Thanks for taking the time to consider and explain these!

I think this change gives us the right results.  Maybe we can fine-tune
it a bit, but the direction looks fine:


 modus-themes.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modus-themes.el b/modus-themes.el
index ba2b87d..8021cda 100644
--- a/modus-themes.el
+++ b/modus-themes.el
@@ -634,7 +634,7 @@ (defconst modus-themes-vivendi-colors
     (bg-diff-focus-added . "#1d3c25") (fg-diff-focus-added . "#b4ddb4")
     (bg-diff-focus-added-deuteran . "#003959") (fg-diff-focus-added-deuteran . "#bfe4ff")
     (bg-diff-focus-changed . "#424200") (fg-diff-focus-changed . "#d0daaf")
-    (bg-diff-focus-removed . "#500f29") (fg-diff-focus-removed . "#eebdba")
+    (bg-diff-focus-removed . "#601f29") (fg-diff-focus-removed . "#eebdba")
 
     (bg-mark-sel . "#002f2f") (fg-mark-sel . "#60cfa2")
     (bg-mark-del . "#5a0000") (fg-mark-del . "#ff99aa")


Please give it a try and let me know what you think.

> [... 37 lines elided]

>>> So far I haven't thought too hard about the best way to improve
>>> things[4], yet decided to report this now in case you have the Muse of
>>> Emacs Themes on speed dial and she deigns impart further bits of her
>>> ineffable colory wisdom to you.
>>
>> Haha, that's brilliant! 🤣
>
> Well, from where I stand you two seem to have a great working
> relationship 😉 (just peeked at the ef-themes so far, but those look
> quite nice!)

Hehe, thanks!  I just released a new version for that package.

-- 
Protesilaos Stavrou
https://protesilaos.com

Protesilaos Stavrou <info@protesilaos.com> writes:

> diff --git a/modus-themes.el b/modus-themes.el
> index ba2b87d..8021cda 100644
> --- a/modus-themes.el
> +++ b/modus-themes.el
> @@ -634,7 +634,7 @@ (defconst modus-themes-vivendi-colors
>      (bg-diff-focus-added . "#1d3c25") (fg-diff-focus-added . "#b4ddb4")
>      (bg-diff-focus-added-deuteran . "#003959") (fg-diff-focus-added-deuteran . "#bfe4ff")
>      (bg-diff-focus-changed . "#424200") (fg-diff-focus-changed . "#d0daaf")
> -    (bg-diff-focus-removed . "#500f29") (fg-diff-focus-removed . "#eebdba")
> +    (bg-diff-focus-removed . "#601f29") (fg-diff-focus-removed . "#eebdba")
>  
>      (bg-mark-sel . "#002f2f") (fg-mark-sel . "#60cfa2")
>      (bg-mark-del . "#5a0000") (fg-mark-del . "#ff99aa")
>
>
> Please give it a try and let me know what you think.

Looks quite fine indeed; tested briefly with Magit, diff-mode & Ediff,
with modus-themes-diffs set to nil and 'bg-only, and AFAICT it works out
pretty well.

Thank you for this, that was quite the surgeon's precision on display 👌

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Mon, 29 Aug 2022 22:23:07 +0200

> [... 18 lines elided]

>> Please give it a try and let me know what you think.
>
> Looks quite fine indeed; tested briefly with Magit, diff-mode & Ediff,
> with modus-themes-diffs set to nil and 'bg-only, and AFAICT it works out
> pretty well.
>
> Thank you for this, that was quite the surgeon's precision on display 👌

Thank you!  I just pushed the change:

    commit 562ebb7e49274ed8cc4f607c6506354a44f90dde
    Author: Protesilaos Stavrou <info@protesilaos.com>
    Date:   Tue Aug 30 04:25:47 2022 +0300

        Refine modus-vivendi bg-diff-focus-removed

        This makes the default removed diff background slightly more luminant.
        The colour is seen in diff-mode, ediff, and the Magit focused diff hunk.

        When the user option 'modus-themes-diffs' is set to either 'bg-only' or
        'desaturated', this colour is used to highlight word-wise ("refined")
        changes.  The increased luminance lets it stand out more compared to the
        more subtle backdrop.

        Thanks to Kévin Le Gouguec for bringing this issue to my attention and
        for discussing it with me:
        <https://lists.sr.ht/~protesilaos/modus-themes/%3C87bks4i9tg.fsf@gmail.com%3E>

     modus-themes.el | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)


-- 
Protesilaos Stavrou
https://protesilaos.com