Repository: microsoft/terminal (language: cpp)

Issue:
Hard reset disables win32-input-mode
### Windows Terminal version

1.18.1421.0

### Windows build number

10.0.19045.2913

### Other Software

_No response_

### Steps to reproduce

1. Start Windows Terminal.
2. Open up a WSL bash shell with the debug tap enabled.
3. Press some keys and note the escape sequences that are generated in the debug pane.
4. Execute `reset`.
5. Press some more keys and note what is generated in the debug pane now.


### Expected Behavior

I'd expected to see win32-input-mode escape sequences for every key press.

### Actual Behavior

After the reset, win32-input-mode is disabled, and the key presses are just passed through as ASCII, or basic VT100 escape for things like arrow keys.

I think this was a regression introduced in PR #14444 when I made the `RIS` operation fully reset all input modes. Technically that was the right thing to do, but for conpty to work correctly, it would now also need to re-request any non-default modes it needs from the conpty client.

I'm not sure how best to fix this. The `HardReset` implementation can't just return false to pass through the `RIS` sequence anymore. It would probably need to call a new "reset" method in the renderer that the VT engine would pass on as an `RIS` with additional mode requests. It must also then return true to prevent the state machine trying to pass through the `RIS` a second time.

I don't know if anyone has any better ideas.


Discussion / hints:
Having looked at the `VtEngine::RequestWin32Input` implementation here:

https://github.com/microsoft/terminal/blob/c9e993a38ec04833e15edac230300948a0fab83b/src/renderer/vt/state.cpp#L542-L548

it appears we also need to request the focus event mode. Otherwise that's also something that'll stop working after a hard reset.
I realised there's an easier way of handling this which doesn't require any new render APIs. We already have access to the `StateMachine` instance from within `AdaptDispatch`, and that gives us the ability pass through sequences directly to the conpty client. 

So a simple fix would be to add something like this to the end of the `HardReset` method.

```cpp
if (_api.IsConsolePty())
{
    auto& stateMachine = _api.GetStateMachine();
    if (stateMachine.FlushToTerminal())
    {
        auto& engine = stateMachine.Engine();
        engine.ActionPassThroughString(L"\033[?9001;1004h");
    }
}
```

The `FlushToTerminal` call should pass through the pending `RIS` sequence, and then we follow that up with mode requests to enable win32 input and focus events.

The only downside I can see is that we're essentially duplicating the code from `VtEngine::RequestWin32Input`, which runs the risk of the two getting out of sync, but that could be mitigated with a couple of comments. It's not as clean as I'd like, but it's probably the simplest option.
I'm alright with "duplicate with comment", until we get to 3 places that need to do it. Then we can figure it out 🙂 

Excellent catch!
FYI, the easy fix I suggested above isn't quite as easy as I had originally thought, because those modes are currently only sent when the conpty client requests them (with the `--win32input` command line option). And since `AdaptDispatch` has no knowledge of that option, we'd still need additional APIs to figure that out somehow.

That said, I have no idea why we have that command line option in the first place. Why don't we send those mode requests regardless? If the connected conpty client doesn't recognise the mode, it'll assumedly just ignore it. We even have a comment in the code saying exactly that!

https://github.com/microsoft/terminal/blob/c9e993a38ec04833e15edac230300948a0fab83b/src/host/VtIo.cpp#L268-L275

So my question is, do we we really need that option, and thus a bunch of additional framework to make it accessible to `AdaptDispatch`, or can we just get rid of it altogether?

Fix patch:
diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp
index 7bb1b399c7f..cecb5c7d759 100644
--- a/src/cascadia/TerminalConnection/ConptyConnection.cpp
+++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp
@@ -325,7 +325,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         // handoff from an already-started PTY process.
         if (!_inPipe)
         {
-            DWORD flags = PSEUDOCONSOLE_RESIZE_QUIRK | PSEUDOCONSOLE_WIN32_INPUT_MODE;
+            DWORD flags = PSEUDOCONSOLE_RESIZE_QUIRK;
 
             // If we're using an existing buffer, we want the new connection
             // to reuse the existing cursor. When not setting this flag, the
diff --git a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
index dc66b3a03ed..cd9ac092898 100644
--- a/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
@@ -1200,7 +1200,9 @@ void ConptyRoundtripTests::PassthroughHardReset()
     }
 
     // Write a Hard Reset VT sequence to the host, it should come through to the Terminal
+    // along with a DECSET sequence to re-enable win32 input and focus events.
     expectedOutput.push_back("\033c");
+    expectedOutput.push_back("\033[?9001;1004h");
     hostSm.ProcessString(L"\033c");
 
     const auto termSecondView = term->GetViewport();
diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp
index 2541cc8ab82..3621bca387f 100644
--- a/src/host/ConsoleArguments.cpp
+++ b/src/host/ConsoleArguments.cpp
@@ -20,7 +20,6 @@ const std::wstring_view ConsoleArguments::WIDTH_ARG = L"--width";
 const std::wstring_view ConsoleArguments::HEIGHT_ARG = L"--height";
 const std::wstring_view ConsoleArguments::INHERIT_CURSOR_ARG = L"--inheritcursor";
 const std::wstring_view ConsoleArguments::RESIZE_QUIRK = L"--resizeQuirk";
-const std::wstring_view ConsoleArguments::WIN32_INPUT_MODE = L"--win32input";
 const std::wstring_view ConsoleArguments::FEATURE_ARG = L"--feature";
 const std::wstring_view ConsoleArguments::FEATURE_PTY_ARG = L"pty";
 const std::wstring_view ConsoleArguments::COM_SERVER_ARG = L"-Embedding";
@@ -515,12 +514,6 @@ void ConsoleArguments::s_ConsumeArg(_Inout_ std::vector<std::wstring>& args, _In
             s_ConsumeArg(args, i);
             hr = S_OK;
         }
-        else if (arg == WIN32_INPUT_MODE)
-        {
-            _win32InputMode = true;
-            s_ConsumeArg(args, i);
-            hr = S_OK;
-        }
         else if (arg == CLIENT_COMMANDLINE_ARG)
         {
             // Everything after this is the explicit commandline
@@ -677,10 +670,6 @@ bool ConsoleArguments::IsResizeQuirkEnabled() const
 {
     return _resizeQuirk;
 }
-bool ConsoleArguments::IsWin32InputModeEnabled() const
-{
-    return _win32InputMode;
-}
 
 #ifdef UNIT_TESTING
 // Method Description:
diff --git a/src/host/ConsoleArguments.hpp b/src/host/ConsoleArguments.hpp
index bb4a95b21e6..8648c92ea66 100644
--- a/src/host/ConsoleArguments.hpp
+++ b/src/host/ConsoleArguments.hpp
@@ -55,7 +55,6 @@ class ConsoleArguments
     short GetHeight() const;
     bool GetInheritCursor() const;
     bool IsResizeQuirkEnabled() const;
-    bool IsWin32InputModeEnabled() const;
 
 #ifdef UNIT_TESTING
     void EnableConptyModeForTests();
@@ -74,7 +73,6 @@ class ConsoleArguments
     static const std::wstring_view HEIGHT_ARG;
     static const std::wstring_view INHERIT_CURSOR_ARG;
     static const std::wstring_view RESIZE_QUIRK;
-    static const std::wstring_view WIN32_INPUT_MODE;
     static const std::wstring_view FEATURE_ARG;
     static const std::wstring_view FEATURE_PTY_ARG;
     static const std::wstring_view COM_SERVER_ARG;
@@ -144,7 +142,6 @@ class ConsoleArguments
     DWORD _signalHandle;
     bool _inheritCursor;
     bool _resizeQuirk{ false };
-    bool _win32InputMode{ false };
 
     [[nodiscard]] HRESULT _GetClientCommandline(_Inout_ std::vector<std::wstring>& args,
                                                 const size_t index,
diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp
index b0f4ebad2c3..5d33e607931 100644
--- a/src/host/VtIo.cpp
+++ b/src/host/VtIo.cpp
@@ -71,7 +71,6 @@ VtIo::VtIo() :
 {
     _lookingForCursorPosition = pArgs->GetInheritCursor();
     _resizeQuirk = pArgs->IsResizeQuirkEnabled();
-    _win32InputMode = pArgs->IsWin32InputModeEnabled();
     _passthroughMode = pArgs->IsPassthroughMode();
 
     // If we were already given VT handles, set up the VT IO engine to use those.
@@ -269,10 +268,7 @@ bool VtIo::IsUsingVt() const
     // win32-input-mode from them. This will enable the connected terminal to
     // send us full INPUT_RECORDs as input. If the terminal doesn't understand
     // this sequence, it'll just ignore it.
-    if (_win32InputMode)
-    {
-        LOG_IF_FAILED(_pVtRenderEngine->RequestWin32Input());
-    }
+    LOG_IF_FAILED(_pVtRenderEngine->RequestWin32Input());
 
     // MSFT: 15813316
     // If the terminal application wants us to inherit the cursor position,
diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp
index e47d37d9ef6..a15653ad084 100644
--- a/src/host/VtIo.hpp
+++ b/src/host/VtIo.hpp
@@ -67,7 +67,6 @@ namespace Microsoft::Console::VirtualTerminal
         bool _lookingForCursorPosition;
 
         bool _resizeQuirk{ false };
-        bool _win32InputMode{ false };
         bool _passthroughMode{ false };
         bool _closeEventSent{ false };
 
diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp
index e1df6552c17..284f4483820 100644
--- a/src/host/srvinit.cpp
+++ b/src/host/srvinit.cpp
@@ -603,11 +603,9 @@ try
     outPipeTheirSide.reset();
     signalPipeTheirSide.reset();
 
-    // GH#13211 - Make sure we request win32input mode and that the terminal
-    // obeys the resizing quirk. Otherwise, defterm connections to the Terminal
-    // are going to have weird resizing, and aren't going to send full fidelity
-    // input messages.
-    const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --resizeQuirk --win32input --signal {:#x}"),
+    // GH#13211 - Make sure the terminal obeys the resizing quirk. Otherwise,
+    // defterm connections to the Terminal are going to have weird resizing.
+    const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --resizeQuirk --signal {:#x}"),
                                          (int64_t)signalPipeOurSide.release());
 
     ConsoleArguments consoleArgs(commandLine, inPipeOurSide.release(), outPipeOurSide.release());
diff --git a/src/inc/conpty-static.h b/src/inc/conpty-static.h
index 89b20468408..4d22048868c 100644
--- a/src/inc/conpty-static.h
+++ b/src/inc/conpty-static.h
@@ -24,7 +24,6 @@
 #endif
 
 #define PSEUDOCONSOLE_RESIZE_QUIRK (2u)
-#define PSEUDOCONSOLE_WIN32_INPUT_MODE (4u)
 #define PSEUDOCONSOLE_PASSTHROUGH_MODE (8u)
 
 CONPTY_EXPORT HRESULT WINAPI ConptyCreatePseudoConsole(COORD size, HANDLE hInput, HANDLE hOutput, DWORD dwFlags, HPCON* phPC);
diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp
index e5ddb08514d..29a8657cd5a 100644
--- a/src/renderer/vt/state.cpp
+++ b/src/renderer/vt/state.cpp
@@ -541,6 +541,9 @@ void VtEngine::SetTerminalCursorTextPosition(const til::point cursor) noexcept
 // - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
 HRESULT VtEngine::RequestWin32Input() noexcept
 {
+    // It's important that any additional modes set here are also mirrored in
+    // the AdaptDispatch::HardReset method, since that needs to re-enable them
+    // in the connected terminal after passing through an RIS sequence.
     RETURN_IF_FAILED(_RequestWin32Input());
     RETURN_IF_FAILED(_RequestFocusEventMode());
     RETURN_IF_FAILED(_Flush());
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index d17d3b740a8..1a0467f7f6f 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -3055,11 +3055,21 @@ bool AdaptDispatch::HardReset()
         _macroBuffer = nullptr;
     }
 
-    // GH#2715 - If all this succeeded, but we're in a conpty, return `false` to
-    // make the state machine propagate this RIS sequence to the connected
-    // terminal application. We've reset our state, but the connected terminal
-    // might need to do more.
-    return !_api.IsConsolePty();
+    // If we're in a conpty, we need flush this RIS sequence to the connected
+    // terminal application, but we also need to follow that up with a DECSET
+    // sequence to re-enable the modes that we require (namely win32 input mode
+    // and focus event mode). It's important that this is kept in sync with the
+    // VtEngine::RequestWin32Input method which requests the modes on startup.
+    if (_api.IsConsolePty())
+    {
+        auto& stateMachine = _api.GetStateMachine();
+        if (stateMachine.FlushToTerminal())
+        {
+            auto& engine = stateMachine.Engine();
+            engine.ActionPassThroughString(L"\033[?9001;1004h");
+        }
+    }
+    return true;
 }
 
 // Routine Description:
diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp
index e964a5691a3..10cfa1fea36 100644
--- a/src/winconpty/winconpty.cpp
+++ b/src/winconpty/winconpty.cpp
@@ -133,19 +133,17 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken,
     RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeConhostSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT));
 
     // GH4061: Ensure that the path to executable in the format is escaped so C:\Program.exe cannot collide with C:\Program Files
-    auto pwszFormat = L"\"%s\" --headless %s%s%s%s--width %hu --height %hu --signal 0x%x --server 0x%x";
+    auto pwszFormat = L"\"%s\" --headless %s%s%s--width %hu --height %hu --signal 0x%x --server 0x%x";
     // This is plenty of space to hold the formatted string
     wchar_t cmd[MAX_PATH]{};
     const BOOL bInheritCursor = (dwFlags & PSEUDOCONSOLE_INHERIT_CURSOR) == PSEUDOCONSOLE_INHERIT_CURSOR;
     const BOOL bResizeQuirk = (dwFlags & PSEUDOCONSOLE_RESIZE_QUIRK) == PSEUDOCONSOLE_RESIZE_QUIRK;
-    const BOOL bWin32InputMode = (dwFlags & PSEUDOCONSOLE_WIN32_INPUT_MODE) == PSEUDOCONSOLE_WIN32_INPUT_MODE;
     const BOOL bPassthroughMode = (dwFlags & PSEUDOCONSOLE_PASSTHROUGH_MODE) == PSEUDOCONSOLE_PASSTHROUGH_MODE;
     swprintf_s(cmd,
                MAX_PATH,
                pwszFormat,
                _ConsoleHostPath(),
                bInheritCursor ? L"--inheritcursor " : L"",
-               bWin32InputMode ? L"--win32input " : L"",
                bResizeQuirk ? L"--resizeQuirk " : L"",
                bPassthroughMode ? L"--passthrough " : L"",
                size.X,


Test patch: