sainnhe / gruvbox-material

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

Invalid expression #60

Closed lithammer closed 3 years ago

lithammer commented 3 years ago
Error detected while processing /Users/foo/.vim/pack/packager/start/gruvbox-material/vimrc[4]../Users/foo/.vim/pack/packager/start/gruvbox-material/colors/gruvbox-material.vim[21]..function gruvbox_material#get_configuration:
line    1:
E15: Invalid expression: {
Error detected while processing /Users/foo/.vim/pack/packager/start/gruvbox-material/vimrc[4]../Users/foo/.vim/pack/packager/start/gruvbox-material/colors/gruvbox-material.vim:
line   22:
E121: Undefined variable: background
E116: Invalid arguments for function gruvbox_material#get_palette(s:configuration.background, s:configuration.palette)
line   29:
E121: Undefined variable: transparent_background
line   51:
E121: Undefined variable: s:palette
E116: Invalid arguments for function gruvbox_material#highlight
line   52:
E121: Undefined variable: s:palette
E116: Invalid arguments for function gruvbox_material#highlight
...

Minimal vimrc that can reproduce this bug

packadd gruvbox-material
set termguicolors
set background=dark
colorscheme gruvbox-material

Steps to reproduce this bug using minimal vimrc

Install gruvbox-material using the native package manager (:h packages) and then start Vim:

$ cat vimrc
packadd gruvbox-material
set termguicolors
set background=dark
colorscheme gruvbox-material
$ vim --noplugin -u vimrc vimrc

Actual behaviour

Loads of errors (see above). Note that this is only a problem in Vim, not in Neovim.

Expected behaviour

Start without errors.

sainnhe commented 3 years ago

This seems to be a bug of vim, I don't know how to fix it in color scheme level.

But I found some solutions here: https://github.com/joshdick/onedark.vim/issues/110

lithammer commented 3 years ago

Thanks for the link. Manually calling packadd! gruvbox-material "solved" it, even if it feels a bit dirty that it's needed.

I suppose one solution is to move required functions from autoload/gruvbox_material.vim to colors/gruvbox-material.vim. Since they're sort of required, I don't see the point of lazily loading them via autoload. One downside I can think of is that the "How to use custom colors?" section of the FAQ would no longer work.

A much more drastic approach could be to use something like https://github.com/lifepillar/vim-colortemplate or https://github.com/romainl/vim-rnb that generates the colors from a template. That would remove the need for functions to begin with. And with the bonus of reducing the load time because it wouldn't have to call hundreds of functions. This would probably involve significant work though.

sainnhe commented 3 years ago

Some explanations:

  1. Why not use lifepillar/vim-colortemplate?

In fact, this color scheme used to use color template in the earlier stage, but I finally decided to switch to pure vim script because there is a very serious disadvantage in color template: you can't change the color palette once the color scheme file is generated.

Let see how many color schemes files we need to generate if using color template: 3 palettes (material, mix, original) 3 contrast options (hard, medium, soft) 2 background options (light, dark) = 18

This is the main reason I decided to switch to pure vim script. Currently, this color scheme uses a single variable g:gruvbox_material_palette to control all these colors, it's much easier for both maintainance and usage.

Another advantage to use a variable to control the color palette is that it's easier to change the colors.

For example, I'm using the original palette but I don't like the green color #b8bb26, I prefer the green color #a9b665 in the material palette. Now all you need to do is simply modify this variable in your vimrc, then the green color in colors/gruvbox-material.vim, airline and lightline will be updated.

Another practice is that you can even use a completely different color palette from some other color schemes. For example, https://github.com/sainnhe/gruvbox-material/blob/2ba842e67fc72c4551d85c3a6c93bc9db5c2aec2/doc/gruvbox-material.txt#L406-L446

  1. Why use autoload?

Well, so you've understood why I choose to use pure vim script to rewrite this color scheme instead of using color template, then there's an emerging problem: since too many file types and plugins are optimized in this color scheme, the performance is getting very bad compared to using color template.

So I decided to add a new option g:gruvbox_material_better_performance to improve the performance. This feature will place some of the code in after/ftplugin so we can load them on demand.

But the optimization code of some file types requires gruvbox_material#highlight() to highlight some groups, so the functions should be accessible not only in colorschemes/airline/lightline, but also in after/ftplugin.

This is why the autoload file is required.

lithammer commented 3 years ago

Thanks for the explaining your decisions! Makes sense I guess given the many permutations and complexity. Though I suspect some of the issues could be solved by using conditionals and includes, at least with lifepillar/vim-colortemplate. See https://github.com/lifepillar/vim-solarized8 which is quite complex.

The part about the plugins (lightline, airline etc) is valid though. Not sure how to make that work in a nice way.

Another practice is that you can even use a completely different color palette from some other color schemes.

I'm not convinced by this. I get doing minor tweaks, but if I swap the color palette completely, why would I use this colorscheme to begin with? It's no longer gruvbox-material at that point.

Anyway, I'll close this since the issue can be solved by following the tips in joshdick/onedark.vim#110. Thanks!

sainnhe commented 3 years ago

Though I suspect some of the issues could be solved by using conditionals and includes, at least with lifepillar/vim-colortemplate.

You can checkout the earlier commits, I tried that, and it's complex indeed.

I get doing minor tweaks, but if I swap the color palette completely, why would I use this colorscheme to begin with? It's no longer gruvbox-material at that point.

Yes, this is just my personal preference. As I said in the doc, there are some bugs in soft-era and I just don't want to maintain a fork, so I use this variable to get a more usable one (IMO), and I mentioned this practice simply in order to point out that possibility. Anyway, I don't encourage anyone else to do this.

lithammer commented 3 years ago

That's fair 🙂