[vim/vim] Fix ANSI colors in terminal on Windows (PR #18931)
MURAOKA Taro
Problem: ANSI escape colors are not displayed correctly in nontermguicolors in vim (cli) on Windows. The red and blue channels seem to be swapped.
A file used for testing: ansi.txt
Result image before applying this change:
image.png (view on web)
(Please ignore translated messages)
Result image after applying this change:
image.png (view on web)
Cause: When converting VTerm ANSI index colors to cterm colors in terminal.c, the Windows case equivalent to NR-16 (:help cterm-colors) is ignored.
Solution: Created and used a table to convert ANSI indexed colors to cterm's NR-16 representation (Windows only). This table corresponds to the inverse conversion of cterm_ansi_idx in term.c. The values in both tables are exactly the same, but the meanings are opposite, so they are separate tables.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/18931
Commit Summary
- 338d664 Fix ANSI colors in terminal on Windows
File Changes
(1 file)
- M src/terminal.c (16)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/terminal.c:
> + if (index < 16) {
+ index = ansi_to_cterm_nr16[index];
+ }
There are coding style issue.
⬇️ Suggested change- if (index < 16) {
- index = ansi_to_cterm_nr16[index];
- }
+ if (index < 16)
+ index = ansi_to_cterm_nr16[index];
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
@koron pushed 1 commit.
- f136762 Fix coding styles
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
@koron commented on this pull request.
Thank you @h-east 👍
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/terminal.c:
> @@ -3013,6 +3013,12 @@ may_toggle_cursor(term_T *term)
cursor_off();
}
+#ifdef MSWIN
+static const uint8_t ansi_to_cterm_nr16[] = {
+ 0, 4, 2, 6, 1, 5, 3, 7, 8, 12, 10, 14, 9, 13, 11, 15
+};
+#endif
We have similar logic already in term.c, search for cterm_ansi_idx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18931)Thanks, but please don't duplicate code. Is it also possible to create a test for this?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
MURAOKA Taro
koron left a comment (vim/vim#18931)please don't duplicate code.
Fixed in cff8223
Is it also possible to create a test for this?
No. That's not possible for now.
Because to write tests for this change, we need to access cterm_attr_table (and `attrentry_T.ae_u.cterm in it) in src/highlight.c, but there are no ways to access it from Vim script.
Such a built-in function can be written, but since it will be a public function, it will require time to discuss the specifications.
Do you think it's worth creating such a built-in function for this PR?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#18931)Wouldn’t you use screen dumps? Try searching for these keywords in vim/src/testdir/:
- CheckScreendump
- RunVimInTerminal
- VerifyScreenDump
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#18931)CheckScreendump shouldn't be used. It'll skip the test on Windows.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18931)And what about using term_scrape() and testing for the returned fg attributes? Would this work? Or perhaps a hex dump including the ansi color attributes that we could use?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)Wouldn’t you use screen dumps?
And what about using term_scrape() and testing for the returned fg attributes?
Yes, of course I tried it.
These output the internal state of VTerm. However, the problem in this case is converting from VTerm to cterm. To test this, we need to know the internal state of cterm after conversion.
The internal state of VTerm was normal before the conversion. Therefore, these functions cannot work for testing.
The following sentence I wrote previously means that there is no way to obtain the internal state of this cterm in Vim script.
Because to write tests for this change, we need to access cterm_attr_table (and `attrentry_T.ae_u.cterm in it) in src/highlight.c, but there are no ways to access it from Vim script.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)A screen dump can capture the attr (number), but it does not provide access to the actual detailed data (attrentry_T.ae_u.cterm). Therefore, it could not be used in this case.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)RunVimInTerminal
I hadn't tried this before, so I gave it a go.
Interestingly, the correct colors were displayed in a Vim terminal launched inside a Vim terminal.
This means that converting twice restores the correct value.
This approach is interesting, so I'll play around with it a bit more, but thinking that the real problem is happening in cterm , don't get your hopes up.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)But shouldn't I use RunVimInTerminal on Windows?
There seems to be a rule that you must use CheckRunVimInTerminal before using RunVimInTerminal.
And CheckRunVimInTerminal says "Skipped: cannot run Vim in a terminal window on Windows".
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#18931)Hmm, then just use term_start() directly.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)I wrote a script like this. Maybe I can write a test.
func Test_ansi_color() let lines = [ \ 'call term_start("cmd /C type ansi.txt")' \ ] call writefile(lines, 'XloadANSI', 'D') let cmd = GetVimCommandCleanTerm() "let options = #{curwin: 1} let buf = term_start(cmd .. '-S XloadANSI') sleep 500m let data = term_scrape(buf, 1) call assert_equal('#e00000', data[5]['bg']) endfunc
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)I added a test for Windows non-GUI specific in c65664d
Thanks @h-east @zeertzjq @chrisbra
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
MURAOKA Taro
koron left a comment (vim/vim#18931)Hmm the last color is different...
https://github.com/vim/vim/actions/runs/20275732854/job/58223772179?pr=18931#step:12:11591
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
