[vim/vim] Add checking the terminal is xterm compatible (#2126)

[IWAMOTO Kouichi]

Some terminals do not properly handle DCS string and/or CSI sequence with intermediate byte.
Therefore, when get the cursor style, garbage string is displayed.

This patch detects such a terminal, checking with termresponse becomes unnecessary.
This patch solves most case of #2008

Tested with following terminals:

• xterm pl#330
• mlterm 3.8.2
• rxvt 2.6.4
• rxvt-unicode 9.22
• xvt 3.0.0
• Gnome Terminal and other VTE-based terminals (VTE 0.44.2, 0.42.4, 0.28.2)
• Konsole 16.12.3
• Terminal.app (macOS 10.12.6)
• iTerm2
• Tera Term 4.96
• PuTTY 0.70
• SecureCRT 6.7.4(*), 7.0.0(*), 8.1.4
• Poderosa 4.4.1(*), 5.4.2(*)
• Absolute Telnet 9.53(*), 10.16(*)
• TTYEmulator 4.3.0.2
• GNU Screen 4.03.01
• tmux 2.5

(*) has problem with plain vim 8.0.1127.

---

You can view, comment on, or merge this pull request online at:

  https://github.com/vim/vim/pull/2126

Commit Summary

• Check the terminal is xterm compatible.

File Changes

• M src/main.c (5)
• M src/proto/term.pro (1)
• M src/term.c (83)

Patch Links:

• https://github.com/vim/vim/pull/2126.patch
• https://github.com/vim/vim/pull/2126.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub

[Kazunobu Kuriyama]

It could be very helpful for the review if you would add your own comment on what will happen to Vim working over SSH of a various combination of platforms and terminals after the proposed patch is merged.

[Codecov]

Codecov Report

Merging #2126 into master will increase coverage by 0.03%.
The diff coverage is 21.73%.

@@            Coverage Diff             @@
##           master    #2126      +/-   ##
==========================================
+ Coverage   73.92%   73.95%   +0.03%     
==========================================
  Files          86       90       +4     
  Lines      131596   131874     +278     
  Branches    28951    28960       +9     
==========================================
+ Hits        97280    97530     +250     
- Misses      34293    34325      +32     
+ Partials       23       19       -4

Impacted Files	Coverage Δ
src/main.c	55.02% <100%> (+0.03%)	⬆️
src/term.c	50.97% <18.18%> (-0.12%)	⬇️
src/gui_beval.c	39.87% <0%> (-4.61%)	⬇️
src/window.c	81.12% <0%> (-0.5%)	⬇️
src/gui_gtk_x11.c	47.66% <0%> (-0.21%)	⬇️
src/if_xcmdsrv.c	83.63% <0%> (-0.18%)	⬇️
src/libvterm/src/screen.c	71.33% <0%> (-0.03%)	⬇️
src/kword_test.c	66.66% <0%> (ø)
src/message_test.c	100% <0%> (ø)
src/memfile_test.c	100% <0%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8e93d6...c19750f. Read the comment docs.

[Bram Moolenaar]

Kouichi Iwamoto wrote:

> Some terminals do not properly handle DCS string and/or CSI sequence
> with intermediate byte.
> Therefore, when get the cursor style, garbage string is displayed.
>
> This patch detects such a terminal, checking with termresponse becomes
> unnecessary.
> This patch solves most case of #2008
>
> Tested with following terminals:

> * xterm pl#330
> * mlterm 3.8.2
> * rxvt 2.6.4
> * rxvt-unicode 9.22
> * xvt 3.0.0
> * Gnome Terminal and other VTE-based terminals (VTE 0.44.2, 0.42.4, 0.28.2)
> * Konsole 16.12.3
> * Terminal.app (macOS 10.12.6)
> * iTerm2
> * Tera Term 4.96
> * PuTTY 0.70
> * SecureCRT 6.7.4(\*), 7.0.0(\*), 8.1.4
> * Poderosa 4.4.1(\*), 5.4.2(\*)
> * Absolute Telnet 9.53(*), 10.16(\*)
> * TTYEmulator 4.3.0.2
> * GNU Screen 4.03.01
> * tmux 2.5
>
> (\*) has problem with plain vim 8.0.1127.

That's an interesting idea.
There were some problems with the similar method used to obtain the
ambiguous width, but I think they were all solved.

Can we integrate the two checks, to reduce the number of flushes and
especially the vpeek calls? Also avoid the cursor moving around
multiple times.

--
hundred-and-one symptoms of being an internet addict:
166. You have been on your computer soo long that you didn't realize
you had grandchildren.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[mattn]

@brammool Do you mean that we should make new function that do both of may_req_ambiguous_char_width and may_req_xterm_compat_test? Or simply reducing out_flush in may_req_xterm_compat_test? The first one may be bit confusable to read code.

[Kazunobu Kuriyama]

There were some problems with the similar method used to obtain the ambiguous width, but I think they were all solved.

Really? shrug. I'm wondering why this sort of problems is persistent with Vim in comparison with other pieces of OSS software. Seems like this community prefers louder voices. and decisions are made more politically than technically, but people hardly know its politics.

Anyway, while I like the idea, but not-mentioning latency makes me uneasy. Is this trivial? Then just mention it; otherwise, we will need another tweak to make our test suite not-flaky.

[Bram Moolenaar]

Yasuhiro wrote:

> @brammool Do you mean that we should make new function that do both of
> may_req_ambiguous_char_width and may_req_xterm_compat_test? Or simply
> reducing out_flush in may_req_xterm_compat_test? The first one may be
> bit confusable to read code.

Make it one function. Yes, it will be a bit harder to read, but
reducing the overhead is relevant. Especially over remote connections.
And once it works we don't have to read it again :-).

--
"The question of whether computers can think is just like the question
of whether submarines can swim." -- Edsger W. Dijkstra

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

—

[IWAMOTO Kouichi]

@ttdoda pushed 1 commit.

• 973f145 Integrate the two checks in one function.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[IWAMOTO Kouichi]

Make it one function.

updated the patch. Could you check it?

[K.Takata]

Could you resolve conflicts?

[IWAMOTO Kouichi]

@ttdoda pushed 149 commits.

• b00fdf6 patch 8.0.1131: not easy to trigger an autocommand for new terminal window
• fc7649f patch 8.0.1132: #if condition is not portable
• 8b21de3 Missing part of 8.0.1131.
• f3d769a patch 8.0.1133: syntax timeout not used correctly
• 7630195 patch 8.0.1134: superfluous call to syn_get_final_id()
• 53f8174 patch 8.0.1135: W_WINCOL() is always the same
• 0263146 patch 8.0.1136: W_WIDTH() is always the same
• e745d75 patch 8.0.1137: cannot build with Ruby
• eb163d7 patch 8.0.1138: click in window toolbar starts Visual mode
• a21a6a9 patch 8.0.1139: using window toolbar changes state
• bb3e641 patch 8.0.1140: still old style tests
• 96e7a6e patch 8.0.1141: MS-Windows build dependencies are incomplete
• 378daf8 patch 8.0.1142: window toolbar menu gets a tear-off item
• e0de17d patch 8.0.1143: macros always expand to the same thing
• 0b05e49 patch 8.0.1144: using wrong #ifdef for computing length
• 0c6a329 patch 8.0.1145: warning when compiling with Perl
• 452030e patch 8.0.1146: redraw when highlight is set with same names
• 7c456a4 patch 8.0.1147: fail to build with tiny features
• 22ab547 patch 8.0.1148: gN doesn't work on last match with 'wrapscan' off
• a8fc0d3 patch 8.0.1149: libvterm colors differ from xterm
• 6edeaf3 patch 8.0.1150: MS-Windows GUI: dialog font size is incorrect
• 09ca932 patch 8.0.1151: "vim -c startinsert!" doesn't append
• 2a02745 patch 8.0.1152: encoding of error message wrong in Cygwin terminal
• 97fbc40 patch 8.0.1153: no tests for diff_hlID() and diff_filler()
• 1b38344 patch 8.0.1154: 'indentkeys' does not work properly
• d1bc96c patch 8.0.1155: Ruby command triggers a warning
• 1ec96c9 patch 8.0.1156: trouble from removing one -W argument from Perl CFLAGS
• 9cf39cc patch 8.0.1157: compiler warning on MS-Windows
• 24a98a0 Update runtime files
• db51007 patch 8.0.1158: still old style tests
• d371bbe patch 8.0.1159: typo in #ifdef
• 816968d patch 8.0.1160: getting tab-local variable fails after closing window
• a5e6621 patch 8.0.1161: popup menu drawing problem when resizing terminal
• 660b85e patch 8.0.1162: shared script for tests cannot be included twice
• c79977a patch 8.0.1163: popup test is flaky
• 65ed136 patch 8.0.1164: changing StatusLine highlight does not always work
• f52c383 patch 8.0.1165: popup test is still flaky
• c958b31 patch 8.0.1166: :terminal doesn't work on Mac High Sierra
• 3a497e1 patch 8.0.1167: Motif: typing in terminal window is slow
• 0aa398f patch 8.0.1168: wrong highlighting with combination of match and 'cursorline'
• 5ece3e3 patch 8.0.1169: highlignting one char too many with 'list' and 'cul'
• f336061 patch 8.0.1170: using termdebug results in 100% CPU time
• 712549e patch 8.0.1171: popup test is still a bit flaky
• 2a6a6c3 patch 8.0.1172: when E734 is given option is still set
• 19a3d68 patch 8.0.1173: terminal window is not redrawn after CTRL-L
• a0a6f27 patch 8.0.1174: Mac Terminal.app has wrong color for white
• c902609 patch 8.0.1175: build failure without +termresponse
• d78f03f patch 8.0.1176: job_start() does not handle quote and backslash correctly
• 54e5dbf patch 8.0.1177: in a terminal window the popup menu is not cleared
• 73f4439 patch 8.0.1178: using old compiler on MS-Windows
• 6318205 patch 8.0.1179: Test_popup_and_window_resize() does not always pass
• 75f69e5 patch 8.0.1180: MS-Windows testclean target deletes the color script
• 4635e11 patch 8.0.1181: tests using Vim command fail on MS-Windows
• 0ab35b2 patch 8.0.1182: cannot see or change mzscheme dll name
• 18cfa94 patch 8.0.1183: MS-Windows build instructions are outdated
• 9b69f22 patch 8.0.1184: the :marks command is not tested
• 9202162 patch 8.0.1185: Ruby library includes minor version number
• 4a6fcf8 patch 8.0.1186: still quite a few old style tests
• 8065cf2 patch 8.0.1187: building with lua fails for OSX on Travis
• 1d68d9b patch 8.0.1188: autocmd test fails on MS-Windows
• 6047e2c patch 8.0.1189: E172 is not actually useful
• 2c33d7b patch 8.0.1190: unusable after opening new window in BufWritePre event
• 6199d43 patch 8.0.1191: MS-Windows: missing 32 and 64 bit files in installer
• ac8069b patch 8.0.1192: MS-Windows: terminal feature not enabled by default
• b2c8750 patch 8.0.1193: crash when wiping out a buffer after using getbufinfo()
• 65e4c4f patch 8.0.1194: actual fg and bg colors of terminal are unknown
• 9377df3 patch 8.0.1195: can't build on MS-Windows
• a20f83d patch 8.0.1196: crash when t_RF is not set
• 81b07b5 patch 8.0.1197: MS-Windows build instructions are not up to date
• 00ce63d patch 8.0.1198: older compilers don't know uint8_t
• 8bfe07b patch 8.0.1199: when 'clipboard' is "autoselectplus" star register is set
• 67418d9 patch 8.0.1200: tests switch the bell off twice
• 44cc4cf patch 8.0.1201: "yL" is affected by 'scrolloff'
• 059db5c patch 8.0.1202: :wall gives an errof for a terminal window
• 6daeef1 patch 8.0.1203: terminal window mistreats composing characters
• 87ffb5c patch 8.0.1204: a QuitPre autocommand may get the wrong file name
• ff930ca patch 8.0.1205: it is possible to unload a changed buffer
• fafcf0d patch 8.0.1206: no autocmd for entering or leaving the command line
• 67435d9 patch 8.0.1207: profiling skips the first and last script line
• 6b89dbb patch 8.0.1208: 'statusline' drops empty group with highlight change
• 53f0c96 patch 8.0.1209: still too many old style tests
• f8e8c06 patch 8.0.1210: CTRL-G and CTRL-T are ignored with typeahead
• ca05aa2 patch 8.0.1211: cannot reorder tab pages with drag & drop
• 66857f4 patch 8.0.1212: MS-Windows: tear-off menu does not work on 64 bit
• 2e4cb3b patch 8.0.1213: setting 'mzschemedll' has no effect
• 4f19828 patch 8.0.1214: accessing freed memory when EXITFREE is set
• 2f40d12 patch 8.0.1215: newer gcc warns for implicit fallthrough
• 6ce6504 patch 8.0.1216: tabline is not always updated for :file command
• d99388b patch 8.0.1217: can't use remote eval to inspect vars in debug mode
• 8d84ff1 patch 8.0.1218: writing to freed memory in autocmd
• f204e05 patch 8.0.1219: terminal test is flaky
• 235dddf patch 8.0.1220: skipping empty statusline groups is not correct
• 15993ce patch 8.0.1221: still too many old style tests
• ce11de8 patch 8.0.1222: test functions interfere with each other
• 9ad89c6 patch 8.0.1223: crash when using autocomplete and tab pages
• cf1ba35 patch 8.0.1224: still interference between test functions
• ee03b94 patch 8.0.1225: no check for spell region being zero
• 2a45d64 patch 8.0.1226: edit and popup tests failing
• dc1c981 patch 8.0.1227: undefined left shift in readfile()
• 0e19fc0 patch 8.0.1228: invalid memory access in GUI test
• 9a91c7a patch 8.0.1229: condition in vim_str2nr() is always true
• ce15775 patch 8.0.1230: CTRL-A in Visual mode uses character after selection
• c312b8b patch 8.0.1231: expanding file name drops dash
• c3fdf7f patch 8.0.1232: MS-Windows users are confused about default mappings
• b9fce6c patch 8.0.1233: typo in dos installer
• a6ce1cc patch 8.0.1234: MS-Windows: composing chars are not shown properly
• ef83956 patch 8.0.1235: cannot disable the terminal feature in a huge build
• d057301 patch 8.0.1236: Mac features are confusing
• af2d20c patch 8.0.1237: ":set scroll&" often gives an error
• 2e51d9a patch 8.0.1238: incremental search only shows one match
• 4857048 patch 8.0.1239: cannot use a lambda for the skip argument to searchpair()
• ba6febd patch 8.0.1240: MS-Windows: term_start() does not support environment
• 89c394f patch 8.0.1241: popup test is flaky
• ffd99f7 patch 8.0.1242: function argument with only dash is seen as number zero
• f45938c patch 8.0.1243: no test for what 8.0.1227 fixes
• b94340c patch 8.0.1244: search test does not work correctly on MS-Windows
• 3e1c617 patch 8.0.1245: when WaitFor() has a wrong expression it just waits a second
• b315876 patch 8.0.1246: popup test has an arbitrary delay
• 86b21bb patch 8.0.1247: not easy to find Debian build info
• 5130f31 patch 8.0.1248: stray + in README file
• c20e0d5 patch 8.0.1249: no error when WaitFor() gets an invalid wrong expression
• f8f8b2e patch 8.0.1250: 'hlsearch' highlighting not removed after incsearch
• d97fbf1 patch 8.0.1251: invalid expressin passed to WaitFor()
• 8889a5c patch 8.0.1252: incomplete translations makefile for MinGW/Cygwin
• 430dc5d patch 8.0.1253: still too many old style tests
• 4c22a91 patch 8.0.1254: undefined left shift in gethexchrs()
• ea84df8 patch 8.0.1255: duplicate badge README file
• 01164a6 Long overdue runtime update.
• a88254f patch 8.0.1256: typo in configure variable vim_cv_tgent
• 2973daa patch 8.0.1257: no test for fix of undefined behavior
• 52a2f0f patch 8.0.1258: 'ttymouse' is set to "sgr" even though it's not supported
• 13deab8 patch 8.0.1259: search test can be flaky
• ab8b1c1 patch 8.0.1260: using global variables for WaitFor()
• 1232624 patch 8.0.1261: program in terminal window gets NL instead of CR
• 7dd88c5 patch 8.0.1262: terminal redir test is flaky
• 5a73e0c patch 8.0.1263: others can read the swap file if a user is careless
• c363251 patch 8.0.1264: terminal debugger gets stuck in small window
• ad7dac8 patch 8.0.1265: swap test not skipped when there is one group
• ffe010f patch 8.0.1266: Test_swap_directory was commented out
• 5842a74 patch 8.0.1267: Test_swap_group may leave file behind
• 3bf8c3c patch 8.0.1268: PC install instructions are incomplete
• aace215 patch 8.0.1269: effect of autocommands on marks is not tested
• b0d45e7 Update runtime files.
• 8fdb35a patch 8.0.1270: mismatching file name with Filelist
• fb094e1 patch 8.0.1271: still too many old style tests
• 53ec795 patch 8.0.1272: warnings for unused variables in tiny build
• 411acc4 Check the terminal is xterm compatible.
• 9cfc6cb Integrate the two checks in one function.
• 0535432 Merge branch 'check-dcs-handling' of github.com:ttdoda/vim into check-dcs-handling

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[K.Takata]

@ttdoda Hmm, it looks you did something wrong. The history looks broken on the PR page.
I think the following commands fix the problem:

$ git checkout check-dcs-handling
$ git reset --hard 9cfc6cb
$ git push -f ttdoda HEAD  # Change "ttdoda" to appropriate remote name if needed. 

[K.Takata]

@k-takata commented on this pull request.

---

In src/term.c:

> @@ -4511,6 +4555,15 @@ check_termcode(
 # endif
 			}
 		    }
+		    /* Third row is xterm compatibility test */
+		    else if (row_char == '3') {
+			if (col != 1) {

Could you move the last { to the next line? (also for the above line.)

[IWAMOTO Kouichi]

@ttdoda pushed 1 commit.

• 6de84a5 fix coding style.

—
You are receiving this because you are subscribed to this thread.

View it on GitHub

[IWAMOTO Kouichi]

@ttdoda commented on this pull request.

---

In src/term.c:

> @@ -4511,6 +4555,15 @@ check_termcode(
 # endif
 			}
 		    }
+		    /* Third row is xterm compatibility test */
+		    else if (row_char == '3') {
+			if (col != 1) {

updated. thanks!

[K.Takata]

@ttdoda Thank you for updating.

@brammool I don't know why you lower the priority of this PR in the todo.txt, but I think this is ready to be merged.

[K.Takata]

Oh, 8.0.1303 causes conflict again. @ttdoda, could you resolve it?

[K.Takata]

@ttdoda Thank you. AppVeyor build failed, but I don't think it is caused by your patch.
I tried your patch on my account, and it successfully passed:
https://ci.appveyor.com/project/k-takata/vim/build/170

[Bram Moolenaar]

This is tricky, but since we keep having problems with various misbehaving terminals I think we should include this. The alternative is to build out the version response checking, which is tedious but perhaps a better long-term solution. We'll see how it goes.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, or unsubscribe.

[Bram Moolenaar]

Closed #2126 via a45551a.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, or unsubscribe.