Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

XtermEngine: Explicitly emit cursor state on the first frame #12434

Merged
merged 2 commits into from
Feb 8, 2022
Merged

XtermEngine: Explicitly emit cursor state on the first frame #12434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
653594b
XtermEngine: Explicitly emit cursor state on the first frame
lhecker Feb 8, 2022
2c3debc
Address feedback
lhecker Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2444,6 +2444,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
Open in desktop
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
Open in desktop
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
Open in desktop
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