NetDriver key handling is broken on Windows Terminal v1.18+

[tig]

To repro simply run UI Catalog and press ESC a few times.

I found this while trying to track down what I broke in #2612.

I kept going back in my commit history to find where the odd behavior I was seeing didn't happen. I eventually just tried v2_develop and discovered this was happening. So I tried it on develop too.

[tig]

It's actually worse. I wonder if something broke in my version of Windows. Keyboard basically doesn't work at all in the develop branch with -usc.

Edition	Windows 11 Pro
Version	22H2
Installed on	‎6/‎11/‎2022
OS build	22631.1972
Serial number	031033494257
Experience	Windows Feature Experience Pack 1000.22658.1000.0

[tig]

It looks like a recent Windows Terminal problem as I can use ConEmu instead of WIndows Terminal and the keyboard works as expected (mouse doesn't work tho).

Bad:
Windows Terminal Preview Version: 1.18.1462.0

Good:
ConEmu
Windows Terminal Version: 1.17.11461.0

[tznind]

What does the call stack look like? GetKeyCharArray() is called in multiple places. If cki is null perhaps all we need is a null guard further up?

I get that it is Windows Terminal that is maybe giving bad info to method call but null checks are never usually a bad thing to have even if it is just to cover corner cases.

[j4james]

I don't know the code base, so this is just a wild guess, but the code here looks suspicious:

Terminal.Gui/Terminal.Gui/ConsoleDrivers/NetDriver.cs

Lines 198 to 201 in db78fe2

	} else if (consoleKeyInfo.KeyChar == (char)Key.Esc && isEscSeq) {
	DecodeEscSeq (ref newConsoleKeyInfo, ref key, cki, ref mod);
	cki = null;
	break;

If you get into that branch, cki is going to be set to null, but isEscSeq is not reset to false. So the next time GetConsoleKey is called, if consoleKeyInfo.KeyChar is Esc again, you're assumedly going to enter that branch with cki now being null. From there you call DecodeEscSeq with null, which in turn calls GetCharArray with null.

At least that's what it looks like to me.

[BDisp]

@j4james is right. If isEscSeq is true and the urrent consoleKeyInfo.KeyChar == (char)Key.Esc then it's only to process a Key.Esc and thus the isEscSeq must be set to false.

[tig]

I'm diving deep into this code now. It has a bunch of issues. I don't yet know definitively what changed in WT 1.8 that broke this but I suspect it has to do with the fact that WT is now reporting additional capabilities:

	/// <summary>
	/// The terminal reply to <see cref="CSI_ReportDeviceAttributes"/> :
	/// Windows Terminal v1.17 and below Will emit “\x1b[?1;0c”, indicating "VT101 with No Options".
	/// Windows Terminal v1.18+ emits: \x1b[?61;6;7;22;23;24;28;32;42c"
	/// - 61 - indicates VT525
	/// - 7 - indicates VT400
	/// - 6 - indicates Selective Erase
	/// - 22 - indicates ANSI Color
	/// - 23 - indicates ANSI Text Locator
	/// - 24 - indicates VT200 Highlighting
	/// - 28 - indicates Rectangular Editing
	/// - 32 - indicates National Replacement Character Sets
	/// - 42 - indicates ISO Latin-1 Character Set
	/// </summary>
	public const string CSI_ReportDeviceAttributes_Terminator = "c";

... and this code is naive:

		case EscSeqUtils.CSI_ReportDeviceAttributes_Terminator:
			try {
				var parent = EscSeqUtils.GetParentProcess (Process.GetCurrentProcess ());
				if (parent == null) { Debug.WriteLine ("Not supported!"); }
			} catch (Exception ex) {
				Debug.WriteLine (ex.Message);
			}
			if (c1Control == "CSI" && values.Length == 2
				&& values [0] == "1" && values [1] == "0") {
				// Reports CSI?1;0c ("VT101 with No Options")
				IsTerminalWithOptions = false;
			} else {
				IsTerminalWithOptions = true;
			}
			break;

When IsTerminalWithOptions is true, we request the terminal size via an ESC request every mainloop iteration:

switch (`IsTerminalWithOptions) {
case false:
	int buffHeight, buffWidth;
	if (((NetDriver)_consoleDriver).IsWinPlatform) {
		buffHeight = Math.Max (Console.BufferHeight, 0);
		buffWidth = Math.Max (Console.BufferWidth, 0);
	} else {
		buffHeight = _consoleDriver.Rows;
		buffWidth = _consoleDriver.Cols;
	}
	if (EnqueueWindowSizeEvent (
		Math.Max (Console.WindowHeight, 0),
		Math.Max (Console.WindowWidth, 0),
		buffHeight,
		buffWidth)) {

		return;
	}
	break;
case true:
	//Request the size of the text area in characters.
	// BUGBUG: This is getting called repeatedly / all the time with
	// Windows terminal v1.18+
	EscSeqRequests.Add (EscSeqUtils.CSI_ReportTerminalSizeInChars_Terminator);
	Console.Out.Write (EscSeqUtils.CSI_ReportTerminalSizeInChars);
	break;
}

As a result, the input queue is flooded which I think is confusing the logic @tznind and @j4james highlight as fragile/suspect.

[tig]

Yep. That's it. Forcing IsTerminalWithOptions to always be false enables NetDriver to work in with WT 1.18.

[BDisp]

@j4james is right. If isEscSeq is true and the current consoleKeyInfo.KeyChar == (char)Key.Esc then it's only to process a Key.Esc and thus the isEscSeq must be set to false.

Well, I thinking better about this and I think the code right as is. The above explanation is wrong. When isEscSeq is true and the the consoleKeyInfo.KeyChar == (char)Key.Esc then it's only to process a Key.Esc and thus the isEscSeq can't be set to false because the second Key.Esc may be only a Key.Esc or belong to a escape sequence which will be handle by the first condition, by checking the Console.KeyAvailable. If this is true and in the next iteration the consoleKeyInfo.KeyChar != (char)Key.Esc, then it's a escape sequence. Otherwise, if it's false it's a simple Key.Esc which will be handled in the second condition where it's the only place where can handle two consecutive Key.Esc, one when consoleKeyInfo.KeyChar == (char)Key.Esc && isEscSeq, meaning that it only have to processing the Key.Esc and two by set the cki = null for the first condition process it in the next iteration because it may be only a Key.Esc or a escape sequence. If isEscSeq is set to false in the second condition the escape sequence will not be processed. Also the first condition ensure that only a Key.Esc is process if Console.KeyAvailable is false.

[tig]

@BDisp see my latest push to this PR.

There is a bug somewhere that is causing isEscSeq to be true with cki being null. I've added a defensive check for this as I've not been able to reliably reproduce it in a way that shows me what's causing it.

Note, I discovered something else that indicates EscSeqUtils may need to be refactored.

If the user presses a key while the terminal is sending an esc sequence (e.g. a '[35` mouse move report), that key info is lost. The result of this is no keystrokes can happen while the user is moving the mouse.

As we move to a world where all drivers rely heavily on emitting and receiving escape sequences, we need EscSeqUtils to be very robust and flexible. It works pretty darn good right now (great job!) but it is fragile and brittle. It doesn't help that the documentation for ANSI sequences and all the implementations are so sketchy.

I still haven't figured out what I screwed up in #2612 that has broken netdriver in this regard, FWIW.