Fix regression in Magit themes #317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
How can we get this moving forward? I'd love to get my diff-refinement in magit back. There certainly seem to be related issues filed, like #305 |
|
I won't work on this in January, sorry. |
|
No worries, take your time. |
|
@tarsius Will you be coming back to this at some point? |
|
@tarsius friendly ping. :) |
|
I'll try to have a look ~ next two weeks. |
Use "C+N" > "C" > "C-N" in all cases.
Make the definitions of Magit's Zenburn faces look like those of the Solarized theme. I now use the latter but used to use the former, and making this look similar for both themes makes it easier for me to fix the regressions introduced by others. Do the same for the diff faces.
Make the definitions of Magit's Zenburn faces look like those of the Solarized theme. I now use the latter but used to use the former, and making this look similar for both themes makes it easier for me to fix the regressions introduced by others.
This reverts commit 8856da4 from pull-request #300. Also see #317. The goal of that commit was to make the diffs prettier by making them darker, which I agree was a success. Unfortunately the diffs not only got prettier but also lost information. The `diff-refine-*' faces quite obviously must be different from the `magit-diff-*-highlight' faces and the commit being reverted violated that in the most explicit way possible by making the latter inherit from the former. The refinement or "word-level" faces are used to change the parts of a line that changed. The highlighting faces are used to emphasize the "selected" part of a diff, e.g. the part of the diff the user is currently concentrating on by placing the cursor inside that part of the diff. Obviously if one concentrates on a part of a diff, then one does not want that part to become *less* informative until one focuses elsewhere. I tried fixing this regression without going as far as to completely reverting the faulty commit, unfortunately I was unable to do so.
|
I have tried to fix this without going as far as to completely revert the original change, but I did not succeed and so I have completely reverted it after all. I think you should merge this now. If someone else would like to use darker colors for diffs, then here are my suggestions: Make the word-level differences darker and more saturated than the basic diff colors, and make the highlighted basic colors lighter than the basic diff colors. The current colors follow this logic already--just make every color darker. |
bbatsov
pushed a commit
that referenced
this pull request
Aug 9, 2019
This reverts commit 8856da4 from pull-request #300. Also see #317. The goal of that commit was to make the diffs prettier by making them darker, which I agree was a success. Unfortunately the diffs not only got prettier but also lost information. The `diff-refine-*' faces quite obviously must be different from the `magit-diff-*-highlight' faces and the commit being reverted violated that in the most explicit way possible by making the latter inherit from the former. The refinement or "word-level" faces are used to change the parts of a line that changed. The highlighting faces are used to emphasize the "selected" part of a diff, e.g. the part of the diff the user is currently concentrating on by placing the cursor inside that part of the diff. Obviously if one concentrates on a part of a diff, then one does not want that part to become *less* informative until one focuses elsewhere. I tried fixing this regression without going as far as to completely reverting the faulty commit, unfortunately I was unable to do so.
|
Thanks! |
This was referenced Aug 14, 2019
archit-rastogi
pushed a commit
to archit-rastogi/zenburn-emacs
that referenced
this pull request
May 24, 2020
This reverts commit 8856da4 from pull-request bbatsov#300. Also see bbatsov#317. The goal of that commit was to make the diffs prettier by making them darker, which I agree was a success. Unfortunately the diffs not only got prettier but also lost information. The `diff-refine-*' faces quite obviously must be different from the `magit-diff-*-highlight' faces and the commit being reverted violated that in the most explicit way possible by making the latter inherit from the former. The refinement or "word-level" faces are used to change the parts of a line that changed. The highlighting faces are used to emphasize the "selected" part of a diff, e.g. the part of the diff the user is currently concentrating on by placing the cursor inside that part of the diff. Obviously if one concentrates on a part of a diff, then one does not want that part to become *less* informative until one focuses elsewhere. I tried fixing this regression without going as far as to completely reverting the faulty commit, unfortunately I was unable to do so.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This isn't done yet. Actually I haven't even begun to work on it yet and for now this pr only contains some preparatory commits.
Theming Magit faces is quite difficult. I have written a 1300 word text on the matter, and that merely mentions the things one has to be aware when doing so. If someone attempts to do it without reading and understanding that, then the likelihood of breaking functionality is very high, as has happened in #300.
Just because I have written that text doesn't mean it is easy for me to do that. Fixing the regression introduced by #300 will be difficult, if we don't just revert it. But I don't want to do that because I think the new base colors actually do look better.
And then I got side-tracked of course (see #315 #316). I don't know when I will return to this, so I will just mention what the issue is, in case there are bug reports:
It is wrong to use the same color for "word-level differences" (
diff-refine-*) and highlighting (magit-diff-*-highlight) of the current diff. The current diff, like any diff, may contain word-level differences; we don't want them to become invisible when we are "narrowing in" on them.