[vim/vim] Add v:termrgbresp (PR #21191)
erry
Suggested fix for #16327.
This patch tries to implement a compromise both for users who want automatic termguicolors and users who don't. For users who don't want termguicolors, nothing needs to change. For users who want termguicolors and already added set termguicolors to their vimrc, nothing needs to change. For users who want their vimrc to adapt to different kinds of terminals which can and can't handle termguicolors, it's now possible to add this to their vimrc:
if str2nr(&t_Co) == 16777216
set termguicolors
endif
autocmd TermResponseAll * {
if expand('<amatch>') == 'RGB' && v:termrgbresp == '8'
set termguicolors
redraw!
endif
}
(this is vim9script, I didn't check whether the syntax is valid for old vim script)
This script exactly matches the behaviour of #16490 (the original fix for #16327), and additionally runs redraw! to fix the bug that the colours only change after the first user action. (I added the same fix to the #if-0-ed out code, for consistency)
It is possible to add this to defaults.vim if this behaviour becomes desirable in the future.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/21191
Commit Summary
- edafc60 Add v:termrgbresp
File Changes
(10 files)
- M runtime/doc/autocmd.txt (13)
- M runtime/doc/eval.txt (7)
- M runtime/doc/tags (2)
- M runtime/syntax/vim.vim (2)
- M src/autocmd.c (18)
- M src/evalvars.c (1)
- M src/term.c (20)
- M src/testdir/test_termcodes.vim (16)
- M src/version.c (2)
- M src/vim.h (3)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#21191)Thanks for the reply. Answers to your points, in your order.
The capability value
You are right - the XTGETTCAP reply is always a string, and ncurses' own
encoding is not what arrives here. Withdrawn. But your reading of the reply
already names two shapes, a single number ("8") and three ("5/6/5"), and the
example in the PR description covers one of them. That is the work I would
rather Vim did once than leave to every vimrc, and it is the same work whether
the variable holds the DCS response or the value pulled out of it.
terminalprops()
Yes, please change the PR to use it. It reports 'y', 'n' or 'u' per property,
so a terminal that answers "5/6/5" reports 'y' like any other, 'u' covers the
terminal that never answers, and the shape of the capability string stays
inside Vim. The TermResponseAll event stays as you have it - what changes is
where a script reads the answer from.
Your reasoning for the negative side is sound: start out assuming no direct
color and move only on a positive answer. terminalprops() records what Vim has
learned so far, which is what 'u' already means for the other entries.
The final call on the interface belongs to @chrisbra, so it is worth waiting
for his view before you rewrite anything.
Lowercase
This one is not a preference. I put two autocommands on the existing da1 event,
au TermResponseAll DA1 and au TermResponseAll da1, and fed the reply: with
'nofileignorecase' only the lowercase one ran, with 'fileignorecase' both did.
The pattern goes through match_file_pat(), which takes rm_ic from p_fic. So a
vimrc with au TermResponseAll RGB works on Unix and one with rgb does not,
while on Windows and macOS both work.
The disabled code
9.1.1114 commented it out with "We may need another way to enable this in the
future". This PR is that other way, so the old auto-enabling code has nothing
left to wait for. Removing it takes the val == 8 test with it, which also
settles the test value question below.
version.c
Someone has to strip that hunk by hand before the change is committed. Please
keep it in your local tree instead.
Splitting the da1 and osc fix
Put it in this same PR as a separate commit, first in the series. The commits
of a PR are applied in order, so the two cannot come apart the way you ran into
before, and each lands with its own message.
On tests: there is no TermResponseAll test in testdir today, and the delayed
firing in unblock_autocmds() has no path a test can reach. block_autocmds() is
called from the clipboard, popup menu, window and undo code, never from
anything that also feeds a terminal reply, and :noautocmd sets 'eventignore'
rather than blocking. The existing six entries went in without a test for the
same reason. A commit message recording that da1 and osc were left out when the
others were added is enough.
The test
The group is testRespRGB, so autocmd! testRespRGB is what you want.
Your reason for feeding "100" is fair, and once the val == 8 test is gone
nothing in the code depends on the value at all. The case still worth adding is
the negative one: feed DCS 0 + r ... ST and assert that nothing changed and
no autocommand fired. That pins down the behavior you describe above.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
erry
AstroSnail left a comment (vim/vim#21191)Thanks, your critique is helpful.
I've given your ideas some thought, and come up with with a plan that sounds sensible to me. I will drop v:termrgbresp in favour of an entry in terminalprops() called 'rgb'. At the time when Vim receives the RGB response, it will check whether the response was positive, and whether the value decodes to '8' or '8/8/8' (the two known formats for "8 bits per component"), and only in that case set the 'rgb' prop to 'y', otherwise 'n'. The TermResponseAll event remains (renamed to "rgb" in lowercase) so that the vimrc can react to the response, should the user want it to.
I'm not yet sure how values besides 8 are reflected in the terminal behaviour. I suspect that they always parse component values from 0 to 255 and either chop less-significant bits off or find the closest representable colour if the display isn't actually 8 bpc. But I haven't found documentation that says this for certain. Either way, 8 bpc is common these days and the direct-color format doesn't afford a greater range than 0 to 255, so I think requiring exactly 8 bpc is reasonable.
What do you think?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#21191)The plan is the right shape, but two parts of it will not hold.
'n' cannot be reached
ctlseqs gives the failure reply as "DCS 0 + r ST for invalid requests" - no
name in it. Vim keeps up to ten XTGETTCAP requests outstanding at once,
req_more_codes_from_term() sending while xt_index_out < xt_index_in + 10, so a
bare "DCS 0 + r ST" cannot be attributed to RGB or to any other capability.
That is why got_code_from_term() looks only at code[0] == '1' today.
So 'rgb' will be 'y' or 'u', never 'n'. That still matches what you described:
Vim starts out assuming no direct color, and a vimrc that has waited and still
reads 'u' has its answer.
The value should not decide
Set 'y' on any positive reply and do not read the value at all.
The RGB capability is the terminal reporting that it understands direct color.
The number is how many bits per channel it can show, which is a separate
question - SGR 38:2 carries 0 to 255 per channel whatever the display does with
them, as you suspected. A terminal answering "5/6/5" understands every sequence
'termguicolors' emits and renders them as closely as it can; reporting 'n' for
it drops the user back to 256 colors.
This also drops the parsing: no "8" against "8/8/8", and nothing to revisit
when a terminal answers something neither of us predicted.
The rest of the plan - dropping v:termrgbresp, keeping TermResponseAll with
"rgb" in lowercase - reads right to me.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
erry
AstroSnail left a comment (vim/vim#21191)That makes sense. It raises another question though. If a terminal doesn't respond to XTGETTCAP RGB, but does support direct colour and exposes this through its terminal description (e.g. TERM=xterm-direct), what should count, the number of colours (e.g. Co#16777216), the presence of RGB, or either? The original patch #16490 doesn't seem to do anything useful with RGB from the terminal description, and relies on number of colours for this purpose. It's not obvious to me whether the RGB capability is even exposed through the termcap library.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#21191)RGB does not reach Vim through the terminal description
Vim reads capabilities with the termcap-compatible calls - tgetent(),
tgetstr(), tgetflag(), tgetnum() - and those names are two characters. RGB is
a three-character extended capability. Against xterm-direct here:
tgetnum("Co")=16777216 tgetnum("RGB")=-1 tgetflag("RGB")=0
tigetnum("colors")=16777216 tigetnum("RGB")=-2 tigetflag("RGB")=1
The terminfo calls do find it, as a boolean - hence the -2 from tigetnum - but
Vim does not use those. So "the presence of RGB" is not one of the options
unless Vim moves to the terminfo API, which is a change of a different size.
That would explain why #16490 went by the color count.
Either source should count
Co is what the description leaves, and it already lands in t_colors. 16777216
there reports direct color as plainly as RGB would.
The two sources are independent. In #16568 the terminal was xterm under
TERM=xterm-256color, an entry with colors#256 and no RGB, and the XTGETTCAP
answer still turned 'termguicolors' on - xterm answers for itself, not from
the entry $TERM names. Your case is the mirror image: an entry with
colors#16777216 and a terminal that never answers.
So set 'rgb' to 'y' when t_colors reaches 16777216, and on a positive
XTGETTCAP RGB reply. A user who wants direct color does not care which of the
two told Vim.
A note on the value
RGB in the xterm-direct entry is a boolean and carries no number. The "8" and
"8/8/8" shapes exist only in the XTGETTCAP reply - one more reason not to build
the decision on them.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#21191)The color count does not need a copy in term_props
t_colors is a value that can be read at any time, not an answer to remember, so keeping a copy of it in term_props is what creates the reset problem you ran into. Report it where the dictionary is built instead, in f_terminalprops(), and both assignments go away:
value[0] = term_props[i].tpr_status; if (i == TPR_RGB && t_colors == 0x1000000) value[0] = TPR_YES;
term_props then holds the XTGETTCAP answer alone, tpr_set_by_termresponse stays TRUE for it, and the property follows 'term' and t_Co on its own.
An accessor keeps term_props to itself
For the bookkeeping in block_autocmds() and unblock_autocmds(), an accessor in term.c that returns term_props[TPR_RGB].tpr_status is enough. The property only ever goes from unknown to yes, so autocmd.c can hold an int and compare it. Making term_props global is more than this needs.
That also replaces what is still commented out there: the old_termrgbresp declaration and four lines in unblock_autocmds(). Vim uses // rather than /* */.
The property does not depend on FEAT_TERMGUICOLORS
The XTGETTCAP branch in got_code_from_term() sits under #ifdef FEAT_TERMGUICOLORS, while the color count checks in ttest() and handle_version_response() do not. A build without the feature then reports 'y' from the color count but stays at 'u' however the terminal answers. What a terminal supports does not depend on how Vim was built. The branch no longer touches 'termguicolors' now that p_tgc_set is gone, so the #ifdef can go with it.
Two-character names in key_names
In get_term_entries(), key_names[i][2] == 0 reads as a puzzle. Please write NUL and add the reason: RGB is asked of the terminal, and the termcap library has no three-character names.
da1 and osc
Adding them to block_autocmds() and unblock_autocmds() is right, but it is unrelated to RGB and would be easier to see as a commit of its own.
Documentation
In term.txt, the note that :redraw! is needed, and that it clears the intro message, documents a rough edge rather than the feature. Should Vim redraw on its own after an autocommand changed 'termguicolors'? If it should, this note is better left out here and the redraw handled separately.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
erry
AstroSnail left a comment (vim/vim#21191)The property only ever goes from unknown to yes
The property may go from yes to unknown during the brief time after the 'term' setting is changed or a terminal version response is received and before the XTGETTCAP RGB response is received. I don't expect this to be a serious problem for block/unblock_autocmds, the worst case is a spurious extra trigger of the TermResponseAll 'rgb' event.
da1 and osc
Adding them to block_autocmds() and unblock_autocmds() is right, but it is unrelated to RGB and would be easier to see as a commit of its own.
Are you referring to commit 605d740? I don't understand what you want me to change.
Should Vim redraw on its own after an autocommand changed 'termguicolors'? If it should, this note is better left out here and the redraw handled separately.
I would really like to solve the redraw problem before considering this PR complete. I did some testing and found that Vim can draw the intro screen as many as 3 times on startup:
- immediately;
- after receiving the terminal background color response and switching the 'background' setting to dark;
- after receiving the XTGETTCAP Co response and changing the colour count;
- maybe once more if the 'ambiwidth' test sets it to double, but I couldn't test this.
As far as I can tell the redraws are triggered by calls to redraw_asap(UPD_CLEAR), but adding a redraw_asap(UPD_CLEAR) call at the end of did_set_termguicolors doesn't seem to force it to redraw the screen (and it adds another bug: Vim draws the intro before the screen has been put in termcap mode, leaving visible trash after Vim exits). I'd like some help here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
erry
AstroSnail left a comment (vim/vim#21191)Nevermind that last point, I think I've solved it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
erry
AstroSnail left a comment (vim/vim#21191)Thanks, I don't think it makes sense in f_termprops().rgb to return the RGB key only when the color count has been set to 16777216, instead let's just return the detected value from the XTGETTCAP query or the TERM value.
To make it clear to me: you are saying that colour count should not affect the value of terminalprops().rgb? The current implementation returns 'y' in either case that colour count is 16777216 or XTGETTCAP response is detected.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#21191)Chris is right about t_Co, and it was my suggestion that put that check in
f_terminalprops(), so please drop it and go with what he proposed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#21191)I tested it out with a sample vimrc file:
cat /tmp/vimrc
set nocompatible
set nu
autocmd TermResponseAll * if expand('<amatch>') == 'rgb' |set termguicolors | endif
Running this under xterm sometimes enables termguicolors but mostly doesn't. It looks like a race, the response is sent too early, before that part of the vimrc file was parsed. Not sure how easy it would be to fix this.
Hm, found it. It seems autocmd_busy is set when calling apply_autocmds(), which makes the TermResponseAll being ignored since force is false. This seems to happen when the response arrives while Vim handles the VimEnter autocommand.
But even when I set force=TRUE, it doesn't work because RedrawingDisabled has been set, so the whole redraw_asap() is just skipped.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
erry
AstroSnail left a comment (vim/vim#21191)It looks like a race, the response is sent too early, before that part of the vimrc file was parsed.
I'm having trouble reproducing this. When I use your sample vimrc like vim -u vimrc it works the way I expect every time. When I add a sleep or a busy loop to the toplevel of the vimrc, before the TermResponseAll autocommand, Vim seems to wait with an uninitialized terminal until vimrc finishes running (which makes sense to me, the source_startup_scripts function is called in main before RedrawingDisabled is turned off for the first time and may_req_termresponse is called in vim_main2).
This seems to happen when the response arrives while Vim handles the VimEnter autocommand.
When I add a sleep or a busy loop to the VimEnter autocommand, Vim seems to get as far as sending out the version and foreground/background requests before waiting, to which the terminal replies quickly, but Vim only starts sending xterm-codes after it ends. Same thing seems to happen when I open a file.
I want to understand how you got the behaviour you described. I've found script (from util-linux) extremely helpful for debugging terminal responses, maybe it can help you tell the sequence of events that happens?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.