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

Default color scheme follow up #26369

Closed
6 tasks done
echasnovski opened this issue Dec 3, 2023 · 28 comments
Closed
6 tasks done

Default color scheme follow up #26369

echasnovski opened this issue Dec 3, 2023 · 28 comments
Labels
defaults issues or PRs involving changing the defaults enhancement feature request highlight ux user experience
Milestone

Comments

@echasnovski
Copy link
Member

echasnovski commented Dec 3, 2023

This is a tracking/roadmap issue for follow-up work after #26334. It is meant more about discussing what should be done with more details inside dedicated issues (if necessary).


  • Tweak default highlight groups based on the feedback. I am not sure it is a good idea to deviate too much from "be minimal and green-blue" requirement, but at least concious consideration should be done here. Some early feedback:

    Issues addressed in feat(highlight): tweak default color scheme #26540
    • Consider changing base lightness values of grey colors to increase contrast. Maybe something like { 4, 8, 16, 32 } (very high contrast) or { 5, 10, 20, 30 } (slightly less increase in contrast for Normal but significantly more for Comment) can work.

      It also seems important to not lean too much into "high contrast" territory, as it is also not an average good UI. Just enough to make it accessible (even more than now) should be good.

    • Use 0-15 colors for cterm attributes after feat(defaults): enable 'termguicolors' by default when supported by terminal #26407 is merged. This allows both to have good experience with nvim --clean for new users and respect terminal colors for anyone who wants/needs it. See feat: only use cterm0-15 for notgc #26389.

    • Right now Special highlight group is the same as Normal. It is a relatively highly linked highlight group, but so is Identifier. As there are only cyan and blue available (green is for strings) I decided to leave Special unhighlighted. It can be made magenta or bold blue/cyan; both don't look quite minimal to me, though.

      Or just cyan as it might be better to be highlighted as something than not be highlighted at all (as per this comment).

    • Use any attribute only when they are intended to be different. Example: revert DiagnosticFloating* back to link to Diagnostic*. Also maybe all "base" syntax groups (Constant, Statement, PreProc, Type) and similar that are intended to "not be extra highlighted".

    • Make EndOfBuffer different from Whitespace as per this comment.

    • Make QuickFixLine blue or cyan (as per this comment).

    • Maybe make Search, CurSearch, and IncSearch be actually different. Ideas: make CurSearch have bold foreground; make CurSearch be the Search from other dark/light variant (so for dark it would have NvimLightYellow as background and NvimDarkGrey1 as foreground).

    • Consider removing these lines from 'runtime/colors/default.vim' as they don't do in Neovim what they claim to do. They reset 'bg' back to default which is 'dark'. This breaks if there is a set bg=light followed by :colorscheme default.

    • Consider reverting WinSeparator to depend on VertSplit. It might be better for Vim's color scheme compatibility. But since the latter was deprecated in 0.8.0, it seems to be a good opportunity to pull the plug now.

    • Update 'runtime/colors/vim.vim' for Visual to have cterm values. See :colorscheme vim sets no cterm highlight for Visual #26468.

  • Add proper terminal colors for built-in terminal. Right now they are inherited from terminal emulator (except background and foreground), but it doesn't seem right and can lead to not really usable built-in terminal.
    Tracked in Change default built-in terminal colors #26857.

  • Consider tweaking built-in tree-sitter and LSP highlight groups. Both defaults (tried to not alter them at all) and maybe for filetypes with built-in tree-sitter parsers (Lua, C, vimdoc, markdown). Related/blocker: Tracking issue: Moving towards Helix captures nvim-treesitter/nvim-treesitter#4799.

  • Consider tweaking filetype plugin highlight groups. One notable example is 'diff' as it links diffRemoved to Special (diff*) making patches look unhighlighted (removed) and blue (added).
    Addressed in f46ae13.

  • ~ Fix bundled colorschemes by adding colorscheme vim after hi clear. ~ Addressed in fix(runtime): revert to Vim's color scheme in bundled color schemes #26641

  • Update tests to use new color scheme instead of relying on colorscheme vim. I am committed to doing it after all highlight groups are settled and some screen test behavior is confirmed to be ok (like using new indexes for attributes instead of replacing previous ones). Tracked in Update tests to use default color scheme #28601

@echasnovski echasnovski added the enhancement feature request label Dec 3, 2023
@justinmk
Copy link
Member

justinmk commented Dec 3, 2023

As there are only cyan and blue available (green is for strings) I decided to leave Special unhighlighted.

I've been using "light cyan" for Special. We can use variations of green/blue/gray where we need distinction. No need to add a color to the palette.

@tomtomjhj
Copy link
Sponsor Contributor

Thanks a lot for working on this. Not sure if this is the right place, but here are some minor issues I encountered

  • insufficient emphasis (even the quiet built-in colorscheme uses noisy colors for these)
    • QuickFixLine
    • Todo
  • undistinguishable pairs
    • Search vs. CurSearch
    • FoldColumn vs. LineNr: if you have small foldcolumn, the fold column shows foldlevel numbers, so it should be distinguishable from line numbers

Fixing these may not align with minimalism, but I think it would be quite useful.

@echasnovski
Copy link
Member Author

I've been using "light cyan" for Special. We can use variations of green/blue/gray where we need distinction. No need to add a color to the palette.

Using any other color outside of Nvim{Dark,Light}{Green,Blue,Cyan} will mean adding a color to palette, though. I tried adding a desaturated variants of green and cyan, but that doesn't work for light color scheme (they are almost indistinguishable from regular green and cyan).


@tomtomjhj, thanks!

  • QuickFixLine and Todo are bold as it seems to be quite sufficiently "noisy" highlighting.
  • Both Search/CurSearch and FoldColumn/LineNr have same highlighting, yes. This is indeed the result of "be minimal" and lack of good colors to use considering "green-blue" requirement.

@justinmk
Copy link
Member

justinmk commented Dec 3, 2023

  • QuickFixLine can be bg highlighted with NvimCyan or NvimBlue. It's important that quickfix selection is very obvious, for usability. It's not too noisy.
  • (Edit): Also Search. I am not seeing a reason why Search needs to be yellow. In general "block highlights" are something that can be used to make special cases standout without introducing a new color. Example:
    hi Search guibg=cyan guifg=black
    hi CurSearch guifg=black guibg=NvimDarkCyan
    
    image
  • (Edit): StatusLine should be more clearly distinct from StatusLineNC (probably by dimming the bg of "NC")
  • (Edit): diffAdded, diffRemoved need explicit overrides, they should be green/red
    hi diffAdded guifg=lightgreen
    hi diffRemoved guifg=lightred
    hi! link Type Comment
    
    image

This is indeed the result of "be minimal" and lack of good colors to use considering "green-blue" requirement.

We can have variations on blue/green/gray.

Using any other color outside of Nvim{Dark,Light}{Green,Blue,Cyan} will mean adding a color to palette, though. I tried adding a desaturated variants of green and cyan, but that doesn't work for light color scheme (they are almost indistinguishable from regular green and cyan).

We'll just have to figure something out. For light theme, the variations would be "dark blue/green/gray", for dark theme they would be "light".

@echasnovski
Copy link
Member Author

echasnovski commented Dec 3, 2023

QuickFixLine can be bg highlighted with NvimCyan or NvimBlue. It's important that quickfix selection is very obvious, for usability. It's not too noisy.

Ok, noted.

We can have variations on blue/green/gray.

We'll just have to figure something out. For light theme, the variations would be "dark blue/green/gray", for dark theme they would be "light".

Of course, there are variations in between present green-cyan-blue. The problem is to make a color that is substantially different from others. It is easy to do if they are always used besides each other in space or time (like for CurSearch and Search), but not so easy to do for "here is a colored text; figure out which shade green-cyan-blue it is".

Another idea to consider for at least CurSearch/Search is to make CurSearch have bold foreground.

@clason
Copy link
Member

clason commented Dec 3, 2023

I think having Special being indistinguishable from another "highlight" color is (much) preferable to Special being indistinguishable from the base foreground color (i.e., not highlighted at all). So just using NvimLightCyan for it would be a significant improvement IMO.

@cbarrete

This comment was marked as resolved.

@echasnovski
Copy link
Member Author

1. I find that the use of bold for keywords is a bit much: it is distracting and pulls my attention to the parts of the code that are the least important

Using bold text for keywords is intentional to show structure without use of color, which matters for accessibility.

2. `StatusLine` being darker than `Normal` looks weird to me

That seems to be the matter of taste, really. It seems better to me to not have permanent line which stands out too much. Between two neighbors of Normal background, the darker differs less.

3. I wish it had a bit more contrast: the foreground colors are nice and bright, but the background could be darker

To reduce subjectivity, all grey values of UI were tested to have enough lightness contrast ratio. Normal has ~12 with the most strictest requirement being 7 (out of 21). So I decided that it should be enough.

Besides, lightness of background was taken from the reference color scheme at the time.

4. I wish `Comment` was not dim: comments matter and they should be very readable. Dim colors make more sense for things like inlay hints and other non-critical tokens.

Comment should be less visible than text, but still distinguishable. It has contrast ratio of 6.3 for dark color scheme which is passable (should be at least 4.5).

5. I agree that it's a shame that `Search`/`CurSearch` are the same. I understand that this is due to the minimalism of this colorscheme, but I also think that the official, default colorscheme should also serve as a reference, and thus use available Neovim features. It would be a shame to require a custom colorscheme/plugin to make use of a stock Neovim feature.

For me having CurSearch have bold text with regular one in Search is enough distinction. But that is, of course, subject to debate.

@famiu
Copy link
Member

famiu commented Dec 3, 2023

My opinion on this is purely subjective as I lack any knowledge of color theory, but I personally find that the default background on both the light/dark themes lack contrast. I think increasing contrast for both backgrounds would be a big improvement.

@justinmk

This comment was marked as resolved.

@cbarrete

This comment was marked as resolved.

@echasnovski
Copy link
Member Author

lightness of background was taken from the reference color scheme at the time.

I appreciate that, but the new theme definitely has less contrast than the reference theme. For Normal we should edge towards a bit more contrast. For things like PMenu/floats we can swap to the lighter bg. The common case should be a bit higher contrast, the less-common cases (PMenu/floats) can opt to lower contrast if needed.

Introducing more contrast is not really a problem. I had reference lightness values being equal to { 6, 13, 20, 35 }, but I can change to, say, { 4, 8, 16, 32 }. This will make dark palette darker and light palette lighter.

I am not really a fan having floating windows having lighter background (for dark variant), as I outlined in this comment. Of course, it is doable, just something I can't consider being a good UI.

@clason
Copy link
Member

clason commented Dec 3, 2023

Identifier in particular is commonly used for "standout" tokens, e.g. fugitive uses it for commit-ids.

Not to gainsay you, but this is a general issue with how Vim uses the default highlight groups for syntax groups -- for many languages, Identifier is the standard token (i.e., what doesn't stand out).

This is one reason why we're doing things differently with treesitter and I'm keen on revisiting these default groups, lessons learned. If anything, this experience shows that things were only working by accident (meaning, links chosen with the default colorscheme in mind, and colorschemes paper over this by (re)defining ALL the syntax-specific groups).

@echasnovski
Copy link
Member Author

Just as a side note. I plan to have this for at least a week to gather feedback (at least until after Neovimconf) and make a follow up PR addressing the least controversial changes.

@justinmk justinmk added ux user experience defaults issues or PRs involving changing the defaults labels Dec 3, 2023
@justinmk justinmk added this to the 0.10 milestone Dec 3, 2023
@MunifTanjim
Copy link
Contributor

DiagnosticFloating* hl groups now has bg, so they don't blend with the NormalFloat. So they look very odd:

image

These hl groups should only define fg, so that the bg from NormalFloat is used.

@echasnovski
Copy link
Member Author

DiagnosticFloating* hl groups now has bg, so they don't blend with the NormalFloat. So they look very odd:

image

These hl groups should only define fg, so that the bg from NormalFloat is used.

My understanding of DiagnosticFloating* is that they should have the same background as the NormalFloat. If user (plugin author) wants only text different, then they should use Diagnostic* groups. Also (I believe) color scheme authors should redefine DiagnosticFloating* to have the same background as NormalFloat in their color scheme.

However, I don't think this is a big issue to revert back to the previous behavior. I'll add it to the list. Thanks for pointing out!

@lazywei

This comment was marked as off-topic.

@towry
Copy link

towry commented Dec 7, 2023

Noticed that with the default colorscheme in background=light mode, the cursor is almost not readable. image

Not sure if this is expected (like do I need to tune any config?) or is this by-design?

Perhaps you need to adjust the cursor color in your terminal settings, such as:

#: Cursor colors

cursor                          #2e2c2f
cursor_text_color               #e1e4dc

in kitty.

@echasnovski
Copy link
Member Author

Noticed that with the default colorscheme in background=light mode, the cursor is almost not readable.

Not sure if this is expected (like do I need to tune any config?) or is this by-design?

As answered above, if you have your cursor set to be that color by terminal emulator, then it should be tweaked in its config. For example, I prefer using cursor coloring which inverts the highlights under cursor, with which there is no such problem.

@craigmac
Copy link
Contributor

craigmac commented Dec 14, 2023

Noticed that with the default colorscheme in background=light mode, the cursor is almost not readable.

@towry If the terminal emulator supports it, most do, you can do:

vim.opt.guicursor:append({'a:Cursor'})

And now the cursor will obey what :hi Cursor is set to. For example in my init.lua I use:

-- Use `:hi Cursor` as default hl-group to cursor in (a)ll modes
vim.opt.guicursor:append({'a:Cursor'})

-- Works in most terminal emulators - always works in GUI
vim.cmd[[hi! Cursor guibg=Magenta]]

@Jendker
Copy link

Jendker commented Dec 15, 2023

Is there a way to avoid the change of the background transparency on nightly to the previous behavior? During startup I can first see black background which jumps later to transparent. Or is it something planned for this follow up?

@clason
Copy link
Member

clason commented Dec 15, 2023

#26550
#26579

@jdrouhard
Copy link
Contributor

Awesome theme! I did notice one thing that might need a slight tweak. Seems like @namespace is linked to Identifier, but @lsp.type.namespace is linked to Structure (which links to Type). This results in a loss of color (since Type is the "normal" color by default) when enabling semantic tokens for languages with namespaces:

screenshots

Without semantic tokens:
image

With semantic tokens:
image

Notice the std namespace is colored with treesitter only, but drops back to uncolored when semantic tokens kick in. I think just updating @lsp.type.namespace to link to Identifier would be a good tweak.

@clason
Copy link
Member

clason commented Dec 16, 2023

Please see above; the non-basic (LSP and treesitter groups) are still todo. There will be a dedicated PR for this that people can test and then suggest improvements.

@etorth

This comment was marked as outdated.

@echasnovski

This comment has been minimized.

@clason clason modified the milestones: 0.10, 0.11 May 2, 2024
@clason
Copy link
Member

clason commented May 2, 2024

I think we can consider the user-facing side (that should be in 0.10) complete; the remaining issues are follow-ups that can go into 0.11 (or be backported if necessary).

Thank you @echasnovski for all your tireless work on this!

(And feel free to close this now if you don't think you'll need a dedicated tracking issue for the test changes.)

@echasnovski
Copy link
Member Author

Yep. The two remaining tasks are split and tracked in #26857 and #28601.

Thanks everyone for the useful feedback!

@clason clason modified the milestones: 0.11, 0.10 May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defaults issues or PRs involving changing the defaults enhancement feature request highlight ux user experience
Projects
None yet
Development

No branches or pull requests