ap / vim-css-color

Preview colours in source code while editing
http://www.vim.org/scripts/script.php?script_id=5056
MIT License
1.79k stars 80 forks source link

Separate check for +syntax feature from other checks #110

Closed lifecrisis closed 5 years ago

lifecrisis commented 5 years ago

In autoload/css_color.vim, the feature tests at the top of the script should be regrouped.

The test appears as follows:

if v:version < 700 || !( has('syntax') || has('gui_running') || has('nvim') || &t_Co==256 )

Notice that an instance of Vim may have &t_Co == 8 and still satisfy has('syntax'). Thus, the test should be:

if v:version < 700 || !has('syntax') || !( has('gui_running') || has('nvim') || &t_Co==256 )

When this plugin is used with 8-color terminals, you get odd effects like the following picture:

capture

... notice that the word "blue" is off-color.

Also: I would like to request that something like the following be placed at the top of this script, but below the feature tests:

if exists('g:loaded_css_color')
  finish
endif
let g:loaded_css_color = 1

This boilerplate is available in almost all plugins. This would allow users to globally disable the plugin even though it's installed in their &runtimepath. This is useful when you use the same Vim configuration in a lot of contexts and want to skip loading this plugin sometimes.

ap commented 5 years ago

Thank you for the report. That is obviously silly… now that you point it out.

I didn’t like your suggested fix because I felt that it doesn’t address the underlying reason why this mistake snuck in in the first place. The problem is that the conditional is trying to check for requirements, but it’s written as a check for the negation of any requirement. The repeated negation and identical operator makes it hard to notice that logically there ought to be a subgrouping. Factoring out the negation makes these cases need different operators so the condition becomes much clearer to read:

if ! ( v:version >= 700 && has('syntax') && ( has('gui_running') || has('nvim') || &t_Co == 256 ) )
ap commented 5 years ago

Also: I would like to request that something like the following be placed at the top of this script, but below the feature tests:

I agree in principle but it will be (slightly) tricky to do in this plugin. Can you make another issue for further discussion please?

lifecrisis commented 5 years ago

I agree in principle but it will be (slightly) tricky to do in this plugin. Can you make another issue for further discussion please?

See #111.