From 16477b734a9696aa47b2cfcb432efc62a8dafc2b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 20 Jan 2022 16:24:51 -0600 Subject: [PATCH 1/5] This fixes #11170, but a test would be nice --- src/cascadia/TerminalCore/Terminal.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index bcb6fa07b3f..8cb1a105d7e 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -989,6 +989,7 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) } cursor.EndDeferDrawing(); + _NotifyTerminalCursorPositionChanged(); } void Terminal::_AdjustCursorPosition(const COORD proposedPosition) From e9eceec37deb4c1a74e9cc22eaabe9a617a228cd Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 20 Jan 2022 16:56:30 -0600 Subject: [PATCH 2/5] a test too, because I'm a good person --- .../TerminalBufferTests.cpp | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index e32c197d3fe..ad1a0054eea 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -51,6 +51,8 @@ class TerminalCoreUnitTests::TerminalBufferTests final TEST_METHOD(TestGetReverseTab); + TEST_METHOD(TestCursorNotifications); + TEST_METHOD_SETUP(MethodSetup) { // STEP 1: Set up the Terminal @@ -592,3 +594,42 @@ void TerminalBufferTests::TestGetReverseTab() L"Cursor adjusted to last item in the sample list from position beyond end."); } } + +void TerminalBufferTests::TestCursorNotifications() +{ + // Test for GH#11170 + + // Suppress test exceptions. If they occur in the lambda, they'll just crash + // TAEF, which is annoying. + const WEX::TestExecution::DisableVerifyExceptions disableExceptionsScope; + + bool callbackWasCalled = false; + int expectedCallbacks = 0; + auto cb = [&expectedCallbacks, &callbackWasCalled]() mutable { + Log::Comment(L"Callback triggered"); + callbackWasCalled = true; + expectedCallbacks--; + VERIFY_IS_GREATER_THAN_OR_EQUAL(expectedCallbacks, 0); + // VERIFY_IS_TRUE(expectedCallbacks >= 0); + }; + term->_pfnCursorPositionChanged = cb; + + expectedCallbacks = 1; + callbackWasCalled = false; + term->_WriteBuffer(L"Foo"); + VERIFY_ARE_EQUAL(0, expectedCallbacks); + VERIFY_IS_TRUE(callbackWasCalled); + + expectedCallbacks = 1; + callbackWasCalled = false; + term->_WriteBuffer(L"Foo\r\nBar"); + VERIFY_ARE_EQUAL(0, expectedCallbacks); + VERIFY_IS_TRUE(callbackWasCalled); + + expectedCallbacks = 2; + callbackWasCalled = false; + term->_WriteBuffer(L"Foo\r\nBar"); + term->_WriteBuffer(L"Foo\r\nBar"); + VERIFY_ARE_EQUAL(0, expectedCallbacks); + VERIFY_IS_TRUE(callbackWasCalled); +} From af05ad84c985290fd80c3bbefc5317eda0a5e6d2 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 25 Jan 2022 06:33:10 -0600 Subject: [PATCH 3/5] Uh oh someone broke main on an FI Fixes FI bugs introduced in 33c2cd458a1fb00077c0ae62cd8043e388d50943 --- .../dll/Microsoft.Terminal.Settings.Model.vcxproj | 3 +++ src/server/IoDispatchers.cpp | 1 + 2 files changed, 4 insertions(+) diff --git a/src/cascadia/TerminalSettingsModel/dll/Microsoft.Terminal.Settings.Model.vcxproj b/src/cascadia/TerminalSettingsModel/dll/Microsoft.Terminal.Settings.Model.vcxproj index 847dc470ba0..9120af6591a 100644 --- a/src/cascadia/TerminalSettingsModel/dll/Microsoft.Terminal.Settings.Model.vcxproj +++ b/src/cascadia/TerminalSettingsModel/dll/Microsoft.Terminal.Settings.Model.vcxproj @@ -57,6 +57,9 @@ {345FD5A4-B32B-4F29-BD1C-B033BD2C35CC} + + {ef3e32a7-5ff6-42b4-b6e2-96cd7d033f00} + diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index 81707aaf415..f52cba56c7d 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -132,6 +132,7 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleCloseObject(_In_ PCONSOLE_API_MSG pMessag } // LsaGetLoginSessionData might also fit the bill here, but it looks like it does RPC with lsass.exe. Using user32 is cheaper. +#pragma warning(suppress : 4505) static bool _isInteractiveUserSession() { DWORD sessionId{}; From 67441eb88fb39429a1a1f0280c72335d7ed2702e Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 25 Jan 2022 06:59:46 -0600 Subject: [PATCH 4/5] This is potentially cleaner, but I have another idea --- src/cascadia/TerminalCore/Terminal.cpp | 12 +++++------- src/cascadia/TerminalCore/TerminalApi.cpp | 4 ++++ .../UnitTests_TerminalCore/TerminalBufferTests.cpp | 12 ++++++------ 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 8cb1a105d7e..55ab3cf5834 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -989,6 +989,11 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) } cursor.EndDeferDrawing(); + + // Firing the CursorPositionChanged event is very expensive so we try not to + // do that when the cursor does not need to be redrawn. We don't do this + // inside _AdjustCursorPosition, only once we're done writing the whole run + // of output. _NotifyTerminalCursorPositionChanged(); } @@ -1094,13 +1099,6 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition) COORD delta{ 0, gsl::narrow_cast(-rowsPushedOffTopOfBuffer) }; _buffer->GetRenderTarget().TriggerScroll(&delta); } - - // Firing the CursorPositionChanged event is very expensive so we try not to do that when - // the cursor does not need to be redrawn. - if (!cursor.IsDeferDrawing()) - { - _NotifyTerminalCursorPositionChanged(); - } } void Terminal::UserScrollViewport(const int viewTop) diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index e2e5eb35253..4a107d3e9d0 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -92,6 +92,10 @@ try } _AdjustCursorPosition(cursorPos); + // Send an updated cursor position event. See GH#12210 for why this is done + // here instead of in _AdjustCursorPosition + _NotifyTerminalCursorPositionChanged(); + return true; } CATCH_RETURN_FALSE() diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index ad1a0054eea..906c83945b1 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -616,20 +616,20 @@ void TerminalBufferTests::TestCursorNotifications() expectedCallbacks = 1; callbackWasCalled = false; - term->_WriteBuffer(L"Foo"); + term->Write(L"Foo"); VERIFY_ARE_EQUAL(0, expectedCallbacks); VERIFY_IS_TRUE(callbackWasCalled); - expectedCallbacks = 1; + expectedCallbacks = 3; // one for foo, one for the newline, one for bar callbackWasCalled = false; - term->_WriteBuffer(L"Foo\r\nBar"); + term->Write(L"Foo\r\nBar"); VERIFY_ARE_EQUAL(0, expectedCallbacks); VERIFY_IS_TRUE(callbackWasCalled); - expectedCallbacks = 2; + expectedCallbacks = 6; callbackWasCalled = false; - term->_WriteBuffer(L"Foo\r\nBar"); - term->_WriteBuffer(L"Foo\r\nBar"); + term->Write(L"Foo\r\nBar"); + term->Write(L"Foo\r\nBar"); VERIFY_ARE_EQUAL(0, expectedCallbacks); VERIFY_IS_TRUE(callbackWasCalled); } From d0fb6dd94cfbe8e67fb80ef47a0b1e9447f26c73 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 25 Jan 2022 07:09:50 -0600 Subject: [PATCH 5/5] Even cleaner solution --- src/cascadia/TerminalCore/Terminal.cpp | 20 +++++++++++++------ src/cascadia/TerminalCore/TerminalApi.cpp | 4 ---- .../TerminalBufferTests.cpp | 7 ++++--- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 55ab3cf5834..d03bcbb3abb 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -431,7 +431,21 @@ void Terminal::Write(std::wstring_view stringView) { auto lock = LockForWriting(); + auto& cursor = _buffer->GetCursor(); + const til::point cursorPosBefore{ cursor.GetPosition() }; + _stateMachine->ProcessString(stringView); + + const til::point cursorPosAfter{ cursor.GetPosition() }; + + // Firing the CursorPositionChanged event is very expensive so we try not to + // do that when the cursor does not need to be redrawn. We don't do this + // inside _AdjustCursorPosition, only once we're done writing the whole run + // of output. + if (cursorPosBefore != cursorPosAfter) + { + _NotifyTerminalCursorPositionChanged(); + } } void Terminal::WritePastedText(std::wstring_view stringView) @@ -989,12 +1003,6 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) } cursor.EndDeferDrawing(); - - // Firing the CursorPositionChanged event is very expensive so we try not to - // do that when the cursor does not need to be redrawn. We don't do this - // inside _AdjustCursorPosition, only once we're done writing the whole run - // of output. - _NotifyTerminalCursorPositionChanged(); } void Terminal::_AdjustCursorPosition(const COORD proposedPosition) diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp index 4a107d3e9d0..e2e5eb35253 100644 --- a/src/cascadia/TerminalCore/TerminalApi.cpp +++ b/src/cascadia/TerminalCore/TerminalApi.cpp @@ -92,10 +92,6 @@ try } _AdjustCursorPosition(cursorPos); - // Send an updated cursor position event. See GH#12210 for why this is done - // here instead of in _AdjustCursorPosition - _NotifyTerminalCursorPositionChanged(); - return true; } CATCH_RETURN_FALSE() diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp index 906c83945b1..a5cbeea6c6f 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp @@ -610,23 +610,24 @@ void TerminalBufferTests::TestCursorNotifications() callbackWasCalled = true; expectedCallbacks--; VERIFY_IS_GREATER_THAN_OR_EQUAL(expectedCallbacks, 0); - // VERIFY_IS_TRUE(expectedCallbacks >= 0); }; term->_pfnCursorPositionChanged = cb; + // The exact number of callbacks here is fungible, if need be. + expectedCallbacks = 1; callbackWasCalled = false; term->Write(L"Foo"); VERIFY_ARE_EQUAL(0, expectedCallbacks); VERIFY_IS_TRUE(callbackWasCalled); - expectedCallbacks = 3; // one for foo, one for the newline, one for bar + expectedCallbacks = 1; callbackWasCalled = false; term->Write(L"Foo\r\nBar"); VERIFY_ARE_EQUAL(0, expectedCallbacks); VERIFY_IS_TRUE(callbackWasCalled); - expectedCallbacks = 6; + expectedCallbacks = 2; // One for each Write callbackWasCalled = false; term->Write(L"Foo\r\nBar"); term->Write(L"Foo\r\nBar");