From c39bb8a2b8903570a9ed078620326a327ea5ac90 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 6 Jan 2020 14:25:42 -0800 Subject: [PATCH 1/5] Adjust the run stored to fix truncation issue. --- src/terminal/parser/stateMachine.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 5c3cb08cd73..e32af8842eb 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -1249,7 +1249,7 @@ void StateMachine::ProcessString(const std::wstring_view string) while (current < string.size()) { - _run = string.substr(start, current - start); + _run = string.substr(start, current - start + 1); if (_processingIndividually) { @@ -1266,8 +1266,10 @@ void StateMachine::ProcessString(const std::wstring_view string) { if (_isActionableFromGround(string.at(current))) // If the current char is the start of an escape sequence, or should be executed in ground state... { - _engine->ActionPrintString(_run); // ... print all the chars leading up to it as part of the run... - _trace.DispatchPrintRunTrace(_run); + const auto allLeadingUpTo = _run.substr(0, _run.size() - 1); + + _engine->ActionPrintString(allLeadingUpTo); // ... print all the chars leading up to it as part of the run... + _trace.DispatchPrintRunTrace(allLeadingUpTo); _processingIndividually = true; // begin processing future characters individually... start = current; continue; @@ -1279,7 +1281,7 @@ void StateMachine::ProcessString(const std::wstring_view string) } } - _run = string.substr(start, current - start); + _run = string.substr(start, current - start + 1); // If we're at the end of the string and have remaining un-printed characters, if (!_processingIndividually && !_run.empty()) From 89bbdc08a4e493ea57245547a5a4dcb3c7e01631 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 6 Jan 2020 14:45:23 -0800 Subject: [PATCH 2/5] Wrote test. --- .../parser/ut_parser/StateMachineTest.cpp | 52 +++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp index 046855d9ba3..f41c0a16ca2 100644 --- a/src/terminal/parser/ut_parser/StateMachineTest.cpp +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -31,9 +31,17 @@ class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStat bool ActionExecute(const wchar_t /* wch */) override { return true; }; bool ActionExecuteFromEscape(const wchar_t /* wch */) override { return true; }; bool ActionPrint(const wchar_t /* wch */) override { return true; }; - bool ActionPrintString(const std::wstring_view /* string */) override { return true; }; + bool ActionPrintString(const std::wstring_view string) override + { + printed = string; + return true; + }; - bool ActionPassThroughString(const std::wstring_view /* string */) override { return true; }; + bool ActionPassThroughString(const std::wstring_view string) override + { + passedThrough = string; + return true; + }; bool ActionEscDispatch(const wchar_t /* wch */, const std::basic_string_view /* intermediates */) override { return true; }; @@ -58,12 +66,30 @@ class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStat const std::basic_string_view /*intermediates*/, const std::basic_string_view parameters) override { - csiParams.emplace(parameters.cbegin(), parameters.cend()); - return true; + // If flush to terminal is registered for a test, then use it. + if (pfnFlushToTerminal) + { + pfnFlushToTerminal(); + return true; + } + else + { + csiParams.emplace(parameters.cbegin(), parameters.cend()); + return true; + } } // This will only be populated if ActionCsiDispatch is called. std::optional> csiParams; + + // Flush function for pass-through test. + std::function pfnFlushToTerminal; + + // Passed through string. + std::wstring passedThrough; + + // Printed string. + std::wstring printed; }; class Microsoft::Console::VirtualTerminal::StateMachineTest @@ -81,6 +107,8 @@ class Microsoft::Console::VirtualTerminal::StateMachineTest } TEST_METHOD(TwoStateMachinesDoNotInterfereWithEachother); + + TEST_METHOD(PassThroughUnhandled); }; void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother() @@ -104,3 +132,19 @@ void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother() VERIFY_ARE_EQUAL(expectedFirstCsi, firstEngine.csiParams); VERIFY_ARE_EQUAL(expectedSecondCsi, secondEngine.csiParams); } + +void StateMachineTest::PassThroughUnhandled() +{ + auto enginePtr{ std::make_unique() }; + // this dance is required because StateMachine presumes to take ownership of its engine. + auto& engine{ *enginePtr.get() }; + StateMachine machine{ std::move(enginePtr) }; + + // Hook up the passthrough function. + engine.pfnFlushToTerminal = std::bind(&StateMachine::FlushToTerminal, &machine); + + machine.ProcessString(L"\x1b[?999h 12345 Hello World"); + + VERIFY_ARE_EQUAL(String(L"\x1b[?999h"), String(engine.passedThrough.c_str())); + VERIFY_ARE_EQUAL(String(L" 12345 Hello World"), String(engine.printed.c_str())); +} From 106bf1b9a8bf7145d12d1ba4e3f7e5b5c0902649 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 6 Jan 2020 14:53:00 -0800 Subject: [PATCH 3/5] Additional test to ensure that run storage for the other case through the ProcessString loop continues to work correctly with _run storage. --- .../parser/ut_parser/StateMachineTest.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp index f41c0a16ca2..794eee2ce35 100644 --- a/src/terminal/parser/ut_parser/StateMachineTest.cpp +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -109,6 +109,8 @@ class Microsoft::Console::VirtualTerminal::StateMachineTest TEST_METHOD(TwoStateMachinesDoNotInterfereWithEachother); TEST_METHOD(PassThroughUnhandled); + + TEST_METHOD(RunStorageBeforeEscape); }; void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother() @@ -148,3 +150,17 @@ void StateMachineTest::PassThroughUnhandled() VERIFY_ARE_EQUAL(String(L"\x1b[?999h"), String(engine.passedThrough.c_str())); VERIFY_ARE_EQUAL(String(L" 12345 Hello World"), String(engine.printed.c_str())); } + +void StateMachineTest::RunStorageBeforeEscape() +{ + auto enginePtr{ std::make_unique() }; + // this dance is required because StateMachine presumes to take ownership of its engine. + auto& engine{ *enginePtr.get() }; + StateMachine machine{ std::move(enginePtr) }; + + // Print a bunch of regular text to build up the run buffer before transitioning state. + machine.ProcessString(L"12345 Hello World\x1b[?999h"); + + // Then ensure the entire buffered run was printed all at once back to us. + VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str())); +} From d20457818091381aa88e3b51904a416152efe88c Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 6 Jan 2020 15:12:27 -0800 Subject: [PATCH 4/5] Add one more test for a large bulk of text and correct the final substring. Add comments to loop to explain _run operation since it's confusing. --- src/terminal/parser/stateMachine.cpp | 13 ++++++++++++- .../parser/ut_parser/StateMachineTest.cpp | 16 +++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index e32af8842eb..3e2995511cb 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -1249,6 +1249,9 @@ void StateMachine::ProcessString(const std::wstring_view string) while (current < string.size()) { + // The run will be everything from the start INCLUDING the current one + // in case we process the current character and it turns into a passthrough + // fallback that picks up this _run inside `FlushToTerminal` above. _run = string.substr(start, current - start + 1); if (_processingIndividually) @@ -1266,6 +1269,9 @@ void StateMachine::ProcessString(const std::wstring_view string) { if (_isActionableFromGround(string.at(current))) // If the current char is the start of an escape sequence, or should be executed in ground state... { + // Because the _run above is composed INCLUDING current, we must + // trim it off here since we just determined it's actionable + // and only pass through everything before it. const auto allLeadingUpTo = _run.substr(0, _run.size() - 1); _engine->ActionPrintString(allLeadingUpTo); // ... print all the chars leading up to it as part of the run... @@ -1281,7 +1287,12 @@ void StateMachine::ProcessString(const std::wstring_view string) } } - _run = string.substr(start, current - start + 1); + // When we leave the loop, current has been advanced to the length of the string itself + // (or one past the array index to the final char) so this `substr` operation doesn't +1 + // to include the final character (unlike the one inside the top of the loop above.) + // NOTE: std::basic_string_view will auto-trim excessively large sizes down to the valid length + // so passing something too large in the second parameter WILL NOT FAIL. + _run = string.substr(start, current - start); // If we're at the end of the string and have remaining un-printed characters, if (!_processingIndividually && !_run.empty()) diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp index 794eee2ce35..3044aa6f809 100644 --- a/src/terminal/parser/ut_parser/StateMachineTest.cpp +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -109,8 +109,8 @@ class Microsoft::Console::VirtualTerminal::StateMachineTest TEST_METHOD(TwoStateMachinesDoNotInterfereWithEachother); TEST_METHOD(PassThroughUnhandled); - TEST_METHOD(RunStorageBeforeEscape); + TEST_METHOD(BulkTextPrint); }; void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother() @@ -164,3 +164,17 @@ void StateMachineTest::RunStorageBeforeEscape() // Then ensure the entire buffered run was printed all at once back to us. VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str())); } + +void StateMachineTest::BulkTextPrint() +{ + auto enginePtr{ std::make_unique() }; + // this dance is required because StateMachine presumes to take ownership of its engine. + auto& engine{ *enginePtr.get() }; + StateMachine machine{ std::move(enginePtr) }; + + // Print a bunch of regular text to build up the run buffer before transitioning state. + machine.ProcessString(L"12345 Hello World"); + + // Then ensure the entire buffered run was printed all at once back to us. + VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str())); +} From 006695db4f116ffd301fcbb3bd721ccb2adc4b5b Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 6 Jan 2020 15:19:35 -0800 Subject: [PATCH 5/5] check the passthrough on the backwards string print too because why not. --- src/terminal/parser/ut_parser/StateMachineTest.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp index 3044aa6f809..85fbb872e8b 100644 --- a/src/terminal/parser/ut_parser/StateMachineTest.cpp +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -158,11 +158,15 @@ void StateMachineTest::RunStorageBeforeEscape() auto& engine{ *enginePtr.get() }; StateMachine machine{ std::move(enginePtr) }; + // Hook up the passthrough function. + engine.pfnFlushToTerminal = std::bind(&StateMachine::FlushToTerminal, &machine); + // Print a bunch of regular text to build up the run buffer before transitioning state. machine.ProcessString(L"12345 Hello World\x1b[?999h"); // Then ensure the entire buffered run was printed all at once back to us. VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str())); + VERIFY_ARE_EQUAL(String(L"\x1b[?999h"), String(engine.passedThrough.c_str())); } void StateMachineTest::BulkTextPrint()