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

[レビュー依頼] xtermとの互換性が低い端末でゴミが表示される問題へのパッチ #1089

Closed
ttdoda opened this Issue Sep 14, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@ttdoda

ttdoda commented Sep 14, 2017

Vim 8.0.0965 で Vim を動かしている端末のカーソル形状の取得機能が、8.0.1009 でカーソルの点滅状態の取得機能が追加されました。
しかし、これらの機能で使用している制御シーケンスは xterm との互換性が低い端末では正しく取り扱えず、起動時にゴミが表示されるという問題が発生しています。
これへの対応として v:termresponse で該当の端末かを判別して上記機能を利用しないという事が行われています。(ブラックリスト方式)

しかしこの方法には以下の問題が有ると考えられます。

  • 新たに問題が出る端末が見つかった場合、ブラックリストに都度追加しなければいけない。
    例えば Vim 8.0.1102 の時点で Poderosa や Absolute Telnet で問題が出ます。
  • 対策済みのはずの端末でも、バージョンによって v:termresponse の値が違う場合に問題が出る。
    例えば SecureCRT 8 の対策が入っていますが、7 およびそれ以前のバージョンでは問題が出ます。
  • ブラックリストに加えられた端末がカーソル形状/点滅状態取得に対応しても、取得が行われない。

そこで v:termresponse を頼らずに、端末の挙動を元に該当機能の使用/不使用を判断するパッチを作成しましたので、レビュー、および動作確認をお願いします。
特に自分には macOS の環境が無いので macOS 上の端末での確認がまったく行えていません。

パッチの内容ですが、xterm との互換性の低い端末で問題が出るような文字列を送信し、カーソル位置が動いたら該当機能を使わないようにしています。:wq

自分では以下を確認しています。

  • xterm, Tera Term 等のカーソル形状取得に対応した端末で機能が使われる事
  • 今まで問題が報告された端末の内 Terminal.app 以外、および前述の Poderosa, Absolute Telnet で問題が出ない事 (確認したのは以下)
    • PuTTY 0.70
    • Gnome Terminal 等の VTE 利用端末 (VTE 0.44.2, 0.42.4, 0.28.2)
    • SecureCRT 6.7.4, 7.0.0, 8.1.4
    • Konsole 16.12.3
    • GNU Screen 4.03.01
    • Poderosa 4.4.1
    • Absolute Telnet 9.53, 10.16
  • 他にいくつかの端末で問題が出ない事 (mintty, tmux 等)
diff --git a/src/main.c b/src/main.c
index 5840085..8b9f53e 100644
--- a/src/main.c
+++ b/src/main.c
@@ -812,6 +812,11 @@ vim_main2(void)
     may_req_ambiguous_char_width();
 #endif
 
+#if defined(FEAT_TERMRESPONSE)
+    /* Must be done before redrawing, puts a few characters on the screen. */
+    may_req_xterm_compat_test();
+#endif
+
     RedrawingDisabled = 0;
     redraw_all_later(NOT_VALID);
     no_wait_return = FALSE;
diff --git a/src/proto/term.pro b/src/proto/term.pro
index 65e6740..fa144eb 100644
--- a/src/proto/term.pro
+++ b/src/proto/term.pro
@@ -43,6 +43,7 @@ void starttermcap(void);
 void stoptermcap(void);
 void may_req_termresponse(void);
 void may_req_ambiguous_char_width(void);
+void may_req_xterm_compat_test(void);
 void may_req_bg_color(void);
 int swapping_screen(void);
 void setmouse(void);
diff --git a/src/term.c b/src/term.c
index 22c7c9a..037cac9 100644
--- a/src/term.c
+++ b/src/term.c
@@ -133,6 +133,9 @@ static int rbm_status = STATUS_GET;
 
 /* Request cursor style report: */
 static int rcs_status = STATUS_GET;
+
+/* xterm compatibility check: */
+static int rdcs_status = STATUS_GET;
 # endif
 
 /*
@@ -3509,6 +3512,46 @@ may_req_ambiguous_char_width(void)
 # endif
 
 /*
+ * Check the terminal is xterm compatible.
+ * First, we move the cursor to (2, 0) and print a test sequence and query
+ * current cursor position.
+ * If the terminal properly handles unknown DCS string and CSI sequence with
+ * intermediate character, test sequence is ignored and the cursor does not
+ * move, or the terminal handles test sequence incorrectly, a garbage string
+ * is displayed and the cursor moves.
+ * This function has the side effect that changes cursor position, so
+ * it must be called immediately after entering termcap mode.
+ */
+    void
+may_req_xterm_compat_test(void)
+{
+    if (rdcs_status == STATUS_GET
+	    && can_get_termresponse()
+	    && starting == 0
+	    && *T_U7 != NUL)
+    {
+	LOG_TR("Sending xterm compatibility test sequence.");
+	/* Do this in the third row. Second row is used by ambiguous character
+	 * check.  */
+	term_windgoto(2, 0);
+	out_str((char_u *)"\033Pzz\033\\");
+	out_str((char_u *)"\033[0%m");
+	out_str(T_U7);
+	rdcs_status = STATUS_SENT;
+	out_flush();
+
+	/* If the terminal does not handle test sequence correctly, garbage
+	 * string is displayed. Clear them out for now. */
+	term_windgoto(2, 0);
+	out_str((char_u *)"           ");
+	term_windgoto(0, 0);
+
+	out_flush();
+	(void)vpeekc_nomap();
+    }
+}
+
+/*
  * Similar to requesting the version string: Request the terminal background
  * color when it is the right moment.
  */
@@ -4491,6 +4534,15 @@ check_termcode(
 # endif
 			}
 		    }
+		    /* Third row is xterm compatibility test */
+		    else if (row_char == '3') {
+			if (col != 1) {
+			    /* Cursor is not on a first column. Then terminal
+			     * is not xterm compatible. */
+			    is_not_xterm = TRUE;
+			}
+			rdcs_status = STATUS_GOT;
+		    }
 		    key_name[0] = (int)KS_EXTRA;
 		    key_name[1] = (int)KE_IGNORE;
 		    slen = i + 1;
@@ -4567,28 +4619,13 @@ check_termcode(
 			 * "xterm-256colors"  but are not fully xterm
 			 * compatible. */
 
-			/* Mac Terminal.app sends 1;95;0 */
-			if (version == 95
-				&& STRNCMP(tp + extra - 2, "1;95;0c", 7) == 0)
-			    is_not_xterm = TRUE;
-
-			/* Gnome terminal sends 1;3801;0, 1;4402;0 or 1;2501;0.
-			 * xfce4-terminal sends 1;2802;0.
-			 * screen sends 83;40500;0
-			 * Assuming any version number over 2500 is not an
-			 * xterm (without the limit for rxvt and screen). */
-			if (col >= 2500)
-			    is_not_xterm = TRUE;
-
-			/* PuTTY sends 0;136;0
-			 * vandyke SecureCRT sends 1;136;0 */
-			if (version == 136
-				&& STRNCMP(tp + extra - 1, ";136;0c", 7) == 0)
-			    is_not_xterm = TRUE;
-
-			/* Konsole sends 0;115;0 */
-			if (version == 115
-				&& STRNCMP(tp + extra - 2, "0;115;0c", 8) == 0)
+			/* GNU screen sends 83;30600;0. 30600 is a version of
+			 * GNU screen. DA2 support is added on 3.6.
+			 * DCS string has a special meaning to GNU screen,
+			 * but xterm compatibility checking does not detect
+			 * GNU screen. */
+			if (col >= 30600
+			    && STRNCMP(tp + extra - 3, "83;", 3) == 0)
 			    is_not_xterm = TRUE;
 
 			/* Only request the cursor style if t_SH and t_RS are
@h-east

This comment has been minimized.

Show comment
Hide comment
@h-east

h-east Sep 14, 2017

Member

素晴らしい。
ロジックまでは見てませんがさらっと眺めて以下の- 3が違和感を感じました。

+			    && STRNCMP(tp + extra - 3, "83;", 3) == 0)

追記:
あー、合ってますね。失礼しました。

Member

h-east commented Sep 14, 2017

素晴らしい。
ロジックまでは見てませんがさらっと眺めて以下の- 3が違和感を感じました。

+			    && STRNCMP(tp + extra - 3, "83;", 3) == 0)

追記:
あー、合ってますね。失礼しました。

@ichizok

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Sep 14, 2017

Member

Terminal.app, iTerm2 (on macOS 10.12.6) で確認、問題なさそうです。

Member

ichizok commented Sep 14, 2017

Terminal.app, iTerm2 (on macOS 10.12.6) で確認、問題なさそうです。

@k-takata

This comment has been minimized.

Show comment
Hide comment
@k-takata

k-takata Sep 15, 2017

Member
 /* Request cursor style report: */
 static int rcs_status = STATUS_GET;
+
+/* xterm compatibility check: */
+static int rdcs_status = STATUS_GET;

rdcs とは何の略でしょう?上のrcsとの対比だと、/* Request xxx xxx xxx report (used for xterm compatibility check): */ のような書き方が分かりやすいのではないかと思いました。

Member

k-takata commented Sep 15, 2017

 /* Request cursor style report: */
 static int rcs_status = STATUS_GET;
+
+/* xterm compatibility check: */
+static int rdcs_status = STATUS_GET;

rdcs とは何の略でしょう?上のrcsとの対比だと、/* Request xxx xxx xxx report (used for xterm compatibility check): */ のような書き方が分かりやすいのではないかと思いました。

@ttdoda

This comment has been minimized.

Show comment
Hide comment
@ttdoda

ttdoda Sep 15, 2017

rdcs とは何の略でしょう?

reply of DCS string test ぐらいのつもりでした。
元々は vim/vim#2008 (DCS stringの取り扱いの問題) への対処としてこのパッチを書き始めたのでした。
しかしもう片方の問題 (CSI Sequence with Intermediate character の取り扱いの問題) の問題も根深いので、両方への対処に方針を変えた時にこの部分の名前を変え忘れていました。

上のrcsとの対比だと、/* Request xxx xxx xxx report (used for xterm compatibility check): */ のような書き方が分かりやすいのではないかと思いました。

使っているリクエストは may_req_ambiguous_char_width() と同じで u7 (Request cursor position) なので名前がかぶってしまうんですよね。
xcc_status (xterm compatibility check status) とかはどうでしょうか?

ttdoda commented Sep 15, 2017

rdcs とは何の略でしょう?

reply of DCS string test ぐらいのつもりでした。
元々は vim/vim#2008 (DCS stringの取り扱いの問題) への対処としてこのパッチを書き始めたのでした。
しかしもう片方の問題 (CSI Sequence with Intermediate character の取り扱いの問題) の問題も根深いので、両方への対処に方針を変えた時にこの部分の名前を変え忘れていました。

上のrcsとの対比だと、/* Request xxx xxx xxx report (used for xterm compatibility check): */ のような書き方が分かりやすいのではないかと思いました。

使っているリクエストは may_req_ambiguous_char_width() と同じで u7 (Request cursor position) なので名前がかぶってしまうんですよね。
xcc_status (xterm compatibility check status) とかはどうでしょうか?

@k-takata

This comment has been minimized.

Show comment
Hide comment
@k-takata

k-takata Sep 15, 2017

Member

そういうことであれば xcc_status がよさそうです。

Member

k-takata commented Sep 15, 2017

そういうことであれば xcc_status がよさそうです。

@ttdoda

This comment has been minimized.

Show comment
Hide comment
@ttdoda

ttdoda Sep 19, 2017

指摘して頂いた点を修正してPull Requestを出してみました vim/vim#2126

ttdoda commented Sep 19, 2017

指摘して頂いた点を修正してPull Requestを出してみました vim/vim#2126

@k-takata

This comment has been minimized.

Show comment
Hide comment
@k-takata

k-takata Sep 28, 2017

Member

todo入りしましたので、一旦閉じます。 vim/vim@24a98a0#diff-38d7929bd26d74d92ceddf984bbfc8dbR46

Universal solution to detect if t_RS is working, using cursor position.
Koichi Iwamoto, #2126

取り込まれるのが数日後なのか、数週間後なのか、はたまた数年後なのかはBramにしか分かりません。辛抱強く待ちましょう。(リストの上の方にあるので、優先度は高いはずですが…)

Member

k-takata commented Sep 28, 2017

todo入りしましたので、一旦閉じます。 vim/vim@24a98a0#diff-38d7929bd26d74d92ceddf984bbfc8dbR46

Universal solution to detect if t_RS is working, using cursor position.
Koichi Iwamoto, #2126

取り込まれるのが数日後なのか、数週間後なのか、はたまた数年後なのかはBramにしか分かりません。辛抱強く待ちましょう。(リストの上の方にあるので、優先度は高いはずですが…)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment