search

sainnhe / gruvbox-material

Gruvbox with Material Palette
MIT License
1.96k stars 166 forks source link

Inconsistent String highlighting #119

Closed mtvec closed 2 years ago

mtvec commented 2 years ago

Operating system/version

Ubuntu 21.10

Terminal emulator/version

alacritty 0.9.0

$TERM environment variable

alacritty

Tmux version

No response

Feature matrix

VIM - Vi IMproved 8.2 (2019 Dec 12, compiled Dec 17 2021 16:55:33)
Included patches: 1-2434, 3402-3403, 3409, 3428, 3487, 3564, 3581-3582, 3611, 3613, 3612, 3625, 3669, 3741
Modified by team+vim@tracker.debian.org
Compiled by team+vim@tracker.debian.org
Huge version with GTK3 GUI.  Features included (+) or not (-):
+acl               -farsi             +mouse_sgr         +tag_binary
+arabic            +file_in_path      -mouse_sysmouse    -tag_old_static
+autocmd           +find_in_path      +mouse_urxvt       -tag_any_white
+autochdir         +float             +mouse_xterm       +tcl
-autoservername    +folding           +multi_byte        +termguicolors
+balloon_eval      -footer            +multi_lang        +terminal
+balloon_eval_term +fork()            -mzscheme          +terminfo
+browse            +gettext           +netbeans_intg     +termresponse
++builtin_terms    -hangul_input      +num64             +textobjects
+byte_offset       +iconv             +packages          +textprop
+channel           +insert_expand     +path_extra        +timers
+cindent           +ipv6              +perl              +title
+clientserver      +job               +persistent_undo   +toolbar
+clipboard         +jumplist          +popupwin          +user_commands
+cmdline_compl     +keymap            +postscript        +vartabs
+cmdline_hist      +lambda            +printer           +vertsplit
+cmdline_info      +langmap           +profile           +virtualedit
+comments          +libcall           -python            +visual
+conceal           +linebreak         +python3           +visualextra
+cryptv            +lispindent        +quickfix          +viminfo
+cscope            +listcmds          +reltime           +vreplace
+cursorbind        +localmap          +rightleft         +wildignore
+cursorshape       +lua               +ruby              +wildmenu
+dialog_con_gui    +menu              +scrollbind        +windows
+diff              +mksession         +signs             +writebackup
+digraphs          +modify_fname      +smartindent       +X11
+dnd               +mouse             +sound             -xfontset
-ebcdic            +mouseshape        +spell             +xim
+emacs_tags        +mouse_dec         +startuptime       +xpm
+eval              +mouse_gpm         +statusline        +xsmp_interact
+ex_extra          -mouse_jsbterm     -sun_workshop      +xterm_clipboard
+extra_search      +mouse_netterm     +syntax            -xterm_save
   system vimrc file: "$VIM/vimrc"
     user vimrc file: "$HOME/.vimrc"
 2nd user vimrc file: "~/.vim/vimrc"
      user exrc file: "$HOME/.exrc"
  system gvimrc file: "$VIM/gvimrc"
    user gvimrc file: "$HOME/.gvimrc"
2nd user gvimrc file: "~/.vim/gvimrc"
       defaults file: "$VIMRUNTIME/defaults.vim"
    system menu file: "$VIMRUNTIME/menu.vim"
  fall-back for $VIM: "/usr/share/vim"
Compilation: gcc -c -I. -Iproto -DHAVE_CONFIG_H -DFEAT_GUI_GTK -pthread -I/usr/include/gtk-3.0 -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/gtk-3.0 -I/usr/include/gio-unix-2.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/fribidi -I/usr/include/harfbuzz -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/uuid -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/x86_64-linux-gnu -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -Wdate-time -g -O2 -ffile-prefix-map=/build/vim-7ASsBs/vim-8.2.2434=. -flto=auto -ffat-lto-objects -fstack-protector-strong -Wformat -Werror=format-security -D_REENTRANT -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 
Linking: gcc -L. -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-z,now -fstack-protector-strong -rdynamic -Wl,-export-dynamic -Wl,-E -Wl,-Bsymbolic-functions -flto=auto -Wl,-z,relro -Wl,-z,now -Wl,--as-needed -o vim -lgtk-3 -lgdk-3 -lpangocairo-1.0 -lpango-1.0 -lharfbuzz -latk-1.0 -lcairo-gobject -lcairo -lgdk_pixbuf-2.0 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lSM -lICE -lXpm -lXt -lX11 -lXdmcp -lSM -lICE -lm -ltinfo -lselinux -lcanberra -lacl -lattr -lgpm -L/usr/lib -llua5.2 -Wl,-E -fstack-protector-strong -L/usr/local/lib -L/usr/lib/x86_64-linux-gnu/perl/5.32/CORE -lperl -ldl -lm -lpthread -lcrypt -L/usr/lib/python3.9/config-3.9-x86_64-linux-gnu -lpython3.9 -lcrypt -ldl -lm -lm -L/usr/lib/x86_64-linux-gnu -ltcl8.6 -ldl -lz -lpthread -lm -lruby-2.7 -lm -L/usr/lib

Minimal vimrc that can reproduce this bug.

if &compatible
  set nocompatible
endif

syntax on

call plug#begin()

Plug 'sainnhe/gruvbox-material'
Plug 'neoclide/coc.nvim', {'branch': 'release'}

call plug#end()

if exists('+termguicolors')
  let &t_8f="\<Esc>[38;2;%lu;%lu;%lum"
  let &t_8b="\<Esc>[48;2;%lu;%lu;%lum"
  set termguicolors
endif

set background=dark
let g:gruvbox_material_better_performance = 1
colorscheme gruvbox-material

Steps to reproduce this bug using minimal vimrc

  1. Run :CocInstall coc-rust-analyzer (or any other language server that supports semantic highlighting of strings).
  2. Edit a Rust file. This one for example: 
    fn test() -> &'static str {
"foo"
}

Expected behavior

The highlighting of strings is independent of whether Vim's builtin syntax rules detected them or the language server did.

Actual behavior

When no language server is used, or one is used that doesn't support semantic highlighting of strings (e.g., clangd), strings are assigned to the String highlighting group which is highlighted using s:palette.green:

image

With a language server that supports this, strings are assigned to the CocSymbolString group which is linked to the Aqua group, causing different highlighting:

image

I think it would make sense to make this consistent and maybe just link CocSymbolString (and also TSString) directly to String.

However, I actually prefer the Aqua highlighting of strings because otherwise, strings are assigned the same color as functions which highlights too many things in the same color IMHO. I could easily change this locally of course.

sainnhe commented 2 years ago

The designs of classic vim syntax highlighting and tree-sitter / semantic highlighting are totally different, see explanation here https://github.com/sainnhe/gruvbox-material/issues/109

mtvec commented 2 years ago

I understand highlighting can be done much more optimal when tree-sitter/semantic highlighting is used. However, I do not understand why there need to be inconsistencies with the classic vim syntax highlighting. Why not use the same colors for String, TSString, and CocSymbolString?

I'm sure I'm missing something here though. Is it maybe a feature to be able to see which syntax engine is being used based on the colors?

sainnhe commented 2 years ago

However, I do not understand why there need to be inconsistencies with the classic vim syntax highlighting.

In short, it's impossible to keep them consistent.

Say if we want to use green in function names, we can link TSFunction to Green, then all function highlights provided by TS will become green, this is expected.

However, this is not the case in the classic vim syntax highlighting. In many languages, you can't get the function name highlighted (e.g. C and JS).

To get a better overall feeling, the highlighting logic are completely redesigned.

mtvec commented 2 years ago

I understand, but wouldn't it still make sense to keep consistency where possible?

Let me give an example of what currently annoys me. Say I like to have italic keywords. Great, I just set g:gruvbox_material_enable_italic and everything seems fine. However, it is only fine for languages for which I don't have tree-sitter of coc.nvim support because TSKeyword is linked to Red instead of RedItalic.

I like to have consistency between the different languages I'm working with. I fully understand (and like!) that better highlighting can be provided when semantic tokens are available. I don't mind seeing the colors of functions switch after the language server has been loaded on a C file because I understand that more information has now become available. However, I do not like seeing keywords becoming non-italic at the same time because those are already correctly parsed without the language server. No extra information is provided by changing the style, so it just becomes a cognitive burden for me.

sainnhe commented 2 years ago

OK I'm going to explain my thoughts in detail:

First of all, when I'm designing a color scheme, the very first thing is to guarantee a good overall feel, and everything else is secondary. That's said, when something else happens that conflicts with this principle, this principle take precedence.

And when designing a color scheme, I personally created 2 new concepts: the primary colors and the secondary colors:

The primary colors are those that can be widely used and give a nice overall feel, while the secondary colors are mainly used to set off the primary colors, which intend to make the color scheme more colorful. The secondary colors should not be widely used.

For example in this color scheme, the primary colors are red, orange, yellow and green, these are warm colors and can be widely used. While aqua, blue and purple are secondary colors and should not be widely used, they tend to be cooler. If we widely use blue, aqua and purple in this color scheme, the overall feel will be very bad.

Now back to your questions.

  1. Why not use aqua as string color in classic vim syntax highlighting?

If we do so, as I mentioned above, in some languages you'll only see aqua and there will be no green because function name can't be highlighted. In other words, aqua which is one of the secondary colors, will be widely used. That will cause a bad overall feel.

And as I said, when I'm designing color schemes, the very first thing is to guarantee a good overall feel, everything else is secondary. So even if this highlighting logic is inconsistent with that is in TS highlighting, I still chose not to use aqua in string.

  1. Why not use RedItalic as keywords colors in TS highlighting?

There are 2 widely accepted highlighting logic for italics:

a. Italicize keywords b. Italicize type, classes, builtin variables (like self, super), etc.

In my personal opinion, b. is actually better, because with a. almost all italics are centered on the left side of your editor, while with b. italicized keywords are more evenly distributed in your whole editor.

That's said, b. have a better overall feel.

However we can't implement b. in classic vim syntax highlighting because the classic highlighting engine is so bad, but it's possible to implement a., so using a. in classic syntax highlighting is actually a compromise. When comes to TS where b. is possible, I chose to implement b.

Yes I must admit that this approach is very aggressive, but that's how I design my color schemes.

If you have reservations about my opinion, that OK because this is an open source project. Neovim community disagree with Vim's on some points so they forked Vim and start over. Also, you are always welcome to fork this repo, or create your new one, or use others' ports (looks like some people have ported this color scheme to neovim in lua).

mtvec commented 2 years ago

Alright, thanks a lot for your detailed explanation! It's very much appreciated. I understand your point better now and I agree with it.