16c Diff Colors - Githubissues

habamax commented 2 years ago

We should probably do 2 sets of diff colors: dark and light suited for 16c palette where foreground should not be transparent. And apply it for all.

@romainl does it make sense?

romainl commented 2 years ago

TBH, that was my thinking when I started to work on that universal theme we talked about on IRC.

Anyway, 16c is too constrained. The main problem I see is all the potential clashes between the syntax highlighted foreground color and the background color of DiffChange/DiffText/DiffAdd.

In the image above…

the code on the left is not syntax-highlighted (:set filetype=text FWIW) and each line shows how a typical white text looks on the default "ANSI" colors (from 0 to 15 except for 13 which was an empty line so it gets the last line)
the code on the right is syntax-highlighted and the same treatment is given to each line

Whether highlighted text is readable or not is pretty much random at this point. I think it's quite clear how difficult it is to have both syntax highlighting and diff background in such a constrained context.

You will notice that using a black foreground makes almost every line more readable.

Back when I was actively designing Apprentice, that diff issue inevitably came up and I had to choose between:

optimizing for syntax highlighting but compromising on legibility
optimizing for legibility but compromising on syntax highlighting

In the end, I chose the latter because I figured syntax highlighting was just noise in that context.

FWIW:

:for i in range(1,16) | call matchadd('x' . (i-1), '\%' . i . 'l.*') | endfor
:for i in range(1,15) | execute 'hi x' . i . ' ctermbg=' . i . '  ctermfg=NONE' | endfor
:for i in range(1,15) | execute 'hi x' . i . ' ctermbg=' . i . '  ctermfg=white' | endfor
:for i in range(1,15) | execute 'hi x' . i . ' ctermbg=' . i . '  ctermfg=black' | endfor

romainl commented 2 years ago

And then, there is the CursorLine problem ;-)

habamax commented 2 years ago

do you think it is a good idea to have it not only for 16c?

romainl commented 2 years ago

This is how I do it in Apprentice, and now with my little "universal" experiment:

DiffAdd, DiffChange, DiffText, DiffDelete are all ctermbg=235 ctermfg=<some diff background color> cterm=reverse,
the "diff blocks" are not syntax-highlighted,
the rest of the buffer is syntax-highlighted,
the reverse makes the diff blocks take precedence over CursorLine so you still have it in syntax-highlighted areas and you have the line numbers column and the cursor when in a "diff block".

It's far from perfect but that's the compromise I settled for. Is it one we should shoot for? I have no idea but I think that would make things simpler for 16c diffs.

habamax commented 2 years ago

It's far from perfect but that's the compromise I settled for. Is it one we should shoot for? I have no idea but I think that would make things simpler for 16c diffs.

Let's do it.

habamax commented 2 years ago