Repository: microsoft/terminal (language: cpp)

Issue:
Terminal reports no longer work in Vttest
### Windows Terminal version

Commit cacf66860f6c175672b3d030dcd166e372b2a8cc

### Windows build number

10.0.19044.2006

### Other Software

Vttest

### Steps to reproduce

1. Open a WSL shell in conhost or Terminal
2. Run `vttest`.
2. Select the _Private Device Attributes_ test (menu 6.4).


### Expected Behavior

The report received from the terminal should be:
```
<27> [ ? 1 ; 0 c
```


### Actual Behavior

In conhost it only seems to receive the initial `ESC` character, while in Terminal it receives nothing at all. And it's not just `DA` - other reports fail in a similar way.

I think this regression was caused by the recent `DECARM` merge (PR #13981), but I have no idea why. I'm still investigating.

The weird thing is that it works correctly in all of my own tests and applications. There's just something about the way vttest is reading the input that is causing it to fail.




Discussion / hints:
OK it seems the problem is that vttest disables `DECARM` on startup, and our VT reports don't work when `DECARM` is disabled. The reason they don't work is because the report sequences are generated with fake keypresses using a virtual key code of zero. And the `DECARM` implementation uses the virtual key code to detect repeats of the same key, so all the characters in the response string appear to be coming from the same key.

We can probably fix this by checking both the virtual key code and the char data when determining whether we've got a repeated key. I'll give that a try now. Hopefully that won't have any unwanted side effects.
It's actually less complicated that I thought. The fact that all the generated key presses appear to be the same shouldn't be a problem. They always include both a press and release event, so that wouldn't typically be recognised as a repeat. The problem is that we're using a 0 virtual key code to indicate the "clean" state (i.e. when nothing has been pressed). That shouldn't normally match anything, since it's not a valid VK value, but in this case it does.

So I guess the solution is to use another value for the clean state - maybe 0xFFFF.
There's always our trusty friend, `std::optional<T>`!
> There's always our trusty friend, `std::optional<T>`!

Yeah, that's a good idea. I was trying to keep things simple but that shouldn't add too much overhead, and feels a lot safer than making another assumption about an unused VK value.

Fix patch:
diff --git a/src/terminal/input/terminalInput.cpp b/src/terminal/input/terminalInput.cpp
index 5826eea57f7..390bc115a2f 100644
--- a/src/terminal/input/terminalInput.cpp
+++ b/src/terminal/input/terminalInput.cpp
@@ -551,20 +551,23 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)
         return true;
     }
 
+    // Check if this key matches the last recorded key code.
+    const auto matchingLastKeyPress = _lastVirtualKeyCode == keyEvent.GetVirtualKeyCode();
+
     // Only need to handle key down. See raw key handler (see RawReadWaitRoutine in stream.cpp)
     if (!keyEvent.IsKeyDown())
     {
         // If this is a release of the last recorded key press, we can reset that.
-        if (keyEvent.GetVirtualKeyCode() == _lastVirtualKeyCode)
+        if (matchingLastKeyPress)
         {
-            _lastVirtualKeyCode = 0;
+            _lastVirtualKeyCode = std::nullopt;
         }
         return false;
     }
 
     // If this is a repeat of the last recorded key press, and Auto Repeat Mode
     // is disabled, then we should suppress this event.
-    if (keyEvent.GetVirtualKeyCode() == _lastVirtualKeyCode && !_inputMode.test(Mode::AutoRepeat))
+    if (matchingLastKeyPress && !_inputMode.test(Mode::AutoRepeat))
     {
         // Note that we must return true here to say we've handled the event,
         // otherwise the key press can still end up being submitted.
diff --git a/src/terminal/input/terminalInput.hpp b/src/terminal/input/terminalInput.hpp
index 70fc0e6b62e..7df69b41f53 100644
--- a/src/terminal/input/terminalInput.hpp
+++ b/src/terminal/input/terminalInput.hpp
@@ -93,7 +93,7 @@ namespace Microsoft::Console::VirtualTerminal
         // storage location for the leading surrogate of a utf-16 surrogate pair
         std::optional<wchar_t> _leadingSurrogate;
 
-        WORD _lastVirtualKeyCode{ 0 };
+        std::optional<WORD> _lastVirtualKeyCode;
 
         til::enumset<Mode> _inputMode{ Mode::Ansi, Mode::AutoRepeat };
         bool _forceDisableWin32InputMode{ false };


Test patch: