mirror of
https://github.com/microsoft/terminal.git
synced 2025-12-10 18:43:54 -06:00
[Conhost] Fix off-by-1 error when copying and coloring selections (#19259)
## Summary of the Pull Request Fixes a bug where copying and coloring selected text would be off by one. This was introduced in #18106 when selection was updated to be stored as an exclusive range. `Selection::_RegenerateSelectionSpans()` was updated then, but copying text and coloring selection didn't rely on selection spans. Copying text relies on `GetSelectionAnchors()`. This function has now been updated to increment the bottom-right point of the selection. This way, `GetTextSpans()` operates on the expected _exclusive_ range. Coloring selection relies on `TextBuffer::SearchText()`, `TextBuffer::GetTextRects` and `GetSelectionSpans()`. Both `Selection::ColorSelection()` were updated to use `rect` over `inclusive_rect` to emphasize that they are exclusive ranges. Converting between the two improves clarity and fixes the bug. ## References and Relevant Issues Introduced in #18106 ## Validation Steps Performed Copying text works in the following scenarios: ✅ single line, left-to-right and right-to-left ✅ multi-line, diagonal directions ✅ block selection Coloring text works in the following scenarios: ✅ctrl+# --> color instance ✅ctrl+shift+# --> color all instances Closes #19053
This commit is contained in:
parent
7055b99acc
commit
642a2aa41e
@ -428,9 +428,9 @@ void Selection::ClearSelection(const bool fStartingNewSelection)
|
||||
// - This does not validate whether there is a valid selection right now or not.
|
||||
// It is assumed to already be in a proper selecting state and the given rectangle should be highlighted with the given color unconditionally.
|
||||
// Arguments:
|
||||
// - psrRect - Rectangular area to fill with color
|
||||
// - psrRect - Rectangular area to fill with color (exclusive)
|
||||
// - attr - The color attributes to apply
|
||||
void Selection::ColorSelection(const til::inclusive_rect& srRect, const TextAttribute attr)
|
||||
void Selection::ColorSelection(const til::rect& srRect, const TextAttribute attr)
|
||||
{
|
||||
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
|
||||
|
||||
@ -438,8 +438,8 @@ void Selection::ColorSelection(const til::inclusive_rect& srRect, const TextAttr
|
||||
auto& screenInfo = gci.GetActiveOutputBuffer();
|
||||
|
||||
til::point coordTargetSize;
|
||||
coordTargetSize.x = CalcWindowSizeX(srRect);
|
||||
coordTargetSize.y = CalcWindowSizeY(srRect);
|
||||
coordTargetSize.x = srRect.width();
|
||||
coordTargetSize.y = srRect.height();
|
||||
|
||||
til::point coordTarget;
|
||||
coordTarget.x = srRect.left;
|
||||
@ -475,9 +475,9 @@ void Selection::ColorSelection(const til::point coordSelectionStart, const til::
|
||||
const auto& screenInfo = gci.GetActiveOutputBuffer();
|
||||
|
||||
const auto rectangles = screenInfo.GetTextBuffer().GetTextRects(coordSelectionStart, coordSelectionEnd, false, true);
|
||||
for (const auto& rect : rectangles)
|
||||
for (const auto& inclusiveRect : rectangles)
|
||||
{
|
||||
ColorSelection(rect, attr);
|
||||
ColorSelection(til::rect{ inclusiveRect }, attr);
|
||||
}
|
||||
}
|
||||
CATCH_LOG();
|
||||
|
||||
@ -58,7 +58,7 @@ public:
|
||||
|
||||
void ClearSelection();
|
||||
void ClearSelection(const bool fStartingNewSelection);
|
||||
void ColorSelection(const til::inclusive_rect& srRect, const TextAttribute attr);
|
||||
void ColorSelection(const til::rect& srRect, const TextAttribute attr);
|
||||
void ColorSelection(const til::point coordSelectionStart, const til::point coordSelectionEnd, const TextAttribute attr);
|
||||
|
||||
// delete these or we can accidentally get copies of the singleton
|
||||
|
||||
@ -698,7 +698,7 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo)
|
||||
for (auto&& sp : selection)
|
||||
{
|
||||
sp.iterate_rows(textBuffer.GetSize().Width(), [&](til::CoordType row, til::CoordType beg, til::CoordType end) {
|
||||
ColorSelection({ beg, row, end, row }, selectionAttr);
|
||||
ColorSelection({ beg, row, end, row + 1 }, selectionAttr);
|
||||
});
|
||||
}
|
||||
ClearSelection();
|
||||
|
||||
@ -220,6 +220,23 @@ std::pair<til::point, til::point> Selection::GetSelectionAnchors() const noexcep
|
||||
endSelectionAnchor.x = (_d->coordSelectionAnchor.x == _d->srSelectionRect.left) ? _d->srSelectionRect.right : _d->srSelectionRect.left;
|
||||
endSelectionAnchor.y = (_d->coordSelectionAnchor.y == _d->srSelectionRect.top) ? _d->srSelectionRect.bottom : _d->srSelectionRect.top;
|
||||
|
||||
// GH #18106: Conhost and Terminal share most of the selection code.
|
||||
// Both now store the selection data as a half-open range [start, end),
|
||||
// where "end" is the bottom-right-most point.
|
||||
// Conhost operates as an inclusive range, so we need to adjust the "end" endpoint by incrementing it by one.
|
||||
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
|
||||
const auto& bufferSize = gci.GetActiveOutputBuffer().GetTextBuffer().GetSize();
|
||||
if (IsLineSelection())
|
||||
{
|
||||
// General comparison for line selection.
|
||||
bufferSize.IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor);
|
||||
}
|
||||
else
|
||||
{
|
||||
// Compare x-values when we're in block selection!
|
||||
bufferSize.IncrementInExclusiveBounds(startSelectionAnchor.x <= endSelectionAnchor.x ? endSelectionAnchor : startSelectionAnchor);
|
||||
}
|
||||
|
||||
if (startSelectionAnchor > endSelectionAnchor)
|
||||
{
|
||||
return { endSelectionAnchor, startSelectionAnchor };
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user