Use more C0 constants in Keyboard #2425

Tyriar · 2019-09-12T18:14:23Z

No description provided.

Tyriar · 2019-09-12T18:15:06Z

src/common/input/Keyboard.ts

        } else if (ev.keyCode === 221) {
-          // ^] - Operating System Command (OSC)


@jerch this comment seemed to be lying?

I cannot tell where this comes from. Time to look into old VT docs / xterm sources. Again GS looks fairly wrong here.

Edit: Correction - input of GS as control char. The comment is wrong. (OSC is ^[] in this notation.)

Tyriar · 2019-09-12T18:15:11Z

src/common/input/Keyboard.ts

        } else if (ev.keyCode === 220) {
-          // ^\ - String Terminator (ST)
-          result.key = String.fromCharCode(28);


@jerch this comment seemed to be lying?

Not sure if does the right thing at all, FS has no special meaning in VT land (treated as LF). I have to look up the DEC docs for this...

Edit: Correction - thats the input of FS as control char itself, so its correct. The comment is wrong. (ST is actually ^[\).

Tyriar · 2019-09-12T18:15:31Z

src/common/input/Keyboard.ts

        } else if (ev.keyCode === 219) {
-          // ^[ - Control Sequence Introducer (CSI)


@jerch while ^[ was right here, that's not CSI?

Well ^[ is a typical way to express ESC [ in some editors (and bash), thus it means CSI. But the code does something else?

Edit: Woops I mixed this with the control codes - so ^[ means ESC itself.

jerch

Can only comment on this atm. There are several things we should clarify before proceeding:

we should have a clear understanding of key to sequence mapping (prolly take it from xterm source / DEC docs)
we should make sure to respect terminal modes correctly (like application mode which changes some key reports)

This code looks very awkward to me, full of magic numbers and unclear range tests. Guess this has grown by trial and error? Also those longish condition checks look error prone to me.

jerch · 2019-09-12T18:57:21Z

src/common/input/Keyboard.ts

@@ -316,23 +316,18 @@ export function evaluateKeyboardEvent(
        if (ev.keyCode >= 65 && ev.keyCode <= 90) {
          result.key = String.fromCharCode(ev.keyCode - 64);
        } else if (ev.keyCode === 32) {
-          // NUL
-          result.key = String.fromCharCode(0);


WTH (who wrote this in the first place?)

jerch · 2019-09-12T19:00:11Z

src/common/input/Keyboard.ts

        } else if (ev.keyCode === 219) {
-          // ^[ - Control Sequence Introducer (CSI)


Well ^[ is a typical way to express ESC [ in some editors (and bash), thus it means CSI. But the code does something else?

Edit: Woops I mixed this with the control codes - so ^[ means ESC itself.

jerch · 2019-09-12T19:01:57Z

src/common/input/Keyboard.ts

        } else if (ev.keyCode === 220) {
-          // ^\ - String Terminator (ST)
-          result.key = String.fromCharCode(28);


Not sure if does the right thing at all, FS has no special meaning in VT land (treated as LF). I have to look up the DEC docs for this...

Edit: Correction - thats the input of FS as control char itself, so its correct. The comment is wrong. (ST is actually ^[\).

jerch · 2019-09-12T19:04:58Z

src/common/input/Keyboard.ts

        } else if (ev.keyCode === 221) {
-          // ^] - Operating System Command (OSC)


I cannot tell where this comes from. Time to look into old VT docs / xterm sources. Again GS looks fairly wrong here.

Edit: Correction - input of GS as control char. The comment is wrong. (OSC is ^[] in this notation.)

jerch · 2019-09-12T19:42:51Z

@Tyriar Yeah most certainly those FS, GS mappings are wrong. They are mentioned in xterm docs for Tektronix mode, but we dont support this at all. Tested it in xterm - the comments were right there and we should actually return what was commented there. But we have the next problem here - this will not work with different keyboard layouts, they are likely to report different numbers (quickly tested with German vs English keyboard). This might be the source of some of the keyboard issues we have seen so far. Guess this needs a very fundamental refactoring...

Edit: See corrections above.

jerch · 2019-09-12T20:32:06Z

@Tyriar Because you wondered where Ctrl+Space == '\x00' comes from: https://archive.org/details/bitsavers_decstandar0VideoSystemsReferenceManualDec91_74264381/page/n561

Tyriar · 2019-09-12T21:43:32Z

Not sure who made those comments, maybe it was me? 🤷‍♂

But anyway, term.js seems to have correct comments so I think these changes are good: https://github.com/chjj/term.js/blob/548af6e0a15c5978a2b62a882f7db03ec2ae2850/src/term.js#L2789-L2804

Tyriar · 2019-09-12T21:44:34Z

This code looks very awkward to me, full of magic numbers and unclear range tests. Guess this has grown by trial and error? Also those longish condition checks look error prone to me.

It's barely changed since xterm.js was forked, mainly just moving from the main file and converting to TS.

Tyriar

Moving forward on this as the code seemed to be right and the comments wrong.

Tyriar requested a review from jerch September 12, 2019 18:14

Tyriar self-assigned this Sep 12, 2019

Use more C0 constants in Keyboard

0a4883b

Tyriar force-pushed the constant_key_board branch from 5289a35 to 0a4883b Compare September 12, 2019 18:14

Tyriar commented Sep 12, 2019

View reviewed changes

jerch reviewed Sep 12, 2019

View reviewed changes

Tyriar commented Sep 26, 2019

View reviewed changes

Tyriar added this to the 4.1.0 milestone Sep 26, 2019

Tyriar merged commit 22b06ca into xtermjs:master Sep 26, 2019

Tyriar deleted the constant_key_board branch September 26, 2019 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use more C0 constants in Keyboard #2425

Use more C0 constants in Keyboard #2425

Tyriar commented Sep 12, 2019

Tyriar Sep 12, 2019

jerch Sep 12, 2019 •

edited

Loading

Tyriar Sep 12, 2019

jerch Sep 12, 2019 •

edited

Loading

Tyriar Sep 12, 2019

jerch Sep 12, 2019 •

edited

Loading

jerch left a comment

jerch Sep 12, 2019

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch commented Sep 12, 2019 •

edited

Loading

jerch commented Sep 12, 2019

Tyriar commented Sep 12, 2019

Tyriar commented Sep 12, 2019

Tyriar left a comment

		} else if (ev.keyCode === 221) {
		// ^] - Operating System Command (OSC)

		} else if (ev.keyCode === 219) {
		// ^[ - Control Sequence Introducer (CSI)

Use more C0 constants in Keyboard #2425

Use more C0 constants in Keyboard #2425

Conversation

Tyriar commented Sep 12, 2019

Tyriar Sep 12, 2019

Choose a reason for hiding this comment

jerch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Tyriar Sep 12, 2019

Choose a reason for hiding this comment

jerch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Tyriar Sep 12, 2019

Choose a reason for hiding this comment

jerch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

jerch left a comment

Choose a reason for hiding this comment

jerch Sep 12, 2019

Choose a reason for hiding this comment

jerch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

jerch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

jerch Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

jerch commented Sep 12, 2019 • edited Loading

jerch commented Sep 12, 2019

Tyriar commented Sep 12, 2019

Tyriar commented Sep 12, 2019

Tyriar left a comment

Choose a reason for hiding this comment

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch Sep 12, 2019 •

edited

Loading

jerch commented Sep 12, 2019 •

edited

Loading