bbatsov / zenburn-emacs

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

Add zenburn red/green for magit diff remove/add blocks #293

Closed impaktor closed 6 years ago

impaktor commented 6 years ago

Not 100% sure on the colour selection, suggestions welcome!

(I've only changed the background, but magit also changes fg colour).

Before

2017-11-08-105730_954x612_scrot

After

2017-11-08-105350_948x614_scrot

impaktor commented 6 years ago

Maybe @basil-conto is interested in reviewing, seeing that's the author of the other open magit-related PR.

basil-conto commented 6 years ago

@impaktor #291 is indeed related to Magit but is otherwise unrelated to this PR. As for the proposed changes, I am in favour of using Zenburn colours, but I would personally prefer a bit more contrast between the foreground and background of magit-diff-added-highlight. It's up to @bbatsov to decide, though.

bbatsov commented 6 years ago

I agree the contrast should be better - light green + some text is not exactly a readable combination.

impaktor commented 6 years ago

I agree. I'll try out some other zen-colours and see what I find.

bbatsov commented 6 years ago

@impaktor ping :-)

bbatsov commented 6 years ago

@impaktor Ping 2 :-)

impaktor commented 6 years ago

Sorry, got other stuff come up. Feel free to close if you want, or anyone else can take it over. I've pointed out the variables, and approximate direction.

Vanilla magit changes both fg and bg color in the red/green fields, I think.

I might look into this after January again.

impaktor commented 6 years ago

For reference; this is vanilla magit:

2018-01-11-093740_949x614_scrot

Need to find two different green bg+fg and two different red bg+fg

bbatsov commented 6 years ago

I'll just merge it as is and tune it down the road. At least we use Zenburn colours now.

jcf commented 6 years ago

If you're using Spacemacs and want to rollback this change while it's still being "tuned", you can do so by enabling the theming layer and then adding this to your spacemacs/user-init function:

(setq theming-modifications
      '((zenburn
         (magit-diff-added :background "#335533"
                           :foreground "#ddffdd")
         (magit-diff-added-highlight :background "#336633"
                                     :foreground "#cceecc")
         (magit-diff-removed :background "#553333"
                             :foreground "#ffdddd")
         (magit-diff-removed-highlight :background "#663333"
                                       :foreground "#eecccc"))))

These are the colours Magit uses with a dark background. The light colours can be found alongside the dark ones in the Magit source code.

impaktor commented 6 years ago

@jcf please feel free to push an improved version, if you like.

bbatsov commented 6 years ago

@jcf Yeah, I think that would be best. I just don't have time to play with the colors, but I think we just need some darker shade of green for bigger contrast and this is going to be ok.

xiongtx commented 6 years ago

(Repost from here)

@bbatsov This creates an inconsistency b/t Magit and diff. E.g. magit-diff-added is no longer the same color as diff-added.

diff colors:

image

Magit colors:

image

We should make Magit inherit from diff colors . In this case, I prefer current diff colors, which are a deeper shade and easier on the eyes, but either way they should be consistent.