From 0c064905b324deaa02b1c3b094496c020a3a464d Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 11 Aug 2025 19:48:35 +0200 Subject: [PATCH] Revert "ConPTY: Emit DSR CPR on resize (#19089)" (#19237) This reverts commit c55aca508beadebc080b0c76b7322de80a294968 because it is unaware of the VT state and may inject the DSR CPR while e.g. a DCS is going on. Reopens #18725 --- src/host/PtySignalInputThread.cpp | 3 -- src/host/VtInputThread.cpp | 9 ++-- src/host/VtInputThread.hpp | 5 +- src/host/VtIo.cpp | 37 +------------ src/host/VtIo.hpp | 4 +- .../parser/InputStateMachineEngine.cpp | 40 +++++--------- .../parser/InputStateMachineEngine.hpp | 7 +-- .../parser/ut_parser/InputEngineTest.cpp | 54 ++++++++++++++----- 8 files changed, 67 insertions(+), 92 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index b078c5f1b9..0b03612d9a 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -184,9 +184,6 @@ void PtySignalInputThread::_DoResizeWindow(const ResizeWindowData& data) } _api.ResizeWindow(data.sx, data.sy); - - auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - gci.GetVtIo()->RequestCursorPositionFromTerminal(); } void PtySignalInputThread::_DoClearBuffer(const bool keepCursorRow) const diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index f094eae35e..3c359a3c8e 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -21,13 +21,13 @@ using namespace Microsoft::Console::VirtualTerminal; // - hPipe - a handle to the file representing the read end of the VT pipe. // - inheritCursor - a bool indicating if the state machine should expect a // cursor positioning sequence. See MSFT:15681311. -VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, std::function capturedCPR) : +VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor) : _hFile{ std::move(hPipe) } { THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); auto dispatch = std::make_unique(); - auto engine = std::make_unique(std::move(dispatch), std::move(capturedCPR)); + auto engine = std::make_unique(std::move(dispatch), inheritCursor); _pInputStateMachine = std::make_unique(std::move(engine)); } @@ -185,7 +185,8 @@ void VtInputThread::_InputThread() return S_OK; } -InputStateMachineEngine& VtInputThread::GetInputStateMachineEngine() const noexcept +til::enumset VtInputThread::WaitUntilDA1(DWORD timeout) const noexcept { - return static_cast(_pInputStateMachine->Engine()); + const auto& engine = static_cast(_pInputStateMachine->Engine()); + return engine.WaitUntilDA1(timeout); } diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index bf07c1e4be..50405b5a25 100644 --- a/src/host/VtInputThread.hpp +++ b/src/host/VtInputThread.hpp @@ -20,17 +20,16 @@ namespace Microsoft::Console { namespace VirtualTerminal { - class InputStateMachineEngine; enum class DeviceAttribute : uint64_t; } class VtInputThread { public: - VtInputThread(_In_ wil::unique_hfile hPipe, std::function capturedCPR = nullptr); + VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor); [[nodiscard]] HRESULT Start(); - VirtualTerminal::InputStateMachineEngine& GetInputStateMachineEngine() const noexcept; + til::enumset WaitUntilDA1(DWORD timeout) const noexcept; private: static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter); diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index f1a2da2368..e2891ab7c6 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -11,7 +11,6 @@ #include "output.h" // CloseConsoleProcessState #include "../interactivity/inc/ServiceLocator.hpp" #include "../renderer/base/renderer.hpp" -#include "../terminal/parser/InputStateMachineEngine.hpp" #include "../types/inc/CodepointWidthDetector.hpp" #include "../types/inc/utils.hpp" @@ -156,9 +155,7 @@ bool VtIo::IsUsingVt() const { if (IsValidHandle(_hInput.get())) { - _pVtInputThread = std::make_unique(std::move(_hInput), [this]() { - _cursorPositionReportReceived(); - }); + _pVtInputThread = std::make_unique(std::move(_hInput), _lookingForCursorPosition); } } CATCH_RETURN(); @@ -180,7 +177,6 @@ bool VtIo::IsUsingVt() const // wait for the DA1 response below and effectively wait for both. if (_lookingForCursorPosition) { - _pVtInputThread->GetInputStateMachineEngine().CaptureNextCPR(); writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR) } @@ -201,7 +197,7 @@ bool VtIo::IsUsingVt() const // Allow the input thread to momentarily gain the console lock. auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); const auto suspension = gci.SuspendLock(); - _deviceAttributes = _pVtInputThread->GetInputStateMachineEngine().WaitUntilDA1(3000); + _deviceAttributes = _pVtInputThread->WaitUntilDA1(3000); } } @@ -252,35 +248,6 @@ void VtIo::Shutdown() noexcept CATCH_LOG(); } -void VtIo::RequestCursorPositionFromTerminal() -{ - if (_lookingForCursorPosition) - { - // By delaying sending another DSR CPR until we received a response to the previous one, - // we debounce our requests to the terminal. We don't want to flood it unnecessarily. - _scheduleAnotherCPR = true; - return; - } - - _lookingForCursorPosition = true; - _pVtInputThread->GetInputStateMachineEngine().CaptureNextCPR(); - - Writer writer{ this }; - writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR) - writer.Submit(); -} - -void VtIo::_cursorPositionReportReceived() -{ - _lookingForCursorPosition = false; - - if (_scheduleAnotherCPR) - { - _scheduleAnotherCPR = false; - RequestCursorPositionFromTerminal(); - } -} - void VtIo::SetDeviceAttributes(const til::enumset attributes) noexcept { _deviceAttributes = attributes; diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index b8eaaf930e..f2164a68c4 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -61,7 +61,6 @@ namespace Microsoft::Console::VirtualTerminal [[nodiscard]] HRESULT StartIfNeeded(); void Shutdown() noexcept; - void RequestCursorPositionFromTerminal(); void SetDeviceAttributes(til::enumset attributes) noexcept; til::enumset GetDeviceAttributes() const noexcept; void SendCloseEvent(); @@ -78,7 +77,7 @@ namespace Microsoft::Console::VirtualTerminal }; [[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, _In_opt_ const HANDLE SignalHandle); - void _cursorPositionReportReceived(); + void _uncork(); void _flushNow(); @@ -106,7 +105,6 @@ namespace Microsoft::Console::VirtualTerminal State _state = State::Uninitialized; bool _lookingForCursorPosition = false; - bool _scheduleAnotherCPR = false; bool _closeEventSent = false; int _corked = 0; diff --git a/src/terminal/parser/InputStateMachineEngine.cpp b/src/terminal/parser/InputStateMachineEngine.cpp index 58b0c864de..aadcb0d5e0 100644 --- a/src/terminal/parser/InputStateMachineEngine.cpp +++ b/src/terminal/parser/InputStateMachineEngine.cpp @@ -89,24 +89,14 @@ static bool operator==(const Ss3ToVkey& pair, const Ss3ActionCodes code) noexcep return pair.action == code; } -InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr pDispatch, std::function capturedCPR) : - _pDispatch{ std::move(pDispatch) }, - _capturedCPR{ std::move(capturedCPR) }, - _doubleClickTime{ std::chrono::milliseconds(GetDoubleClickTime()) } +InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr pDispatch, const bool lookingForDSR) : + _pDispatch(std::move(pDispatch)), + _lookingForDSR(lookingForDSR), + _doubleClickTime(std::chrono::milliseconds(GetDoubleClickTime())) { THROW_HR_IF_NULL(E_INVALIDARG, _pDispatch.get()); } -IInteractDispatch& InputStateMachineEngine::GetDispatch() const noexcept -{ - return *_pDispatch.get(); -} - -void InputStateMachineEngine::CaptureNextCPR() noexcept -{ - _lookingForCPR = true; -} - til::enumset InputStateMachineEngine::WaitUntilDA1(DWORD timeout) const noexcept { uint64_t val = 0; @@ -423,19 +413,13 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter // The F3 case is special - it shares a code with the DeviceStatusResponse. // If we're looking for that response, then do that, and break out. // Else, fall though to the _GetCursorKeysModifierState handler. - if (_lookingForCPR) + if (_lookingForDSR) { - _lookingForCPR = false; - _capturedCPR(); - - const auto y = parameters.at(0).value(); - const auto x = parameters.at(1).value(); - - if (y > 0 && x > 0) - { - _pDispatch->MoveCursor(y, x); - return true; - } + _pDispatch->MoveCursor(parameters.at(0), parameters.at(1)); + // Right now we're only looking for on initial cursor + // position response. After that, only look for F3. + _lookingForDSR = false; + return true; } // Heuristic: If the hosting terminal used the win32 input mode, chances are high // that this is a CPR requested by the terminal application as opposed to a F3 key. @@ -507,6 +491,10 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter _deviceAttributes.fetch_or(attributes.bits(), std::memory_order_relaxed); til::atomic_notify_all(_deviceAttributes); + + // VtIo first sends a DSR CPR and then a DA1 request. + // If we encountered a DA1 response here, the DSR request is definitely done now. + _lookingForDSR = false; return true; } return false; diff --git a/src/terminal/parser/InputStateMachineEngine.hpp b/src/terminal/parser/InputStateMachineEngine.hpp index 7583b24f6c..e91737d107 100644 --- a/src/terminal/parser/InputStateMachineEngine.hpp +++ b/src/terminal/parser/InputStateMachineEngine.hpp @@ -159,10 +159,8 @@ namespace Microsoft::Console::VirtualTerminal class InputStateMachineEngine : public IStateMachineEngine { public: - InputStateMachineEngine(std::unique_ptr pDispatch, std::function capturedCPR = nullptr); + InputStateMachineEngine(std::unique_ptr pDispatch, const bool lookingForDSR = false); - IInteractDispatch& GetDispatch() const noexcept; - void CaptureNextCPR() noexcept; til::enumset WaitUntilDA1(DWORD timeout) const noexcept; bool EncounteredWin32InputModeSequence() const noexcept override; @@ -191,8 +189,7 @@ namespace Microsoft::Console::VirtualTerminal private: const std::unique_ptr _pDispatch; std::atomic _deviceAttributes{ 0 }; - bool _lookingForCPR = false; - std::function _capturedCPR; + bool _lookingForDSR = false; bool _encounteredWin32InputModeSequence = false; bool _expectingStringTerminator = false; DWORD _mouseButtonState = 0; diff --git a/src/terminal/parser/ut_parser/InputEngineTest.cpp b/src/terminal/parser/ut_parser/InputEngineTest.cpp index e1d657919c..cdfaf1ac1e 100644 --- a/src/terminal/parser/ut_parser/InputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/InputEngineTest.cpp @@ -2,26 +2,39 @@ // Licensed under the MIT license. #include "precomp.h" +#include "WexTestClass.h" +#include "../../inc/consoletaeftemplates.hpp" -#include -#include -#include -#include -#include +#include "stateMachine.hpp" +#include "InputStateMachineEngine.hpp" +#include "ascii.hpp" +#include "../input/terminalInput.hpp" +#include "../../inc/unicode.hpp" +#include "../../interactivity/inc/EventSynthesis.hpp" + +#include +#include +#include +#include +#include -#include "../../../interactivity/inc/EventSynthesis.hpp" #include "../../../interactivity/inc/VtApiRedirection.hpp" -#include "../../input/terminalInput.hpp" using namespace WEX::Common; using namespace WEX::Logging; using namespace WEX::TestExecution; -namespace Microsoft::Console::VirtualTerminal +namespace Microsoft { - class InputEngineTest; - class TestInteractDispatch; -} + namespace Console + { + namespace VirtualTerminal + { + class InputEngineTest; + class TestInteractDispatch; + }; + }; +}; using namespace Microsoft::Console::VirtualTerminal; bool IsShiftPressed(const DWORD modifierState) @@ -400,6 +413,7 @@ void InputEngineTest::C0Test() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); Log::Comment(L"Sending 0x0-0x19 to parser to make sure they're translated correctly back to C-key"); @@ -499,6 +513,7 @@ void InputEngineTest::AlphanumericTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); Log::Comment(L"Sending every printable ASCII character"); @@ -548,6 +563,7 @@ void InputEngineTest::RoundTripTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); // Send Every VKEY through the TerminalInput module, then take the char's @@ -610,6 +626,7 @@ void InputEngineTest::NonAsciiTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine.get()); testState._stateMachine = _stateMachine.get(); Log::Comment(L"Sending various non-ascii strings, and seeing what we get out"); @@ -662,9 +679,11 @@ void InputEngineTest::CursorPositioningTest() auto pfn = std::bind(&TestState::TestInputCallback, &testState, std::placeholders::_1); auto dispatch = std::make_unique(pfn, &testState); - auto inputEngine = std::make_unique(std::move(dispatch), []() {}); - inputEngine->CaptureNextCPR(); + VERIFY_IS_NOT_NULL(dispatch.get()); + auto inputEngine = std::make_unique(std::move(dispatch), true); + VERIFY_IS_NOT_NULL(inputEngine.get()); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); Log::Comment(NoThrowString().Format( @@ -705,6 +724,7 @@ void InputEngineTest::CSICursorBackTabTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); INPUT_RECORD inputRec; @@ -732,6 +752,7 @@ void InputEngineTest::EnhancedKeysTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); // The following vkeys should be handled as enhanced keys @@ -781,6 +802,7 @@ void InputEngineTest::SS3CursorKeyTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); // clang-format off @@ -824,6 +846,7 @@ void InputEngineTest::AltBackspaceTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); INPUT_RECORD inputRec; @@ -851,6 +874,7 @@ void InputEngineTest::AltCtrlDTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); INPUT_RECORD inputRec; @@ -899,6 +923,7 @@ void InputEngineTest::AltIntermediateTest() auto dispatch = std::make_unique(pfnInputStateMachineCallback, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(stateMachine); testState._stateMachine = stateMachine.get(); // Write a Alt+/, Ctrl+e pair to the input engine, then take its output and @@ -930,6 +955,7 @@ void InputEngineTest::AltBackspaceEnterTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); INPUT_RECORD inputRec; @@ -1025,6 +1051,7 @@ void InputEngineTest::VerifySGRMouseData(const std::vector_doubleClickTime = std::chrono::milliseconds(1000); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); SGR_PARAMS input; @@ -1255,6 +1282,7 @@ void InputEngineTest::CtrlAltZCtrlAltXTest() auto dispatch = std::make_unique(pfn, &testState); auto inputEngine = std::make_unique(std::move(dispatch)); auto _stateMachine = std::make_unique(std::move(inputEngine)); + VERIFY_IS_NOT_NULL(_stateMachine); testState._stateMachine = _stateMachine.get(); // This is a test for GH#4201. See that issue for more details.