From d11e4dd2eb2763a96d3b67301184e017a724f6e1 Mon Sep 17 00:00:00 2001 From: James Holderness Date: Fri, 2 Jun 2023 19:41:49 +0100 Subject: [PATCH] Make sure RIS re-enables win32 input and focus events (#15476) When an `RIS` (hard reset) sequence is executed, ConHost will pass that through to the connected conpty terminal, which results in all modes being reset to their initial state. To work best, though, conpty needs to have the win32 input mode enabled, as well as the focus event mode. This PR addresses that by explicitly requesting the required modes after an `RIS` is passed through. Originally these modes were only set if the `--win32input` option was given, but there is really no need for that, since terminals that don't support them should simply ignore the request. To avoid that additional complication, I've now removed the option (i.e. ConHost will now always attempt to set the modes it needs). I've manually confirmed that keypresses are still passed through with win32 input sequences after a hard reset, and that focus events are still being generated. I've also updated the existing conpty round-trip test for `RIS` to confirm that it's now also passing through the mode requests that it needs. Closes #15461 (cherry picked from commit 8aefc7a697ec155258d2fc0d84ea3466aaa0754e) Service-Card-Id: 89384907 Service-Version: 1.18 --- .../TerminalConnection/ConptyConnection.cpp | 2 +- .../ConptyRoundtripTests.cpp | 2 ++ src/host/ConsoleArguments.cpp | 11 ---------- src/host/ConsoleArguments.hpp | 3 --- src/host/VtIo.cpp | 6 +----- src/host/VtIo.hpp | 1 - src/host/srvinit.cpp | 8 +++----- src/inc/conpty-static.h | 1 - src/renderer/vt/state.cpp | 3 +++ src/terminal/adapter/adaptDispatch.cpp | 20 ++++++++++++++----- src/winconpty/winconpty.cpp | 4 +--- 11 files changed, 26 insertions(+), 35 deletions(-) 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& 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& 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 64d88723ff2..48ae8d2e834 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -2941,11 +2941,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,