feat: set s:t_Co to -1 if &t_Co is undefined

[habamax]

Fixes https://github.com/vim/colorschemes/issues/198

@romainl, @neutaaaaan fyi

[lifepillar]

Thanks for preparing this! These changes to how t_Co is handled are always a bit delicate, and tend to break tests (have you tried sourcing runtests.vim?). I may fix the tests, but just to be sure that there is no important use case slipping through this fix 🙂

In the meantime, I will read through the long thread you linked and try to understand why this is needed.

[habamax]

s:t_Co is never less than 1 and it makes variant: 0 always being applied provided you run gvim and a :colorscheme something is in .vimrc.

[lifepillar]

Cool, thanks! I'll try it tonight!

[habamax]

@lifepillar I am not sure how to properly test it.

With the latest version (without this patch) I get this:

The same with gvim

[habamax]

No matter if I run it as :so% or vim +"source runtests.vim

[lifepillar]

Try:

vim --clean runtests.vim
:set rtp+=~/.vim/path/to/colortemplate
:so%

[habamax]

42 tests are failed, but all of them because of anchoring to the t_Co guard, which was changed:

[habamax]

Fixed all the tests:

[lifepillar]

Thanks for taking care of the tests, too! So, let me recap, and please tell me if I am missing anything:

1. without this fix, if t_Co is undefined, s:t_Co is set to 1. Therefore, the definitions inside an if s:t_Co >= 0 block are applied.
2. When has('gui_running') is true and t_Co is undefined, the definitions inside an if s:t_Co >= 0 block are added to the GUI definitions.
3. Behaviour (2) is a consequence of a change that was made not long ago (3746218181e757ac1dc7dceafe97ab7e8fa137d2 should be the relevant commit). Before that commit, when has('gui_running') was true (or termguicolors was set), only GUI definitions were applied (Colortemplate would insert finish statement after those).
4. In principle, (2) should not cause any issue: definitions inside an if s:t_Co >= 0 block should only add term options, which should not conflict with guifg/guibg/… options. But there is a glitch: if a highlight group, say Terminal, is a link, then executing something like Terminal term=NONE would remove the link, and effectively clear the highlight group.

I see two problems now:

• with the proposed fix, when t_Co is undefined and you are in a (probably not very capable) terminal, no color scheme definition is applied (whereas, right now, at least term definitions are applied). This is a minor break of backward compatibility, which can be worked around by setting t_Co explicitly. I can live with that.

• More importantly, it seems to me that (4) may be experienced in other, more common situations. For instance (contrived example):

  
  if  " is GUI vim or has termguicolors
  hi! link Foo Statement
  […]
  " finish  # Statement removed by commit 3746218181e
  endif

if s:t_Co >= 256 hi Foo ctermbg=NONE […] finish endif

In GUI Vim or in a terminal with `termguicolors` set, now the link will be lost. This would have not happened before (3); so, a way to fix (4) would be to revert (3). But there were valid (although perhaps not compelling) reasons to introduce (3) (as summarized in the commit message of c0fbbe742ea995e6bc2c91633bed395fc6631a25), so that is not an option, I'd say.

I will merge this. But this corner behaviour makes me think that (3) has made reasoning about highlight group definitions more difficult than it should be.

[habamax]

Thx!

You get it all correct/right.

Indeed there is still a possibility of the situation you're describing, but this could be handled by the template source itself unlike the thing this PR were fixing.

This is probably another reason @romainl were asking for a joint setting of both gui and tui attributes in a single highlight.

hi Something guifg=... guibg=... ctermfg=... etc

I wasn't able to do it as it would require quite a change last time I did another PR here.

[lifepillar]

I see. Yes, what @romainl proposes is the most logical thing to do. Something like this (not good practice, but admissible):

Color: Green green ~
Color: Red red ~
Color: Blue blue ~
Variant: gui
Foo Green Red
Variant: 256
Foo Blue None

should be translated into (pseudocode):

if  " has GUI or termguicolors
  hi Foo guifg=green guibg=red ctermfg=green ctermbg=red
  finish
endif
if t_Co >= 256
  hi Foo ctermfg=blue ctermbg=NONE
  finish
endif

rather than the current fallthrough:

if  " has GUI or termguicolors
  hi Foo guifg=green guibg=red
endif
if t_Co >= 256
  hi Foo ctermfg=blue ctermbg=NONE
  finish
endif

Merging definitions from different variants is calling for trouble.

Edit: fixed typos

[lifepillar]

One limitation of @romainl's proposal is that generating something like this:

hi Foo guifg=green guibg=red ctermfg=blue ctermbg=NONE

would not be possible without extending the template's syntax. For the foreground, one might define an additional “mixed” color Color: C4 green blue (where blue stands for some 256-based color). But for the background, there is no way to specify in the same variant that you would want NONE for ctermbg.