Please fix Magit diff regression

[tarsius]

On https://github.com/bbatsov/zenburn-emacs/issues/334 @thomasf commented.

I don't know if it helps here but I created an simple color selection algorithm for diff/refine yesterday that works very well for solarized and gruvbox palettes.

There's two issues with that unfortunately:

1. I don't know how to use those tools, please document them.
2. These changes actually break Magit's diff/refine for your themes as well.

The issue is essentially the same as the regression introduced by https://github.com/bbatsov/zenburn-emacs/pull/310, which was then reported and ultimately fixed in https://github.com/bbatsov/zenburn-emacs/pull/317. The fix was to simply revert to pre-310, which then led to https://github.com/bbatsov/zenburn-emacs/issues/334.

Lets not allow this to escalate the same way here. I.e. we should either fix or revert this immediately instead of letting it sit in a broken state that some users then grow attached to.

I did have a stab at this which you can see in the magit branch of my fork, but because of issue (1) above I was not able to test if that is any good.

Please read my initial report in https://github.com/bbatsov/zenburn-emacs/pull/317 for why Magit's diff colors are broken right now--while it is about a different theme the same applies here.

[thomasf]

From what I remember the highlighting already was making individual words diffs invisible when a selection and refined hunks were overlayed before my patches. I recall thinking that it was a bit strange but proceeded without investigating that any further.

I can probably fix it if it's supported by the faces but not until maybe early next week, I have already promised to not commit anything to master until ( https://github.com/bbatsov/solarized-emacs/pull/353 ) is merged or discarded.

[thomasf]

And about the tooling, yes it's very bare bones and not documented until I know that everything is covered. After that I might port it to elisp or at the very least document it and probably make it easy to use from the command line stand alone without having to know how to write Go.

[thomasf]

No, I must have swithced some comparison samples up when I was creating that patch. In any case I'll have a look at it.. I hope it's not too hard to fix since I still have a lot of this in my head.

[tarsius]

From what I remember the highlighting already was making individual words diffs invisible when a selection and refined hunks were overlayed before my patches. I recall thinking that it was a bit strange but proceeded without investigating that any further.

You remember that wrong... ah I see you realized that already...

I can probably fix it if it's supported by the faces but not until maybe early next week,

That's soon enough.

[thomasf]

yeah, this change was brought on by two other pull requests, one that also changed magit https://github.com/bbatsov/solarized-emacs/pull/341#issuecomment-549185060 and another that changed how color palettes are associated with a theme variant. It's been a lot this week,

[tarsius]

It's been a lot this week,

I recommend you don't try Emacs 27 before you have sorted all that out. That will open a whole new can of worms. The new :extend face attribute is gonna be a lot of fun.

[thomasf]

It seems like it might be possible to cram in yet another level of highlight the simplest solution would be just to not change the diffs when highlighting a region..

Doing that does however make it less clear what the selection is because ,base02 as a backgorund color. Picking one of the other generated extra fg/bg pairs (in this case cyan) reemphatizes the selection..

same for zenburn

If possible it would be great to avoid additional colors being added.

The sudden appearance of cyan on selection does feel a bit surprising in a not all positive way though because it probably makes less sense in an intuitive way but at least the fg/bg contrast of each individual block is a lot closer to the palettes own contrasts than with the default magit colors.

[thomasf]

diff --git a/solarized-faces.el b/solarized-faces.el
index 027b813..38d8165 100644
--- a/solarized-faces.el
+++ b/solarized-faces.el
@@ -574,8 +574,8 @@ customize the resulting theme."
                                                   :weight normal :slant italic))))
 ;;;;; fixmee
        `(fixmee-notice-face ((,class (:background nil :foreground ,base1
-                                                  :underline nil :slant italic :weight bold))))
-
+                                                  :underline nil :slant italic :weight bold)))
+)
 ;;;;; flx
        `(flx-highlight-face ((,class (:foreground ,blue
                                                   :weight normal :underline nil))))
@@ -1035,7 +1035,7 @@ customize the resulting theme."
                           :weight bold))))
        `(magit-diff-lines-heading          ((t (:background ,orange
                                                             :foreground ,base3))))
-       `(magit-diff-context-highlight      ((t (:background ,base02))))
+       `(magit-diff-context-highlight      ((t (:background ,cyan-1bg :foreground ,cyan-1fg))))
        `(magit-diffstat-added              ((t (:foreground ,s-diffstat-added-fg))))
        `(magit-diffstat-removed            ((t (:foreground ,s-diffstat-removed-fg))))
 ;;;;;; process
@@ -1103,14 +1103,16 @@ customize the resulting theme."
                                                :background unspecified))))

        `(magit-diff-added ((,class (:background ,green-1bg :foreground ,green-1fg))))
-       `(magit-diff-added-highlight ((,class (:background ,green-2bg :foreground ,green-2fg))))
+       `(magit-diff-added-highlight ((,class (:background ,green-1bg :foreground ,green-2fg))))
+       ;; `(magit-diff-added-highlight ((,class (:background ,(solarized-color-blend green-1bg green-2bg 0.5)
+                                                          ;; :foreground ,(solarized-color-blend green-1fg green-2fgl 0.5)))))
        `(magit-diff-base ((,class (:background ,yellow-1bg :foreground ,yellow-1fg))))
-       `(magit-diff-base-highlight ((,class (:background ,yellow-2bg  :foreground ,yellow-2fg))))
+       `(magit-diff-base-highlight ((,class (:background ,yellow-1bg  :foreground ,yellow-2fg))))

        `(magit-diff-conflict-heading ((,class (:inherit magit-diff-hunk-heading))))
        `(magit-diff-context ((,class (:foreground ,base0))))
        `(magit-diff-removed ((,class (:background ,red-1bg :foreground ,red-1fg))))
-       `(magit-diff-removed-highlight ((,class (:background ,red-2bg :foreground ,red-2fg))))
+       `(magit-diff-removed-highlight ((,class (:background ,red-1bg :foreground ,red-2fg))))

        `(markdown-comment-face ((,class (:foreground ,base01 :strike-through t))))

[thomasf]

Over all it feels good to browse a diff when only the context switches colors because it keeps the lines you are most interested in from changing. When the context is removed (- - -) the hunk header still gives a very small highlight. I don't know how often people would browse diffs with 0 context lines though so maybe it's nothing that should be optimized against?

[thomasf]

Going further by adding the two levels of highlight for yellow to the hunk header makes that situation a little bit better but it's starting to get busy with colors overall instead.

diff --git a/solarized-faces.el b/solarized-faces.el
index 027b813..8dc505a 100644
--- a/solarized-faces.el
+++ b/solarized-faces.el
@@ -1025,17 +1025,16 @@ customize the resulting theme."
        `(magit-diff-file-heading-highlight ((t (:background ,base02))))
        `(magit-diff-file-heading-selection ((t (:background ,base02
                                                             :foreground ,orange))))
-       `(magit-diff-hunk-heading
-         ((t (:background ,(solarized-color-blend yellow base03 0.1)))))
-       `(magit-diff-hunk-heading-highlight
-         ((t (:background ,(solarized-color-blend yellow base02 0.1)))))
+       `(magit-diff-hunk-heading ((t (:background ,yellow-1bg :foreground ,yellow-1fg))))
+
+       `(magit-diff-hunk-heading-highlight ((t (:background ,yellow-2bg :foreground ,yellow-2fg))))
        `(magit-diff-hunk-heading-selection
          ((t (:background ,(solarized-color-blend yellow base02 0.1)
                           :foreground ,orange
                           :weight bold))))
        `(magit-diff-lines-heading          ((t (:background ,orange
                                                             :foreground ,base3))))
-       `(magit-diff-context-highlight      ((t (:background ,base02))))
+       `(magit-diff-context-highlight      ((t (:background ,cyan-1bg :foreground ,cyan-1fg))))
        `(magit-diffstat-added              ((t (:foreground ,s-diffstat-added-fg))))
        `(magit-diffstat-removed            ((t (:foreground ,s-diffstat-removed-fg))))
 ;;;;;; process
@@ -1103,14 +1102,14 @@ customize the resulting theme."
                                                :background unspecified))))

        `(magit-diff-added ((,class (:background ,green-1bg :foreground ,green-1fg))))
-       `(magit-diff-added-highlight ((,class (:background ,green-2bg :foreground ,green-2fg))))
+       `(magit-diff-added-highlight ((,class (:background ,green-1bg :foreground ,green-2fg))))
        `(magit-diff-base ((,class (:background ,yellow-1bg :foreground ,yellow-1fg))))
-       `(magit-diff-base-highlight ((,class (:background ,yellow-2bg  :foreground ,yellow-2fg))))
+       `(magit-diff-base-highlight ((,class (:background ,yellow-1bg  :foreground ,yellow-2fg))))

        `(magit-diff-conflict-heading ((,class (:inherit magit-diff-hunk-heading))))
        `(magit-diff-context ((,class (:foreground ,base0))))
        `(magit-diff-removed ((,class (:background ,red-1bg :foreground ,red-1fg))))
-       `(magit-diff-removed-highlight ((,class (:background ,red-2bg :foreground ,red-2fg))))
+       `(magit-diff-removed-highlight ((,class (:background ,red-1bg :foreground ,red-2fg))))

        `(markdown-comment-face ((,class (:foreground ,base01 :strike-through t))))

[thomasf]

yellow instead of cyan for context and header makes it look more as something that is expected:

to

So maybe letting the context headers and selected context highlight share a base color is a good idea. This does hovever "merge" the next unselected block at the end so there are drawbacks here as well.

[thomasf]

back to cyan for context diff but keeping the new yellow-2 fg/bg for selected header. AFAIK this causes no collisions in color selection and keeps the overall contrasts in the best way possible.

diff --git a/solarized-faces.el b/solarized-faces.el
index 027b813..234dec9 100644
--- a/solarized-faces.el
+++ b/solarized-faces.el
@@ -1025,17 +1025,15 @@ customize the resulting theme."
        `(magit-diff-file-heading-highlight ((t (:background ,base02))))
        `(magit-diff-file-heading-selection ((t (:background ,base02
                                                             :foreground ,orange))))
-       `(magit-diff-hunk-heading
-         ((t (:background ,(solarized-color-blend yellow base03 0.1)))))
-       `(magit-diff-hunk-heading-highlight
-         ((t (:background ,(solarized-color-blend yellow base02 0.1)))))
+       `(magit-diff-hunk-heading ((t (:background ,yellow-1bg :foreground ,yellow-1fg))))
+       `(magit-diff-hunk-heading-highlight ((t (:background ,yellow-2bg :foreground ,yellow-2fg))))
        `(magit-diff-hunk-heading-selection
          ((t (:background ,(solarized-color-blend yellow base02 0.1)
                           :foreground ,orange
                           :weight bold))))
        `(magit-diff-lines-heading          ((t (:background ,orange
                                                             :foreground ,base3))))
-       `(magit-diff-context-highlight      ((t (:background ,base02))))
+       `(magit-diff-context-highlight      ((t (:background ,cyan-1bg :foreground ,cyan-1fg))))
        `(magit-diffstat-added              ((t (:foreground ,s-diffstat-added-fg))))
        `(magit-diffstat-removed            ((t (:foreground ,s-diffstat-removed-fg))))
 ;;;;;; process
@@ -1103,15 +1101,14 @@ customize the resulting theme."
                                                :background unspecified))))

        `(magit-diff-added ((,class (:background ,green-1bg :foreground ,green-1fg))))
-       `(magit-diff-added-highlight ((,class (:background ,green-2bg :foreground ,green-2fg))))
+       `(magit-diff-added-highlight ((,class (:background ,green-1bg :foreground ,green-1fg))))
        `(magit-diff-base ((,class (:background ,yellow-1bg :foreground ,yellow-1fg))))
-       `(magit-diff-base-highlight ((,class (:background ,yellow-2bg  :foreground ,yellow-2fg))))
+       `(magit-diff-base-highlight ((,class (:background ,yellow-1bg  :foreground ,yellow-1fg))))

        `(magit-diff-conflict-heading ((,class (:inherit magit-diff-hunk-heading))))
        `(magit-diff-context ((,class (:foreground ,base0))))
        `(magit-diff-removed ((,class (:background ,red-1bg :foreground ,red-1fg))))
-       `(magit-diff-removed-highlight ((,class (:background ,red-2bg :foreground ,red-2fg))))
-
+       `(magit-diff-removed-highlight ((,class (:background ,red-1bg :foreground ,red-1fg))))

        `(markdown-comment-face ((,class (:foreground ,base01 :strike-through t))))
        `(markdown-footnote-face ((,class (:inherit default))))

[thomasf]

I think the last one feels pretty solid and from what I can tell, it keeps the fg/bg contrasts consistent even if it means that the whole selected block isn't brightened.

With merge diffs we get a collision with usage of yellow-1fg/bg

Creating a region maybe doens't look the best it could but It's still clear what is happening.

These are the only places where I can see any problems at all (and they might also be fixable). IMHO the overall contrast balance is a larger net win than these issues, but maybe there is more that can be done.

Even without these fixes the new colors actually made me prefer looking at diffs through magit-diff instead of some external tool only because the relative color harmony is fixed so there is that as well.

[tarsius]

Related: #341.

[thomasf]

The actual breaking is probably more related to https://github.com/bbatsov/solarized-emacs/pull/348 though which came after #341 . I made a mistake with my test environment to not have any refine hunks in the git scenarios I have created to test with so I didn't catch the breakage while assigning new colors.

[thomasf]

merged one pull request, not closing this issue yet though. Will make attempt at solving the remaining issues ( https://github.com/bbatsov/solarized-emacs/issues/358#issuecomment-552196036 ) as well.

[tarsius]

I can't say I like the cyan context.

I would still strongly prefer if the magit diffs changed their look the same way as they did before the refactoring except for the actual shades being more solarizedy. I quite confident that that would be possible.

I guess I could live with the added and removed lines not changing at all on focus and the context taking on a different shade of solarized yellowish background.

Sorry for not replying earlier and not attempting to do this myself but the conflict in #354 makes me wanna stay away from all of this. By the way I am not blaming either one of you. Things like this happen eventually if you maintain or contribute to a popular project.

[thomasf]

Yeah, the cyan in magit-diff-context-highlight certainly isn't ideal looking. I will try to figure something better out later when other color related issues are worked out. At least it's not broken now. Using the regular highlight with base02 background kind of works as well but it gets lost among the more pronounced accent colors.

(It actually looks a lot better now than it did when I started because the selected yellow heading is more visible)

[thomasf]

I don't think there is much room for adding yet another highlight level which adds to the 2bg/2fg ones for a selected item purpose.. The darkest/lightest colors (depending on theme variant) are already almost topping/bottoming out for several colors. They will just starting to go towards black/white if being pushed further and that looks strange.

[tarsius]

Using the regular highlight with base02 background kind of works as well but it gets lost among the more pronounced accent colors.

I like that better and IMO it does not get lost.

[thomasf]

Yeah, I'm changing it now.. It was worse earlier.

[thomasf]

Having completed a full working day with the current changes I might actually prefer that the diffs regions themselves don't change in relation to segment selection selection. It feels slightly better that it's only the less important context around the diffs that changes.

This is of course yet again one of those things that are impossible to know without an eye tracking user study or something like that. I can very well just be fooling myself that it's true because one of the other changes as well.

[tarsius]

So, is there a branch that I can use to check that out?

[thomasf]

I was referring to what was committed to master yesterday, what you saw in the screenshot earlier. It’s in Melpa

On Mon, 18 Nov 2019 at 20:09, Jonas Bernoulli notifications@github.com wrote:

So, is there a branch that I can use to check that out?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/bbatsov/solarized-emacs/issues/358?email_source=notifications&email_token=AACQYX2J4CLE7UG3ET336NTQULR7VA5CNFSM4JLH4B3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEELSAFQ#issuecomment-555163670, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACQYX34NOHXGJA4BPPO5MLQULR7VANCNFSM4JLH4B3A .

-- Thomas Frössman http://t.jossystem.se

[tarsius]

Okay I have updated and will use this for a while to see how I feel about it.

[tarsius]

I think the highlighted hunk headers are way too much in your face baaam!

(Especially since the important parts of the diff are now very washed out.)

[thomasf]

They are not very washed out but certianly more washed out than regular solarized colors. There is a relative LAB perceptual lightness difference of 50% between base0 and base03 or base1 and base02.

These new fg/bg pairs all have slightly over 40% fg/bg lightness contrast (it differs a little bit depending on the color) so it is dimmer but not super much..

Ideally I would like to tweak it into around 50% everywhere. I do not yet know if it's possible. Since we blend a little bit towards the background I guess that the yellow pairs (hunk header) might have a tendency to become more yellow (increased saturation) than the amount of green that green has. Solarized dark is overall a little bit worse I think it's not that hard to get it into a better place.

Personally I think that a more empathized hunk header frames the selection in a good way but that's of course always debatable.

Lightness by itself does of course not answer all questions and LAB is an approximation of vision so how it actually feels is also important but lightness is an important aspect of the original solarized palette and therefore I think it should be an guiding factor for these colors as well.

[thomasf]

I need to develop the color generation tools a bit more before going any further with this.. I have just refactored out most of the code stuff into something that is more declarative and composable ( https://github.com/bbatsov/solarized-emacs/blob/master/colorlab/main.go#L45 ). It's at least heading towards the point where it's easy to maybe create an web ui with sliders or something to get better feedback while trying things out. I did write som elisp to autoreload the theme but that is a bit limiting, I thik I want need side by side comparisons with multiple palette mutations at once because. A browser interface is maybe the way to go, would be easy to just html export some emacs screens and then just replace colors by css or something along those lines.

[blaenk]

For what it's worth, I was such a huge fan of the old color scheme in magit. I absolutely loved the colors and the aesthetic of the magit-status buffer with solarized light, so much so that I think I'm just gonna dive into the commit history to find the old colors and create my own palette or something.

Maybe it can be part of solarized-emacs so users can opt into it? I mean we're already even having gruvbox and who knows what else in here, doesn't seem like that much of a stretch.

[thomasf]

I always kind of disliked all colors ever up until the recent changes because they never felt like something that actually fit in with its surroundings, now I prefer magit to most web based diff viewers (I used to go to my companies internal gitlab/github to look at history instead). Especially Solarized dark has always been an extra bad fit, colors that only works for one variant is kind of out of the question if we are going to take this seriously.

I'm at least somewhat against to adding larger swats of opt outs until we have something that makes composability easier ( as noted here https://github.com/bbatsov/solarized-emacs/issues/354#issuecomment-550583426 ) but it could maybe happen. Until then you have child theme feature which is there because not everyone will like the same look and the option to just fork the whole theme if you think that will serve you better so there are more then one option already available. I think the old colors were just magits own colors.

[articuluxe]

People can always fork if they want. For what it’s worth, I love the new changes including the diffs, it’s like a new theme that combines the best of all variants up through now. (Well, except for the show-paren-match face that I have to customize, but it wouldn’t be emacs otherwise...)

On Nov 24, 2019, at 2:32 AM, Thomas Frössman notifications@github.com wrote:

I always kind of disliked all colors ever up until the recent changes because they never felt like something that actually fit in, especially Solarized dark has always been an extra bad fit. Something that only works for one variant is kind of out of the question if we are going to take this seriously.

I'm at least somewhat against to adding larger swats of opt outs until we have something that makes composability easier ( as noted here #354 (comment) ) but it could maybe happen.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[tarsius]

I am closing this now because I consider the new colors good enough for the most part.

(Never-the-less I am going to stick to 55cd77b61b6968048c61e13358ba487d217f24c0 myself. Please consider tagging that commit as v1.3.1, thanks.)

I would encourage one change though:

diff --git a/solarized-faces.el b/solarized-faces.el
index 8efba61..78c3b5a 100644
--- a/solarized-faces.el
+++ b/solarized-faces.el
@@ -1044,13 +1044,13 @@ (defvar solarized-definition
      `(magit-diff-hunk-heading
        ((t (,@(and (>= emacs-major-version 27) '(:extend t))
             :background ,yellow-1bg
             :foreground ,yellow-1fg))))
      `(magit-diff-hunk-heading-highlight
        ((t (,@(and (>= emacs-major-version 27) '(:extend t))
-            :background ,yellow-2bg
+            :background ,(solarized-color-blend yellow base02 0.1)
             :foreground ,yellow-2fg))))
      `(magit-diff-hunk-heading-selection
        ((t (,@(and (>= emacs-major-version 27) '(:extend t))
             :background ,(solarized-color-blend yellow base02 0.1)
             :foreground ,orange
             :weight bold))))

I.e. use the same background color for magit-diff-hunk-heading-highlight as for magit-diff-hunk-heading-selection.

[thomasf]

I have been paying attention to the various interactive merge and diff viewers I have come across since making this change and one thing that none of them do is change how the active diff region looks when a hunk is selected. If they do have highlighting it's almost always emphasized using a clear left (or middle in the case of side by side merge diffs) column that frames the active selection.
I think that it makes some sens that there isn't two different color combinations representing the same information on screen at once which happens with an overlay color style.

I think it maybe would be worth exploring an option to only change the +/-/ column to the left when highlighting a hunk. That would of course maybe present a whole new problem for solarized if we need a separate color for that as well but I haven't really though that far because I'm thinking along the lines of end user usability and not how problematic the implementation would be in this theme right now.

[thomasf]

I tagged v1.3.1 now.

[thomasf]

continue in regards to hunk highlight.. I haven't done a deep analysis, just been paying attention to the tools I have happened to be using professionally and it's mostly web based code review tools that are meant for collaborative reading of changesets.

When I launched meld now I see that id actually does highlighting in a way similar to magit so magit isn't entirely alone in doing this.

(the middle section is highlighted here)

I do have a feeling that a UX designer maybe would point towards not having multiple colors representing the same thing usually is a positive thing. I currently does not have daily access to an UX designer so I don't have an actual professional to ask atm.

[tarsius]

I think that it makes some sens that there isn't two different color combinations representing the same information on screen at once which happens with an overlay color style.

I actually like the highlighting, but if that feature had not already existed when I became the maintainer, then I would probably not have implemented it. It certainly causes a disproportional amount of work, especially when it comes to diffs.

I tagged v1.3.1 now.

Thanks!