search

bbatsov / zenburn-emacs

The Zenburn colour theme ported to Emacs
GNU General Public License v3.0
977 stars 269 forks source link

Fix regression in Magit themes #317

Closed tarsius closed 4 years ago

tarsius commented 5 years ago

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.

larstiq commented 5 years ago

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 https://github.com/bbatsov/zenburn-emacs/issues/305

tarsius commented 5 years ago

I won't work on this in January, sorry.

bbatsov commented 5 years ago

No worries, take your time.

bbatsov commented 5 years ago

@tarsius Will you be coming back to this at some point?

manuel-uberti commented 5 years ago

@tarsius friendly ping. :)

tarsius commented 5 years ago

I'll try to have a look ~ next two weeks.

tarsius commented 4 years ago

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 commented 4 years ago

Thanks!