tinted-theming / base16-emacs

Base16 themes for Emacs
MIT License
382 stars 76 forks source link

Add missing ediff faces. #93

Closed kurnevsky closed 3 years ago

kurnevsky commented 5 years ago

Before: screenshot-2019-04-14-18:02:49 After: screenshot-2019-04-14-18:01:46

Also it seems setting background/foreground explicitly to nil is useless? So I removed it.

belak commented 5 years ago

I vaguely remember this - I think I avoided setting these on purpose because there's no "lighter green" (or other light versions of colors) face... How does this look on an actual diff with text?

kurnevsky commented 5 years ago

But isn't it the purpose of the base16 theme to make everything use only 16 colors? So not setting colors is not better than setting more than 16 colors explicitly (actually it's even worse because default colors stand out much).

How does this look on an actual diff with text?

What do you mean? On my screens you can see ediff3 with 3 diffs one active diff. Actually I don't really get the purpose of these lighter colors. Do I miss something?

kurnevsky commented 5 years ago

Ah, darker colors highlight actual difference in lines. So what can we do about it? Maybe use the same colors for all 3 buffers and different colors for diffs as lighter/darker colors are used by default? Or just same color for actual diffs, like this: screenshot-2019-04-15-09:11:36

kurnevsky commented 5 years ago

@belak I suggest to add at least C faces that are missing.

Here how I solved it in my config: https://github.com/kurnevsky/dotfiles/blob/9a4ebc2b3873ae826e93d68e0b79bb55a0ad3f42/.emacs.d/init.el#L169-L171

belak commented 5 years ago

Someone requested a :darken modifier on colors (check out this branch: https://github.com/belak/base16-emacs/tree/color-darken) . Maybe something like that would work for the additional diff faces as well?

https://github.com/belak/base16-emacs/pull/98

kurnevsky commented 5 years ago

@belak I'd rather add a easier way to access colors as variables and modify them. See what I do in my config: https://github.com/kurnevsky/dotfiles/blob/c8a350540e99b8fec67dfc92bf59030fb51d7701/.emacs.d/init.el#L206-L214

Also this PR can be merged as is because I don't touch ediff red/green/yellow colors here, only add missing diff-C faces.

kurnevsky commented 3 years ago

@belak hi. Just noticed that you mentioned this PR in the discussion and said that it would't be resolved. But I actually changed this PR to just support ediff-even-diff-C and ediff-odd-diff-C that were forgotten (emacs have ediff3 command for this). And I don't add any intermediate color variants here. Also if I got it right specifying :foreground nil :background nil doesn't do anything so I took liberty to remove it. So would you mind merging it?

kurnevsky commented 3 years ago

Here how it looks without this fix: screenshot-2021-04-06-22:48:15

belak commented 3 years ago

Thanks for commenting - you're right that this is an improvement and I probably should merge it, but it also doesn't fully resolve it. Emacs diffs tend to have changed lines be one color, and other lines in the same block a darker color, something we can't do with base16 because there aren't enough colors defined to do that.

kurnevsky commented 3 years ago

Yeah, I agree. But at least it makes ediff3 consistent with ediff2 :)

belak commented 3 years ago

Thanks for your patience - just merged it. No sense waiting until there's a perfect solution when there's one that will work for now available already. Thanks again for submitting this!

kurnevsky commented 3 years ago

Thanks. Just for the record, screenshot after: screenshot-2021-04-06-22:59:37