From 33589cd8db4fd7c38ae47405038488c01e29aeff Mon Sep 17 00:00:00 2001 From: James Holderness Date: Mon, 4 Mar 2024 20:03:14 +0000 Subject: [PATCH] Add support for the DECSWT (Set Window Title) escape sequence (#16804) This PR adds support for the `OSC 21` sequence used on DEC terminals to set the window title. It's just an alias of the `OSC 2` title sequence used by XTerm. This PR also corrects the handling of blank title sequences, which are supposed to reset the title to its default value, but were previously ignored. ## Detailed Description of the Pull Request / Additional comments To handle the blank title parsing correctly, I had to make some changes to the state machine. Previously it would not have dispatched an `OSC` sequence unless it received a semicolon following the `OSC` number, but when there's a blank string, that semicolon should not be required. I also took this opportunity to simplify the `OSC` parsing in the state machine, and eliminate the `_GetOscTitle` method which no longer served any purpose. ## Validation Steps Performed I've manually confirmed that the title sequences are now working as expected, and added some state machine unit tests covering the blank value handling for these sequences. I also had to update one of the existing state machine tests to account for the changes I made to allow the semicolon to be omitted. Closes #16783 Closes #16784 --- .github/actions/spelling/expect/expect.txt | 1 + src/host/outputStream.cpp | 5 +- src/terminal/adapter/ITermDispatch.hpp | 2 +- src/terminal/adapter/adaptDispatch.cpp | 2 +- src/terminal/adapter/adaptDispatch.hpp | 2 +- src/terminal/adapter/termDispatch.hpp | 2 +- src/terminal/parser/IStateMachineEngine.hpp | 4 +- .../parser/InputStateMachineEngine.cpp | 5 +- .../parser/InputStateMachineEngine.hpp | 4 +- .../parser/OutputStateMachineEngine.cpp | 25 +------ .../parser/OutputStateMachineEngine.hpp | 8 +-- src/terminal/parser/stateMachine.cpp | 32 +++++---- src/terminal/parser/stateMachine.hpp | 4 +- .../parser/ut_parser/OutputEngineTest.cpp | 71 ++++++++++++++++--- .../parser/ut_parser/StateMachineTest.cpp | 4 +- 15 files changed, 100 insertions(+), 71 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 02d18ffcb97..9c3928d83cb 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -453,6 +453,7 @@ DECSTBM DECSTGLT DECSTR DECSWL +DECSWT DECTABSR DECTCEM DECXCPR diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 6cb84a4ef5b..70b270e3cd8 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -159,12 +159,13 @@ void ConhostInternalGetSet::WarningBell() // Routine Description: // - Sets the title of the console window. // Arguments: -// - title - The null-terminated string to set as the window title +// - title - The string to set as the window title // Return Value: // - void ConhostInternalGetSet::SetWindowTitle(std::wstring_view title) { - ServiceLocator::LocateGlobals().getConsoleInformation().SetTitle(title); + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + gci.SetTitle(title.empty() ? gci.GetOriginalTitle() : title); } // Routine Description: diff --git a/src/terminal/adapter/ITermDispatch.hpp b/src/terminal/adapter/ITermDispatch.hpp index 0c46f975a8e..d0c96b6949e 100644 --- a/src/terminal/adapter/ITermDispatch.hpp +++ b/src/terminal/adapter/ITermDispatch.hpp @@ -63,7 +63,7 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch virtual bool ReverseLineFeed() = 0; // RI virtual bool BackIndex() = 0; // DECBI virtual bool ForwardIndex() = 0; // DECFI - virtual bool SetWindowTitle(std::wstring_view title) = 0; // OscWindowTitle + virtual bool SetWindowTitle(std::wstring_view title) = 0; // DECSWT, OscWindowTitle virtual bool HorizontalTabSet() = 0; // HTS virtual bool ForwardTab(const VTInt numTabs) = 0; // CHT, HT virtual bool BackwardsTab(const VTInt numTabs) = 0; // CBT diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 323707520bd..dfe540936d0 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -2644,7 +2644,7 @@ bool AdaptDispatch::ForwardIndex() // Routine Description: // - OSC Set Window Title - Sets the title of the window // Arguments: -// - title - The string to set the title to. Must be null terminated. +// - title - The string to set the title to. // Return Value: // - True. bool AdaptDispatch::SetWindowTitle(std::wstring_view title) diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 2a22f87e340..9285d1126b7 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -100,7 +100,7 @@ namespace Microsoft::Console::VirtualTerminal bool ReverseLineFeed() override; // RI bool BackIndex() override; // DECBI bool ForwardIndex() override; // DECFI - bool SetWindowTitle(const std::wstring_view title) override; // OSCWindowTitle + bool SetWindowTitle(const std::wstring_view title) override; // DECSWT, OSCWindowTitle bool HorizontalTabSet() override; // HTS bool ForwardTab(const VTInt numTabs) override; // CHT, HT bool BackwardsTab(const VTInt numTabs) override; // CBT diff --git a/src/terminal/adapter/termDispatch.hpp b/src/terminal/adapter/termDispatch.hpp index 5c41999dc8a..e2b8f298329 100644 --- a/src/terminal/adapter/termDispatch.hpp +++ b/src/terminal/adapter/termDispatch.hpp @@ -56,7 +56,7 @@ class Microsoft::Console::VirtualTerminal::TermDispatch : public Microsoft::Cons bool ReverseLineFeed() override { return false; } // RI bool BackIndex() override { return false; } // DECBI bool ForwardIndex() override { return false; } // DECFI - bool SetWindowTitle(std::wstring_view /*title*/) override { return false; } // OscWindowTitle + bool SetWindowTitle(std::wstring_view /*title*/) override { return false; } // DECSWT, OscWindowTitle bool HorizontalTabSet() override { return false; } // HTS bool ForwardTab(const VTInt /*numTabs*/) override { return false; } // CHT, HT bool BackwardsTab(const VTInt /*numTabs*/) override { return false; } // CBT diff --git a/src/terminal/parser/IStateMachineEngine.hpp b/src/terminal/parser/IStateMachineEngine.hpp index 3f0356ebb84..1eefc5be2a5 100644 --- a/src/terminal/parser/IStateMachineEngine.hpp +++ b/src/terminal/parser/IStateMachineEngine.hpp @@ -46,9 +46,7 @@ namespace Microsoft::Console::VirtualTerminal virtual bool ActionIgnore() = 0; - virtual bool ActionOscDispatch(const wchar_t wch, - const size_t parameter, - const std::wstring_view string) = 0; + virtual bool ActionOscDispatch(const size_t parameter, const std::wstring_view string) = 0; virtual bool ActionSs3Dispatch(const wchar_t wch, const VTParameters parameters) = 0; diff --git a/src/terminal/parser/InputStateMachineEngine.cpp b/src/terminal/parser/InputStateMachineEngine.cpp index cdc270c1858..df7a8c2cfb7 100644 --- a/src/terminal/parser/InputStateMachineEngine.cpp +++ b/src/terminal/parser/InputStateMachineEngine.cpp @@ -538,14 +538,11 @@ bool InputStateMachineEngine::ActionIgnore() noexcept // - Triggers the OscDispatch action to indicate that the listener should handle a control sequence. // These sequences perform various API-type commands that can include many parameters. // Arguments: -// - wch - Character to dispatch. This will be a BEL or ST char. // - parameter - identifier of the OSC action to perform // - string - OSC string we've collected. NOT null terminated. // Return Value: // - true if we handled the dispatch. -bool InputStateMachineEngine::ActionOscDispatch(const wchar_t /*wch*/, - const size_t /*parameter*/, - const std::wstring_view /*string*/) noexcept +bool InputStateMachineEngine::ActionOscDispatch(const size_t /*parameter*/, const std::wstring_view /*string*/) noexcept { return false; } diff --git a/src/terminal/parser/InputStateMachineEngine.hpp b/src/terminal/parser/InputStateMachineEngine.hpp index b0687f6a0b3..6b90c729492 100644 --- a/src/terminal/parser/InputStateMachineEngine.hpp +++ b/src/terminal/parser/InputStateMachineEngine.hpp @@ -156,9 +156,7 @@ namespace Microsoft::Console::VirtualTerminal bool ActionIgnore() noexcept override; - bool ActionOscDispatch(const wchar_t wch, - const size_t parameter, - const std::wstring_view string) noexcept override; + bool ActionOscDispatch(const size_t parameter, const std::wstring_view string) noexcept override; bool ActionSs3Dispatch(const wchar_t wch, const VTParameters parameters) override; diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index 143734bd62b..34d40fc850d 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -766,14 +766,11 @@ bool OutputStateMachineEngine::ActionIgnore() noexcept // - Triggers the OscDispatch action to indicate that the listener should handle a control sequence. // These sequences perform various API-type commands that can include many parameters. // Arguments: -// - wch - Character to dispatch. This will be a BEL or ST char. // - parameter - identifier of the OSC action to perform // - string - OSC string we've collected. NOT null terminated. // Return Value: // - true if we handled the dispatch. -bool OutputStateMachineEngine::ActionOscDispatch(const wchar_t /*wch*/, - const size_t parameter, - const std::wstring_view string) +bool OutputStateMachineEngine::ActionOscDispatch(const size_t parameter, const std::wstring_view string) { auto success = false; @@ -782,10 +779,9 @@ bool OutputStateMachineEngine::ActionOscDispatch(const wchar_t /*wch*/, case OscActionCodes::SetIconAndWindowTitle: case OscActionCodes::SetWindowIcon: case OscActionCodes::SetWindowTitle: + case OscActionCodes::DECSWT_SetWindowTitle: { - std::wstring title; - success = _GetOscTitle(string, title); - success = success && _dispatch->SetWindowTitle(title); + success = _dispatch->SetWindowTitle(string); break; } case OscActionCodes::SetColor: @@ -932,21 +928,6 @@ bool OutputStateMachineEngine::ActionSs3Dispatch(const wchar_t /*wch*/, const VT return false; } -// Routine Description: -// - Null terminates, then returns, the string that we've collected as part of the OSC string. -// Arguments: -// - string - Osc String input -// - title - Where to place the Osc String to use as a title. -// Return Value: -// - True if there was a title to output. (a title with length=0 is still valid) -bool OutputStateMachineEngine::_GetOscTitle(const std::wstring_view string, - std::wstring& title) const -{ - title = string; - - return !string.empty(); -} - // Routine Description: // - OSC 4 ; c ; spec ST // c: the index of the ansi color table diff --git a/src/terminal/parser/OutputStateMachineEngine.hpp b/src/terminal/parser/OutputStateMachineEngine.hpp index b405179890f..1ff22ab5a99 100644 --- a/src/terminal/parser/OutputStateMachineEngine.hpp +++ b/src/terminal/parser/OutputStateMachineEngine.hpp @@ -52,9 +52,7 @@ namespace Microsoft::Console::VirtualTerminal bool ActionIgnore() noexcept override; - bool ActionOscDispatch(const wchar_t wch, - const size_t parameter, - const std::wstring_view string) override; + bool ActionOscDispatch(const size_t parameter, const std::wstring_view string) override; bool ActionSs3Dispatch(const wchar_t wch, const VTParameters parameters) noexcept override; @@ -217,6 +215,7 @@ namespace Microsoft::Console::VirtualTerminal SetForegroundColor = 10, SetBackgroundColor = 11, SetCursorColor = 12, + DECSWT_SetWindowTitle = 21, SetClipboard = 52, ResetForegroundColor = 110, // Not implemented ResetBackgroundColor = 111, // Not implemented @@ -226,9 +225,6 @@ namespace Microsoft::Console::VirtualTerminal ITerm2Action = 1337, }; - bool _GetOscTitle(const std::wstring_view string, - std::wstring& title) const; - bool _GetOscSetColorTable(const std::wstring_view string, std::vector& tableIndexes, std::vector& rgbs) const; diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 88727ad6ec2..38dea307072 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -685,17 +685,17 @@ void StateMachine::_ActionOscPut(const wchar_t wch) } // Routine Description: -// - Triggers the CsiDispatch action to indicate that the listener should handle a control sequence. +// - Triggers the OscDispatch action to indicate that the listener should handle a control sequence. // These sequences perform various API-type commands that can include many parameters. // Arguments: -// - wch - Character to dispatch. +// - // Return Value: // - -void StateMachine::_ActionOscDispatch(const wchar_t wch) +void StateMachine::_ActionOscDispatch() { _trace.TraceOnAction(L"OscDispatch"); _trace.DispatchSequenceTrace(_SafeExecute([=]() { - return _engine->ActionOscDispatch(wch, _oscParameter, _oscString); + return _engine->ActionOscDispatch(_oscParameter, _oscString); })); } @@ -1434,20 +1434,28 @@ void StateMachine::_EventCsiSubParam(const wchar_t wch) // Routine Description: // - Processes a character event into an Action that occurs while in the OscParam state. // Events in this state will: -// 1. Collect numeric values into an Osc Param -// 2. Move to the OscString state on a delimiter -// 3. Ignore everything else. +// 1. Trigger the OSC action associated with the param on an OscTerminator +// 2. If we see a ESC, enter the OscTermination state. We'll wait for one +// more character before we dispatch the (empty) string. +// 3. Collect numeric values into an Osc Param +// 4. Move to the OscString state on a delimiter +// 5. Ignore everything else. // Arguments: // - wch - Character that triggered the event // Return Value: // - -void StateMachine::_EventOscParam(const wchar_t wch) noexcept +void StateMachine::_EventOscParam(const wchar_t wch) { _trace.TraceOnEvent(L"OscParam"); if (_isOscTerminator(wch)) { + _ActionOscDispatch(); _EnterGround(); } + else if (_isEscape(wch)) + { + _EnterOscTermination(); + } else if (_isNumericParamValue(wch)) { _ActionOscParam(wch); @@ -1479,7 +1487,7 @@ void StateMachine::_EventOscString(const wchar_t wch) _trace.TraceOnEvent(L"OscString"); if (_isOscTerminator(wch)) { - _ActionOscDispatch(wch); + _ActionOscDispatch(); _EnterGround(); } else if (_isEscape(wch)) @@ -1511,7 +1519,7 @@ void StateMachine::_EventOscTermination(const wchar_t wch) _trace.TraceOnEvent(L"OscTermination"); if (_isStringTerminatorIndicator(wch)) { - _ActionOscDispatch(wch); + _ActionOscDispatch(); _EnterGround(); } else @@ -1856,8 +1864,8 @@ void StateMachine::ProcessCharacter(const wchar_t wch) ProcessCharacter(_c1To7Bit(wch)); } } - // Don't go to escape from the OSC string state - ESC can be used to terminate OSC strings. - else if (_isEscape(wch) && _state != VTStates::OscString) + // Don't go to escape from the OSC string/param states - ESC can be used to terminate OSC strings. + else if (_isEscape(wch) && _state != VTStates::OscString && _state != VTStates::OscParam) { _ActionInterrupt(); _EnterEscape(); diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 01976b21b23..245e25fefa6 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -95,7 +95,7 @@ namespace Microsoft::Console::VirtualTerminal void _ActionCsiDispatch(const wchar_t wch); void _ActionOscParam(const wchar_t wch) noexcept; void _ActionOscPut(const wchar_t wch); - void _ActionOscDispatch(const wchar_t wch); + void _ActionOscDispatch(); void _ActionSs3Dispatch(const wchar_t wch); void _ActionDcsDispatch(const wchar_t wch); @@ -132,7 +132,7 @@ namespace Microsoft::Console::VirtualTerminal void _EventCsiIgnore(const wchar_t wch); void _EventCsiParam(const wchar_t wch); void _EventCsiSubParam(const wchar_t wch); - void _EventOscParam(const wchar_t wch) noexcept; + void _EventOscParam(const wchar_t wch); void _EventOscString(const wchar_t wch); void _EventOscTermination(const wchar_t wch); void _EventSs3Entry(const wchar_t wch); diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index c5517a89af6..9409f064080 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -61,9 +61,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final auto engine = std::make_unique(std::move(dispatch)); StateMachine mach(std::move(engine)); - // The OscString state shouldn't escape out after an ESC. - // Same for DcsPassThrough and SosPmApcString state. - auto shouldEscapeOut = true; + auto expectedEscapeState = StateMachine::VTStates::Escape; switch (uiTest) { @@ -109,17 +107,21 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final mach._state = StateMachine::VTStates::CsiIntermediate; break; } + // The OscParam and OscString states shouldn't escape out after an ESC. + // They enter the OscTermination state to wait for the `\` char of the + // string terminator, without which the OSC operation won't be executed. case 7: { Log::Comment(L"Escape from OscParam"); mach._state = StateMachine::VTStates::OscParam; + expectedEscapeState = StateMachine::VTStates::OscTermination; break; } case 8: { Log::Comment(L"Escape from OscString"); - shouldEscapeOut = false; mach._state = StateMachine::VTStates::OscString; + expectedEscapeState = StateMachine::VTStates::OscTermination; break; } case 9: @@ -167,7 +169,6 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final case 16: { Log::Comment(L"Escape from DcsPassThrough"); - shouldEscapeOut = false; mach._state = StateMachine::VTStates::DcsPassThrough; mach._dcsStringHandler = [](const auto) { return true; }; break; @@ -175,17 +176,13 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final case 17: { Log::Comment(L"Escape from SosPmApcString"); - shouldEscapeOut = false; mach._state = StateMachine::VTStates::SosPmApcString; break; } } mach.ProcessCharacter(AsciiChars::ESC); - if (shouldEscapeOut) - { - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); - } + VERIFY_ARE_EQUAL(expectedEscapeState, mach._state); } TEST_METHOD(TestEscapeImmediatePath) @@ -1156,6 +1153,7 @@ class StatefulDispatch final : public TermDispatch _lineFeed{ false }, _lineFeedType{ (DispatchTypes::LineFeedType)-1 }, _reverseLineFeed{ false }, + _setWindowTitle{ false }, _forwardTab{ false }, _numTabs{ 0 }, _tabClear{ false }, @@ -1406,6 +1404,13 @@ class StatefulDispatch final : public TermDispatch return true; } + bool SetWindowTitle(std::wstring_view title) override + { + _setWindowTitle = true; + _setWindowTitleText = title; + return true; + } + bool ForwardTab(const VTInt numTabs) noexcept override { _forwardTab = true; @@ -1419,6 +1424,7 @@ class StatefulDispatch final : public TermDispatch _tabClearTypes.push_back(clearType); return true; } + bool SetColorTableEntry(const size_t tableIndex, const COLORREF color) noexcept override { _setColorTableEntry = true; @@ -1517,6 +1523,8 @@ class StatefulDispatch final : public TermDispatch bool _lineFeed; DispatchTypes::LineFeedType _lineFeedType; bool _reverseLineFeed; + bool _setWindowTitle; + std::wstring _setWindowTitleText; bool _forwardTab; size_t _numTabs; bool _tabClear; @@ -3170,6 +3178,49 @@ class StateMachineExternalTest final pDispatch->ClearState(); } + TEST_METHOD(TestOscSetWindowTitle) + { + BEGIN_TEST_METHOD_PROPERTIES() + TEST_METHOD_PROPERTY(L"Data:oscNumber", L"{0, 1, 2, 21}") + END_TEST_METHOD_PROPERTIES() + + VTInt oscNumber; + VERIFY_SUCCEEDED_RETURN(TestData::TryGetValue(L"oscNumber", oscNumber)); + + const auto oscPrefix = wil::str_printf(L"\x1b]%d", oscNumber); + const auto stringTerminator = L"\033\\"; + + auto dispatch = std::make_unique(); + auto pDispatch = dispatch.get(); + auto engine = std::make_unique(std::move(dispatch)); + StateMachine mach(std::move(engine)); + + mach.ProcessString(oscPrefix); + mach.ProcessString(L";Title Text"); + mach.ProcessString(stringTerminator); + VERIFY_IS_TRUE(pDispatch->_setWindowTitle); + VERIFY_ARE_EQUAL(L"Title Text", pDispatch->_setWindowTitleText); + + pDispatch->ClearState(); + pDispatch->_setWindowTitleText = L"****"; // Make sure this is cleared + + mach.ProcessString(oscPrefix); + mach.ProcessString(L";"); + mach.ProcessString(stringTerminator); + VERIFY_IS_TRUE(pDispatch->_setWindowTitle); + VERIFY_ARE_EQUAL(L"", pDispatch->_setWindowTitleText); + + pDispatch->ClearState(); + pDispatch->_setWindowTitleText = L"****"; // Make sure this is cleared + + mach.ProcessString(oscPrefix); + mach.ProcessString(stringTerminator); + VERIFY_IS_TRUE(pDispatch->_setWindowTitle); + VERIFY_ARE_EQUAL(L"", pDispatch->_setWindowTitleText); + + pDispatch->ClearState(); + } + TEST_METHOD(TestSetClipboard) { auto dispatch = std::make_unique(); diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp index d7e49537207..0bf36050e95 100644 --- a/src/terminal/parser/ut_parser/StateMachineTest.cpp +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -73,9 +73,7 @@ class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStat bool ActionIgnore() override { return true; }; - bool ActionOscDispatch(const wchar_t /* wch */, - const size_t /* parameter */, - const std::wstring_view /* string */) override + bool ActionOscDispatch(const size_t /* parameter */, const std::wstring_view /* string */) override { if (pfnFlushToTerminal) {