Skip to content

Commit

Permalink
Fix multiple cursor invalidation issues (#17181)
Browse files Browse the repository at this point in the history 
There were multiple bugs:
* GDI engine only paints whatever has been invalidated.
  This means we need to not just invalidate the old cursor rect
  but also the new one, or else movements may not be visible.
* The composition should be drawn at the cursor position even if
  the cursor is invisible, but inside the renderer viewport.
* Conceptually, scrolling the viewport moves the relative cursor
  position even if the cursor is invisible.
* An invisible cursor is not the same as one that's outside the
  viewport. It's more like a cursor that's not turned on.

To resolve the first issue we simply need to call `InvalidateCursor`
again. To do so, it was abstracted into `_invalidateCurrentCursor()`.

The next 2 issues are resolved by un-`optional`-izing `CursorOptions`.
After all, even an invisible or an out-of-bounds cursor still has a
coordinate and it may still be scrolled into view.
Instead, it has the new `inViewport` property as a replacement.
This allows for instance the IME composition code in the renderer
to use the cursor coordinate while the cursor is invisible.

The last issue is fixed by simply changing the `.isOn` logic.

Closes #17150

## Validation Steps Performed
* In conhost with the GDI renderer:
  `printf "\e[2 q"; sleep 2; printf "\e[A"; sleep 2; printf "\e[B"`
  Cursor moves up after 2s and then down again after 2s. ✅
* Hide the cursor (`"\e[?25l"`) and use a CJK IME.
  Words can still be written and deleted correctly. ✅
* Turning the cursor back on (`"\e[?25h"`) works ✅
* Scrolling shows/hides the cursor ✅
  • Loading branch information
@lhecker
lhecker authored May 2, 2024
1 parent 92e05f2 commit 4fbcd65
Show file tree
Hide file tree
Showing 3 changed files with 170 additions and 138 deletions.
296 changes: 160 additions & 136 deletions src/renderer/base/renderer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,96 +121,14 @@ IRenderData* Renderer::GetRenderData() const noexcept
// Last chance check if anything scrolled without an explicit invalidate notification since the last frame.
_CheckViewportAndScroll();

if (_currentCursorOptions)
{
const auto& buffer = _pData->GetTextBuffer();
const auto view = buffer.GetSize();
const auto coord = _currentCursorOptions->coordCursor;

// If we had previously drawn a composition at the previous cursor position
// we need to invalidate the entire line because who knows what changed.
// (It's possible to figure that out, but not worth the effort right now.)
if (_compositionCache)
{
til::rect rect{ 0, coord.y, til::CoordTypeMax, coord.y + 1 };
if (view.TrimToViewport(&rect))
{
FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->Invalidate(&rect));
}
}
}
else
{
const auto lineRendition = buffer.GetLineRendition(coord.y);
const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;

til::rect rect{ coord.x, coord.y, coord.x + cursorWidth, coord.y + 1 };
rect = BufferToScreenLine(rect, lineRendition);
_invalidateCurrentCursor(); // Invalidate the previous cursor position.
_invalidateOldComposition();

if (view.TrimToViewport(&rect))
{
FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->InvalidateCursor(&rect));
}
}
}
}

_currentCursorOptions = _GetCursorInfo();
_updateCursorInfo();
_compositionCache.reset();

// Invalidate the line that the active TSF composition is on,
// so that _PaintBufferOutput() actually gets a chance to draw it.
if (!_pData->activeComposition.text.empty())
{
const auto viewport = _pData->GetViewport();
const auto coordCursor = _pData->GetCursorPosition();

til::rect line{ 0, coordCursor.y, til::CoordTypeMax, coordCursor.y + 1 };
if (viewport.TrimToViewport(&line))
{
viewport.ConvertToOrigin(&line);

FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->Invalidate(&line));
}

auto& buffer = _pData->GetTextBuffer();
auto& scratch = buffer.GetScratchpadRow();

std::wstring_view text{ _pData->activeComposition.text };
RowWriteState state{
.columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
};

state.text = text.substr(0, _pData->activeComposition.cursorPos);
scratch.ReplaceText(state);
const auto cursorOffset = state.columnEnd;

state.text = text.substr(_pData->activeComposition.cursorPos);
state.columnBegin = state.columnEnd;
scratch.ReplaceText(state);

// Ideally the text is inserted at the position of the cursor (`coordCursor`),
// but if we got more text than fits into the remaining space until the end of the line,
// then we'll insert the text aligned to the end of the line.
const auto remaining = state.columnLimit - state.columnEnd;
const auto beg = std::clamp(coordCursor.x, 0, remaining);

const auto baseAttribute = buffer.GetRowByOffset(coordCursor.y).GetAttrByColumn(coordCursor.x);
_compositionCache.emplace(til::point{ beg, coordCursor.y }, baseAttribute);

// Fake-move the cursor to where it needs to be in the active composition.
if (_currentCursorOptions)
{
_currentCursorOptions->coordCursor.x = std::min(beg + cursorOffset, line.right - 1);
}
}
}
_invalidateCurrentCursor(); // Invalidate the new cursor position.
_prepareNewComposition();

FOREACH_ENGINE(pEngine)
{
Expand Down Expand Up @@ -510,6 +428,20 @@ bool Renderer::_CheckViewportAndScroll()
}

_ScrollPreviousSelection(coordDelta);

// The cursor may have moved out of or into the viewport. Update the .inViewport property.
{
const auto view = ScreenToBufferLine(srNewViewport, _currentCursorOptions.lineRendition);
const auto coordCursor = _currentCursorOptions.coordCursor;

// Note that we allow the X coordinate to be outside the left border by 1 position,
// because the cursor could still be visible if the focused character is double width.
const auto xInRange = coordCursor.x >= view.left - 1 && coordCursor.x <= view.right;
const auto yInRange = coordCursor.y >= view.top && coordCursor.y <= view.bottom;

_currentCursorOptions.inViewport = xInRange && yInRange;
}

return true;
}

Expand Down Expand Up @@ -1188,58 +1120,153 @@ bool Renderer::_isInHoveredInterval(const til::point coordTarget) const noexcept
// - <none>
// Return Value:
// - nullopt if the cursor is off or out-of-frame, otherwise a CursorOptions
[[nodiscard]] std::optional<CursorOptions> Renderer::_GetCursorInfo()
void Renderer::_updateCursorInfo()
{
if (_pData->IsCursorVisible())
// Get cursor position in buffer
auto coordCursor = _pData->GetCursorPosition();

// GH#3166: Only draw the cursor if it's actually in the viewport. It
// might be on the line that's in that partially visible row at the
// bottom of the viewport, the space that's not quite a full line in
// height. Since we don't draw that text, we shouldn't draw the cursor
// there either.

// The cursor is never rendered as double height, so we don't care about
// the exact line rendition - only whether it's double width or not.
const auto doubleWidth = _pData->GetTextBuffer().IsDoubleWidthLine(coordCursor.y);
const auto lineRendition = doubleWidth ? LineRendition::DoubleWidth : LineRendition::SingleWidth;

// We need to convert the screen coordinates of the viewport to an
// equivalent range of buffer cells, taking line rendition into account.
const auto viewport = _pData->GetViewport().ToInclusive();
const auto view = ScreenToBufferLine(viewport, lineRendition);

// Note that we allow the X coordinate to be outside the left border by 1 position,
// because the cursor could still be visible if the focused character is double width.
const auto xInRange = coordCursor.x >= view.left - 1 && coordCursor.x <= view.right;
const auto yInRange = coordCursor.y >= view.top && coordCursor.y <= view.bottom;

// Adjust cursor Y offset to viewport.
// The viewport X offset is saved in the options and handled with a transform.
coordCursor.y -= view.top;

const auto cursorColor = _renderSettings.GetColorTableEntry(TextColor::CURSOR_COLOR);
const auto useColor = cursorColor != INVALID_COLOR;

_currentCursorOptions.coordCursor = coordCursor;
_currentCursorOptions.viewportLeft = viewport.left;
_currentCursorOptions.lineRendition = lineRendition;
_currentCursorOptions.ulCursorHeightPercent = _pData->GetCursorHeight();
_currentCursorOptions.cursorPixelWidth = _pData->GetCursorPixelWidth();
_currentCursorOptions.fIsDoubleWidth = _pData->IsCursorDoubleWidth();
_currentCursorOptions.cursorType = _pData->GetCursorStyle();
_currentCursorOptions.fUseColor = useColor;
_currentCursorOptions.cursorColor = cursorColor;
_currentCursorOptions.isVisible = _pData->IsCursorVisible();
_currentCursorOptions.isOn = _currentCursorOptions.isVisible && _pData->IsCursorOn();
_currentCursorOptions.inViewport = xInRange && yInRange;
}

void Renderer::_invalidateCurrentCursor() const
{
if (!_currentCursorOptions.inViewport || !_currentCursorOptions.isOn)
{
// Get cursor position in buffer
auto coordCursor = _pData->GetCursorPosition();
return;
}

// GH#3166: Only draw the cursor if it's actually in the viewport. It
// might be on the line that's in that partially visible row at the
// bottom of the viewport, the space that's not quite a full line in
// height. Since we don't draw that text, we shouldn't draw the cursor
// there either.
const auto& buffer = _pData->GetTextBuffer();
const auto view = buffer.GetSize();
const auto coord = _currentCursorOptions.coordCursor;

// The cursor is never rendered as double height, so we don't care about
// the exact line rendition - only whether it's double width or not.
const auto doubleWidth = _pData->GetTextBuffer().IsDoubleWidthLine(coordCursor.y);
const auto lineRendition = doubleWidth ? LineRendition::DoubleWidth : LineRendition::SingleWidth;
const auto lineRendition = buffer.GetLineRendition(coord.y);
const auto cursorWidth = _pData->IsCursorDoubleWidth() ? 2 : 1;

// We need to convert the screen coordinates of the viewport to an
// equivalent range of buffer cells, taking line rendition into account.
const auto view = ScreenToBufferLine(_pData->GetViewport().ToInclusive(), lineRendition);
til::rect rect{ coord.x, coord.y, coord.x + cursorWidth, coord.y + 1 };
rect = BufferToScreenLine(rect, lineRendition);

// Note that we allow the X coordinate to be outside the left border by 1 position,
// because the cursor could still be visible if the focused character is double width.
const auto xInRange = coordCursor.x >= view.left - 1 && coordCursor.x <= view.right;
const auto yInRange = coordCursor.y >= view.top && coordCursor.y <= view.bottom;
if (xInRange && yInRange)
if (view.TrimToViewport(&rect))
{
FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->InvalidateCursor(&rect));
}
}
}

// If we had previously drawn a composition at the previous cursor position
// we need to invalidate the entire line because who knows what changed.
// (It's possible to figure that out, but not worth the effort right now.)
void Renderer::_invalidateOldComposition() const
{
if (!_compositionCache || !_currentCursorOptions.inViewport)
{
return;
}

const auto& buffer = _pData->GetTextBuffer();
const auto view = buffer.GetSize();
const auto coord = _currentCursorOptions.coordCursor;

til::rect rect{ 0, coord.y, til::CoordTypeMax, coord.y + 1 };
if (view.TrimToViewport(&rect))
{
FOREACH_ENGINE(pEngine)
{
// Adjust cursor Y offset to viewport.
// The viewport X offset is saved in the options and handled with a transform.
coordCursor.y -= view.top;

auto cursorColor = _renderSettings.GetColorTableEntry(TextColor::CURSOR_COLOR);
auto useColor = cursorColor != INVALID_COLOR;

// Build up the cursor parameters including position, color, and drawing options
CursorOptions options;
options.coordCursor = coordCursor;
options.viewportLeft = _pData->GetViewport().Left();
options.lineRendition = lineRendition;
options.ulCursorHeightPercent = _pData->GetCursorHeight();
options.cursorPixelWidth = _pData->GetCursorPixelWidth();
options.fIsDoubleWidth = _pData->IsCursorDoubleWidth();
options.cursorType = _pData->GetCursorStyle();
options.fUseColor = useColor;
options.cursorColor = cursorColor;
options.isOn = _pData->IsCursorOn();

return { options };
LOG_IF_FAILED(pEngine->Invalidate(&rect));
}
}
return std::nullopt;
}

// Invalidate the line that the active TSF composition is on,
// so that _PaintBufferOutput() actually gets a chance to draw it.
void Renderer::_prepareNewComposition()
{
if (_pData->activeComposition.text.empty())
{
return;
}

const auto viewport = _pData->GetViewport();
const auto coordCursor = _pData->GetCursorPosition();

til::rect line{ 0, coordCursor.y, til::CoordTypeMax, coordCursor.y + 1 };
if (viewport.TrimToViewport(&line))
{
viewport.ConvertToOrigin(&line);

FOREACH_ENGINE(pEngine)
{
LOG_IF_FAILED(pEngine->Invalidate(&line));
}

auto& buffer = _pData->GetTextBuffer();
auto& scratch = buffer.GetScratchpadRow();

std::wstring_view text{ _pData->activeComposition.text };
RowWriteState state{
.columnLimit = buffer.GetRowByOffset(line.top).GetReadableColumnCount(),
};

state.text = text.substr(0, _pData->activeComposition.cursorPos);
scratch.ReplaceText(state);
const auto cursorOffset = state.columnEnd;

state.text = text.substr(_pData->activeComposition.cursorPos);
state.columnBegin = state.columnEnd;
scratch.ReplaceText(state);

// Ideally the text is inserted at the position of the cursor (`coordCursor`),
// but if we got more text than fits into the remaining space until the end of the line,
// then we'll insert the text aligned to the end of the line.
const auto remaining = state.columnLimit - state.columnEnd;
const auto beg = std::clamp(coordCursor.x, 0, remaining);

const auto baseAttribute = buffer.GetRowByOffset(coordCursor.y).GetAttrByColumn(coordCursor.x);
_compositionCache.emplace(til::point{ beg, coordCursor.y }, baseAttribute);

// Fake-move the cursor to where it needs to be in the active composition.
_currentCursorOptions.coordCursor.x = std::min(beg + cursorOffset, line.right - 1);
}
}

// Routine Description:
Expand All @@ -1250,9 +1277,9 @@ bool Renderer::_isInHoveredInterval(const til::point coordTarget) const noexcept
// - <none>
void Renderer::_PaintCursor(_In_ IRenderEngine* const pEngine)
{
if (_currentCursorOptions)
if (_currentCursorOptions.inViewport && _currentCursorOptions.isVisible)
{
LOG_IF_FAILED(pEngine->PaintCursor(*_currentCursorOptions));
LOG_IF_FAILED(pEngine->PaintCursor(_currentCursorOptions));
}
}

Expand Down Expand Up @@ -1385,10 +1412,7 @@ void Renderer::_ScrollPreviousSelection(const til::point delta)
rc += delta;
}

if (_currentCursorOptions)
{
_currentCursorOptions->coordCursor += delta;
}
_currentCursorOptions.coordCursor += delta;
}
}

Expand Down
7 changes: 5 additions & 2 deletions src/renderer/base/renderer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ namespace Microsoft::Console::Render
void _ScrollPreviousSelection(const til::point delta);
[[nodiscard]] HRESULT _PaintTitle(IRenderEngine* const pEngine);
bool _isInHoveredInterval(til::point coordTarget) const noexcept;
[[nodiscard]] std::optional<CursorOptions> _GetCursorInfo();
void _updateCursorInfo();
void _invalidateCurrentCursor() const;
void _invalidateOldComposition() const;
void _prepareNewComposition();
[[nodiscard]] HRESULT _PrepareRenderInfo(_In_ IRenderEngine* const pEngine);

const RenderSettings& _renderSettings;
Expand All @@ -134,7 +137,7 @@ namespace Microsoft::Console::Render
uint16_t _hyperlinkHoveredId = 0;
std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _hoveredInterval;
Microsoft::Console::Types::Viewport _viewport;
std::optional<CursorOptions> _currentCursorOptions;
CursorOptions _currentCursorOptions;
std::optional<CompositionCache> _compositionCache;
std::vector<Cluster> _clusterBuffer;
std::vector<til::rect> _previousSelection;
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/inc/CursorOptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,14 @@ namespace Microsoft::Console::Render
// Color to use for drawing instead of the default
COLORREF cursorColor;

// The other kind of on/off state for the cursor, because VtEngine needs it to handle \x1b[?25l/h.
bool isVisible;
// Is the cursor currently visually visible?
// If the cursor has blinked off, this is false.
// if the cursor has blinked on, this is true.
bool isOn;

// Is the cursor within the viewport of the renderer?
bool inViewport;
};
}

0 comments on commit 4fbcd65

Please sign in to comment.