Toggling 'termguicolors' changes the colorscheme

gdupras commented 3 years ago

Steps to reproduce bugs

vim --clean
:view $VIMRUNTIME/vimrc_example.vim " Just to see some syntax highlighting.
:colorscheme blue
:set termguicolors " The highlighting changes to something that isn't blue.
:colorscheme " Output is 'blue' but it clearly doesn't look like blue.
:colorscheme blue " Now correctly looks like the blue colorscheme.
:set notermguicolors " The highlighting changes to something that isn't blue.
:colorscheme " Output is 'blue' but it clearly doesn't look like blue.
:colorscheme blue " Now correctly looks like the blue colorscheme.

This affects most (all?) the remade colorschemes but not the original ones.

Vim version: 8.2.2488 Terminal: Konsole OS: Kubuntu 20.10

romainl commented 3 years ago

Confirmed with 8.2.2164 in iTerm (true colors) and Terminal (not true colors) on macOS. Thank you for spotting this.

lifepillar commented 3 years ago

Well, that is by design. Color schemes generated by Colortemplate are optimized so that they only define the attributes that apply to the current environment. That is to say, if termguicolors is set, when a color scheme is loaded only guifg/guibg/gui attributes are defined, but not ctermfg/ctermbg/cterm, and vice versa. So, if you change the environment at runtime (why would you do that, btw?), you need to reload the color scheme.

@romainl has already criticized this approach (I can't find the discussion right now, but it might be useful to link it here), and I have promised to address this in Colortemplate: once that is done, rebuilding the color schemes would fix this (no change in the templates will be necessary). Life is getting in the way in this period, but I'll try to use some weekends to work on that.

gdupras commented 3 years ago

(why would you do that, btw?)

I wanted to check that the color scheme with and without termguicolors looks the same or is similar enough. I would only do this while trying out new color schemes. Once I settle for one of course I then leave termguicolors alone.

habamax commented 3 years ago

I wanted to check that the color scheme with and without termguicolors looks the same or is similar enough. I would only do this while trying out new color schemes. Once I settle for one of course I then leave termguicolors alone.

for now you can just reapply the same colorscheme after changing termguicolors

PS. But you know this, as far as I can see (missed it from your post)

habamax commented 3 years ago

Hi @lifepillar, is there anything from your side on this?

lifepillar commented 3 years ago

I am not planning to address this in Colortemplate v2, as I don't think it is a high (or even medium) priority issue (I don't see compelling use cases for changing termguicolors at runtime—what was mentioned above is kind of a niche use case, which can be easily addressed by the minor inconvenience of reloading the color scheme).

Colortemplate v3 (which is not published yet—but I am working on it) will provide more flexibility in the generated code, and will fix this.

habamax commented 3 years ago

Thanks for the update, @lifepillar!

@romainl would we wait for v3 before asking Bram and team to checkout what we have?

romainl commented 3 years ago

Vimmers have been living with broken colorschemes for decades so they can probably accept that termguicolors-related annoyance until v3 is ready.

lifepillar commented 3 years ago

Note that I have no ETA for v3 yet. The new version will be a complete rewriting in Vim9 script, but the template's syntax won't change, at least not in a way that will affect this project—e.g., it most likely won't support a Neovim directive.

The main advantages of v3 will (hopefully) be speed and a better (and I hope smaller) code base. The generated output will change in a way that will fix this issue, or it will be possible to choose between different “optimization levels” for the generated code: I will ask for feedback when I am ready to ship a usable version.

So, if you feel that the color schemes are now up to date (you have been doing a great job btw!), IMO definitely go ahead and send them to Bram. I think it won't be a problem to send further improvements later on, as with other parts of the runtime.

habamax commented 2 years ago

@lifepillar fyi: https://groups.google.com/g/vim_dev/c/6eI4doV2Mi0/m/9-AENzagAgAJ

jeanluc2020 commented 2 years ago

So, if you change the environment at runtime (why would you do that, btw?), you need to reload the color scheme.

I sometimes start vim, editing a file, and later realize this will be a longer editing session and I want to have my terminal back during that session without jumping in and out of vim or suspending it all the time, so i type ":gui" to continue working on the file. For me personally, that is something common enough to be annoyed by reloading the colorscheme again.

I suggest adding something like this: augroup COLORSCHEME au! GuiEnter * exec "colorscheme " . g:colors_name augroup END

lifepillar commented 2 years ago

Ok, this has come up a few times already… I was not aware of :gui (is there an end to Vim's knowledge?)

So, the solution here is to build the color schemes so that GUI and terminal definitions are both applied, regardless of the values of termguicolors. I was planning to address it for v3, but it's clear that it is needed now. I hope be able to work on that during the weekend. I'll keep you updated.

romainl commented 2 years ago

Well, there's also the case of Vim honouring g:terminal_ansi_colors even when not in GUI and termguicolors is disabled. Isn't there a line to draw in the sand between what can/should be done at the colorscheme level and what should be fixed in Vim?

IMO…

[COLORSCHEME] gui* attributes should be defined unconditionally (this should fix the :gui problem innocuously).
[COLORSCHEME] cterm* attributes should be defined against &t_Co: 0-15 if < 256 and 16-255 if >= 256.
[COLORSCHEME] g:terminal_ansi_colors should be defined unconditionally.
[VIM CORE] g:terminal_ansi_colors should only be honoured by Vim in the documented cases.
[VIM CORE] Startup sequence should be the same after $ gvim foo.txt and after :gui.

habamax commented 2 years ago

For the last 2 we have to create issues against vim repo I guess

lifepillar commented 2 years ago

The current Colortemplate's master fixes this issue. As proposed by @romainl, gui attributes and g:terminal_ansi_colors are now defined unconditionally.

lifepillar commented 2 years ago

Regarding g:terminal_ansi_colors in particular, please tell me what to do: is it ok to always have that variable defined?

romainl commented 2 years ago

@lifepillar in theory yes, as it is supposed to be a harmless, passive, global variable. But I found out recently that, at least in some builds, attempts were made by Vim to interpret those colors in situations where it shouldn't, leading to unexpected results.

For that reason I would suggest putting g:terminal_ansi_colors behind a guard that prevents it from being defined outside of the documented scenarios, which are:

I am in a GUI Vim,
I am running Vim in a terminal emulator, Vim supports termguicolors, and I have enabled it.

This is what I have in Apprentice:

if (has('termguicolors') && &termguicolors) || has('gui_running')
  let g:terminal_ansi_colors = [...]
endif

With that guard in place and the gui attributes set unconditionally, we should be good.

romainl commented 2 years ago

@jeanluc2020 If you have time, could you try again with the master branch?

jeanluc2020 commented 2 years ago

@jeanluc2020 If you have time, could you try again with the master branch?

Thanks, looks much better now, switching from vim inside an xterm to gvim via :gui now works perfectly.

lifepillar commented 1 year ago

Sorry to hijack a closed thread, but regarding this:

at least in some builds, attempts were made by Vim to interpret those colors in situations where it shouldn't, leading to unexpected results.

I haven't been able to reproduce it. I'm using Terminal.app, and /Applications/MacVim.app/Contents/bin/vim (v9.0.1276) launched from a terminal session, but even if my colorscheme unconditionally sets g:terminal_ansi_colors to sixteen copies of the same color, colors in Vim's terminal look just fine:

Here, the shown colors are those from the underlying terminal (which are not the colors of the active colorscheme), as expected. g:terminal_ansi_colors is set to all #11AAFF and it's clearly not used, as expected. termguicolors is off, of course, because Terminal.app does not support it.

Am I missing something?

I am trying to understand whether guarding g:terminal_ansi_colors as suggested above is something that Colortemplate should do.

lifepillar commented 1 year ago

Or perhaps, https://github.com/vim/vim/commit/b2b3acbf2b3ae29ecf517f03b46fbeadf0c1a905 did fix it?

romainl commented 1 year ago

@lifepillar I can't reproduce the bug either so it must have been fixed.

vim / colorschemes

Toggling 'termguicolors' changes the colorscheme #54