Possible bug: Inconsistency between WM_KEYDOWN and WM_KEYUP

[logiclrd]

While investigating something related to control key events, I was looking at the source code to TerminalContainer_MessageHook in TerminalContainer.cs and saw this:

terminal/src/cascadia/WpfTerminalControl/TerminalContainer.cs

Lines 349 to 364 in 0f5d883

	case NativeMethods.WindowMessage.WM_SYSKEYDOWN: // fallthrough
	case NativeMethods.WindowMessage.WM_KEYDOWN:
	{
	UnpackKeyMessage(wParam, lParam, out ushort vkey, out ushort scanCode, out ushort flags);
	NativeMethods.TerminalSendKeyEvent(this.terminal, vkey, scanCode, flags, true);
	break;
	}
	case NativeMethods.WindowMessage.WM_SYSKEYUP: // fallthrough
	case NativeMethods.WindowMessage.WM_KEYUP:
	{
	// WM_KEYUP lParam layout documentation: https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-keyup
	UnpackKeyMessage(wParam, lParam, out ushort vkey, out ushort scanCode, out ushort flags);
	NativeMethods.TerminalSendKeyEvent(this.terminal, (ushort)wParam, scanCode, flags, false);
	break;
	}

This has a "smell" to it: Both handler cases call UnpackKeyMessage populating a local variable vkey, but only one of them actually uses it. The WM_KEYDOWN handler passes vkey on to TerminalSendKeyEvent, but the WM_KEYUP handler passes wParam unaltered for the same parameter.

Is this a bug?

If not, I would recommend calling out the difference between the two TerminalSendKeyEvent calls explicitly, with a comment or a strategically-named local variable or what have you.

[logiclrd]

I just looked one layer deeper, and I see that UnpackKeyMessage fills vkey with the value of wParam. So this is definitely not a functional bug. But, it is still a bug for the calls to not match; if UnpackKeyMessage were ever updated in the future to perform some more complicated logic to produce vkey, it would graduate to being an actual functional bug at that time.