Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking-Change]: re-work the (default) Visual Highlighting #13663

Closed
wants to merge 2 commits into from

Conversation

chrisbra
Copy link
Member

The default visual highlighting currently is nice in that it overlays the actual syntax highlighting by using a separate distinct background color.

However, this can cause hard to read text, because the contrast between the actual syntax element and the background color is way too low. That is an issue, that has been bothering colorschemes authors for quite some time so much, that they are defining the Visual highlighting group to use a separate foreground and background color, so that the syntax highlighting vanishes, but the text remains readable (ref: vim/colorschemes#250)

So this is an attempt to perform the same fix for the default Visual highlighting and just use a default foreground and background color instead of using reverse.

Current issues:

  • doesn't work well in 8 color mode, since the background and foreground colors used remain un-distinguishable.

I also removed the hard-coded changes to the Visual highlighting in init_highlight. It's not quite clear to me, why those were there and not added directly to the highlighting_init_<dark|light> struct.

I hope this makes kind of sense.

@habamax
Copy link
Contributor

habamax commented Dec 11, 2023

Windows terminal

Light on light

background=light and actual terminal background is light (termguicolors is set):

image

background=light and actual terminal background is light (termguicolors is NOT set, t_Co=256):

image

background=light and actual terminal background is light (termguicolors is NOT set, t_Co=16):

image

background=light and actual terminal background is light (termguicolors is NOT set, t_Co=8):

image

background=light and actual terminal background is light (termguicolors is NOT set, t_Co=2):

image

Dark on light

background=dark and actual terminal background is light (termguicolors is set):

image

background=dark and actual terminal background is light (termguicolors is NOT set, t_Co=256):

image

background=dark and actual terminal background is light (termguicolors is NOT set, t_Co=16):

image

background=dark and actual terminal background is light (termguicolors is NOT set, t_Co=8):

image

background=dark and actual terminal background is light (termguicolors is NOT set, t_Co=2):

image

@habamax
Copy link
Contributor

habamax commented Dec 11, 2023

I would keep GUI Visual as before -- It didn't have the issue we try to fix (left is NEW, right is OLD):

image

Well, at least default visual with bg=light, with bg=dark and hi normal guibg=black guifg=white it is not that good, although still readable:

image

@chrisbra
Copy link
Member Author

I have no idea, why compilation fails in CI 🤔

neutaaaaan added a commit to neutaaaaan/vim that referenced this pull request Dec 26, 2023
Applying ChrisBra's temptative fixes to highlight.c
src/highlight.c Outdated
CENT("Visual term=reverse",
"Visual term=reverse guibg=LightGrey"),
CENT("Visual ctermbg=DarkGrey ctermfg=White",
"Visual ctermbg=DarkGrey ctermfg=White guibg=#6b6b6b guifg=LightGrey"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not to leave GUI visual as it was before,
I would have made foreground white here then:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reiterate on GUI visual hl, I would leave light bg the same as before (guifg=NONE guibg=LightGrey):

image

And would made dark bg a bit more visible(guifg=NONE guibg=#444444):

image

The main problem with default Visual is in terminal worlds where it is mostly unreadable.

src/highlight.c Outdated
CENT("Visual term=reverse",
"Visual term=reverse guibg=DarkGrey"),
CENT("Visual ctermbg=235 ctermfg=LightGrey",
"Visual ctermbg=235 ctermfg=LightGrey guifg=LightGrey guibg=#262626"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a bit lighter guibg (#444444) would be better:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ctermbg=235 is too optimistic, I think DarkGray would be a safer bet

with ctermfg=White:
image

with ctermfg=Black:
image

@romainl, @neutaaaaan what do you think?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your tweaks to the 24bit version are fine. It's a completely different colourscheme anyway.

On the other hand, I disagree with the changes made to the 256c selections , as both versions are inherently 8c colourschemes that haphazardly use colors from the extended palette.
Being the default colourscheme, this is actually a fairly sensible behavior to ensure the best compatibility possible regardless of configuration mishaps, and I've extended that behavior by bolting as many of the hl groups used during common editing down in my back of the napkin patch referenced earlier.

This is what the end user will see first in most cases.
This is what the end user will end up using in most cases.
We cannot ever assume their environment will be set up correctly.
We cannot ever assume vim will be able to autodetect the terminal background color.
The only safe assumption, is that the light version of the colorscheme is going to be loaded, eventually, and rework that as the new background agnostic default.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grey is hard to get right in 8c and 16c.

Assuming the user's terminal emulator's palette follows the same structure as the default xterm palette, we only have two greys that can be reliably used for background in 16c:

  • 7 is very light at #dfdfdf,
  • 8 is very dark at #6c6c6c.

This limits what can be done for visual selection if we insist on using grey:

  • 0 on 7 works,
  • 7 on 8 works,
  • 15 on 8 works.

In 8c, the only viable grey background is 7, which restricts us to:

  • 0 on 7

My advice would thus be to use ctermbg=grey ctermfg=black for both 8c (because of 8c limitations) and 16c (because of consistency with 8c):

Capture d’écran 2024-01-23 à 17 41 56

Note that the limited grey palette is quite problematic when it comes to UI. A mostly grey chrome is quite practical because it doesn't distract from the syntax highlighting. The problem is that we have lots of greys to play with in GUI and 256c but very few in 16c and 8c. If we shoot for consistency, we should, IMO keep all the greys we can for the chrome (status line, tab line, completion menu, etc.). This means using colors as much as possible for in-buffer highlighting.

Something like the following might be more effective?

Capture d’écran 2024-01-23 à 18 03 00

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grey is hard to get right in 8c and 16c.
Which is why I don't use it.

The only available colors that in the default colourschemes don't outright disappear inside diffs would be green and yellow.
I settled on black foreground, yellow background long ago.

Please take a look at this branch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert the gui changes, even so I think it doesn't work very well with 'termguicolors' set in dark mode:
grafik

I'll also make the suggested changes to use ctermbg=grey ctermfg=black then.

src/highlight.c Outdated
CENT("Visual term=reverse",
"Visual term=reverse guibg=DarkGrey"),
CENT("Visual ctermbg=Grey ctermfg=Black",
"Visual ctermbg=Grey ctermfg=Black guibg=DarkGrey"),
Copy link
Contributor

@habamax habamax Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you mentioned earlier, in bg=dark, gui visual looks not that great, we can make it grey34:

image

The default visual highlighting currently is nice in that it overlays
the actual syntax highlighting by using a separate distinct background
color.

However, this can cause hard to read text, because the contrast
between the actual syntax element and the background color is way too
low. That is an issue, that has been bothering colorschemes authors for
quite some time so much, that they are defining the Visual highlighting
group to use a separate foreground and background color, so that the
syntax highlighting vanishes, but the text remains readable (ref:
vim/colorschemes#250)

So this is an attempt to perform the same fix for the default Visual
highlighting and just use a default foreground and background color
instead of using reverse.

I also removed the hard-coded changes to the Visual highlighting in
init_highlight. It's not quite clear to me, why those were there and not
added directly to the highlighting_init_<dark|light> struct.

Signed-off-by: Christian Brabandt <cb@256bit.org>
@chrisbra chrisbra marked this pull request as ready for review January 25, 2024 22:25
@chrisbra
Copy link
Member Author

okay, I think I have addressed all concerns and I mark it as ready.

@habamax
Copy link
Contributor

habamax commented Jan 26, 2024

Looks good!

Now we need to tackle other things (diff colors and others)

@chrisbra chrisbra closed this in e6d8b46 Jan 28, 2024
echasnovski added a commit to echasnovski/neovim that referenced this pull request Jan 29, 2024
Problem:  UX of visual highlighting can be improved
Solution: Improve readibility of visual highlighting,
          by setting better foreground and background
          colors

The default visual highlighting currently is nice in that it overlays
the actual syntax highlighting by using a separate distinct background
color.

However, this can cause hard to read text, because the contrast
between the actual syntax element and the background color is way too
low. That is an issue, that has been bothering colorschemes authors for
quite some time so much, that they are defining the Visual highlighting
group to use a separate foreground and background color, so that the
syntax highlighting vanishes, but the text remains readable (ref:
vim/colorschemesvim/vim#250)

So this is an attempt to perform the same fix for the default Visual
highlighting and just use a default foreground and background color
instead of using reverse.

I also removed the hard-coded changes to the Visual highlighting in
init_highlight. It's not quite clear to me, why those were there and not
added directly to the highlighting_init_<dark|light> struct.

closes: vim/vim#13663
related: vim/colorschemesvim/vim#250

vim/vim@e6d8b46

Co-authored-by: Christian Brabandt <cb@256bit.org>
zeertzjq pushed a commit to neovim/neovim that referenced this pull request Jan 29, 2024
Problem:  UX of visual highlighting can be improved
Solution: Improve readibility of visual highlighting,
          by setting better foreground and background
          colors

The default visual highlighting currently is nice in that it overlays
the actual syntax highlighting by using a separate distinct background
color.

However, this can cause hard to read text, because the contrast
between the actual syntax element and the background color is way too
low. That is an issue, that has been bothering colorschemes authors for
quite some time so much, that they are defining the Visual highlighting
group to use a separate foreground and background color, so that the
syntax highlighting vanishes, but the text remains readable (ref:
vim/colorschemesvim/vim#250)

So this is an attempt to perform the same fix for the default Visual
highlighting and just use a default foreground and background color
instead of using reverse.

I also removed the hard-coded changes to the Visual highlighting in
init_highlight. It's not quite clear to me, why those were there and not
added directly to the highlighting_init_<dark|light> struct.

closes: vim/vim#13663
related: vim/colorschemes#250

vim/vim@e6d8b46

Co-authored-by: Christian Brabandt <cb@256bit.org>
{
do_highlight((char_u *)"Visual cterm=reverse ctermbg=NONE",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need this for terminals without (or less than 8) color support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, seems so

@habamax habamax mentioned this pull request Jan 30, 2024
lilydjwg added a commit to lilydjwg/dotvim that referenced this pull request Jan 31, 2024
lilydjwg added a commit to lilydjwg/dotvim that referenced this pull request Jan 31, 2024
@lilydjwg
Copy link
Contributor

This is indeed a breaking change. Fortunately the fix was simple (and I don't have a lot of colorschemes).

konosubakonoakua pushed a commit to konosubakonoakua/neovim that referenced this pull request Feb 4, 2024
…#27256)

Problem:  UX of visual highlighting can be improved
Solution: Improve readibility of visual highlighting,
          by setting better foreground and background
          colors

The default visual highlighting currently is nice in that it overlays
the actual syntax highlighting by using a separate distinct background
color.

However, this can cause hard to read text, because the contrast
between the actual syntax element and the background color is way too
low. That is an issue, that has been bothering colorschemes authors for
quite some time so much, that they are defining the Visual highlighting
group to use a separate foreground and background color, so that the
syntax highlighting vanishes, but the text remains readable (ref:
vim/colorschemesvim/vim#250)

So this is an attempt to perform the same fix for the default Visual
highlighting and just use a default foreground and background color
instead of using reverse.

I also removed the hard-coded changes to the Visual highlighting in
init_highlight. It's not quite clear to me, why those were there and not
added directly to the highlighting_init_<dark|light> struct.

closes: vim/vim#13663
related: vim/colorschemes#250

vim/vim@e6d8b46

Co-authored-by: Christian Brabandt <cb@256bit.org>
glepnir pushed a commit to glepnir/neovim that referenced this pull request Mar 31, 2024
…#27256)

Problem:  UX of visual highlighting can be improved
Solution: Improve readibility of visual highlighting,
          by setting better foreground and background
          colors

The default visual highlighting currently is nice in that it overlays
the actual syntax highlighting by using a separate distinct background
color.

However, this can cause hard to read text, because the contrast
between the actual syntax element and the background color is way too
low. That is an issue, that has been bothering colorschemes authors for
quite some time so much, that they are defining the Visual highlighting
group to use a separate foreground and background color, so that the
syntax highlighting vanishes, but the text remains readable (ref:
vim/colorschemesvim/vim#250)

So this is an attempt to perform the same fix for the default Visual
highlighting and just use a default foreground and background color
instead of using reverse.

I also removed the hard-coded changes to the Visual highlighting in
init_highlight. It's not quite clear to me, why those were there and not
added directly to the highlighting_init_<dark|light> struct.

closes: vim/vim#13663
related: vim/colorschemes#250

vim/vim@e6d8b46

Co-authored-by: Christian Brabandt <cb@256bit.org>
dmitrivereshchagin added a commit to dmitrivereshchagin/skel that referenced this pull request Apr 10, 2024
Essentially it undoes the following change in Vim:

<vim/vim#13663>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants