Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(defaults): enable 'termguicolors' by default when supported by terminal #26407

Merged
merged 2 commits into from Dec 6, 2023

Conversation

gpanders
Copy link
Member

@gpanders gpanders commented Dec 5, 2023

Enable 'termguicolors' automatically when Nvim can detect that truecolor is supported by the host terminal.

If $COLORTERM is set to "truecolor" or "24bit", or the terminal's terminfo entry contains capabilities for Tc, RGB, or setrgbf and setrgbb, then we assume that the terminal supports truecolor. Otherwise, the terminal is queried (using both XTGETTCAP and SGR + DECRQSS). If the terminal's response to these queries (if any) indicates that it supports truecolor, then 'termguicolors' is enabled.


This follows the process documented in https://github.com/termstandard/colors#checking-for-colorterm.

Experimental/test results:

'termguicolors' is correctly set on:

  • iTerm2
  • Ghostty
  • Wezterm
  • Kitty
  • Alacritty
  • tmux

All of the terminal emulators listed above set $COLORTERM, so 'termguicolors' support is assumed from that. If we explicitly unset $COLORTERM and force TERM=xterm-256color (mimicking an SSH session lacking proper terminfo, for instance), the results are all still correct except for Alacritty, which does not include Tc, RGB, setrgbf, or setrgbb in its terminfo and does not support XTGETTCAP or DECRQSS.

'termguicolors' is correctly not set on:

  • Terminal.app

I was not able to directly test foot or Xterm. I'm confident foot will work. Xterm should work since it parses DECRQSS (and I even bent over backwards to ensure we parse Xterm's DECRQSS response, since it differs from everyone else's).

@github-actions github-actions bot added tui defaults issues or PRs involving changing the defaults labels Dec 5, 2023
@echasnovski
Copy link
Member

I can confirm that stock st does set termguicolors by default and it does indeed support it.

runtime/doc/options.txt Outdated Show resolved Hide resolved
@justinmk
Copy link
Member

justinmk commented Dec 5, 2023

Would it reduce potential "flicker" if we actually make this the hardcoded default?

defaults = { if_true = false },

So the detection logic is used to disable the default, if necessary. But the common case is that 'tgc' is enabled and don't need to redo its side-effects.

--- OSC 11 ; rgba:<red>/<green>/<blue>/<alpha>
---
--- where
if tty then
Copy link
Member

@justinmk justinmk Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good time to introduce a _tui.lua or _tty.lua file, since we will be adding more and more TUI stuff here.

src/nvim/tui/tui.c Outdated Show resolved Hide resolved
@gpanders gpanders force-pushed the default-tgc branch 2 times, most recently from 99bda5b to 7a7f72a Compare December 5, 2023 22:27
@gpanders
Copy link
Member Author

gpanders commented Dec 5, 2023

Would it reduce potential "flicker" if we actually make this the hardcoded default?

defaults = { if_true = false },

So the detection logic is used to disable the default, if necessary. But the common case is that 'tgc' is enabled and don't need to redo its side-effects.

Either way it has no effect on the flicker. But I agree that 'termguicolors' being enabled will be the common case, so better to optimize for that. I've changed the hardcoded default to enabled now.

Probably a good time to introduce a _tui.lua or _tty.lua file, since we will be adding more and more TUI stuff here.

Maybe. For now all of this stuff is still just part of "defaults" though.

@gpanders
Copy link
Member Author

gpanders commented Dec 5, 2023

While updating the tests in tui_spec.lua I did a little bit of refactoring to remove some of the messy and difficult to understand/modify string munging and eliminated a few old TODOs along the way. Hopefully the tui_spec tests are slightly less scary now.

@gpanders gpanders changed the title feat(defaults): enable 'termguicolors' if supported by host terminal feat(defaults): enable 'termguicolors' by default Dec 5, 2023
@gpanders
Copy link
Member Author

gpanders commented Dec 5, 2023

Would it reduce potential "flicker" if we actually make this the hardcoded default?

defaults = { if_true = false },

So the detection logic is used to disable the default, if necessary. But the common case is that 'tgc' is enabled and don't need to redo its side-effects.

Either way it has no effect on the flicker. But I agree that 'termguicolors' being enabled will be the common case, so better to optimize for that. I've changed the hardcoded default to enabled now.

Actually, I need to think about this more. This gets tricky in the case that $COLORTERM and terminfo aren't available:

  1. In that case, we disable 'termguicolors'
  2. We then query the terminal to see if it supports 'termguicolors', and enable it if it does, but only if the user hasn't already set 'termguicolors'
  3. But because we ourselves set 'termguicolors' in (1), we can no longer tell if the user has set it!

src/nvim/tui/tui.c Outdated Show resolved Hide resolved
@justinmk
Copy link
Member

justinmk commented Dec 5, 2023

3. But because we ourselves set 'termguicolors' in (1), we can no longer tell if the user has set it!

But if it's already enabled, why do we need to set it again? And conversely, if the user disabled it, we can skip/ignore all of our detection stuff.

@gpanders
Copy link
Member Author

gpanders commented Dec 6, 2023

  1. But because we ourselves set 'termguicolors' in (1), we can no longer tell if the user has set it!

But if it's already enabled, why do we need to set it again?

We disable it if we can't tell that the terminal emulator supports truecolor (i.e. when $COLORTERM is undefined or not set to truecolor/24bit and when terminfo doesn't contain Tc/RGB). We then re-enable it only if querying the terminal shows that the terminal actually does support truecolor. We cannot wait until the query is finished to disable 'tgc' because that would result in a visible (and annoying) flicker effect on terminals which don't support truecolor.

Keeping the hardcoded default false and only enabling if we detect support avoids this problem (this is what I had implemented before this comment). I may revert back to that if I can't think of a solution here.

EDIT: We can check against last_set_sid. A little hacky but it will work.

EDIT 2: No, that won't work, because all Lua scripts have the same SID (-8).

And conversely, if the user disabled it, we can skip/ignore all of our detection stuff.

The detection stuff runs before any user config files, so it's not possible for the user to set it before that happens. We have to check if the user has disabled it when the responses to our terminal queries arrive.

@gpanders gpanders changed the title feat(defaults): enable 'termguicolors' by default feat(defaults): enable 'termguicolors' by default when supported by terminal Dec 6, 2023
@gpanders
Copy link
Member Author

gpanders commented Dec 6, 2023

Reverting the hardcoded default back to false and enabling in _defaults.lua (instead of vice versa). This works around the issues described above (and the visible effect for the user is identical).

…erminal

Enable 'termguicolors' automatically when Nvim can detect that truecolor
is supported by the host terminal.

If $COLORTERM is set to "truecolor" or "24bit", or the terminal's
terminfo entry contains capabilities for Tc, RGB, or setrgbf and
setrgbb, then we assume that the terminal supports truecolor. Otherwise,
the terminal is queried (using both XTGETTCAP and SGR + DECRQSS). If the
terminal's response to these queries (if any) indicates that it supports
truecolor, then 'termguicolors' is enabled.
@gpanders gpanders force-pushed the default-tgc branch 4 times, most recently from 2765071 to 5c7a01f Compare December 6, 2023 16:51
@gpanders
Copy link
Member Author

gpanders commented Dec 6, 2023

Last failing test:

FAILED   1 test, listed below:
FAILED   test/functional/terminal/tui_spec.lua @ 1851: TUI argv[0] can be overridden #23953
test/functional/terminal/tui_spec.lua:1865: Row 1 did not match.
Expected:
  |*{1: }                                                 |
  |*{4:~                                                 }|
  |*{4:~                                                 }|
  |*{4:~                                                 }|
  |*{5:[No Name]                       0,0-1          All}|
  |                                                  |
  |{3:-- TERMINAL --}                                    |
Actual:
  |*                                                  |
  |*[Process exited 0]{1: }                               |
  |*                                                  |
  |*                                                  |
  |*                                                  |
  |                                                  |
  |{3:-- TERMINAL --}                                    |

But this fails for me on master as well.

EDIT: To clarify, this fails for me on master on macOS. I can't reproduce the failure on Linux on this branch, with any build options.

@gpanders gpanders force-pushed the default-tgc branch 2 times, most recently from bd15be7 to db7f119 Compare December 6, 2023 18:33
Set 'notermguicolors' in tests which spawn a child Nvim process to force
existing tests to use 16 colors. Also refactor the child process
invocation to make things a little bit less messy.
@gpanders gpanders merged commit 08545bd into neovim:master Dec 6, 2023
21 of 24 checks passed
@gpanders gpanders deleted the default-tgc branch December 6, 2023 18:55
@lewis6991
Copy link
Member

Great work on the turnaround time for this!

@gpanders gpanders mentioned this pull request Dec 6, 2023
8 tasks
@justinmk
Copy link
Member

justinmk commented Dec 6, 2023

This test is failing about 40% of the time now:

FAILED   test/functional/terminal/tui_spec.lua @ 1449: TUI paste: streamed paste with isolated "stop paste" code
test/functional/terminal/tui_spec.lua:1493: Expected objects to be the same.
Passed in:
(table: 0x56350fb6d640) {
  [1] = 1
  [2] = 2
 *[3] = 2
  [4] = 3 }
Expected:
(table: 0x56350fb587a0) {
  [1] = 1
  [2] = 2
 *[3] = 3 }

stack traceback:
	(tail call): ?
	test/functional/terminal/tui_spec.lua:1493: in function <test/functional/terminal/tui_spec.lua:1449>

@gpanders
Copy link
Member Author

gpanders commented Dec 7, 2023

@justinmk should be fixed by #26437

fitrh added a commit to fitrh/init.nvim that referenced this pull request Dec 8, 2023
[1] implelements terminal trucolors capability detection and sets
`'termguicolors'` if supported.

[1]: neovim/neovim#26407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defaults issues or PRs involving changing the defaults tui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants