Skip to content

Commit

Permalink
Send an updated cursor position at the end of writing a run (#12210)
Browse files Browse the repository at this point in the history
I can find the commit where this regressed tomorrow if needed. In #10685 we stopped emitting these notifications while we were deferring cursor drawing. We however forgot to send the notification at the end of the defer.

This likely has a small perf impact. We however do need these cursor position events for IMEs to be able to track the cursor position (and low key for #10821)

* [x] regressed in #10685
* [x] Closes #11170
* [x] I work here
* [x] Tests added

(cherry picked from commit 01fd714)
  • Loading branch information
zadjii-msft authored and DHowett committed Jan 31, 2022
1 parent 0d1e4d3 commit c4398ba
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 7 deletions.
21 changes: 14 additions & 7 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,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)
Expand Down Expand Up @@ -1087,13 +1101,6 @@ void Terminal::_AdjustCursorPosition(const COORD proposedPosition)
COORD delta{ 0, -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)
Expand Down
42 changes: 42 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/TerminalBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ class TerminalCoreUnitTests::TerminalBufferTests final

TEST_METHOD(TestGetReverseTab);

TEST_METHOD(TestCursorNotifications);

TEST_METHOD_SETUP(MethodSetup)
{
// STEP 1: Set up the Terminal
Expand Down Expand Up @@ -592,3 +594,43 @@ 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);
};
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 = 1;
callbackWasCalled = false;
term->Write(L"Foo\r\nBar");
VERIFY_ARE_EQUAL(0, expectedCallbacks);
VERIFY_IS_TRUE(callbackWasCalled);

expectedCallbacks = 2; // One for each Write
callbackWasCalled = false;
term->Write(L"Foo\r\nBar");
term->Write(L"Foo\r\nBar");
VERIFY_ARE_EQUAL(0, expectedCallbacks);
VERIFY_IS_TRUE(callbackWasCalled);
}

0 comments on commit c4398ba

Please sign in to comment.