Skip to content

Commit

Permalink
Correct the cursor invalidation region (#14661)
Browse files Browse the repository at this point in the history
Depending on the line rendition, and whether the cursor is over a wide
character or not, it's possible for the width to take up anywhere from 1
to 4 cells. And when it's more than 1 cell wide, part of the cursor may
end up off screen. However, our bounds check requires the entire cursor
to be on screen, otherwise it doesn't render anything, and that can
result in cursor droppings being left behind. This PR fixes that.

The bounds check that is causing this issue was introduced in #13001 to
fix a debug assertion.

To fix this, I've removed the bounds checking, and instead clip the
cursor rect to the boundaries of the viewport. This is what the original
code was trying to do prior to the #13001 fix, but was mistakenly using
the `Viewport:Clamp` method, instead of `TrimToViewport`. Since this
implementation doesn't require a clamp, there should be no risk of the
debug assertion returning.

## Validation Steps Performed

I've confirmed that the test case in #14657 is now working correctly,
and not leaving cursor droppings behind. I've also tested under conhost
with buffer sizes wider than the viewport, and confirmed it can handle a
wide cursor partially scrolled off screen.

Closes #14657
  • Loading branch information
j4james committed Jan 11, 2023
1 parent b7e537e commit f1090a0
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,15 +276,16 @@ void Renderer::TriggerRedrawCursor(const til::point* const pcoord)
// cells for the cursor, taking line rendition into account.
const auto lineRendition = buffer.GetLineRendition(pcoord->y);
const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;
const til::inclusive_rect cursorRect = { pcoord->x, pcoord->y, pcoord->x + cursorWidth - 1, pcoord->y };
auto cursorView = Viewport::FromInclusive(BufferToScreenLine(cursorRect, lineRendition));
til::inclusive_rect cursorRect = { pcoord->x, pcoord->y, pcoord->x + cursorWidth - 1, pcoord->y };
cursorRect = BufferToScreenLine(cursorRect, lineRendition);

// The region is clamped within the viewport boundaries and we only
// trigger a redraw if the region is not empty.
// That region is then clipped within the viewport boundaries and we
// only trigger a redraw if the resulting region is not empty.
auto view = _pData->GetViewport();
if (view.IsInBounds(cursorView))
auto updateRect = til::rect{ cursorRect };
if (view.TrimToViewport(&updateRect))
{
const auto updateRect = view.ConvertToOrigin(cursorView).ToExclusive();
view.ConvertToOrigin(&updateRect);
FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->InvalidateCursor(&updateRect));
Expand Down

0 comments on commit f1090a0

Please sign in to comment.