Skip to content

Commit

Permalink
XtermEngine: Explicitly emit cursor state on the first frame (microso…
Browse files Browse the repository at this point in the history 
…ft#12434)

This commit fixes an issue, where we failed to emit a DECTCEM sequence to hide
the cursor if it was hidden before XtermEngine's first frame was finalized.
Even in such cases we need to emit a DECTCEM sequence
in order to ensure we're in a consistent state.

## Validation Steps Performed
* Added test✅
* Run microsoft#12401's repro steps around 30 times✅

Closes microsoft#12401

(cherry picked from commit dbb7077)
  • Loading branch information
@lhecker @DHowett
lhecker authored and DHowett committed Mar 23, 2022
1 parent c857f83 commit 4abd57e
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 104 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2437,6 +2437,7 @@ TREX
triaged
triaging
TRIANGLESTRIP
Tribool
TRIMZEROHEADINGS
truetype
trx
Expand Down
125 changes: 39 additions & 86 deletions src/host/ut_host/VtRendererTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,29 @@ class Microsoft::Console::Render::VtRendererTest
void TestPaint(VtEngine& engine, std::function<void()> pfn);
Viewport SetUpViewport();

void VerifyExpectedInputsDrained();
void VerifyFirstPaint(VtEngine& engine)
{
// Verify the first BeginPaint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
// Verify the first EndPaint sets the cursor state
qExpectedInput.push_back("\x1b[?25l");
VERIFY_IS_TRUE(engine._firstPaint);
TestPaint(engine, [&]() {
VERIFY_IS_FALSE(engine._firstPaint);
});
}

void VerifyExpectedInputsDrained()
{
if (!qExpectedInput.empty())
{
for (const auto& exp : qExpectedInput)
{
Log::Error(NoThrowString().Format(L"EXPECTED INPUT NEVER RECEIVED: %hs", exp.c_str()));
}
VERIFY_FAIL(L"there should be no remaining un-drained expected input");
}
}
};

Viewport VtRendererTest::SetUpViewport()
Expand All @@ -103,18 +125,6 @@ Viewport VtRendererTest::SetUpViewport()
return Viewport::FromInclusive(view);
}

void VtRendererTest::VerifyExpectedInputsDrained()
{
if (!qExpectedInput.empty())
{
for (const auto& exp : qExpectedInput)
{
Log::Error(NoThrowString().Format(L"EXPECTED INPUT NEVER RECEIVED: %hs", exp.c_str()));
}
VERIFY_FAIL(L"there should be no remaining un-drained expected input");
}
}

bool VtRendererTest::WriteCallback(const char* const pch, size_t const cch)
{
std::string actualString = std::string(pch, cch);
Expand Down Expand Up @@ -212,12 +222,7 @@ void VtRendererTest::Xterm256TestInvalidate()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

const Viewport view = SetUpViewport();

Expand Down Expand Up @@ -402,12 +407,7 @@ void VtRendererTest::Xterm256TestColors()
RenderSettings renderSettings;
RenderData renderData;

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -585,12 +585,7 @@ void VtRendererTest::Xterm256TestCursor()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -766,12 +761,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -907,12 +897,7 @@ void VtRendererTest::XtermTestInvalidate()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -1096,12 +1081,7 @@ void VtRendererTest::XtermTestColors()
RenderSettings renderSettings;
RenderData renderData;

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -1234,12 +1214,7 @@ void VtRendererTest::XtermTestCursor()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -1412,12 +1387,7 @@ void VtRendererTest::TestWrapping()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_firstPaint);
});
VerifyFirstPaint(*engine);

Viewport view = SetUpViewport();

Expand Down Expand Up @@ -1465,8 +1435,10 @@ void VtRendererTest::TestResize()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear and go home
// Verify the first BeginPaint emits a clear and go home
qExpectedInput.push_back("\x1b[2J");
// Verify the first EndPaint sets the cursor state
qExpectedInput.push_back("\x1b[?25l");
VERIFY_IS_TRUE(engine->_firstPaint);
VERIFY_IS_TRUE(engine->_suppressResizeRepaint);

Expand Down Expand Up @@ -1502,21 +1474,6 @@ void VtRendererTest::TestCursorVisibility()
auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2);
engine->SetTestCallback(pfn);

// Verify the first paint emits a clear
qExpectedInput.push_back("\x1b[2J");
VERIFY_IS_TRUE(engine->_firstPaint);
VERIFY_IS_FALSE(engine->_lastCursorIsVisible);
VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
TestPaint(*engine, [&]() {
// During StartPaint, we'll mark the cursor as off. make sure that happens.
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_firstPaint);
});

// The cursor wasn't painted in the last frame.
VERIFY_IS_FALSE(engine->_lastCursorIsVisible);
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);

COORD origin{ 0, 0 };

VERIFY_ARE_NOT_EQUAL(origin, engine->_lastText);
Expand All @@ -1527,8 +1484,8 @@ void VtRendererTest::TestCursorVisibility()
// Frame 1: Paint the cursor at the home position. At the end of the frame,
// the cursor should be on. Because we're moving the cursor with CUP, we
// need to disable the cursor during this frame.
qExpectedInput.push_back("\x1b[2J");
TestPaint(*engine, [&]() {
VERIFY_IS_FALSE(engine->_lastCursorIsVisible);
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

Expand All @@ -1539,18 +1496,20 @@ void VtRendererTest::TestCursorVisibility()
VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
VERIFY_IS_TRUE(engine->_needToDisableCursor);

// GH#12401:
// The other tests verify that the cursor is explicitly hidden on the
// first frame (VerifyFirstPaint). This test on the other hand does
// the opposite by calling PaintCursor() during the first paint cycle.
qExpectedInput.push_back("\x1b[?25h");
});

VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

// Frame 2: Paint the cursor again at the home position. At the end of the
// frame, the cursor should be on, the same as before. We aren't moving the
// cursor during this frame, so _needToDisableCursor will stay false.
TestPaint(*engine, [&]() {
VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

Expand All @@ -1561,15 +1520,13 @@ void VtRendererTest::TestCursorVisibility()
VERIFY_IS_FALSE(engine->_needToDisableCursor);
});

VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

// Frame 3: Paint the cursor at 2,2. At the end of the frame, the cursor
// should be on, the same as before. Because we're moving the cursor with
// CUP, we need to disable the cursor during this frame.
TestPaint(*engine, [&]() {
VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

Expand All @@ -1580,7 +1537,6 @@ void VtRendererTest::TestCursorVisibility()

VERIFY_SUCCEEDED(engine->PaintCursor(options));

VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
VERIFY_IS_TRUE(engine->_needToDisableCursor);

Expand All @@ -1590,22 +1546,19 @@ void VtRendererTest::TestCursorVisibility()
qExpectedInput.push_back("\x1b[?25h");
});

VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_TRUE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

// Frame 4: Don't paint the cursor. At the end of the frame, the cursor
// should be off.
Log::Comment(NoThrowString().Format(L"Painting without calling PaintCursor will hide the cursor"));
TestPaint(*engine, [&]() {
VERIFY_IS_TRUE(engine->_lastCursorIsVisible);
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);

qExpectedInput.push_back("\x1b[?25l");
});

VERIFY_IS_FALSE(engine->_lastCursorIsVisible);
VERIFY_IS_FALSE(engine->_nextCursorIsVisible);
VERIFY_IS_FALSE(engine->_needToDisableCursor);
}
Expand Down
25 changes: 8 additions & 17 deletions src/renderer/vt/XtermEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
VtEngine(std::move(hPipe), initialViewport),
_fUseAsciiOnly(fUseAsciiOnly),
_needToDisableCursor(false),
_lastCursorIsVisible(false),
// GH#12401: Ensure a DECTCEM cursor show/hide sequence
// is emitted on the first frame no matter what.
_lastCursorIsVisible(Tribool::Invalid),
_nextCursorIsVisible(true)
{
// Set out initial cursor position to -1, -1. This will force our initial
Expand Down Expand Up @@ -98,31 +100,20 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
{
// If the cursor was previously visible, let's hide it for this frame,
// by prepending a cursor off.
if (_lastCursorIsVisible)
if (_lastCursorIsVisible != Tribool::False)
{
_buffer.insert(0, "\x1b[?25l");
_lastCursorIsVisible = false;
_lastCursorIsVisible = Tribool::False;
}
// If the cursor was NOT previously visible, then that's fine! we don't
// need to worry, it's already off.
}

// If the cursor is moving from off -> on (including cases where we just
// disabled if for this frame), show the cursor at the end of the frame
if (_nextCursorIsVisible && !_lastCursorIsVisible)
if (_lastCursorIsVisible != static_cast<Tribool>(_nextCursorIsVisible))
{
RETURN_IF_FAILED(_ShowCursor());
RETURN_IF_FAILED(_nextCursorIsVisible ? _ShowCursor() : _HideCursor());
_lastCursorIsVisible = static_cast<Tribool>(_nextCursorIsVisible);
}
// Otherwise, if the cursor previously was visible, and it should be hidden
// (on -> off), hide it at the end of the frame.
else if (!_nextCursorIsVisible && _lastCursorIsVisible)
{
RETURN_IF_FAILED(_HideCursor());
}

// Update our tracker of what we thought the last cursor state of the
// terminal was.
_lastCursorIsVisible = _nextCursorIsVisible;

RETURN_IF_FAILED(VtEngine::EndPaint());

Expand Down
11 changes: 10 additions & 1 deletion src/renderer/vt/XtermEngine.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,18 @@ namespace Microsoft::Console::Render
[[nodiscard]] HRESULT WriteTerminalW(const std::wstring_view str) noexcept override;

protected:
// I'm using a non-class enum here, so that the values
// are trivially convertible and comparable to bool.
enum class Tribool : uint8_t
{
False = 0,
True,
Invalid,
};

const bool _fUseAsciiOnly;
bool _needToDisableCursor;
bool _lastCursorIsVisible;
Tribool _lastCursorIsVisible;
bool _nextCursorIsVisible;

[[nodiscard]] HRESULT _MoveCursor(const COORD coord) noexcept override;
Expand Down

0 comments on commit 4abd57e

Please sign in to comment.