Please fix Magit diff regression #358
Comments
|
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 ( #353 ) is merged or discarded. |
|
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. |
|
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. |
You remember that wrong... ah I see you realized that already...
That's soon enough. |
|
yeah, this change was brought on by two other pull requests, one that also changed magit #341 (comment) and another that changed how color palettes are associated with a theme variant. 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 |
|
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. |
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)))) |
|
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? |
|
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)))) |
|
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. |
|
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)))) |
|
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. |
|
Related: #341. |
There are still some color choice collosion issues ( bbatsov#358 (comment) ). But highlighted diff sections are no longer hidden on block selection.
There are still some color choice collosion issues ( #358 (comment) ). But highlighted diff sections are no longer hidden on block selection.
|
merged one pull request, not closing this issue yet though. Will make attempt at solving the remaining issues ( #358 (comment) ) as well. |
|
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. |
|
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) |
|
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. |
I like that better and IMO it does not get lost. |
|
Yeah, I'm changing it now.. It was worse earlier. |
|
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. |
|
So, is there a branch that I can use to check that out? |
|
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 ***@***.***> 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
<#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
|
|
Okay I have updated and will use this for a while to see how I feel about it. |
|
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.) |
|
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. |
|
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. |
|
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. |
|
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 #354 (comment) ) but it could maybe happen. |
|
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 ***@***.***> 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.
|
|
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 55cd77b myself. Please consider tagging that commit as 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 |
|
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 it maybe would be worth exploring an option to only change the |
|
I tagged v1.3.1 now. |
|
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. |
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.
Thanks! |
On bbatsov/zenburn-emacs#334 @thomasf commented.
There's two issues with that unfortunately:
The issue is essentially the same as the regression introduced by bbatsov/zenburn-emacs#310, which was then reported and ultimately fixed in bbatsov/zenburn-emacs#317. The fix was to simply revert to pre-310, which then led to bbatsov/zenburn-emacs#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
magitbranch 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 bbatsov/zenburn-emacs#317 for why Magit's diff colors are broken right now--while it is about a different theme the same applies here.
The text was updated successfully, but these errors were encountered: