From aa95790c0e627da843e05daf787550a4ec5ab2e7 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 15 Oct 2024 10:05:45 -0700 Subject: [PATCH 01/34] [AtlasEngine] Render selection as exclusive range --- src/renderer/atlas/AtlasEngine.cpp | 2 +- src/renderer/base/renderer.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/renderer/atlas/AtlasEngine.cpp b/src/renderer/atlas/AtlasEngine.cpp index 0de92b62373..b6b0274c987 100644 --- a/src/renderer/atlas/AtlasEngine.cpp +++ b/src/renderer/atlas/AtlasEngine.cpp @@ -451,7 +451,7 @@ try const auto isEndInside = y == hiEnd.y && hiEnd.x < x2; if (isStartInside && isEndInside) { - _fillColorBitmap(row, hiStart.x, static_cast(hiEnd.x) + 1, fgColor, bgColor); + _fillColorBitmap(row, hiStart.x, static_cast(hiEnd.x), fgColor, bgColor); ++it; } else diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index c44216ac470..c7fbec88bfc 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -344,7 +344,6 @@ try { sp.iterate_rows(bufferWidth, [&](til::CoordType row, til::CoordType min, til::CoordType max) { const auto shift = buffer.GetLineRendition(row) != LineRendition::SingleWidth ? 1 : 0; - max += 1; // Selection spans are inclusive (still) min <<= shift; max <<= shift; til::rect r{ min, row, max, row + 1 }; From 4697f49dcabe18a8beb0aa72cb2097661d59504e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 15 Oct 2024 13:52:18 -0700 Subject: [PATCH 02/34] Fix any side-effects of making GetText exclusive --- src/buffer/out/Row.cpp | 2 +- src/buffer/out/textBuffer.cpp | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 7a46215b6fc..4c4b6621464 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -1118,7 +1118,7 @@ std::wstring_view ROW::GetText(til::CoordType columnBegin, til::CoordType column { const auto columns = GetReadableColumnCount(); const auto colBeg = clamp(columnBegin, 0, columns); - const auto colEnd = clamp(columnEnd, colBeg, columns); + const auto colEnd = clamp(columnEnd - 1, colBeg, columns); const size_t chBeg = _uncheckedCharOffset(gsl::narrow_cast(colBeg)); const size_t chEnd = _uncheckedCharOffset(gsl::narrow_cast(colEnd)); #pragma warning(suppress : 26481) // Don't use pointer arithmetic. Use span instead (bounds.1). diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 802e48e7c3e..14d4dd2e7b3 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2597,7 +2597,9 @@ void TextBuffer::Serialize(const wchar_t* destination) const newX = lastCharX; } - buffer.append(row.GetText(oldX, newX)); + // GetText() has an exclusive end, + // add 1 to newX to get the correct range + buffer.append(row.GetText(oldX, newX + 1)); if (clearToEndOfLine) { @@ -3300,7 +3302,9 @@ std::wstring TextBuffer::_commandForRow(const til::CoordType rowOffset, if (markKind == MarkKind::Command) { - commandBuilder += row.GetText(x, nextX); + // GetText() has an exclusive end, + // add 1 to newX to get the correct range + commandBuilder += row.GetText(x, nextX + 1); } // advance to next run of text x = nextX; From 4364643631f7cc297c1a3158e36354d2ece699ed Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 15 Oct 2024 14:26:42 -0700 Subject: [PATCH 03/34] Update selection markers appropriately --- src/cascadia/TerminalControl/ControlCore.cpp | 2 +- src/cascadia/TerminalCore/TerminalSelection.cpp | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 9cc7732f452..208517bcf03 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1200,7 +1200,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto bufferSize{ _terminal->GetTextBuffer().GetSize() }; info.StartAtLeftBoundary = _terminal->GetSelectionAnchor().x == bufferSize.Left(); - info.EndAtRightBoundary = _terminal->GetSelectionEnd().x == bufferSize.RightInclusive(); + info.EndAtRightBoundary = _terminal->GetSelectionEnd().x == bufferSize.RightExclusive(); return info; } diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 2cb3ee08bfd..d34bce32824 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -113,14 +113,6 @@ til::point Terminal::SelectionEndForRendering() const { auto pos{ _selection->end }; const auto bufferSize{ GetTextBuffer().GetSize() }; - if (pos.x != bufferSize.RightInclusive()) - { - // In general, we need to draw the marker one after the - // end of the selection. - // When we're at the right boundary, we want to - // flip the marker, so we skip this step. - bufferSize.IncrementInBounds(pos); - } pos.y = base::ClampSub(pos.y, _VisibleStartIndex()); return til::point{ pos }; } From 014f33852a21193ceab890def9abe47427f57def Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 16 Oct 2024 12:16:12 -0700 Subject: [PATCH 04/34] [Mark Mode] Move by character (includes wide-glyph handling) --- src/buffer/out/textBuffer.cpp | 88 +++++++++++++------ src/buffer/out/textBuffer.hpp | 2 + .../TerminalCore/TerminalSelection.cpp | 22 +++-- src/types/inc/viewport.hpp | 6 ++ src/types/viewport.cpp | 60 +++++++++++++ 5 files changed, 146 insertions(+), 32 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 14d4dd2e7b3..593b2644aa1 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1520,13 +1520,14 @@ til::point TextBuffer::GetGlyphStart(const til::point pos, std::optional 0) + if (resultPos > limit) { - resultPos = limit; + return limit; } - // limit is exclusive, so we need to move back to be within valid bounds - if (resultPos != limit && GetCellDataAt(resultPos)->DbcsAttr() == DbcsAttribute::Trailing) + // if we're on a trailing byte, move to the leading byte + if (bufferSize.IsInBounds(resultPos) && + GetCellDataAt(resultPos)->DbcsAttr() == DbcsAttribute::Trailing) { bufferSize.DecrementInBounds(resultPos, true); } @@ -1548,12 +1549,13 @@ til::point TextBuffer::GetGlyphEnd(const til::point pos, bool accessibilityMode, const auto limit{ limitOptional.value_or(bufferSize.EndExclusive()) }; // Clamp pos to limit - if (bufferSize.CompareInBounds(resultPos, limit, true) > 0) + if (resultPos > limit) { - resultPos = limit; + return limit; } - if (resultPos != limit && GetCellDataAt(resultPos)->DbcsAttr() == DbcsAttribute::Leading) + if (bufferSize.IsInBounds(resultPos) && + GetCellDataAt(resultPos)->DbcsAttr() == DbcsAttribute::Leading) { bufferSize.IncrementInBounds(resultPos, true); } @@ -1610,6 +1612,31 @@ bool TextBuffer::MoveToNextGlyph(til::point& pos, bool allowExclusiveEnd, std::o return success; } +bool TextBuffer::MoveToNextGlyph2(til::point& pos, std::optional limitOptional) const +{ + const auto bufferSize = GetSize(); + const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) }; + + if (pos >= limit) + { + // Corner Case: we're on/past the limit + // Clamp us to the limit + pos = limit; + return false; + } + + // Try to move forward, but if we hit the buffer boundary, we fail to move. + const bool success = bufferSize.IncrementInExclusiveBounds(pos); + if (success && + bufferSize.IsInBounds(pos) && + GetCellDataAt(pos)->DbcsAttr() == DbcsAttribute::Trailing) + { + // Move again if we're on a wide glyph + bufferSize.IncrementInExclusiveBounds(pos); + } + return success; +} + // Method Description: // - Update pos to be the beginning of the previous glyph/character. This is used for accessibility // Arguments: @@ -1642,6 +1669,31 @@ bool TextBuffer::MoveToPreviousGlyph(til::point& pos, std::optional return success; } +bool TextBuffer::MoveToPreviousGlyph2(til::point& pos, std::optional limitOptional) const +{ + const auto bufferSize = GetSize(); + const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) }; + + if (pos >= limit) + { + // Corner Case: we're on/past the limit + // Clamp us to the limit + pos = limit; + return false; + } + + // Try to move backward, but if we hit the buffer boundary, we fail to move. + const bool success = bufferSize.DecrementInExclusiveBounds(pos); + if (success && + bufferSize.IsInBounds(pos) && + GetCellDataAt(pos)->DbcsAttr() == DbcsAttribute::Trailing) + { + // Move again if we're on a wide glyph + bufferSize.DecrementInExclusiveBounds(pos); + } + return success; +} + // Method Description: // - Determines the line-by-line rectangles based on two COORDs // - expands the rectangles to support wide glyphs @@ -1780,31 +1832,17 @@ void TextBuffer::_ExpandTextRow(til::inclusive_rect& textRow) const // expand left side of rect til::point targetPoint{ textRow.left, textRow.top }; - if (GetCellDataAt(targetPoint)->DbcsAttr() == DbcsAttribute::Trailing) + if (bufferSize.IsInBounds(targetPoint) && GetCellDataAt(targetPoint)->DbcsAttr() == DbcsAttribute::Trailing) { - if (targetPoint.x == bufferSize.Left()) - { - bufferSize.IncrementInBounds(targetPoint); - } - else - { - bufferSize.DecrementInBounds(targetPoint); - } + bufferSize.DecrementInExclusiveBounds(targetPoint); textRow.left = targetPoint.x; } // expand right side of rect targetPoint = { textRow.right, textRow.bottom }; - if (GetCellDataAt(targetPoint)->DbcsAttr() == DbcsAttribute::Leading) + if (bufferSize.IsInBounds(targetPoint) && GetCellDataAt(targetPoint)->DbcsAttr() == DbcsAttribute::Trailing) { - if (targetPoint.x == bufferSize.RightInclusive()) - { - bufferSize.DecrementInBounds(targetPoint); - } - else - { - bufferSize.IncrementInBounds(targetPoint); - } + bufferSize.IncrementInExclusiveBounds(targetPoint); textRow.right = targetPoint.x; } } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index a39ebd4db40..8fa3d86c26c 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -180,6 +180,8 @@ class TextBuffer final til::point GetGlyphEnd(const til::point pos, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; bool MoveToNextGlyph(til::point& pos, bool allowBottomExclusive = false, std::optional limitOptional = std::nullopt) const; bool MoveToPreviousGlyph(til::point& pos, std::optional limitOptional = std::nullopt) const; + bool MoveToNextGlyph2(til::point& pos, std::optional limitOptional = std::nullopt) const; + bool MoveToPreviousGlyph2(til::point& pos, std::optional limitOptional = std::nullopt) const; const std::vector GetTextRects(til::point start, til::point end, bool blockSelection, bool bufferCoordinates) const; std::vector GetTextSpans(til::point start, til::point end, bool blockSelection, bool bufferCoordinates) const; diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index d34bce32824..360969448ad 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -100,7 +100,7 @@ til::point Terminal::SelectionStartForRendering() const // beginning of the selection. // When we're at the left boundary, we want to // flip the marker, so we skip this step. - bufferSize.DecrementInBounds(pos); + bufferSize.DecrementInExclusiveBounds(pos); } pos.y = base::ClampSub(pos.y, _VisibleStartIndex()); return til::point{ pos }; @@ -113,6 +113,14 @@ til::point Terminal::SelectionEndForRendering() const { auto pos{ _selection->end }; const auto bufferSize{ GetTextBuffer().GetSize() }; + if (pos.x == bufferSize.RightExclusive()) + { + // sln->end is exclusive + // In general, we need to draw the marker on the same cell. + // When we're at the right boundary, we want to + // flip the marker, so we move one cell to the left. + bufferSize.DecrementInExclusiveBounds(pos); + } pos.y = base::ClampSub(pos.y, _VisibleStartIndex()); return til::point{ pos }; } @@ -617,7 +625,7 @@ void Terminal::UpdateSelection(SelectionDirection direction, SelectionExpansion } auto targetPos{ WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::Start) ? _selection->start : _selection->end }; - // 2 Perform the movement + // 2. Perform the movement switch (mode) { case SelectionExpansion::Char: @@ -687,12 +695,12 @@ void Terminal::_MoveByChar(SelectionDirection direction, til::point& pos) switch (direction) { case SelectionDirection::Left: - _activeBuffer().GetSize().DecrementInBounds(pos); - pos = _activeBuffer().GetGlyphStart(pos); + _activeBuffer().MoveToPreviousGlyph2(pos); break; case SelectionDirection::Right: - _activeBuffer().GetSize().IncrementInBounds(pos); - pos = _activeBuffer().GetGlyphEnd(pos); + // We need the limit to be the mutable viewport here, + // otherwise we're allowed to navigate by character past the mutable bottom + _activeBuffer().MoveToNextGlyph2(pos, _GetMutableViewport().BottomInclusiveRightExclusive()); break; case SelectionDirection::Up: { @@ -706,7 +714,7 @@ void Terminal::_MoveByChar(SelectionDirection direction, til::point& pos) const auto bufferSize{ _activeBuffer().GetSize() }; const auto mutableBottom{ _GetMutableViewport().BottomInclusive() }; const auto newY{ pos.y + 1 }; - pos = newY > mutableBottom ? til::point{ bufferSize.RightInclusive(), mutableBottom } : til::point{ pos.x, newY }; + pos = newY > mutableBottom ? til::point{ bufferSize.RightExclusive(), mutableBottom } : til::point{ pos.x, newY }; break; } } diff --git a/src/types/inc/viewport.hpp b/src/types/inc/viewport.hpp index fd0d5b302f8..0f1ea0bdeb8 100644 --- a/src/types/inc/viewport.hpp +++ b/src/types/inc/viewport.hpp @@ -41,20 +41,26 @@ namespace Microsoft::Console::Types til::point Origin() const noexcept; til::point BottomRightInclusive() const noexcept; til::point BottomRightExclusive() const noexcept; + til::point BottomInclusiveRightExclusive() const noexcept; til::point EndExclusive() const noexcept; til::size Dimensions() const noexcept; bool IsInBounds(const Viewport& other) const noexcept; bool IsInBounds(const til::point pos, bool allowEndExclusive = false) const noexcept; + bool IsInExclusiveBounds(const til::point pos) const noexcept; void Clamp(til::point& pos) const; Viewport Clamp(const Viewport& other) const noexcept; bool IncrementInBounds(til::point& pos, bool allowEndExclusive = false) const noexcept; bool DecrementInBounds(til::point& pos, bool allowEndExclusive = false) const noexcept; + bool IncrementInExclusiveBounds(til::point& pos) const noexcept; + bool DecrementInExclusiveBounds(til::point& pos) const noexcept; int CompareInBounds(const til::point first, const til::point second, bool allowEndExclusive = false) const noexcept; + int CompareInExclusiveBounds(const til::point first, const til::point second) const noexcept; bool WalkInBounds(til::point& pos, const til::CoordType delta, bool allowEndExclusive = false) const noexcept; + bool WalkInExclusiveBounds(til::point& pos, const til::CoordType delta) const noexcept; til::point GetWalkOrigin(const til::CoordType delta) const noexcept; static til::CoordType DetermineWalkDirection(const Viewport& source, const Viewport& target) noexcept; diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp index 018212c2165..5d09c17ed88 100644 --- a/src/types/viewport.cpp +++ b/src/types/viewport.cpp @@ -118,6 +118,11 @@ til::point Viewport::BottomRightExclusive() const noexcept return { RightExclusive(), BottomExclusive() }; } +til::point Viewport::BottomInclusiveRightExclusive() const noexcept +{ + return { RightExclusive(), BottomInclusive() }; +} + // Method Description: // - For Accessibility, get a til::point representing the end of this viewport in exclusive terms. // - This is needed to represent an exclusive endpoint in UiaTextRange that includes the last @@ -295,6 +300,61 @@ bool Viewport::WalkInBounds(til::point& pos, const til::CoordType delta, bool al return off == offClamped; } +bool Viewport::IncrementInExclusiveBounds(til::point& pos) const noexcept +{ + return WalkInExclusiveBounds(pos, 1); +} + +bool Viewport::DecrementInExclusiveBounds(til::point& pos) const noexcept +{ + return WalkInExclusiveBounds(pos, -1); +} + +bool Viewport::IsInExclusiveBounds(const til::point pos) const noexcept +{ + return pos.x >= Left() && pos.x <= RightExclusive() && + pos.y >= Top() && pos.y <= BottomExclusive(); +} + +int Viewport::CompareInExclusiveBounds(const til::point first, const til::point second) const noexcept +{ + // Assert that our coordinates are within the expected boundaries + assert(IsInExclusiveBounds(first)); + assert(IsInExclusiveBounds(second)); + + // First set the distance vertically + // If first is on row 4 and second is on row 6, first will be -2 rows behind second * an 80 character row would be -160. + // For the same row, it'll be 0 rows * 80 character width = 0 difference. + auto retVal = (first.y - second.y) * Width(); + + // Now adjust for horizontal differences + // If first is in position 15 and second is in position 30, first is -15 left in relation to 30. + retVal += (first.x - second.x); + + // Further notes: + // If we already moved behind one row, this will help correct for when first is right of second. + // For example, with row 4, col 79 and row 5, col 0 as first and second respectively, the distance is -1. + // Assume the row width is 80. + // Step one will set the retVal as -80 as first is one row behind the second. + // Step two will then see that first is 79 - 0 = +79 right of second and add 79 + // The total is -80 + 79 = -1. + return retVal; +} + +bool Viewport::WalkInExclusiveBounds(til::point& pos, const til::CoordType delta) const noexcept +{ + const auto l = static_cast(_sr.left); + const auto t = static_cast(_sr.top); + const auto w = static_cast(std::max(0, _sr.right - _sr.left + 2)); + const auto h = static_cast(std::max(0, _sr.bottom - _sr.top + 1)); + const auto max = w * h; + const auto off = w * (pos.y - t) + (pos.x - l) + delta; + const auto offClamped = std::clamp(off, ptrdiff_t{ 0 }, max); + pos.x = gsl::narrow_cast(offClamped % w + l); + pos.y = gsl::narrow_cast(offClamped / w + t); + return off == offClamped; +} + // Routine Description: // - If walking through a viewport, one might want to know the origin // for the direction walking. From ba673a4cd084e6473ce0922feec7bbdc17d156de Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 16 Oct 2024 12:48:33 -0700 Subject: [PATCH 05/34] [Mark Mode] Move by viewport --- src/cascadia/TerminalCore/TerminalSelection.cpp | 6 +++--- src/types/viewport.cpp | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 360969448ad..43bb44d36ab 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -790,7 +790,7 @@ void Terminal::_MoveByViewport(SelectionDirection direction, til::point& pos) no pos = { bufferSize.Left(), pos.y }; break; case SelectionDirection::Right: - pos = { bufferSize.RightInclusive(), pos.y }; + pos = { bufferSize.RightExclusive(), pos.y }; break; case SelectionDirection::Up: { @@ -804,7 +804,7 @@ void Terminal::_MoveByViewport(SelectionDirection direction, til::point& pos) no const auto viewportHeight{ _GetMutableViewport().Height() }; const auto mutableBottom{ _GetMutableViewport().BottomInclusive() }; const auto newY{ pos.y + viewportHeight }; - pos = newY > mutableBottom ? til::point{ bufferSize.RightInclusive(), mutableBottom } : til::point{ pos.x, newY }; + pos = newY > mutableBottom ? til::point{ bufferSize.RightExclusive(), mutableBottom } : til::point{ pos.x, newY }; break; } } @@ -909,7 +909,7 @@ til::point Terminal::_ConvertToBufferCell(const til::point viewportPos) const // - pos: a coordinate relative to the buffer (not viewport) void Terminal::_ScrollToPoint(const til::point pos) { - if (const auto visibleViewport = _GetVisibleViewport(); !visibleViewport.IsInBounds(pos)) + if (const auto visibleViewport = _GetVisibleViewport(); !visibleViewport.IsInExclusiveBounds(pos)) { if (const auto amtAboveView = visibleViewport.Top() - pos.y; amtAboveView > 0) { diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp index 5d09c17ed88..fbeecd4a137 100644 --- a/src/types/viewport.cpp +++ b/src/types/viewport.cpp @@ -313,7 +313,7 @@ bool Viewport::DecrementInExclusiveBounds(til::point& pos) const noexcept bool Viewport::IsInExclusiveBounds(const til::point pos) const noexcept { return pos.x >= Left() && pos.x <= RightExclusive() && - pos.y >= Top() && pos.y <= BottomExclusive(); + pos.y >= Top() && pos.y <= BottomInclusive(); } int Viewport::CompareInExclusiveBounds(const til::point first, const til::point second) const noexcept From cd2da0ececb836ec87c70cb2fc541d9472ef11af Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 16 Oct 2024 12:53:39 -0700 Subject: [PATCH 06/34] [Mark Mode] Move by buffer --- src/cascadia/TerminalCore/TerminalSelection.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 43bb44d36ab..49d4461ab45 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -821,7 +821,7 @@ void Terminal::_MoveByBuffer(SelectionDirection direction, til::point& pos) noex break; case SelectionDirection::Right: case SelectionDirection::Down: - pos = { bufferSize.RightInclusive(), _GetMutableViewport().BottomInclusive() }; + pos = { bufferSize.RightExclusive(), _GetMutableViewport().BottomInclusive() }; break; } } From ff45eb9719f88ab5092226d4b1fbd6af5d335760 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 17 Oct 2024 10:58:19 -0700 Subject: [PATCH 07/34] [Mark Mode] Move by word --- src/buffer/out/textBuffer.cpp | 152 ++++++++++++++++++ src/buffer/out/textBuffer.hpp | 4 + .../TerminalCore/TerminalSelection.cpp | 51 ++---- 3 files changed, 170 insertions(+), 37 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 593b2644aa1..536b2c30238 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1132,6 +1132,158 @@ DelimiterClass TextBuffer::_GetDelimiterClassAt(const til::point pos, const std: return GetRowByOffset(realPos.y).DelimiterClassAt(realPos.x, wordDelimiters); } +til::point TextBuffer::GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional) const +{ + const auto bufferSize{ GetSize() }; + const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) }; + + if (pos < bufferSize.Origin()) + { + // can't move further back, so return early at origin + return bufferSize.Origin(); + } + else if (pos >= limit) + { + // clamp to limit, + // but still do movement + pos = limit; + } + + // Consider the delimiter classes represented as these chars: + // - ControlChar: "_" + // - DelimiterChar: "D" + // - RegularChar: "C" + // Expected results ("|" is the position): + // CCC___| --> |CCC___ + // DDD___| --> |DDD___ + // ___CCC| --> ___|CCC + // DDDCCC| --> DDD|CCC + // ___DDD| --> ___|DDD + // CCCDDD| --> CCC|DDD + // So the heuristic we use is: + // 1. move to the beginning of the delimiter class run + // 2. if we were on a ControlChar, go back one more delimiter class run + const auto initialDelimiter = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; + pos = _GetDelimiterClassRunStart(pos, wordDelimiters); + if (initialDelimiter == DelimiterClass::ControlChar) + { + bufferSize.DecrementInExclusiveBounds(pos); + pos = _GetDelimiterClassRunStart(pos, wordDelimiters); + } + return pos; +} + +til::point TextBuffer::GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional) const +{ + const auto bufferSize{ GetSize() }; + const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) }; + + if (pos >= limit) + { + // can't move further forward, + // so return early at limit + return limit; + } + else if (const auto origin{ bufferSize.Origin() }; pos < origin) + { + // clamp to origin, + // but still do movement + pos = origin; + } + + // Consider the delimiter classes represented as these chars: + // - ControlChar: "_" + // - DelimiterChar: "D" + // - RegularChar: "C" + // Expected results ("|" is the position): + // |CCC___ --> CCC__|_ + // |DDD___ --> DDD__|_ + // |___CCC --> __|_CCC + // |DDDCCC --> DD|DCCC + // |___DDD --> __|_DDD + // |CCCDDD --> CC|CDDD + // So the heuristic we use is: + // 1. move to the end of the delimiter class run + // 2. if the next delimiter class run is a ControlChar, go forward one more delimiter class run + pos = _GetDelimiterClassRunEnd(pos, wordDelimiters); + if (pos.x == bufferSize.RightExclusive()) + { + // Special case: + // we're at the right boundary (and end of a delimiter class run), + // we already know we can't wrap, so return early + return pos; + } + + auto nextPos = pos; + bufferSize.IncrementInExclusiveBounds(nextPos); + if (const auto nextDelimClass = bufferSize.IsInBounds(nextPos) ? _GetDelimiterClassAt(nextPos, wordDelimiters) : DelimiterClass::ControlChar; + nextDelimClass == DelimiterClass::ControlChar) + { + return _GetDelimiterClassRunEnd(nextPos, wordDelimiters); + } + return pos; +} + +til::point TextBuffer::_GetDelimiterClassRunStart(til::point pos, const std::wstring_view wordDelimiters) const +{ + const auto bufferSize = GetSize(); + const auto initialDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; + for (auto nextPos = pos; nextPos != bufferSize.Origin();) + { + bufferSize.DecrementInExclusiveBounds(nextPos); + + if (nextPos.x == bufferSize.RightExclusive()) + { + // wrapped onto previous line, + // check if it was forced to wrap + const auto& row = GetRowByOffset(nextPos.y); + if (!row.WasWrapForced()) + { + return pos; + } + } + else if (_GetDelimiterClassAt(nextPos, wordDelimiters) != initialDelimClass) + { + // if we changed delim class, we're done (don't apply move) + return pos; + } + + // apply the move + pos = nextPos; + } + return pos; +} + +til::point TextBuffer::_GetDelimiterClassRunEnd(til::point pos, const std::wstring_view wordDelimiters) const +{ + const auto bufferSize = GetSize(); + const auto initialDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; + for (auto nextPos = pos; nextPos != bufferSize.BottomInclusiveRightExclusive();) + { + bufferSize.IncrementInExclusiveBounds(nextPos); + + if (nextPos.x == bufferSize.Left()) + { + // wrapped onto next line, + // check if it was forced to wrap or switched delimiter class + const auto& row = GetRowByOffset(pos.y); + if (!row.WasWrapForced() || _GetDelimiterClassAt(nextPos, wordDelimiters) != initialDelimClass) + { + return pos; + } + } + else if (bufferSize.IsInBounds(nextPos) && _GetDelimiterClassAt(nextPos, wordDelimiters) != initialDelimClass) + { + // if we changed delim class, we're done (don't apply move) + return pos; + } + + // apply the move + pos = nextPos; + } + return pos; +} + // Method Description: // - Get the til::point for the beginning of the word you are on // Arguments: diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 8fa3d86c26c..fef7228e101 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -173,6 +173,8 @@ class TextBuffer final til::point GetWordStart(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; til::point GetWordEnd(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; + til::point GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; + til::point GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; bool MoveToNextWord(til::point& pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; bool MoveToPreviousWord(til::point& pos, const std::wstring_view wordDelimiters) const; @@ -322,6 +324,8 @@ class TextBuffer final void _SetFirstRowIndex(const til::CoordType FirstRowIndex) noexcept; void _ExpandTextRow(til::inclusive_rect& selectionRow) const; DelimiterClass _GetDelimiterClassAt(const til::point pos, const std::wstring_view wordDelimiters) const; + til::point _GetDelimiterClassRunStart(til::point pos, const std::wstring_view wordDelimiters) const; + til::point _GetDelimiterClassRunEnd(til::point pos, const std::wstring_view wordDelimiters) const; til::point _GetWordStartForAccessibility(const til::point target, const std::wstring_view wordDelimiters) const; til::point _GetWordStartForSelection(const til::point target, const std::wstring_view wordDelimiters) const; til::point _GetWordEndForAccessibility(const til::point target, const std::wstring_view wordDelimiters, const til::point limit) const; diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 49d4461ab45..a1b77ab5283 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -248,7 +248,7 @@ void Terminal::_SetSelectionEnd(SelectionInfo* selection, const til::point viewp // - the new start/end for a selection std::pair Terminal::_PivotSelection(const til::point targetPos, bool& targetStart) const noexcept { - if (targetStart = _activeBuffer().GetSize().CompareInBounds(targetPos, _selection->pivot) <= 0) + if (targetStart = targetPos <= _selection->pivot) { // target is before pivot // treat target as start @@ -726,48 +726,25 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos) { case SelectionDirection::Left: { - const auto wordStartPos{ _activeBuffer().GetWordStart(pos, _wordDelimiters) }; - if (_activeBuffer().GetSize().CompareInBounds(_selection->pivot, pos) < 0) - { - // If we're moving towards the pivot, move one more cell - pos = wordStartPos; - _activeBuffer().GetSize().DecrementInBounds(pos); - } - else if (wordStartPos == pos) - { - // already at the beginning of the current word, - // move to the beginning of the previous word - _activeBuffer().GetSize().DecrementInBounds(pos); - pos = _activeBuffer().GetWordStart(pos, _wordDelimiters); - } - else + const auto& buffer = _activeBuffer(); + auto nextPos = pos; + nextPos = buffer.GetWordStart2(nextPos, _wordDelimiters); + if (nextPos == pos) { - // move to the beginning of the current word - pos = wordStartPos; + // didn't move because we're already at the beginning of a word, + // so move to the beginning of the previous word + buffer.GetSize().DecrementInExclusiveBounds(nextPos); + nextPos = buffer.GetWordStart2(nextPos, _wordDelimiters); } + pos = nextPos; break; } case SelectionDirection::Right: { - const auto wordEndPos{ _activeBuffer().GetWordEnd(pos, _wordDelimiters) }; - if (_activeBuffer().GetSize().CompareInBounds(pos, _selection->pivot) < 0) - { - // If we're moving towards the pivot, move one more cell - pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters); - _activeBuffer().GetSize().IncrementInBounds(pos); - } - else if (wordEndPos == pos) - { - // already at the end of the current word, - // move to the end of the next word - _activeBuffer().GetSize().IncrementInBounds(pos); - pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters); - } - else - { - // move to the end of the current word - pos = wordEndPos; - } + // increment one more because end is exclusive + const auto& buffer = _activeBuffer(); + pos = buffer.GetWordEnd2(pos, _wordDelimiters); + buffer.GetSize().IncrementInExclusiveBounds(pos); break; } case SelectionDirection::Up: From 659cd027465b0773c359902ceff37687ea0b562f Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 17 Oct 2024 11:32:38 -0700 Subject: [PATCH 08/34] [Mark Mode] Handle move up/down onto middle of emoji --- .../TerminalCore/TerminalSelection.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index a1b77ab5283..e59c3f5266e 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -93,7 +93,14 @@ const til::point Terminal::GetSelectionEnd() const noexcept til::point Terminal::SelectionStartForRendering() const { auto pos{ _selection->start }; - const auto bufferSize{ GetTextBuffer().GetSize() }; + const auto& buffer = GetTextBuffer(); + const auto bufferSize{ buffer.GetSize() }; + if (bufferSize.IsInBounds(pos) && buffer.GetCellDataAt(pos)->DbcsAttr() == DbcsAttribute::Trailing) + { + // if we're on a trailing byte, move off of it to include it + bufferSize.DecrementInExclusiveBounds(pos); + } + if (pos.x != bufferSize.Left()) { // In general, we need to draw the marker one before the @@ -112,7 +119,14 @@ til::point Terminal::SelectionStartForRendering() const til::point Terminal::SelectionEndForRendering() const { auto pos{ _selection->end }; - const auto bufferSize{ GetTextBuffer().GetSize() }; + const auto& buffer = GetTextBuffer(); + const auto bufferSize{ buffer.GetSize() }; + if (bufferSize.IsInBounds(pos) && buffer.GetCellDataAt(pos)->DbcsAttr() == DbcsAttribute::Trailing) + { + // if we're on a trailing byte, move off of it to include it + bufferSize.IncrementInExclusiveBounds(pos); + } + if (pos.x == bufferSize.RightExclusive()) { // sln->end is exclusive From e7ca8073297c6f9fc080523503b6f17a02c88ae7 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 18 Oct 2024 10:11:55 -0700 Subject: [PATCH 09/34] Bugfix: renderer highlights lines when at x-boundary --- src/inc/til/point.h | 36 ++++++++++++++++++++++++++++++++++ src/renderer/base/renderer.cpp | 2 +- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/inc/til/point.h b/src/inc/til/point.h index 17bc9d1c861..95eed691671 100644 --- a/src/inc/til/point.h +++ b/src/inc/til/point.h @@ -322,6 +322,42 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned" func(y, x1, x2); } } + + // Similar to iterate_rows above, but the "end" point is exclusive + // and RightExclusive (aka "width") is an allowable coordinate + constexpr void iterate_rows_exclusive(til::CoordType width, auto&& func) const + { + // Copy the members so that the compiler knows it doesn't + // need to re-read them on every loop iteration. + const auto w = width; + auto ax = std::clamp(start.x, 0, w); + auto ay = start.y; + auto bx = std::clamp(end.x, 0, w); + auto by = end.y; + + // if start is at RightExclusive, + // treat it as (0, y+1) (left-most point on next line) + if (ax == w) + { + ay++; + ax = 0; + } + + // if end is on left boundary, + // treat it as (w, y-1) (RightExclusive on previous line) + if (bx == 0) + { + by--; + bx = w; + } + + for (auto y = ay; y <= by; ++y) + { + const auto x1 = y != ay ? 0 : ax; + const auto x2 = y != by ? w : bx; + func(y, x1, x2); + } + } }; } diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index c7fbec88bfc..31ab5a410e3 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -342,7 +342,7 @@ try const til::rect vp{ _viewport.ToExclusive() }; for (auto&& sp : spans) { - sp.iterate_rows(bufferWidth, [&](til::CoordType row, til::CoordType min, til::CoordType max) { + sp.iterate_rows_exclusive(bufferWidth, [&](til::CoordType row, til::CoordType min, til::CoordType max) { const auto shift = buffer.GetLineRendition(row) != LineRendition::SingleWidth ? 1 : 0; min <<= shift; max <<= shift; From 7ff4838388eb667bd4a5dcd9cbcbc3e5a5d898b1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 18 Oct 2024 11:23:39 -0700 Subject: [PATCH 10/34] Bugfix: move to next word would move past mutable bottom --- src/cascadia/TerminalCore/TerminalSelection.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index e59c3f5266e..f9d7eefcbd5 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -757,8 +757,12 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos) { // increment one more because end is exclusive const auto& buffer = _activeBuffer(); - pos = buffer.GetWordEnd2(pos, _wordDelimiters); - buffer.GetSize().IncrementInExclusiveBounds(pos); + const auto mutableViewportEndExclusive = _GetMutableViewport().BottomInclusiveRightExclusive(); + pos = buffer.GetWordEnd2(pos, _wordDelimiters, mutableViewportEndExclusive); + if (pos != mutableViewportEndExclusive) + { + buffer.GetSize().IncrementInExclusiveBounds(pos); + } break; } case SelectionDirection::Up: From 928e9a9fac448f37500ffcaf75c1674d8927d58a Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 21 Oct 2024 15:04:53 -0700 Subject: [PATCH 11/34] Fix some more rendering issues --- src/buffer/out/Row.cpp | 5 ++++- src/buffer/out/textBuffer.cpp | 6 +++--- src/cascadia/TerminalCore/TerminalSelection.cpp | 4 ++-- src/renderer/atlas/AtlasEngine.cpp | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 4c4b6621464..6c72a74d0e4 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -1114,11 +1114,14 @@ std::wstring_view ROW::GetText() const noexcept return { _chars.data(), width }; } +// Arguments: +// - columnBegin: inclusive +// - columnEnd: exclusive std::wstring_view ROW::GetText(til::CoordType columnBegin, til::CoordType columnEnd) const noexcept { const auto columns = GetReadableColumnCount(); const auto colBeg = clamp(columnBegin, 0, columns); - const auto colEnd = clamp(columnEnd - 1, colBeg, columns); + const auto colEnd = clamp(columnEnd, colBeg, columns); const size_t chBeg = _uncheckedCharOffset(gsl::narrow_cast(colBeg)); const size_t chEnd = _uncheckedCharOffset(gsl::narrow_cast(colEnd)); #pragma warning(suppress : 26481) // Don't use pointer arithmetic. Use span instead (bounds.1). diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 536b2c30238..659f7a8bae4 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2011,8 +2011,8 @@ size_t TextBuffer::SpanLength(const til::point coordStart, const til::point coor // - Retrieves the plain text data between the specified coordinates. // Arguments: // - trimTrailingWhitespace - remove the trailing whitespace at the end of the result. -// - start - where to start getting text (should be at or prior to "end") -// - end - where to end getting text +// - start - where to start getting text (should be at or prior to "end") (inclusive) +// - end - where to end getting text (exclusive) // Return Value: // - Just the text. std::wstring TextBuffer::GetPlainText(const til::point start, const til::point end) const @@ -2050,7 +2050,7 @@ std::tuple TextBuffer::_RowCopyHelper(cons const auto end = req.bufferCoordinates ? req.end : ScreenToBufferLineInclusive(req.end, lineRendition); rowBeg = iRow != beg.y ? 0 : beg.x; - rowEnd = iRow != end.y ? row.GetReadableColumnCount() : end.x + 1; // +1 to get an exclusive end + rowEnd = iRow != end.y ? row.GetReadableColumnCount(): end.x; } // Our selection mechanism doesn't stick to glyph boundaries at the moment. diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index f9d7eefcbd5..56311387d79 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -767,11 +767,11 @@ void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos) } case SelectionDirection::Up: _MoveByChar(direction, pos); - pos = _activeBuffer().GetWordStart(pos, _wordDelimiters); + pos = _activeBuffer().GetWordStart2(pos, _wordDelimiters); break; case SelectionDirection::Down: _MoveByChar(direction, pos); - pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters); + pos = _activeBuffer().GetWordEnd2(pos, _wordDelimiters); break; } } diff --git a/src/renderer/atlas/AtlasEngine.cpp b/src/renderer/atlas/AtlasEngine.cpp index b6b0274c987..407353f8d6d 100644 --- a/src/renderer/atlas/AtlasEngine.cpp +++ b/src/renderer/atlas/AtlasEngine.cpp @@ -427,7 +427,7 @@ try if (y > hiStart.y) { const auto isFinalRow = y == hiEnd.y; - const auto end = isFinalRow ? std::min(hiEnd.x + 1, x2) : x2; + const auto end = isFinalRow ? std::min(hiEnd.x, x2) : x2; _fillColorBitmap(row, x1, end, fgColor, bgColor); // Return early if we couldn't paint the whole region (either this was not the last row, or From 9d361d2c33e5bb87b283be70bfbed70a562c5018 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 21 Oct 2024 16:14:09 -0700 Subject: [PATCH 12/34] Fix more rendering issues w/ Leonard --- src/buffer/out/UTextAdapter.cpp | 5 +---- src/renderer/atlas/AtlasEngine.api.cpp | 4 ++-- src/renderer/atlas/AtlasEngine.cpp | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/buffer/out/UTextAdapter.cpp b/src/buffer/out/UTextAdapter.cpp index ff0861ce54d..a76d2aa55e9 100644 --- a/src/buffer/out/UTextAdapter.cpp +++ b/src/buffer/out/UTextAdapter.cpp @@ -417,10 +417,7 @@ til::point_span Microsoft::Console::ICU::BufferRangeFromMatch(UText* ut, URegula { UErrorCode status = U_ZERO_ERROR; const auto nativeIndexBeg = uregex_start64(re, 0, &status); - auto nativeIndexEnd = uregex_end64(re, 0, &status); - - // The parameters are given as a half-open [beg,end) range, but the point_span we return in closed [beg,end]. - nativeIndexEnd--; + const auto nativeIndexEnd = uregex_end64(re, 0, &status); const auto& textBuffer = *static_cast(ut->context); til::point_span ret; diff --git a/src/renderer/atlas/AtlasEngine.api.cpp b/src/renderer/atlas/AtlasEngine.api.cpp index a80ae930ba9..10cf2e3a405 100644 --- a/src/renderer/atlas/AtlasEngine.api.cpp +++ b/src/renderer/atlas/AtlasEngine.api.cpp @@ -89,11 +89,11 @@ void AtlasEngine::_invalidateSpans(std::span spans, const const auto viewport = til::rect{ 0, 0, _api.s->viewportCellCount.x, _api.s->viewportCellCount.y }; for (auto&& sp : spans) { - sp.iterate_rows(til::CoordTypeMax, [&](til::CoordType row, til::CoordType beg, til::CoordType end) { + sp.iterate_rows_exclusive(til::CoordTypeMax, [&](til::CoordType row, til::CoordType beg, til::CoordType end) { const auto shift = buffer.GetLineRendition(row) != LineRendition::SingleWidth ? 1 : 0; beg <<= shift; end <<= shift; - til::rect rect{ beg, row, end + 1, row + 1 }; + til::rect rect{ beg, row, end, row + 1 }; rect = rect.to_origin(viewportOrigin); rect &= viewport; _api.invalidatedRows.start = gsl::narrow_cast(std::min(_api.invalidatedRows.start, std::max(0, rect.top))); diff --git a/src/renderer/atlas/AtlasEngine.cpp b/src/renderer/atlas/AtlasEngine.cpp index 407353f8d6d..ac1cdc6a6f1 100644 --- a/src/renderer/atlas/AtlasEngine.cpp +++ b/src/renderer/atlas/AtlasEngine.cpp @@ -433,7 +433,7 @@ try // Return early if we couldn't paint the whole region (either this was not the last row, or // it was the last row but the highlight ends outside of our x range.) // We will resume from here in the next call. - if (!isFinalRow || hiEnd.x /*inclusive*/ >= x2 /*exclusive*/) + if (!isFinalRow || hiEnd.x /*inclusive*/ > x2 /*exclusive*/) { return S_OK; } From 845e43d147cf4f68bb35ffa0b4a5d5e0acfd7573 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 22 Oct 2024 16:43:27 +0200 Subject: [PATCH 13/34] Fix highlighting in AtlasEngine --- src/buffer/out/textBuffer.cpp | 2 +- src/renderer/atlas/AtlasEngine.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 659f7a8bae4..eb09f4d5583 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2050,7 +2050,7 @@ std::tuple TextBuffer::_RowCopyHelper(cons const auto end = req.bufferCoordinates ? req.end : ScreenToBufferLineInclusive(req.end, lineRendition); rowBeg = iRow != beg.y ? 0 : beg.x; - rowEnd = iRow != end.y ? row.GetReadableColumnCount(): end.x; + rowEnd = iRow != end.y ? row.GetReadableColumnCount() : end.x; } // Our selection mechanism doesn't stick to glyph boundaries at the moment. diff --git a/src/renderer/atlas/AtlasEngine.cpp b/src/renderer/atlas/AtlasEngine.cpp index ac1cdc6a6f1..90b9083b5d3 100644 --- a/src/renderer/atlas/AtlasEngine.cpp +++ b/src/renderer/atlas/AtlasEngine.cpp @@ -433,7 +433,7 @@ try // Return early if we couldn't paint the whole region (either this was not the last row, or // it was the last row but the highlight ends outside of our x range.) // We will resume from here in the next call. - if (!isFinalRow || hiEnd.x /*inclusive*/ > x2 /*exclusive*/) + if (!isFinalRow || hiEnd.x > x2) { return S_OK; } @@ -448,7 +448,7 @@ try hiEnd = it->end - offset; const auto isStartInside = y == hiStart.y && hiStart.x < x2; - const auto isEndInside = y == hiEnd.y && hiEnd.x < x2; + const auto isEndInside = y == hiEnd.y && hiEnd.x <= x2; if (isStartInside && isEndInside) { _fillColorBitmap(row, hiStart.x, static_cast(hiEnd.x), fgColor, bgColor); From 8b996a96e97bee8599f9fe4f0289d98b815f71fc Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 22 Oct 2024 10:51:43 -0700 Subject: [PATCH 14/34] fix session restore --- src/buffer/out/textBuffer.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index eb09f4d5583..d2ce5101844 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2787,9 +2787,7 @@ void TextBuffer::Serialize(const wchar_t* destination) const newX = lastCharX; } - // GetText() has an exclusive end, - // add 1 to newX to get the correct range - buffer.append(row.GetText(oldX, newX + 1)); + buffer.append(row.GetText(oldX, newX)); if (clearToEndOfLine) { From df89ad0e861ac697d930e3e2c2e706f1ac7871c6 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 23 Oct 2024 09:31:19 -0700 Subject: [PATCH 15/34] Ensure mouse selection scenarios work right --- src/buffer/out/textBuffer.cpp | 81 ++++++++++++++----- src/buffer/out/textBuffer.hpp | 1 + src/cascadia/TerminalControl/ControlCore.cpp | 4 +- .../TerminalControl/ControlInteractivity.cpp | 26 ++++-- .../TerminalControl/ControlInteractivity.h | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 2 +- src/cascadia/TerminalCore/Terminal.hpp | 2 +- .../TerminalCore/TerminalSelection.cpp | 63 ++++++++++----- 8 files changed, 131 insertions(+), 50 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index d2ce5101844..794761c5c8f 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1196,12 +1196,12 @@ til::point TextBuffer::GetWordEnd2(til::point pos, const std::wstring_view wordD // - DelimiterChar: "D" // - RegularChar: "C" // Expected results ("|" is the position): - // |CCC___ --> CCC__|_ - // |DDD___ --> DDD__|_ - // |___CCC --> __|_CCC - // |DDDCCC --> DD|DCCC - // |___DDD --> __|_DDD - // |CCCDDD --> CC|CDDD + // |CCC___ --> CCC___| + // |DDD___ --> DDD___| + // |___CCC --> ___|CCC + // |DDDCCC --> DDD|CCC + // |___DDD --> ___|DDD + // |CCCDDD --> CCC|DDD // So the heuristic we use is: // 1. move to the end of the delimiter class run // 2. if the next delimiter class run is a ControlChar, go forward one more delimiter class run @@ -1214,21 +1214,67 @@ til::point TextBuffer::GetWordEnd2(til::point pos, const std::wstring_view wordD return pos; } - auto nextPos = pos; - bufferSize.IncrementInExclusiveBounds(nextPos); - if (const auto nextDelimClass = bufferSize.IsInBounds(nextPos) ? _GetDelimiterClassAt(nextPos, wordDelimiters) : DelimiterClass::ControlChar; + if (const auto nextDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; nextDelimClass == DelimiterClass::ControlChar) { - return _GetDelimiterClassRunEnd(nextPos, wordDelimiters); + return _GetDelimiterClassRunEnd(pos, wordDelimiters); } return pos; } +bool TextBuffer::IsWordBoundary(const til::point pos, const std::wstring_view wordDelimiters) const +{ + const auto bufferSize = GetSize(); + if (!bufferSize.IsInExclusiveBounds(pos)) + { + // not in bounds + return false; + } + + // buffer boundaries are always word boundaries + if (pos == bufferSize.Origin() || pos == bufferSize.BottomInclusiveRightExclusive()) + { + return true; + } + + // at beginning of the row, but we didn't wrap + if (pos.x == bufferSize.Left()) + { + const auto& row = GetRowByOffset(pos.y - 1); + if (!row.WasWrapForced()) + { + return true; + } + } + + // at end of the row, but we didn't wrap + if (pos.x == bufferSize.RightExclusive()) + { + const auto& row = GetRowByOffset(pos.y); + if (!row.WasWrapForced()) + { + return true; + } + } + + // we can treat text as contiguous, + // use DecrementInBounds (not exclusive) here + auto prevPos = pos; + bufferSize.DecrementInBounds(prevPos); + const auto prevDelimiterClass = _GetDelimiterClassAt(prevPos, wordDelimiters); + + // if we changed delimiter class + // and the current delimiter class is not a control char, + // we're at a word boundary + const auto currentDelimiterClass = _GetDelimiterClassAt(pos, wordDelimiters); + return prevDelimiterClass != currentDelimiterClass && currentDelimiterClass != DelimiterClass::ControlChar; +} + til::point TextBuffer::_GetDelimiterClassRunStart(til::point pos, const std::wstring_view wordDelimiters) const { const auto bufferSize = GetSize(); const auto initialDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; - for (auto nextPos = pos; nextPos != bufferSize.Origin();) + for (auto nextPos = pos; nextPos != bufferSize.Origin(); pos = nextPos) { bufferSize.DecrementInExclusiveBounds(nextPos); @@ -1247,9 +1293,6 @@ til::point TextBuffer::_GetDelimiterClassRunStart(til::point pos, const std::wst // if we changed delim class, we're done (don't apply move) return pos; } - - // apply the move - pos = nextPos; } return pos; } @@ -1258,7 +1301,7 @@ til::point TextBuffer::_GetDelimiterClassRunEnd(til::point pos, const std::wstri { const auto bufferSize = GetSize(); const auto initialDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; - for (auto nextPos = pos; nextPos != bufferSize.BottomInclusiveRightExclusive();) + for (auto nextPos = pos; nextPos != bufferSize.BottomInclusiveRightExclusive(); pos = nextPos) { bufferSize.IncrementInExclusiveBounds(nextPos); @@ -1274,12 +1317,10 @@ til::point TextBuffer::_GetDelimiterClassRunEnd(til::point pos, const std::wstri } else if (bufferSize.IsInBounds(nextPos) && _GetDelimiterClassAt(nextPos, wordDelimiters) != initialDelimClass) { - // if we changed delim class, we're done (don't apply move) - return pos; + // if we changed delim class, + // apply the move and return + return nextPos; } - - // apply the move - pos = nextPos; } return pos; } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index fef7228e101..22dc8b80ffd 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -175,6 +175,7 @@ class TextBuffer final til::point GetWordEnd(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; til::point GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; til::point GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; + bool IsWordBoundary(const til::point pos, const std::wstring_view wordDelimiters) const; bool MoveToNextWord(til::point& pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; bool MoveToPreviousWord(til::point& pos, const std::wstring_view wordDelimiters) const; diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 208517bcf03..ce89c251496 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1217,8 +1217,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation return; } + // clamp the converted position to be within the viewport bounds + // x: allow range of [-1, RightExclusive]. til::point terminalPosition{ - std::clamp(position.x, 0, _terminal->GetViewport().Width() - 1), + std::clamp(position.x, -1, _terminal->GetViewport().Width()), std::clamp(position.y, 0, _terminal->GetViewport().Height() - 1) }; diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index f18851c2fbb..b07b881f887 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -239,7 +239,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const Core::Point pixelPosition) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, false); const auto altEnabled = modifiers.IsAltPressed(); const auto shiftEnabled = modifiers.IsShiftPressed(); @@ -336,7 +336,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const Core::Point pixelPosition, const bool pointerPressedInBounds) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, true); // Returning true from this function indicates that the caller should do no further processing of this movement. bool handledCompletely = false; @@ -370,7 +370,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // _touchdown_ point here. We want to start the selection // from where the user initially clicked, not where they are // now. - _core->SetSelectionAnchor(_getTerminalPosition(til::point{ touchdownPoint })); + _core->SetSelectionAnchor(_getTerminalPosition(til::point{ touchdownPoint }, false)); // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; @@ -426,7 +426,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const Core::Point pixelPosition) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, false); // Short-circuit isReadOnly check to avoid warning dialog if (!_core->IsInReadOnlyMode() && _canSendVTMouseInput(modifiers)) { @@ -473,7 +473,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const Core::Point pixelPosition, const Control::MouseButtonState buttonState) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, true); // Short-circuit isReadOnly check to avoid warning dialog. // @@ -660,7 +660,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // - cursorPosition: in pixels, relative to the origin of the control void ControlInteractivity::SetEndSelectionPoint(const Core::Point pixelPosition) { - _core->SetEndSelectionPoint(_getTerminalPosition(til::point{ pixelPosition })); + _core->SetEndSelectionPoint(_getTerminalPosition(til::point{ pixelPosition }, true)); _selectionNeedsToBeCopied = true; } @@ -670,12 +670,22 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Arguments: // - pixelPosition: the (x,y) position of a given point (i.e.: mouse cursor). // NOTE: origin (0,0) is top-left. + // - roundToNearestCell: if true, round the x-value. Otherwise, floor it (standard int division) // Return Value: // - the corresponding viewport terminal position for the given Point parameter - til::point ControlInteractivity::_getTerminalPosition(const til::point pixelPosition) + til::point ControlInteractivity::_getTerminalPosition(const til::point pixelPosition, bool roundToNearestCell) { // Get the size of the font, which is in pixels - const til::size fontSize{ _core->GetFont().GetSize() }; + const auto fontSize{ _core->GetFont().GetSize() }; + + if (roundToNearestCell) + { + // GH#5099: round the x-value to the nearest cell + til::point result; + result.x = gsl::narrow_cast(std::round(gsl::narrow_cast(pixelPosition.x) / fontSize.width)); + result.y = pixelPosition.y / fontSize.height; + return result; + } // Convert the location in pixels to characters within the current viewport. return pixelPosition / fontSize; } diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index 22eaa95c397..70b4de09ff5 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -155,7 +155,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation bool _canSendVTMouseInput(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers); bool _shouldSendAlternateScroll(const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const int32_t delta); - til::point _getTerminalPosition(const til::point pixelPosition); + til::point _getTerminalPosition(const til::point pixelPosition, bool roundToNearestCell); bool _sendMouseEventHelper(const til::point terminalPosition, const unsigned int pointerUpdateKind, diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index c61c339b79e..2a6553922ea 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -543,7 +543,7 @@ std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos) // - The hyperlink ID uint16_t Terminal::GetHyperlinkIdAtViewportPosition(const til::point viewportPos) { - return _activeBuffer().GetCellDataAt(_ConvertToBufferCell(viewportPos))->TextAttr().GetHyperlinkId(); + return _activeBuffer().GetCellDataAt(_ConvertToBufferCell(viewportPos, false))->TextAttr().GetHyperlinkId(); } // Method description: diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 7fdf0995013..1fcbeec8ffe 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -477,7 +477,7 @@ class Microsoft::Terminal::Core::Terminal final : std::vector _GetSelectionSpans() const noexcept; std::pair _PivotSelection(const til::point targetPos, bool& targetStart) const noexcept; std::pair _ExpandSelectionAnchors(std::pair anchors) const; - til::point _ConvertToBufferCell(const til::point viewportPos) const; + til::point _ConvertToBufferCell(const til::point viewportPos, bool allowRightExclusive = false) const; void _ScrollToPoint(const til::point pos); void _MoveByChar(SelectionDirection direction, til::point& pos); void _MoveByWord(SelectionDirection direction, til::point& pos); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 56311387d79..5a5bfce7f9a 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -171,9 +171,10 @@ void Terminal::MultiClickSelection(const til::point viewportPos, SelectionExpans auto selection{ _selection.write() }; wil::hide_name _selection; - selection->pivot = _ConvertToBufferCell(viewportPos); + selection->pivot = _ConvertToBufferCell(viewportPos, true); selection->active = true; + // TODO CARLOS: validate _multiClickSelectionMode = expansionMode; SetSelectionEnd(viewportPos); @@ -193,9 +194,10 @@ void Terminal::SetSelectionAnchor(const til::point viewportPos) auto selection{ _selection.write() }; wil::hide_name _selection; - selection->pivot = _ConvertToBufferCell(viewportPos); + selection->pivot = _ConvertToBufferCell(viewportPos, true); selection->active = true; + // TODO CARLOS: validate _multiClickSelectionMode = SelectionExpansion::Char; SetSelectionEnd(viewportPos); @@ -212,7 +214,7 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional newExpansionMode) { wil::hide_name _selection; @@ -223,7 +225,12 @@ void Terminal::_SetSelectionEnd(SelectionInfo* selection, const til::point viewp return; } - const auto textBufferPos = _ConvertToBufferCell(viewportPos); + auto textBufferPos = _ConvertToBufferCell(viewportPos, true); + if (newExpansionMode && *newExpansionMode == SelectionExpansion::Char && textBufferPos >= selection->pivot) + { + // Shift+Click forwards should highlight the clicked space + _activeBuffer().GetSize().IncrementInExclusiveBounds(textBufferPos); + } // if this is a shiftClick action, we need to overwrite the _multiClickSelectionMode value (even if it's the same) // Otherwise, we may accidentally expand during other selection-based actions @@ -287,17 +294,29 @@ std::pair Terminal::_ExpandSelectionAnchors(std::pair Date: Wed, 23 Oct 2024 10:34:36 -0700 Subject: [PATCH 16/34] Fix UIA --- src/buffer/out/textBuffer.cpp | 8 ++++++++ src/buffer/out/textBuffer.hpp | 1 + src/cascadia/TerminalControl/ControlCore.cpp | 1 - src/cascadia/TerminalCore/TerminalSelection.cpp | 3 --- src/cascadia/TerminalCore/terminalrenderdata.cpp | 1 + src/types/TermControlUiaProvider.cpp | 8 +------- src/types/UiaTextRangeBase.cpp | 8 ++------ 7 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 794761c5c8f..8698980ae45 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1118,6 +1118,14 @@ void TextBuffer::TriggerNewTextNotification(const std::wstring_view newText) } } +void TextBuffer::TriggerSelection() +{ + if (_isActiveBuffer && _renderer) + { + _renderer->TriggerSelection(); + } +} + // Method Description: // - get delimiter class for buffer cell position // - used for double click selection and uia word navigation diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 22dc8b80ffd..3425ea6fbf2 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -170,6 +170,7 @@ class TextBuffer final void TriggerScroll(); void TriggerScroll(const til::point delta); void TriggerNewTextNotification(const std::wstring_view newText); + void TriggerSelection(); til::point GetWordStart(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; til::point GetWordEnd(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index ce89c251496..caa45430617 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -2716,7 +2716,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation bufferSize.DecrementInBounds(inclusiveEnd); _terminal->SelectNewRegion(s.start, inclusiveEnd); - _renderer->TriggerSelection(); } void ControlCore::SelectCommand(const bool goUp) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 5a5bfce7f9a..70d0b7ca08b 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -174,7 +174,6 @@ void Terminal::MultiClickSelection(const til::point viewportPos, SelectionExpans selection->pivot = _ConvertToBufferCell(viewportPos, true); selection->active = true; - // TODO CARLOS: validate _multiClickSelectionMode = expansionMode; SetSelectionEnd(viewportPos); @@ -197,7 +196,6 @@ void Terminal::SetSelectionAnchor(const til::point viewportPos) selection->pivot = _ConvertToBufferCell(viewportPos, true); selection->active = true; - // TODO CARLOS: validate _multiClickSelectionMode = SelectionExpansion::Char; SetSelectionEnd(viewportPos); @@ -916,7 +914,6 @@ Terminal::TextCopyData Terminal::RetrieveSelectedTextFromBuffer(const bool singl // - the corresponding location on the buffer til::point Terminal::_ConvertToBufferCell(const til::point viewportPos, bool allowRightExclusive) const { - // TODO CARLOS: this is a dangerous change. Validate side-effects! const auto yPos = _VisibleStartIndex() + viewportPos.y; til::point bufferPos = { viewportPos.x, yPos }; const auto bufferSize = _activeBuffer().GetSize(); diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index 006d87aabe5..c381d478451 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -210,6 +210,7 @@ void Terminal::SelectNewRegion(const til::point coordStart, const til::point coo const auto newCoordEnd = til::point{ coordEnd.x, coordEnd.y - newScrollOffset }; SetSelectionAnchor(newCoordStart); SetSelectionEnd(newCoordEnd, SelectionExpansion::Char); + _activeBuffer().TriggerSelection(); } const std::wstring_view Terminal::GetConsoleTitle() const noexcept diff --git a/src/types/TermControlUiaProvider.cpp b/src/types/TermControlUiaProvider.cpp index 3cfc041f5da..caf842ad9b3 100644 --- a/src/types/TermControlUiaProvider.cpp +++ b/src/types/TermControlUiaProvider.cpp @@ -160,14 +160,8 @@ HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr); *ppUtr = nullptr; - const auto start = _pData->GetSelectionAnchor(); - - // we need to make end exclusive - auto end = _pData->GetSelectionEnd(); - _pData->GetTextBuffer().GetSize().IncrementInBounds(end, true); - TermControlUiaTextRange* result = nullptr; - RETURN_IF_FAILED(MakeAndInitialize(&result, _pData, pProvider, start, end, _pData->IsBlockSelection(), wordDelimiters)); + RETURN_IF_FAILED(MakeAndInitialize(&result, _pData, pProvider, _pData->GetSelectionAnchor(), _pData->GetSelectionEnd(), _pData->IsBlockSelection(), wordDelimiters)); *ppUtr = result; return S_OK; } diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 0ce958e6985..857cf32c71e 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -987,13 +987,9 @@ std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const // TODO GH#5406: create a different UIA parent object for each TextBuffer // nvaccess/nvda#11428: Ensure our endpoints are in bounds - THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start, true) || !bufferSize.IsInBounds(_end, true)); + THROW_HR_IF(E_FAIL, !bufferSize.IsInExclusiveBounds(_start) || !bufferSize.IsInExclusiveBounds(_end)); - // convert _end to be inclusive - auto inclusiveEnd = _end; - bufferSize.DecrementInBounds(inclusiveEnd, true); - - const auto req = TextBuffer::CopyRequest{ buffer, _start, inclusiveEnd, _blockRange, true, false, false, true }; + const auto req = TextBuffer::CopyRequest{ buffer, _start, _end, _blockRange, true, false, false, true }; auto plainText = buffer.GetPlainText(req); if (plainText.size() > maxLengthAsSize) From eea839990833f8eebfbc795c2e61a30814ff2dbd Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 23 Oct 2024 12:27:52 -0700 Subject: [PATCH 17/34] Fix hyperlinks --- src/cascadia/TerminalCore/Terminal.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 2a6553922ea..4629081e3b3 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -477,7 +477,7 @@ bool Terminal::ShouldSendAlternateScroll(const unsigned int uiButton, // - The position relative to the viewport std::wstring Terminal::GetHyperlinkAtViewportPosition(const til::point viewportPos) { - return GetHyperlinkAtBufferPosition(_ConvertToBufferCell(viewportPos)); + return GetHyperlinkAtBufferPosition(_ConvertToBufferCell(viewportPos, false)); } std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos) @@ -501,12 +501,8 @@ std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos) result = GetHyperlinkIntervalFromViewportPosition(viewportPos); if (result.has_value()) { - // GetPlainText and _ConvertToBufferCell work with inclusive coordinates, but interval's - // stop point is (horizontally) exclusive, so let's just update it. - result->stop.x--; - - result->start = _ConvertToBufferCell(result->start); - result->stop = _ConvertToBufferCell(result->stop); + result->start = _ConvertToBufferCell(result->start, false); + result->stop = _ConvertToBufferCell(result->stop, false); } } else @@ -1465,7 +1461,6 @@ PointTree Terminal::_getPatterns(til::CoordType beg, til::CoordType end) const // PointTree uses half-open ranges and viewport-relative coordinates. range.start.y -= beg; range.end.y -= beg; - range.end.x++; intervals.push_back(PointTree::interval(range.start, range.end, 0)); } while (uregex_findNext(re.get(), &status)); } From 7bfd647ca5a9adb117a74b023ea81bf3605f9779 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 23 Oct 2024 17:02:39 -0700 Subject: [PATCH 18/34] fix tests --- src/buffer/out/textBuffer.cpp | 9 +- .../UnitTests_TerminalCore/SelectionTest.cpp | 129 +++++++++--------- 2 files changed, 73 insertions(+), 65 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 8698980ae45..ce12407efc9 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1173,7 +1173,14 @@ til::point TextBuffer::GetWordStart2(til::point pos, const std::wstring_view wor // 2. if we were on a ControlChar, go back one more delimiter class run const auto initialDelimiter = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; pos = _GetDelimiterClassRunStart(pos, wordDelimiters); - if (initialDelimiter == DelimiterClass::ControlChar) + if (pos.x == bufferSize.Left()) + { + // Special case: + // we're at the left boundary (and end of a delimiter class run), + // we already know we can't wrap, so return early + return pos; + } + else if (initialDelimiter == DelimiterClass::ControlChar) { bufferSize.DecrementInExclusiveBounds(pos); pos = _GetDelimiterClassRunStart(pos, wordDelimiters); diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp index 11f01c75297..339c2927ff2 100644 --- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp @@ -126,20 +126,20 @@ namespace TerminalCoreUnitTests // Test with no scrollback Log::Comment(L"Single click selection with NO scrollback value"); - ValidateSingleClickSelection(0, { 9, 9 }, { 9, 9 }); + ValidateSingleClickSelection(0, { 10, 9 }, { 10, 9 }); Log::Comment(L"Double click selection with NO scrollback value"); - ValidateDoubleClickSelection(0, { 0, 9 }, { 9, 9 }); + ValidateDoubleClickSelection(0, { 0, 9 }, { 10, 9 }); Log::Comment(L"Triple click selection with NO scrollback value"); - ValidateTripleClickSelection(0, { 0, 9 }, { 9, 9 }); + ValidateTripleClickSelection(0, { 0, 9 }, { 10, 9 }); // Test with max scrollback const til::CoordType expected_row = SHRT_MAX - 1; Log::Comment(L"Single click selection with MAXIMUM scrollback value"); - ValidateSingleClickSelection(SHRT_MAX, { 9, expected_row }, { 9, expected_row }); + ValidateSingleClickSelection(SHRT_MAX, { 10, expected_row }, { 10, expected_row }); Log::Comment(L"Double click selection with MAXIMUM scrollback value"); - ValidateDoubleClickSelection(SHRT_MAX, { 0, expected_row }, { 9, expected_row }); + ValidateDoubleClickSelection(SHRT_MAX, { 0, expected_row }, { 10, expected_row }); Log::Comment(L"Triple click selection with MAXIMUM scrollback value"); - ValidateTripleClickSelection(SHRT_MAX, { 0, expected_row }, { 9, expected_row }); + ValidateTripleClickSelection(SHRT_MAX, { 0, expected_row }, { 10, expected_row }); } TEST_METHOD(SelectFromOutofBounds) @@ -155,7 +155,7 @@ namespace TerminalCoreUnitTests auto viewport = term.GetViewport(); const auto leftBoundary = viewport.Left(); - const auto rightBoundary = viewport.RightInclusive(); + const auto rightExclusiveBoundary = viewport.RightExclusive(); const auto topBoundary = viewport.Top(); const auto bottomBoundary = viewport.BottomInclusive(); @@ -163,7 +163,7 @@ namespace TerminalCoreUnitTests // should clamp to right boundary term.SetSelectionAnchor({ 20, 5 }); Log::Comment(L"Out of bounds: X-value too large"); - ValidateLinearSelection(term, { rightBoundary, 5 }, { rightBoundary, 5 }); + ValidateLinearSelection(term, { rightExclusiveBoundary, 5 }, { rightExclusiveBoundary, 5 }); // Case 2: Simulate click past left (x,y) = (-20,5) // should clamp to left boundary @@ -197,7 +197,7 @@ namespace TerminalCoreUnitTests auto viewport = term.GetViewport(); const til::CoordType leftBoundary = 0; - const auto rightBoundary = viewport.RightInclusive(); + const auto rightExclusiveBoundary = viewport.RightExclusive(); // Simulate click at (x,y) = (5,5) term.SetSelectionAnchor({ 5, 5 }); @@ -205,7 +205,7 @@ namespace TerminalCoreUnitTests // Case 1: Move out of right boundary Log::Comment(L"Out of bounds: X-value too large"); term.SetSelectionEnd({ 20, 5 }); - ValidateLinearSelection(term, { 5, 5 }, { rightBoundary, 5 }); + ValidateLinearSelection(term, { 5, 5 }, { rightExclusiveBoundary, 5 }); // Case 2: Move out of left boundary Log::Comment(L"Out of bounds: X-value negative"); @@ -312,7 +312,7 @@ namespace TerminalCoreUnitTests // Validate selection area // Selection should expand one to the left to get the leading half of the wide glyph - ValidateLinearSelection(term, { 4, 10 }, { 5, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 6, 10 }); } TEST_METHOD(SelectWideGlyph_Leading) @@ -334,8 +334,8 @@ namespace TerminalCoreUnitTests term.SetSelectionAnchor(clickPos); // Validate selection area - // Selection should expand one to the left to get the leading half of the wide glyph - ValidateLinearSelection(term, { 4, 10 }, { 5, 10 }); + // Selection should clamp to the left side of the glyph and stay degenerate + ValidateLinearSelection(term, { 4, 10 }, { 4, 10 }); } TEST_METHOD(SelectWideGlyphsInBoxSelection) @@ -381,7 +381,7 @@ namespace TerminalCoreUnitTests else if (rowValue == 11) { VERIFY_ARE_EQUAL((til::point{ 5, rowValue }), sp.start); - VERIFY_ARE_EQUAL((til::point{ 8, rowValue }), sp.end); + VERIFY_ARE_EQUAL((til::point{ 7, rowValue }), sp.end); } else { @@ -414,7 +414,7 @@ namespace TerminalCoreUnitTests term.MultiClickSelection(clickPos, Terminal::SelectionExpansion::Word); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size() - 1), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); } TEST_METHOD(DoubleClick_Delimiter) @@ -432,7 +432,7 @@ namespace TerminalCoreUnitTests term.MultiClickSelection(clickPos, Terminal::SelectionExpansion::Word); // Validate selection area - ValidateLinearSelection(term, { 0, 10 }, { 99, 10 }); + ValidateLinearSelection(term, { 0, 10 }, { term.GetViewport().RightExclusive(), 10 }); } TEST_METHOD(DoubleClick_DelimiterClass) @@ -457,10 +457,10 @@ namespace TerminalCoreUnitTests // ---Validate selection area--- // "Terminal" is in class 2 - // ">" is in class 1 + // ":" and ">" are in class 1 // the white space to the right of the ">" is in class 0 - // Double-clicking the ">" should only highlight that cell - ValidateLinearSelection(term, { 15, 10 }, { 15, 10 }); + // Double-clicking the ">" should highlight that cell and the whitespace + ValidateLinearSelection(term, { 15, 10 }, { term.GetViewport().RightExclusive(), 10 }); } TEST_METHOD(DoubleClickDrag_Right) @@ -489,7 +489,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); } TEST_METHOD(DoubleClickDrag_Left) @@ -502,7 +502,7 @@ namespace TerminalCoreUnitTests auto settings = winrt::make(0, 100, 100); term.UpdateSettings(settings); - // Insert text at position (21,10) + // Insert text at position (4,10) const std::wstring_view text = L"doubleClickMe dragThroughHere"; GetTextBuffer(term).GetCursor().SetPosition({ 4, 10 }); term.Write(text); @@ -513,12 +513,12 @@ namespace TerminalCoreUnitTests // Simulate move to (x,y) = (5,10) // // buffer: doubleClickMe dragThroughHere - // ^ ^ - // finish start + // ^ ^ + // finish start term.SetSelectionEnd({ 5, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 18, 10 }); } TEST_METHOD(TripleClick_GeneralCase) @@ -532,7 +532,7 @@ namespace TerminalCoreUnitTests term.MultiClickSelection(clickPos, Terminal::SelectionExpansion::Line); // Validate selection area - ValidateLinearSelection(term, { 0, 10 }, { 99, 10 }); + ValidateLinearSelection(term, { 0, 10 }, { term.GetViewport().RightExclusive(), 10 }); } TEST_METHOD(TripleClickDrag_Horizontal) @@ -549,7 +549,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 7, 10 }); // Validate selection area - ValidateLinearSelection(term, { 0, 10 }, { 99, 10 }); + ValidateLinearSelection(term, { 0, 10 }, { term.GetViewport().RightExclusive(), 10 }); } TEST_METHOD(TripleClickDrag_Vertical) @@ -565,7 +565,7 @@ namespace TerminalCoreUnitTests // Simulate move to (x,y) = (5,11) term.SetSelectionEnd({ 5, 11 }); - ValidateLinearSelection(term, { 0, 10 }, { 99, 11 }); + ValidateLinearSelection(term, { 0, 10 }, { term.GetViewport().RightExclusive(), 11 }); } TEST_METHOD(ShiftClick) @@ -579,20 +579,20 @@ namespace TerminalCoreUnitTests term.UpdateSettings(settings); // Insert text at position (4,10) - const std::wstring_view text = L"doubleClickMe dragThroughHere"; + const std::wstring_view text = L"doubleClickMe dragThroughHere anotherWord"; GetTextBuffer(term).GetCursor().SetPosition({ 4, 10 }); term.Write(text); - // Step 1: Create a selection on "doubleClickMe" + Log::Comment(L"Step 1 : Create a selection on \"doubleClickMe\""); { // Simulate double click at (x,y) = (5,10) term.MultiClickSelection({ 5, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe" selected - ValidateLinearSelection(term, { 4, 10 }, { 16, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 18, 10 }); } - // Step 2: Shift+Click to "dragThroughHere" + Log::Comment(L"Step 2: Shift+Click to \"dragThroughHere\""); { // Simulate Shift+Click at (x,y) = (21,10) // @@ -602,10 +602,10 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Char); // Validate selection area: "doubleClickMe drag" selected - ValidateLinearSelection(term, { 4, 10 }, { 21, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 22, 10 }); } - // Step 3: Shift+Double-Click at "dragThroughHere" + Log::Comment(L"Step 3: Shift+Double-Click at \"dragThroughHere\""); { // Simulate Shift+DoubleClick at (x,y) = (21,10) // @@ -614,11 +614,11 @@ namespace TerminalCoreUnitTests // start click finish term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); - // Validate selection area: "doubleClickMe dragThroughHere" selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + // Validate selection area: "doubleClickMe dragThroughHere " selected + ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); } - // Step 4: Shift+Triple-Click at "dragThroughHere" + Log::Comment(L"Step 4: Shift+Triple-Click at \"dragThroughHere\""); { // Simulate Shift+TripleClick at (x,y) = (21,10) // @@ -628,23 +628,23 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Line); // Validate selection area: "doubleClickMe dragThroughHere..." selected - ValidateLinearSelection(term, { 4, 10 }, { 99, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); } - // Step 5: Shift+Double-Click at "dragThroughHere" + Log::Comment(L"Step 5: Shift+Double-Click at \"dragThroughHere\""); { // Simulate Shift+DoubleClick at (x,y) = (21,10) // - // buffer: doubleClickMe dragThroughHere - // ^ ^ ^ - // start click finish + // buffer: doubleClickMe dragThroughHere anotherWord + // ^ ^ ^ + // start click finish term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe dragThroughHere" selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); } - // Step 6: Drag past "dragThroughHere" + Log::Comment(L"Step 6: Drag past \"dragThroughHere\""); { // Simulate drag to (x,y) = (35,10) // Since we were preceded by a double-click, we're in "word" expansion mode @@ -655,33 +655,33 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 35, 10 }); // Validate selection area: "doubleClickMe dragThroughHere..." selected - ValidateLinearSelection(term, { 4, 10 }, { 99, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); } - // Step 6: Drag back to "dragThroughHere" + Log::Comment(L"Step 7: Drag back to \"dragThroughHere\""); { // Simulate drag to (x,y) = (21,10) // - // buffer: doubleClickMe dragThroughHere - // ^ ^ ^ - // start drag finish + // buffer: doubleClickMe dragThroughHere anotherWord + // ^ ^ ^ + // start drag finish term.SetSelectionEnd({ 21, 10 }); - // Validate selection area: "doubleClickMe dragThroughHere" selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + // Validate selection area: "doubleClickMe dragThroughHere " selected + ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); } - // Step 7: Drag within "dragThroughHere" + Log::Comment(L"Step 8: Drag within \"dragThroughHere\""); { // Simulate drag to (x,y) = (25,10) // - // buffer: doubleClickMe dragThroughHere - // ^ ^ ^ - // start drag finish + // buffer: doubleClickMe dragThroughHere anotherWord + // ^ ^ ^ + // start drag finish term.SetSelectionEnd({ 25, 10 }); // Validate selection area: "doubleClickMe dragThroughHere" still selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); } } @@ -691,16 +691,16 @@ namespace TerminalCoreUnitTests DummyRenderer renderer{ &term }; term.Create({ 100, 100 }, 0, renderer); - // Step 1: Create a selection + Log::Comment(L"Step 1: Create a selection"); { - // (10,10) to (20, 10) + // (10,10) to (20, 10) (inclusive) term.SelectNewRegion({ 10, 10 }, { 20, 10 }); // Validate selection area - ValidateLinearSelection(term, { 10, 10 }, { 20, 10 }); + ValidateLinearSelection(term, { 10, 10 }, { 21, 10 }); } - // Step 2: Drag to (5,10) + Log::Comment(L"Step 2: Drag to (5,10)"); { term.SetSelectionEnd({ 5, 10 }); @@ -709,7 +709,7 @@ namespace TerminalCoreUnitTests ValidateLinearSelection(term, { 5, 10 }, { 10, 10 }); } - // Step 3: Drag back to (20,10) + Log::Comment(L"Step 3: Drag back to (20,10)"); { term.SetSelectionEnd({ 20, 10 }); @@ -718,7 +718,7 @@ namespace TerminalCoreUnitTests ValidateLinearSelection(term, { 10, 10 }, { 20, 10 }); } - // Step 4: Shift+Click at (5,10) + Log::Comment(L"Step 4: Shift+Click at (5,10)"); { term.SetSelectionEnd({ 5, 10 }, Terminal::SelectionExpansion::Char); @@ -727,13 +727,14 @@ namespace TerminalCoreUnitTests ValidateLinearSelection(term, { 5, 10 }, { 10, 10 }); } - // Step 5: Shift+Click back at (20,10) + Log::Comment(L"Step 5: Shift+Click back at (20,10)"); { term.SetSelectionEnd({ 20, 10 }, Terminal::SelectionExpansion::Char); // Validate selection area - // NOTE: Pivot should still be (10, 10) - ValidateLinearSelection(term, { 10, 10 }, { 20, 10 }); + // Pivot should still be (10, 10) + // Shift+Click makes end inclusive (so add 1) + ValidateLinearSelection(term, { 10, 10 }, { 21, 10 }); } } }; From 52f3bc05c601ee69b94c50bec921f02fe0b45f34 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 5 Nov 2024 15:34:10 -0800 Subject: [PATCH 19/34] fix other tests --- src/buffer/out/textBuffer.cpp | 6 +- src/host/ut_host/ClipboardTests.cpp | 2 +- src/host/ut_host/SearchTests.cpp | 30 ++++---- src/host/ut_host/TextBufferTests.cpp | 110 ++++++++++++++++----------- 4 files changed, 83 insertions(+), 65 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index ce12407efc9..b1282ac9477 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2097,7 +2097,7 @@ std::tuple TextBuffer::_RowCopyHelper(cons const auto maxX = req.bufferCoordinates ? req.maxX : ScreenToBufferLineInclusive(til::point{ req.maxX, iRow }, lineRendition).x; rowBeg = minX; - rowEnd = maxX + 1; // +1 to get an exclusive end + rowEnd = maxX; } else { @@ -3546,9 +3546,7 @@ std::wstring TextBuffer::_commandForRow(const til::CoordType rowOffset, if (markKind == MarkKind::Command) { - // GetText() has an exclusive end, - // add 1 to newX to get the correct range - commandBuilder += row.GetText(x, nextX + 1); + commandBuilder += row.GetText(x, nextX); } // advance to next run of text x = nextX; diff --git a/src/host/ut_host/ClipboardTests.cpp b/src/host/ut_host/ClipboardTests.cpp index 21d4c7b13b8..24f2693c643 100644 --- a/src/host/ut_host/ClipboardTests.cpp +++ b/src/host/ut_host/ClipboardTests.cpp @@ -84,7 +84,7 @@ class ClipboardTests const auto& screenInfo = gci.GetActiveOutputBuffer(); const auto& buffer = screenInfo.GetTextBuffer(); - constexpr til::point_span selection = { { 0, 0 }, { 14, 3 } }; + constexpr til::point_span selection = { { 0, 0 }, { 15, 3 } }; const auto req = TextBuffer::CopyRequest::FromConfig(buffer, selection.start, selection.end, false, !fLineSelection, false); return buffer.GetPlainText(req); } diff --git a/src/host/ut_host/SearchTests.cpp b/src/host/ut_host/SearchTests.cpp index 423273b3c38..e68a5359f4a 100644 --- a/src/host/ut_host/SearchTests.cpp +++ b/src/host/ut_host/SearchTests.cpp @@ -55,12 +55,12 @@ class SearchTests return true; } - static void DoFoundChecks(Search& s, til::point coordStartExpected, til::CoordType lineDelta, bool reverse) + static void DoFoundChecks(Search& s, til::point coordStartExpected, til::CoordType lineDelta, bool reverse, bool japaneseChar) { const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto coordEndExpected = coordStartExpected; - coordEndExpected.x += 1; + coordEndExpected.x += japaneseChar ? 2 : 3; VERIFY_IS_TRUE(s.SelectCurrent()); VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor()); @@ -97,7 +97,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"AB", SearchFlag::None, false); - DoFoundChecks(s, {}, 1, false); + DoFoundChecks(s, {}, 1, false, false); } TEST_METHOD(ForwardCaseSensitiveJapanese) @@ -105,7 +105,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::None, false); - DoFoundChecks(s, { 2, 0 }, 1, false); + DoFoundChecks(s, { 2, 0 }, 1, false, true); } TEST_METHOD(ForwardCaseInsensitive) @@ -114,7 +114,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"ab", SearchFlag::CaseInsensitive, false); - DoFoundChecks(s, {}, 1, false); + DoFoundChecks(s, {}, 1, false, false); } TEST_METHOD(ForwardCaseInsensitiveJapanese) @@ -122,7 +122,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::CaseInsensitive, false); - DoFoundChecks(s, { 2, 0 }, 1, false); + DoFoundChecks(s, { 2, 0 }, 1, false, true); } TEST_METHOD(BackwardCaseSensitive) @@ -130,7 +130,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"AB", SearchFlag::None, true); - DoFoundChecks(s, { 0, 3 }, -1, true); + DoFoundChecks(s, { 0, 3 }, -1, true, false); } TEST_METHOD(BackwardCaseSensitiveJapanese) @@ -138,7 +138,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::None, true); - DoFoundChecks(s, { 2, 3 }, -1, true); + DoFoundChecks(s, { 2, 3 }, -1, true, true); } TEST_METHOD(BackwardCaseInsensitive) @@ -146,7 +146,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"ab", SearchFlag::CaseInsensitive, true); - DoFoundChecks(s, { 0, 3 }, -1, true); + DoFoundChecks(s, { 0, 3 }, -1, true, false); } TEST_METHOD(BackwardCaseInsensitiveJapanese) @@ -154,7 +154,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::CaseInsensitive, true); - DoFoundChecks(s, { 2, 3 }, -1, true); + DoFoundChecks(s, { 2, 3 }, -1, true, true); } TEST_METHOD(ForwardCaseSensitiveRegex) @@ -163,7 +163,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"[BA]{2}", SearchFlag::RegularExpression, false); - DoFoundChecks(s, {}, 1, false); + DoFoundChecks(s, {}, 1, false, false); } TEST_METHOD(ForwardCaseSensitiveRegexJapanese) @@ -172,7 +172,7 @@ class SearchTests Search s; // N.B. this is not a literal U+30xx, but a regex escape sequence \x{30xx} s.Reset(gci.renderData, LR"-([\x{3041}-\x{304c}])-", SearchFlag::RegularExpression, false); - DoFoundChecks(s, { 2, 0 }, 1, false); + DoFoundChecks(s, { 2, 0 }, 1, false, true); } TEST_METHOD(ForwardCaseInsensitiveRegex) @@ -181,7 +181,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"ab", SearchFlag::CaseInsensitive | SearchFlag::RegularExpression, false); - DoFoundChecks(s, {}, 1, false); + DoFoundChecks(s, {}, 1, false, false); } TEST_METHOD(ForwardCaseInsensitiveRegexJapanese) @@ -190,7 +190,7 @@ class SearchTests Search s; // N.B. this is not a literal U+30xx, but a regex escape sequence \x{30xx} s.Reset(gci.renderData, LR"-([\x{3041}-\x{304c}])-", SearchFlag::CaseInsensitive | SearchFlag::RegularExpression, false); - DoFoundChecks(s, { 2, 0 }, 1, false); + DoFoundChecks(s, { 2, 0 }, 1, false, true); } TEST_METHOD(ForwardCaseSensitiveRegexWithCaseInsensitiveFlag) @@ -199,6 +199,6 @@ class SearchTests Search s; s.Reset(gci.renderData, L"(?i)ab", SearchFlag::RegularExpression, false); - DoFoundChecks(s, {}, 1, false); + DoFoundChecks(s, {}, 1, false, false); } }; diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 2d6c1d1628e..d92d82d10e4 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -2432,11 +2432,11 @@ void TextBufferTests::GetTextRects() std::vector expected{}; if (blockSelection) { - expected.push_back({ 1, 0, 7, 0 }); - expected.push_back({ 1, 1, 8, 1 }); // expand right - expected.push_back({ 1, 2, 7, 2 }); - expected.push_back({ 0, 3, 7, 3 }); // expand left - expected.push_back({ 1, 4, 7, 4 }); + expected.push_back({ 1, 0, 8, 0 }); + expected.push_back({ 1, 1, 9, 1 }); // expand right + expected.push_back({ 1, 2, 8, 2 }); + expected.push_back({ 0, 3, 8, 3 }); // do not expand + expected.push_back({ 1, 4, 8, 4 }); } else { @@ -2444,11 +2444,11 @@ void TextBufferTests::GetTextRects() expected.push_back({ 0, 1, 19, 1 }); expected.push_back({ 0, 2, 19, 2 }); expected.push_back({ 0, 3, 19, 3 }); - expected.push_back({ 0, 4, 7, 4 }); + expected.push_back({ 0, 4, 8, 4 }); } til::point start{ 1, 0 }; - til::point end{ 7, 4 }; + til::point end{ 8, 4 }; const auto result = _buffer->GetTextRects(start, end, blockSelection, false); VERIFY_ARE_EQUAL(expected.size(), result.size()); for (size_t i = 0; i < expected.size(); ++i) @@ -2506,21 +2506,31 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"Standard Copy to Clipboard"); - expectedText += L"12345\r\n"; - expectedText += L" 345\r\n"; - expectedText += L"123\r\n"; - expectedText += L" 3\r\n"; + if (blockSelection) + { + expectedText += L"1234\r\n"; + expectedText += L" 34\r\n"; + expectedText += L"123\r\n"; + expectedText += L" 3\r\n"; + } + else + { + expectedText += L"12345\r\n"; + expectedText += L" 345\r\n"; + expectedText += L"123\r\n"; + expectedText += L" 3\r\n"; + } } else { Log::Comment(L"UI Automation"); if (blockSelection) { - expectedText += L"12345\r\n"; - expectedText += L" 345\r\n"; - expectedText += L"123 \r\n"; - expectedText += L" 3 \r\n"; - expectedText += L" "; + expectedText += L"1234\r\n"; + expectedText += L" 34\r\n"; + expectedText += L"123 \r\n"; + expectedText += L" 3 \r\n"; + expectedText += L" "; } else { @@ -2528,7 +2538,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345 \r\n"; expectedText += L"123 \r\n"; expectedText += L" 3 \r\n"; - expectedText += L" "; + expectedText += L" "; } } } @@ -2537,21 +2547,31 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"UNDEFINED"); - expectedText += L"12345"; - expectedText += L" 345"; - expectedText += L"123"; - expectedText += L" 3"; + if (blockSelection) + { + expectedText += L"1234"; + expectedText += L" 34"; + expectedText += L"123"; + expectedText += L" 3"; + } + else + { + expectedText += L"12345"; + expectedText += L" 345"; + expectedText += L"123"; + expectedText += L" 3"; + } } else { Log::Comment(L"Shift+Copy to Clipboard"); if (blockSelection) { - expectedText += L"12345"; - expectedText += L" 345"; - expectedText += L"123 "; - expectedText += L" 3 "; - expectedText += L" "; + expectedText += L"1234"; + expectedText += L" 34"; + expectedText += L"123 "; + expectedText += L" 3 "; + expectedText += L" "; } else { @@ -2559,7 +2579,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345 "; expectedText += L"123 "; expectedText += L" 3 "; - expectedText += L" "; + expectedText += L" "; } } } @@ -2604,21 +2624,21 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"UNDEFINED"); - expectedText += L"12345\r\n"; + expectedText += L"1234\r\n"; expectedText += L"67\r\n"; - expectedText += L" 345\r\n"; + expectedText += L" 34\r\n"; expectedText += L"123\r\n"; expectedText += L"\r\n"; } else { Log::Comment(L"Copy block selection to Clipboard"); - expectedText += L"12345\r\n"; - expectedText += L"67 \r\n"; - expectedText += L" 345\r\n"; - expectedText += L"123 \r\n"; - expectedText += L" \r\n"; - expectedText += L" "; + expectedText += L"1234\r\n"; + expectedText += L"67 \r\n"; + expectedText += L" 34\r\n"; + expectedText += L"123 \r\n"; + expectedText += L" \r\n"; + expectedText += L" "; } } else @@ -2626,20 +2646,20 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"UNDEFINED"); - expectedText += L"12345"; + expectedText += L"1234"; expectedText += L"67"; - expectedText += L" 345"; + expectedText += L" 34"; expectedText += L"123"; } else { Log::Comment(L"UNDEFINED"); - expectedText += L"12345"; - expectedText += L"67 "; - expectedText += L" 345"; - expectedText += L"123 "; - expectedText += L" "; - expectedText += L" "; + expectedText += L"1234"; + expectedText += L"67 "; + expectedText += L" 34"; + expectedText += L"123 "; + expectedText += L" "; + expectedText += L" "; } } } @@ -2663,7 +2683,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345"; expectedText += L"123 "; expectedText += L" \r\n"; - expectedText += L" "; + expectedText += L" "; } } else @@ -2684,7 +2704,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345"; expectedText += L"123 "; expectedText += L" "; - expectedText += L" "; + expectedText += L" "; } } } From 408d14976eeee91916a4010a811515187bece554 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 5 Nov 2024 15:43:55 -0800 Subject: [PATCH 20/34] spell --- .github/actions/spelling/expect/expect.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 3a6ca5cf14e..4d96c2bf438 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -163,6 +163,7 @@ CBN cbt Ccc CCCBB +CCCDDD cch CCHAR CCmd @@ -365,6 +366,7 @@ dcommon dcompiler DComposition dde +DDDCCC DDESHARE DDevice DEADCHAR From 57b82cbf5f1e06938bcb2ac327d18c2d12207197 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 5 Nov 2024 16:02:32 -0800 Subject: [PATCH 21/34] [UIA] allow end exclusive --- src/types/UiaTextRangeBase.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 857cf32c71e..60cfbc280bb 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -987,7 +987,10 @@ std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const // TODO GH#5406: create a different UIA parent object for each TextBuffer // nvaccess/nvda#11428: Ensure our endpoints are in bounds - THROW_HR_IF(E_FAIL, !bufferSize.IsInExclusiveBounds(_start) || !bufferSize.IsInExclusiveBounds(_end)); + auto isValid = [&](const til::point& point) { + return bufferSize.IsInExclusiveBounds(point) || point == bufferSize.EndExclusive(); + }; + THROW_HR_IF(E_FAIL, !isValid(_start) || !isValid(_end)); const auto req = TextBuffer::CopyRequest{ buffer, _start, _end, _blockRange, true, false, false, true }; auto plainText = buffer.GetPlainText(req); From e98033e18fc6447ce5dcca430d0f0dca8c51a9f6 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 5 Nov 2024 16:23:36 -0800 Subject: [PATCH 22/34] audit --- src/types/UiaTextRangeBase.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 60cfbc280bb..9b3d7040a5e 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -972,7 +972,7 @@ CATCH_RETURN(); // Return Value: // - the text that the UiaTextRange encompasses #pragma warning(push) -#pragma warning(disable : 26447) // compiler isn't filtering throws inside the try/catch +#pragma warning(disable : 26440) // The function ... can be declared as noexcept. std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const { std::wstring textData{}; From e621c1c00ace7c842f53edc0da4dbdf28b3bfb23 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 7 Nov 2024 11:54:58 -0800 Subject: [PATCH 23/34] fix some more tests --- .../out/ut_textbuffer/UTextAdapterTests.cpp | 6 ++-- .../UnitTests_Control/ControlCoreTests.cpp | 20 ++++++------- .../ControlInteractivityTests.cpp | 29 ++++++++++++------- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp b/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp index be9e941c757..c9280b201c0 100644 --- a/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp +++ b/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp @@ -49,15 +49,15 @@ class UTextAdapterTests return { { beg, 0 }, { end, 0 } }; }; - auto expected = std::vector{ s(0, 2), s(8, 10) }; + auto expected = std::vector{ s(0, 3), s(8, 11) }; auto actual = buffer.SearchText(L"abc", SearchFlag::None); VERIFY_ARE_EQUAL(expected, actual); - expected = std::vector{ s(5, 5) }; + expected = std::vector{ s(5, 6) }; actual = buffer.SearchText(L"𝒷", SearchFlag::None); VERIFY_ARE_EQUAL(expected, actual); - expected = std::vector{ s(12, 15) }; + expected = std::vector{ s(12, 17) }; actual = buffer.SearchText(L"ネコ", SearchFlag::None); VERIFY_ARE_EQUAL(expected, actual); } diff --git a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp index e018e1fd2ad..999809cb397 100644 --- a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp @@ -419,7 +419,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 17, 0 }; - const til::point expectedEnd{ 23, 0 }; + const til::point expectedEnd{ 24, 0 }; VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -440,7 +440,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 17, 4 }; - const til::point expectedEnd{ 23, 4 }; + const til::point expectedEnd{ 24, 4 }; VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -450,7 +450,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 17, 0 }; - const til::point expectedEnd{ 23, 0 }; + const til::point expectedEnd{ 24, 0 }; VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -460,7 +460,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 17, 4 }; - const til::point expectedEnd{ 23, 4 }; + const til::point expectedEnd{ 24, 4 }; VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -502,7 +502,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 24, 0 }; // The character after the prompt - const til::point expectedEnd{ 21, 3 }; // x = the end of the text + const til::point expectedEnd{ 22, 3 }; // x = the end of the text + 1 (exclusive end) VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -663,7 +663,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 20, 4 }; // The character after the prompt - const til::point expectedEnd{ 26, 34 }; // x = the end of the text + const til::point expectedEnd{ 27, 34 }; // x = the end of the text + 1 (exclusive end) VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -673,7 +673,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 24, 0 }; // The character after the prompt - const til::point expectedEnd{ 21, 3 }; // x = the end of the text + const til::point expectedEnd{ 22, 3 }; // x = the end of the text + 1 (exclusive end) VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -731,7 +731,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 20, 4 }; // The character after the prompt - const til::point expectedEnd{ 29, 34 }; // x = the end of the text + const til::point expectedEnd{ 30, 34 }; // x = the end of the text + 1 (exclusive end) VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -741,7 +741,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 24, 0 }; // The character after the prompt - const til::point expectedEnd{ 21, 3 }; // x = the end of the text + const til::point expectedEnd{ 22, 3 }; // x = the end of the text + 1 (exclusive end) VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } @@ -804,7 +804,7 @@ namespace ControlUnitTests const auto& start = core->_terminal->GetSelectionAnchor(); const auto& end = core->_terminal->GetSelectionEnd(); const til::point expectedStart{ 1, 1 }; - const til::point expectedEnd{ 1, 2 }; + const til::point expectedEnd{ 2, 2 }; VERIFY_ARE_EQUAL(expectedStart, start); VERIFY_ARE_EQUAL(expectedEnd, end); } diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index 49fcd6d21eb..8e98348e542 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -438,21 +438,23 @@ namespace ControlUnitTests // The viewport is on row 21, so the selection will be on: // {(5, 5)+(0, 21)} to {(5, 5)+(0, 21)} til::point expectedAnchor{ 5, 26 }; + til::point expectedEnd{ 6, 26 }; // add 1 to x-coordinate because end is exclusive VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); - VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); + VERIFY_ARE_EQUAL(expectedEnd, core->_terminal->GetSelectionEnd()); Log::Comment(L"Scroll up a line, with the left mouse button selected"); interactivity->MouseWheel(modifiers, WHEEL_DELTA, cursorPosition1.to_core_point(), leftMouseDown); - + // TODO CARLOS Log::Comment(L"Verify the location of the selection"); // The viewport is now on row 20, so the selection will be on: - // {(5, 5)+(0, 20)} to {(5, 5)+(0, 21)} - til::point newExpectedAnchor{ 5, 25 }; + // {(5 + 1, 5)+(0, 20)} to {(5, 5)+(0, 21)} + // NOTE: newExpectedAnchor should be expectedEnd moved up one row + til::point newExpectedAnchor{ 6, 25 }; // Remember, the anchor is always before the end in the buffer. So yes, - // se started the selection on 5,26, but now that's the end. + // we started the selection on 5,26, but now that's the end. VERIFY_ARE_EQUAL(newExpectedAnchor, core->_terminal->GetSelectionAnchor()); VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); } @@ -628,7 +630,7 @@ namespace ControlUnitTests Log::Comment(L"Verify that it started on the first cell we clicked on, not the one we dragged to"); til::point expectedAnchor{ 0, 0 }; VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); - til::point expectedEnd{ 2, 0 }; + til::point expectedEnd{ 3, 0 }; // add 1 to x-coordinate because end is exclusive VERIFY_ARE_EQUAL(expectedEnd, core->_terminal->GetSelectionEnd()); interactivity->PointerReleased(noMouseDown, @@ -798,8 +800,9 @@ namespace ControlUnitTests // The viewport is on row (historySize + 5), so the selection will be on: // {(5, (historySize+5))+(0, 21)} to {(5, (historySize+5))+(0, 21)} til::point expectedAnchor{ 5, settings->HistorySize() + 5 }; + til::point expectedEnd{ 6, expectedAnchor.y }; // add 1 to x-coordinate because end is exclusive VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionAnchor()); - VERIFY_ARE_EQUAL(expectedAnchor, core->_terminal->GetSelectionEnd()); + VERIFY_ARE_EQUAL(expectedEnd, core->_terminal->GetSelectionEnd()); Log::Comment(L"Output a line of text"); conn->WriteInput(winrt_wstring_to_array_view(L"Foo\r\n")); @@ -807,6 +810,7 @@ namespace ControlUnitTests Log::Comment(L"Verify the location of the selection"); // The selection should now be 1 row lower expectedAnchor.y -= 1; + expectedEnd.y -= 1; { const auto anchor{ core->_terminal->GetSelectionAnchor() }; const auto end{ core->_terminal->GetSelectionEnd() }; @@ -815,7 +819,7 @@ namespace ControlUnitTests Log::Comment(fmt::format(L"end:({},{})", end.x, end.y).c_str()); VERIFY_ARE_EQUAL(expectedAnchor, anchor); - VERIFY_ARE_EQUAL(expectedAnchor, end); + VERIFY_ARE_EQUAL(expectedEnd, end); } VERIFY_ARE_EQUAL(scrollbackLength - 1, core->_terminal->GetScrollOffset()); @@ -825,6 +829,7 @@ namespace ControlUnitTests Log::Comment(L"Verify the location of the selection"); // The selection should now be 1 row lower expectedAnchor.y -= 1; + expectedEnd.y -= 1; { const auto anchor{ core->_terminal->GetSelectionAnchor() }; const auto end{ core->_terminal->GetSelectionEnd() }; @@ -833,7 +838,7 @@ namespace ControlUnitTests Log::Comment(fmt::format(L"end:({},{})", end.x, end.y).c_str()); VERIFY_ARE_EQUAL(expectedAnchor, anchor); - VERIFY_ARE_EQUAL(expectedAnchor, end); + VERIFY_ARE_EQUAL(expectedEnd, end); } VERIFY_ARE_EQUAL(scrollbackLength - 2, core->_terminal->GetScrollOffset()); @@ -858,15 +863,17 @@ namespace ControlUnitTests Log::Comment(fmt::format(L"anchor:({},{})", anchor.x, anchor.y).c_str()); Log::Comment(fmt::format(L"end:({},{})", end.x, end.y).c_str()); + // Selection was updated, but we didn't highlight a full cell VERIFY_ARE_EQUAL(expectedAnchor, anchor); VERIFY_ARE_EQUAL(expectedAnchor, end); } - Log::Comment(L"Output a line ant move the mouse a little to update the selection, all at once"); + Log::Comment(L"Output a line and move the mouse a little to update the selection, all at once"); // Same as above. The viewport has moved, so the mouse is still over the // same character, even though it's at a new offset. conn->WriteInput(winrt_wstring_to_array_view(L"Foo\r\n")); expectedAnchor.y -= 1; + expectedEnd.y -= 1; VERIFY_ARE_EQUAL(scrollbackLength - 3, core->_terminal->GetScrollOffset()); interactivity->PointerMoved(leftMouseDown, WM_LBUTTONDOWN, //pointerUpdateKind @@ -883,7 +890,7 @@ namespace ControlUnitTests Log::Comment(fmt::format(L"end:({},{})", end.x, end.y).c_str()); VERIFY_ARE_EQUAL(expectedAnchor, anchor); - VERIFY_ARE_EQUAL(expectedAnchor, end); + VERIFY_ARE_EQUAL(expectedEnd, end); } // Output enough text for the selection to get pushed off the buffer From 83d2bff4906cf6ba5ab2d58e36c3b18ca5c06dbe Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 8 Nov 2024 14:13:38 -0800 Subject: [PATCH 24/34] apply feedback --- src/buffer/out/UTextAdapter.cpp | 2 +- src/cascadia/TerminalControl/ControlCore.cpp | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/buffer/out/UTextAdapter.cpp b/src/buffer/out/UTextAdapter.cpp index a76d2aa55e9..fbd5ffa6a51 100644 --- a/src/buffer/out/UTextAdapter.cpp +++ b/src/buffer/out/UTextAdapter.cpp @@ -411,7 +411,7 @@ Microsoft::Console::ICU::unique_uregex Microsoft::Console::ICU::CreateRegex(cons return unique_uregex{ re }; } -// Returns an inclusive point range given a text start and end position. +// Returns a half-open [beg,end) range given a text start and end position. // This function is designed to be used with uregex_start64/uregex_end64. til::point_span Microsoft::Console::ICU::BufferRangeFromMatch(UText* ut, URegularExpression* re) { diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index caa45430617..48d28071579 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1218,9 +1218,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation } // clamp the converted position to be within the viewport bounds - // x: allow range of [-1, RightExclusive]. + // x: allow range of [0, RightExclusive] + // GH #18106: right exclusive needed for selection to support exclusive end til::point terminalPosition{ - std::clamp(position.x, -1, _terminal->GetViewport().Width()), + std::clamp(position.x, 0, _terminal->GetViewport().Width()), std::clamp(position.y, 0, _terminal->GetViewport().Height() - 1) }; From 6aed5f98f4a55ca61fad27d9e08020e2afc9509c Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 15 Nov 2024 13:36:53 -0800 Subject: [PATCH 25/34] Fix shift+tab to select hyperlink --- src/cascadia/TerminalCore/TerminalSelection.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 70d0b7ca08b..20e55a86e2f 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -560,7 +560,6 @@ void Terminal::SelectHyperlink(const SearchDirection dir) selection->start = result->first; selection->pivot = result->first; selection->end = result->second; - bufferSize.DecrementInBounds(selection->end); _selectionIsTargetingUrl = true; _selectionEndpoint = SelectionEndpoint::End; } From 7a8d4486ce03b1f5510d3e4e5671f0acb321f76c Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 15 Nov 2024 13:40:35 -0800 Subject: [PATCH 26/34] Fix ConHost --- src/buffer/out/textBuffer.cpp | 2 +- src/host/selection.cpp | 15 +++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index b1282ac9477..9bdd74ff8da 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1970,7 +1970,7 @@ const std::vector TextBuffer::GetTextRects(til::point start // - Else if a blockSelection, returns spans corresponding to each line in the block selection // Arguments: // - start: beginning of the text region of interest (inclusive) -// - end: the other end of the text region of interest (inclusive) +// - end: the other end of the text region of interest (exclusive) // - blockSelection: when enabled, get spans for each line covered by the block // - bufferCoordinates: when enabled, treat the coordinates as relative to // the buffer rather than the screen. diff --git a/src/host/selection.cpp b/src/host/selection.cpp index 300068d9671..e95387125cf 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -43,11 +43,18 @@ void Selection::_RegenerateSelectionSpans() const 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 uses an inclusive range for selection. GetTextSpans() needs an exclusive range. + // Increment the "end" point (bottom-right-most) to make it exclusive. + // NOTE: if start is past end, increment start instead + const auto& buffer = screenInfo.GetTextBuffer(); + auto startSelectionAnchor = _d->coordSelectionAnchor; + buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor); + const auto blockSelection = !IsLineSelection(); - _lastSelectionSpans = screenInfo.GetTextBuffer().GetTextSpans(_d->coordSelectionAnchor, - endSelectionAnchor, - blockSelection, - false); + _lastSelectionSpans = buffer.GetTextSpans(startSelectionAnchor, + endSelectionAnchor, + blockSelection, + false); _lastSelectionGeneration = _d.generation(); } From 6b99816e570d16958cfdc13eed2ca017fab2a4e6 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 15 Nov 2024 15:50:07 -0800 Subject: [PATCH 27/34] Fix 'off by 1' hyperlink detection --- src/buffer/out/Row.cpp | 25 +++++++++++++++---------- src/buffer/out/Row.hpp | 5 +++-- src/buffer/out/UTextAdapter.cpp | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 2 +- src/cascadia/TerminalCore/Terminal.hpp | 2 +- 5 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 6c72a74d0e4..63ba83f4cd5 100644 --- a/src/buffer/out/Row.cpp +++ b/src/buffer/out/Row.cpp @@ -80,11 +80,12 @@ constexpr OutIt copy_n_small(InIt first, Diff count, OutIt dest) return dest; } -CharToColumnMapper::CharToColumnMapper(const wchar_t* chars, const uint16_t* charOffsets, ptrdiff_t lastCharOffset, til::CoordType currentColumn) noexcept : +CharToColumnMapper::CharToColumnMapper(const wchar_t* chars, const uint16_t* charOffsets, ptrdiff_t charsLength, til::CoordType currentColumn, til::CoordType columnCount) noexcept : _chars{ chars }, _charOffsets{ charOffsets }, - _lastCharOffset{ lastCharOffset }, - _currentColumn{ currentColumn } + _charsLength{ charsLength }, + _currentColumn{ currentColumn }, + _columnCount{ columnCount } { } @@ -92,7 +93,7 @@ CharToColumnMapper::CharToColumnMapper(const wchar_t* chars, const uint16_t* cha // This function in particular returns the glyph's first column. til::CoordType CharToColumnMapper::GetLeadingColumnAt(ptrdiff_t targetOffset) noexcept { - targetOffset = clamp(targetOffset, 0, _lastCharOffset); + targetOffset = clamp(targetOffset, 0, _charsLength); // This code needs to fulfill two conditions on top of the obvious (a forward/backward search): // A: We never want to stop on a column that is marked with CharOffsetsTrailer (= "GetLeadingColumn"). @@ -130,10 +131,14 @@ til::CoordType CharToColumnMapper::GetLeadingColumnAt(ptrdiff_t targetOffset) no til::CoordType CharToColumnMapper::GetTrailingColumnAt(ptrdiff_t offset) noexcept { auto col = GetLeadingColumnAt(offset); - // This loop is a little redundant with the forward search loop in GetLeadingColumnAt() - // but it's realistically not worth caring about this. This code is not a bottleneck. - for (; WI_IsFlagSet(_charOffsets[col + 1], CharOffsetsTrailer); ++col) + + if (col < _columnCount) { + // This loop is a little redundant with the forward search loop in GetLeadingColumnAt() + // but it's realistically not worth caring about this. This code is not a bottleneck. + for (; WI_IsFlagSet(_charOffsets[col + 1], CharOffsetsTrailer); ++col) + { + } } return col; } @@ -1222,15 +1227,15 @@ T ROW::_adjustForward(T column) const noexcept } // Creates a CharToColumnMapper given an offset into _chars.data(). -// In other words, for a 120 column ROW with just ASCII text, the offset should be [0,120). +// In other words, for a 120 column ROW with just ASCII text, the offset should be [0,120]. CharToColumnMapper ROW::_createCharToColumnMapper(ptrdiff_t offset) const noexcept { const auto charsSize = _charSize(); - const auto lastChar = gsl::narrow_cast(charsSize - 1); + const auto lastChar = gsl::narrow_cast(charsSize); // We can sort of guess what column belongs to what offset because BMP glyphs are very common and // UTF-16 stores them in 1 char. In other words, usually a ROW will have N chars for N columns. const auto guessedColumn = gsl::narrow_cast(clamp(offset, 0, _columnCount)); - return CharToColumnMapper{ _chars.data(), _charOffsets.data(), lastChar, guessedColumn }; + return CharToColumnMapper{ _chars.data(), _charOffsets.data(), lastChar, guessedColumn, _columnCount }; } const std::optional& ROW::GetScrollbarData() const noexcept diff --git a/src/buffer/out/Row.hpp b/src/buffer/out/Row.hpp index d2c19036baf..44156d1b881 100644 --- a/src/buffer/out/Row.hpp +++ b/src/buffer/out/Row.hpp @@ -71,7 +71,7 @@ struct RowCopyTextFromState // into a ROW's text this class can tell you what cell that pointer belongs to. struct CharToColumnMapper { - CharToColumnMapper(const wchar_t* chars, const uint16_t* charOffsets, ptrdiff_t lastCharOffset, til::CoordType currentColumn) noexcept; + CharToColumnMapper(const wchar_t* chars, const uint16_t* charOffsets, ptrdiff_t lastCharOffset, til::CoordType currentColumn, til::CoordType columnCount) noexcept; til::CoordType GetLeadingColumnAt(ptrdiff_t targetOffset) noexcept; til::CoordType GetTrailingColumnAt(ptrdiff_t offset) noexcept; @@ -85,8 +85,9 @@ struct CharToColumnMapper const wchar_t* _chars; const uint16_t* _charOffsets; - ptrdiff_t _lastCharOffset; + ptrdiff_t _charsLength; til::CoordType _currentColumn; + til::CoordType _columnCount; }; class ROW final diff --git a/src/buffer/out/UTextAdapter.cpp b/src/buffer/out/UTextAdapter.cpp index fbd5ffa6a51..717d97812aa 100644 --- a/src/buffer/out/UTextAdapter.cpp +++ b/src/buffer/out/UTextAdapter.cpp @@ -436,7 +436,7 @@ til::point_span Microsoft::Console::ICU::BufferRangeFromMatch(UText* ut, URegula if (utextAccess(ut, nativeIndexEnd, true)) { const auto y = accessCurrentRow(ut); - ret.end.x = textBuffer.GetRowByOffset(y).GetTrailingColumnAtCharOffset(ut->chunkOffset); + ret.end.x = textBuffer.GetRowByOffset(y).GetLeadingColumnAtCharOffset(ut->chunkOffset); ret.end.y = y; } else diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 4629081e3b3..37b3324eb33 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -502,7 +502,7 @@ std::wstring Terminal::GetHyperlinkAtBufferPosition(const til::point bufferPos) if (result.has_value()) { result->start = _ConvertToBufferCell(result->start, false); - result->stop = _ConvertToBufferCell(result->stop, false); + result->stop = _ConvertToBufferCell(result->stop, true); } } else diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 1fcbeec8ffe..1d03b5c9009 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -477,7 +477,7 @@ class Microsoft::Terminal::Core::Terminal final : std::vector _GetSelectionSpans() const noexcept; std::pair _PivotSelection(const til::point targetPos, bool& targetStart) const noexcept; std::pair _ExpandSelectionAnchors(std::pair anchors) const; - til::point _ConvertToBufferCell(const til::point viewportPos, bool allowRightExclusive = false) const; + til::point _ConvertToBufferCell(const til::point viewportPos, bool allowRightExclusive) const; void _ScrollToPoint(const til::point pos); void _MoveByChar(SelectionDirection direction, til::point& pos); void _MoveByWord(SelectionDirection direction, til::point& pos); From d179e9c606f5e19ef4f6275ce831850ea4a6ed6f Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 26 Nov 2024 15:08:55 -0800 Subject: [PATCH 28/34] bugfix: don't select trailing whitespace with mouse word selection --- src/buffer/out/textBuffer.cpp | 43 +++++++++++-------- src/buffer/out/textBuffer.hpp | 6 ++- .../TerminalCore/TerminalSelection.cpp | 16 +++---- 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 9bdd74ff8da..ce77a86913e 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1140,7 +1140,7 @@ DelimiterClass TextBuffer::_GetDelimiterClassAt(const til::point pos, const std: return GetRowByOffset(realPos.y).DelimiterClassAt(realPos.x, wordDelimiters); } -til::point TextBuffer::GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional) const +til::point TextBuffer::GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, bool includeWhitespace, std::optional limitOptional) const { const auto bufferSize{ GetSize() }; const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) }; @@ -1162,18 +1162,19 @@ til::point TextBuffer::GetWordStart2(til::point pos, const std::wstring_view wor // - DelimiterChar: "D" // - RegularChar: "C" // Expected results ("|" is the position): - // CCC___| --> |CCC___ - // DDD___| --> |DDD___ - // ___CCC| --> ___|CCC - // DDDCCC| --> DDD|CCC - // ___DDD| --> ___|DDD - // CCCDDD| --> CCC|DDD + // includeWhitespace: true false + // CCC___| --> |CCC___ CCC|___ + // DDD___| --> |DDD___ DDD|___ + // ___CCC| --> ___|CCC ___|CCC + // DDDCCC| --> DDD|CCC DDD|CCC + // ___DDD| --> ___|DDD ___|DDD + // CCCDDD| --> CCC|DDD CCC|DDD // So the heuristic we use is: // 1. move to the beginning of the delimiter class run - // 2. if we were on a ControlChar, go back one more delimiter class run + // 2. (includeWhitespace) if we were on a ControlChar, go back one more delimiter class run const auto initialDelimiter = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; pos = _GetDelimiterClassRunStart(pos, wordDelimiters); - if (pos.x == bufferSize.Left()) + if (!includeWhitespace || pos.x == bufferSize.Left()) { // Special case: // we're at the left boundary (and end of a delimiter class run), @@ -1188,7 +1189,7 @@ til::point TextBuffer::GetWordStart2(til::point pos, const std::wstring_view wor return pos; } -til::point TextBuffer::GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional) const +til::point TextBuffer::GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, bool includeWhitespace, std::optional limitOptional) const { const auto bufferSize{ GetSize() }; const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) }; @@ -1211,17 +1212,18 @@ til::point TextBuffer::GetWordEnd2(til::point pos, const std::wstring_view wordD // - DelimiterChar: "D" // - RegularChar: "C" // Expected results ("|" is the position): - // |CCC___ --> CCC___| - // |DDD___ --> DDD___| - // |___CCC --> ___|CCC - // |DDDCCC --> DDD|CCC - // |___DDD --> ___|DDD - // |CCCDDD --> CCC|DDD + // includeWhitespace: true false + // |CCC___ --> CCC___| CCC|___ + // |DDD___ --> DDD___| DDD|___ + // |___CCC --> ___|CCC ___|CCC + // |DDDCCC --> DDD|CCC DDD|CCC + // |___DDD --> ___|DDD ___|DDD + // |CCCDDD --> CCC|DDD CCC|DDD // So the heuristic we use is: // 1. move to the end of the delimiter class run - // 2. if the next delimiter class run is a ControlChar, go forward one more delimiter class run + // 2. (includeWhitespace) if the next delimiter class run is a ControlChar, go forward one more delimiter class run pos = _GetDelimiterClassRunEnd(pos, wordDelimiters); - if (pos.x == bufferSize.RightExclusive()) + if (!includeWhitespace || pos.x == bufferSize.RightExclusive()) { // Special case: // we're at the right boundary (and end of a delimiter class run), @@ -1312,6 +1314,11 @@ til::point TextBuffer::_GetDelimiterClassRunStart(til::point pos, const std::wst return pos; } +// Method Description: +// - Get the exclusive position for the end of the current delimiter class run +// Arguments: +// - pos - the buffer position being within the current delimiter class +// - wordDelimiters - what characters are we considering for the separation of words til::point TextBuffer::_GetDelimiterClassRunEnd(til::point pos, const std::wstring_view wordDelimiters) const { const auto bufferSize = GetSize(); diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 3425ea6fbf2..b834d516a13 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -174,8 +174,10 @@ class TextBuffer final til::point GetWordStart(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; til::point GetWordEnd(const til::point target, const std::wstring_view wordDelimiters, bool accessibilityMode = false, std::optional limitOptional = std::nullopt) const; - til::point GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; - til::point GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; + + til::point GetWordStart2(til::point pos, const std::wstring_view wordDelimiters, bool includeWhitespace, std::optional limitOptional = std::nullopt) const; + til::point GetWordEnd2(til::point pos, const std::wstring_view wordDelimiters, bool includeWhitespace, std::optional limitOptional = std::nullopt) const; + bool IsWordBoundary(const til::point pos, const std::wstring_view wordDelimiters) const; bool MoveToNextWord(til::point& pos, const std::wstring_view wordDelimiters, std::optional limitOptional = std::nullopt) const; bool MoveToPreviousWord(til::point& pos, const std::wstring_view wordDelimiters) const; diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 20e55a86e2f..c583f8c8ab9 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -302,7 +302,7 @@ std::pair Terminal::_ExpandSelectionAnchors(std::pair Terminal::_ExpandSelectionAnchors(std::pair Date: Tue, 26 Nov 2024 15:09:32 -0800 Subject: [PATCH 29/34] bugfix: click+drag on cell should select the cell we're on --- .../TerminalControl/ControlInteractivity.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index b07b881f887..af624305eb8 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -239,7 +239,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const ::Microsoft::Terminal::Core::ControlKeyStates modifiers, const Core::Point pixelPosition) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, false); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, true); const auto altEnabled = modifiers.IsAltPressed(); const auto shiftEnabled = modifiers.IsShiftPressed(); @@ -370,7 +370,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation // _touchdown_ point here. We want to start the selection // from where the user initially clicked, not where they are // now. - _core->SetSelectionAnchor(_getTerminalPosition(til::point{ touchdownPoint }, false)); + auto termPos = _getTerminalPosition(til::point{ touchdownPoint }, false); + if (dx < 0) + { + // _getTerminalPosition(_, false) will floor the x-value, + // meaning that the selection will start on the left-side + // of the current cell. This is great if the use is dragging + // towards the right. + // If the user is dragging towards the left (dx < 0), + // we want to select the current cell, so place the anchor on the right + // side of the current cell. + termPos.x++; + } + _core->SetSelectionAnchor(termPos); // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; From 37ad74f81cf52f727c6e01181b2e7264dfa642ad Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 3 Dec 2024 10:16:20 -0800 Subject: [PATCH 30/34] bugfix: double-click + drag would prematurely expand --- src/cascadia/TerminalCore/TerminalSelection.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index c583f8c8ab9..3bbaf2c1d7a 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -307,12 +307,14 @@ std::pair Terminal::_ExpandSelectionAnchors(std::pair _selection->pivot) { - end = buffer.GetWordEnd2(end, _wordDelimiters, false); + bufferSize.DecrementInExclusiveBounds(end); } + end = buffer.GetWordEnd2(end, _wordDelimiters, false); break; } case SelectionExpansion::Char: From dbfdf8159a06a0aee0e9e927af61b6c1b4a927b2 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 3 Dec 2024 12:01:42 -0800 Subject: [PATCH 31/34] fix tests; fix conhost block selection --- .../out/ut_textbuffer/UTextAdapterTests.cpp | 2 +- src/host/selection.cpp | 20 +++++++++---- src/host/ut_host/SearchTests.cpp | 30 +++++++++---------- src/host/ut_host/SelectionTests.cpp | 6 ++-- 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp b/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp index c9280b201c0..81974fe83ab 100644 --- a/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp +++ b/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp @@ -57,7 +57,7 @@ class UTextAdapterTests actual = buffer.SearchText(L"𝒷", SearchFlag::None); VERIFY_ARE_EQUAL(expected, actual); - expected = std::vector{ s(12, 17) }; + expected = std::vector{ s(12, 16) }; actual = buffer.SearchText(L"ネコ", SearchFlag::None); VERIFY_ARE_EQUAL(expected, actual); } diff --git a/src/host/selection.cpp b/src/host/selection.cpp index e95387125cf..ff599fc3dc2 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -43,14 +43,24 @@ void Selection::_RegenerateSelectionSpans() const 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 uses an inclusive range for selection. GetTextSpans() needs an exclusive range. - // Increment the "end" point (bottom-right-most) to make it exclusive. - // NOTE: if start is past end, increment start instead + // GH #18106: Conhost uses an inclusive range for selection, + // but GetTextSpans() needs an exclusive range. + const auto blockSelection = !IsLineSelection(); const auto& buffer = screenInfo.GetTextBuffer(); auto startSelectionAnchor = _d->coordSelectionAnchor; - buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor); + if (blockSelection) + { + auto& targetEndpoint = startSelectionAnchor.x <= endSelectionAnchor.x ? endSelectionAnchor : startSelectionAnchor; + if (targetEndpoint.x != buffer.GetSize().RightInclusive()) + { + buffer.GetSize().IncrementInBounds(targetEndpoint); + } + } + else + { + buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor); + } - const auto blockSelection = !IsLineSelection(); _lastSelectionSpans = buffer.GetTextSpans(startSelectionAnchor, endSelectionAnchor, blockSelection, diff --git a/src/host/ut_host/SearchTests.cpp b/src/host/ut_host/SearchTests.cpp index e68a5359f4a..d37c3556d40 100644 --- a/src/host/ut_host/SearchTests.cpp +++ b/src/host/ut_host/SearchTests.cpp @@ -55,12 +55,12 @@ class SearchTests return true; } - static void DoFoundChecks(Search& s, til::point coordStartExpected, til::CoordType lineDelta, bool reverse, bool japaneseChar) + static void DoFoundChecks(Search& s, til::point coordStartExpected, til::CoordType lineDelta, bool reverse) { const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto coordEndExpected = coordStartExpected; - coordEndExpected.x += japaneseChar ? 2 : 3; + coordEndExpected.x += 2; VERIFY_IS_TRUE(s.SelectCurrent()); VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor()); @@ -97,7 +97,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"AB", SearchFlag::None, false); - DoFoundChecks(s, {}, 1, false, false); + DoFoundChecks(s, {}, 1, false); } TEST_METHOD(ForwardCaseSensitiveJapanese) @@ -105,7 +105,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::None, false); - DoFoundChecks(s, { 2, 0 }, 1, false, true); + DoFoundChecks(s, { 2, 0 }, 1, false); } TEST_METHOD(ForwardCaseInsensitive) @@ -114,7 +114,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"ab", SearchFlag::CaseInsensitive, false); - DoFoundChecks(s, {}, 1, false, false); + DoFoundChecks(s, {}, 1, false); } TEST_METHOD(ForwardCaseInsensitiveJapanese) @@ -122,7 +122,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::CaseInsensitive, false); - DoFoundChecks(s, { 2, 0 }, 1, false, true); + DoFoundChecks(s, { 2, 0 }, 1, false); } TEST_METHOD(BackwardCaseSensitive) @@ -130,7 +130,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"AB", SearchFlag::None, true); - DoFoundChecks(s, { 0, 3 }, -1, true, false); + DoFoundChecks(s, { 0, 3 }, -1, true); } TEST_METHOD(BackwardCaseSensitiveJapanese) @@ -138,7 +138,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::None, true); - DoFoundChecks(s, { 2, 3 }, -1, true, true); + DoFoundChecks(s, { 2, 3 }, -1, true); } TEST_METHOD(BackwardCaseInsensitive) @@ -146,7 +146,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"ab", SearchFlag::CaseInsensitive, true); - DoFoundChecks(s, { 0, 3 }, -1, true, false); + DoFoundChecks(s, { 0, 3 }, -1, true); } TEST_METHOD(BackwardCaseInsensitiveJapanese) @@ -154,7 +154,7 @@ class SearchTests auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); Search s; s.Reset(gci.renderData, L"\x304b", SearchFlag::CaseInsensitive, true); - DoFoundChecks(s, { 2, 3 }, -1, true, true); + DoFoundChecks(s, { 2, 3 }, -1, true); } TEST_METHOD(ForwardCaseSensitiveRegex) @@ -163,7 +163,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"[BA]{2}", SearchFlag::RegularExpression, false); - DoFoundChecks(s, {}, 1, false, false); + DoFoundChecks(s, {}, 1, false); } TEST_METHOD(ForwardCaseSensitiveRegexJapanese) @@ -172,7 +172,7 @@ class SearchTests Search s; // N.B. this is not a literal U+30xx, but a regex escape sequence \x{30xx} s.Reset(gci.renderData, LR"-([\x{3041}-\x{304c}])-", SearchFlag::RegularExpression, false); - DoFoundChecks(s, { 2, 0 }, 1, false, true); + DoFoundChecks(s, { 2, 0 }, 1, false); } TEST_METHOD(ForwardCaseInsensitiveRegex) @@ -181,7 +181,7 @@ class SearchTests Search s; s.Reset(gci.renderData, L"ab", SearchFlag::CaseInsensitive | SearchFlag::RegularExpression, false); - DoFoundChecks(s, {}, 1, false, false); + DoFoundChecks(s, {}, 1, false); } TEST_METHOD(ForwardCaseInsensitiveRegexJapanese) @@ -190,7 +190,7 @@ class SearchTests Search s; // N.B. this is not a literal U+30xx, but a regex escape sequence \x{30xx} s.Reset(gci.renderData, LR"-([\x{3041}-\x{304c}])-", SearchFlag::CaseInsensitive | SearchFlag::RegularExpression, false); - DoFoundChecks(s, { 2, 0 }, 1, false, true); + DoFoundChecks(s, { 2, 0 }, 1, false); } TEST_METHOD(ForwardCaseSensitiveRegexWithCaseInsensitiveFlag) @@ -199,6 +199,6 @@ class SearchTests Search s; s.Reset(gci.renderData, L"(?i)ab", SearchFlag::RegularExpression, false); - DoFoundChecks(s, {}, 1, false, false); + DoFoundChecks(s, {}, 1, false); } }; diff --git a/src/host/ut_host/SelectionTests.cpp b/src/host/ut_host/SelectionTests.cpp index 62eac812053..cd27e1eae5d 100644 --- a/src/host/ut_host/SelectionTests.cpp +++ b/src/host/ut_host/SelectionTests.cpp @@ -74,7 +74,7 @@ class SelectionTests VERIFY_ARE_EQUAL(span.end.y, sRectangleLineNumber); VERIFY_ARE_EQUAL(span.start.x, m_pSelection->_d->srSelectionRect.left); - VERIFY_ARE_EQUAL(span.end.x, m_pSelection->_d->srSelectionRect.right); + VERIFY_ARE_EQUAL(span.end.x, m_pSelection->_d->srSelectionRect.right + 1); } } } @@ -149,7 +149,9 @@ class SelectionTests { auto& span{ selectionSpans[0] }; VERIFY_ARE_EQUAL(start, span.start, L"start"); - VERIFY_ARE_EQUAL(end, span.end, L"end"); + + const til::point exclusiveEnd{ end.x + 1, end.y }; + VERIFY_ARE_EQUAL(exclusiveEnd, span.end, L"end"); } } From bee9371c0f00d36ec206bce740f6a614e6a1e06e Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 3 Dec 2024 14:37:17 -0800 Subject: [PATCH 32/34] fix terminal core selection tests --- .../UnitTests_TerminalCore/SelectionTest.cpp | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp index 339c2927ff2..94021cf9bd7 100644 --- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp @@ -414,7 +414,7 @@ namespace TerminalCoreUnitTests term.MultiClickSelection(clickPos, Terminal::SelectionExpansion::Word); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); } TEST_METHOD(DoubleClick_Delimiter) @@ -459,8 +459,8 @@ namespace TerminalCoreUnitTests // "Terminal" is in class 2 // ":" and ">" are in class 1 // the white space to the right of the ">" is in class 0 - // Double-clicking the ">" should highlight that cell and the whitespace - ValidateLinearSelection(term, { 15, 10 }, { term.GetViewport().RightExclusive(), 10 }); + // Double-clicking the ">" should only highlight that cell + ValidateLinearSelection(term, { 15, 10 }, { 16, 10 }); } TEST_METHOD(DoubleClickDrag_Right) @@ -489,7 +489,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); } TEST_METHOD(DoubleClickDrag_Left) @@ -518,7 +518,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 5, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { 18, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); } TEST_METHOD(TripleClick_GeneralCase) @@ -589,7 +589,7 @@ namespace TerminalCoreUnitTests term.MultiClickSelection({ 5, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe" selected - ValidateLinearSelection(term, { 4, 10 }, { 18, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 17, 10 }); } Log::Comment(L"Step 2: Shift+Click to \"dragThroughHere\""); @@ -615,7 +615,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe dragThroughHere " selected - ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } Log::Comment(L"Step 4: Shift+Triple-Click at \"dragThroughHere\""); @@ -641,7 +641,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe dragThroughHere" selected - ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } Log::Comment(L"Step 6: Drag past \"dragThroughHere\""); @@ -649,13 +649,13 @@ namespace TerminalCoreUnitTests // Simulate drag to (x,y) = (35,10) // Since we were preceded by a double-click, we're in "word" expansion mode // - // buffer: doubleClickMe dragThroughHere | + // buffer: doubleClickMe dragThroughHere anotherWord // ^ ^ // start finish (boundary) term.SetSelectionEnd({ 35, 10 }); // Validate selection area: "doubleClickMe dragThroughHere..." selected - ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); } Log::Comment(L"Step 7: Drag back to \"dragThroughHere\""); @@ -663,12 +663,12 @@ namespace TerminalCoreUnitTests // Simulate drag to (x,y) = (21,10) // // buffer: doubleClickMe dragThroughHere anotherWord - // ^ ^ ^ - // start drag finish + // ^ ^ ^ + // start drag finish term.SetSelectionEnd({ 21, 10 }); // Validate selection area: "doubleClickMe dragThroughHere " selected - ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } Log::Comment(L"Step 8: Drag within \"dragThroughHere\""); @@ -676,12 +676,12 @@ namespace TerminalCoreUnitTests // Simulate drag to (x,y) = (25,10) // // buffer: doubleClickMe dragThroughHere anotherWord - // ^ ^ ^ - // start drag finish + // ^ ^ ^ + // start drag finish term.SetSelectionEnd({ 25, 10 }); // Validate selection area: "doubleClickMe dragThroughHere" still selected - ValidateLinearSelection(term, { 4, 10 }, { 34, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } } From 44a1e4263c8da3ab6750b83da6d1678f4a2ca1a6 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 13 Dec 2024 17:26:19 -0800 Subject: [PATCH 33/34] fix tests and add more comments to preserve sanity --- src/buffer/out/textBuffer.cpp | 2 +- .../TerminalCore/terminalrenderdata.cpp | 5 + .../UnitTests_TerminalCore/SelectionTest.cpp | 60 ++++++---- src/host/ut_host/TextBufferTests.cpp | 106 ++++++++---------- 4 files changed, 89 insertions(+), 84 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index ce77a86913e..69ec7092e5c 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2158,7 +2158,7 @@ std::wstring TextBuffer::GetPlainText(const CopyRequest& req) const const auto& row = GetRowByOffset(iRow); const auto& [rowBeg, rowEnd, addLineBreak] = _RowCopyHelper(req, iRow, row); - // save selected text + // save selected text (exclusive end) selectedText += row.GetText(rowBeg, rowEnd); if (addLineBreak && iRow != req.end.y) diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index c381d478451..d7bca1077cc 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -201,6 +201,11 @@ til::CoordType Terminal::_ScrollToPoints(const til::point coordStart, const til: return _VisibleStartIndex(); } +// Method Description: +// - selects the region from coordStart to coordEnd +// Arguments: +// - coordStart - The start point (inclusive) +// - coordEnd - The end point (inclusive) void Terminal::SelectNewRegion(const til::point coordStart, const til::point coordEnd) { const auto newScrollOffset = _ScrollToPoints(coordStart, coordEnd); diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp index 94021cf9bd7..1585fef905f 100644 --- a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp @@ -356,12 +356,26 @@ namespace TerminalCoreUnitTests GetTextBuffer(term).GetCursor().SetPosition({ 7, 11 }); term.Write(burrito); + // Text buffer should look like this: + // ------------- + // | A | + // | | + // | 🌯 | + // | 🌯 | + // | B | + // ------------- + // A: selection anchor + // B: selection end + // The boundaries of the selection should cut through + // the middle of the burritos, but the selection + // should expand to encompass each burrito entirely. + // Simulate ALT + click at (x,y) = (5,8) term.SetSelectionAnchor({ 5, 8 }); term.SetBlockSelection(true); - // Simulate move to (x,y) = (7,12) - term.SetSelectionEnd({ 7, 12 }); + // Simulate move to (x,y) = (8,12) + term.SetSelectionEnd({ 8, 12 }); // Simulate renderer calling TriggerSelection and acquiring selection area auto selectionSpans = term.GetSelectionSpans(); @@ -376,18 +390,18 @@ namespace TerminalCoreUnitTests if (rowValue == 10) { VERIFY_ARE_EQUAL((til::point{ 4, rowValue }), sp.start); - VERIFY_ARE_EQUAL((til::point{ 7, rowValue }), sp.end); + VERIFY_ARE_EQUAL((til::point{ 8, rowValue }), sp.end); } else if (rowValue == 11) { VERIFY_ARE_EQUAL((til::point{ 5, rowValue }), sp.start); - VERIFY_ARE_EQUAL((til::point{ 7, rowValue }), sp.end); + VERIFY_ARE_EQUAL((til::point{ 9, rowValue }), sp.end); } else { // Verify all lines VERIFY_ARE_EQUAL((til::point{ 5, rowValue }), sp.start); - VERIFY_ARE_EQUAL((til::point{ 7, rowValue }), sp.end); + VERIFY_ARE_EQUAL((til::point{ 8, rowValue }), sp.end); } rowValue++; @@ -489,7 +503,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } TEST_METHOD(DoubleClickDrag_Left) @@ -518,7 +532,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 5, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } TEST_METHOD(TripleClick_GeneralCase) @@ -614,7 +628,7 @@ namespace TerminalCoreUnitTests // start click finish term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); - // Validate selection area: "doubleClickMe dragThroughHere " selected + // Validate selection area: "doubleClickMe dragThroughHere" selected ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } @@ -628,7 +642,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Line); // Validate selection area: "doubleClickMe dragThroughHere..." selected - ValidateLinearSelection(term, { 4, 10 }, { term.GetViewport().RightExclusive(), 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 100, 10 }); } Log::Comment(L"Step 5: Shift+Double-Click at \"dragThroughHere\""); @@ -636,8 +650,9 @@ namespace TerminalCoreUnitTests // Simulate Shift+DoubleClick at (x,y) = (21,10) // // buffer: doubleClickMe dragThroughHere anotherWord - // ^ ^ ^ - // start click finish + // ^ ^ ^ + // start click finish + // NOTE: end is exclusive, so finish should point to the spot AFTER "dragThroughHere" term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe dragThroughHere" selected @@ -650,24 +665,25 @@ namespace TerminalCoreUnitTests // Since we were preceded by a double-click, we're in "word" expansion mode // // buffer: doubleClickMe dragThroughHere anotherWord - // ^ ^ - // start finish (boundary) + // ^ ^ + // start finish term.SetSelectionEnd({ 35, 10 }); - // Validate selection area: "doubleClickMe dragThroughHere..." selected - ValidateLinearSelection(term, { 4, 10 }, { gsl::narrow(4 + text.size()), 10 }); + // Validate selection area: "doubleClickMe dragThroughHere anotherWord" selected + ValidateLinearSelection(term, { 4, 10 }, { 45, 10 }); } Log::Comment(L"Step 7: Drag back to \"dragThroughHere\""); { // Simulate drag to (x,y) = (21,10) + // Should still be in "word" expansion mode! // // buffer: doubleClickMe dragThroughHere anotherWord - // ^ ^ ^ - // start drag finish + // ^ ^ + // start finish term.SetSelectionEnd({ 21, 10 }); - // Validate selection area: "doubleClickMe dragThroughHere " selected + // Validate selection area: "doubleClickMe dragThroughHere" selected ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } @@ -676,8 +692,8 @@ namespace TerminalCoreUnitTests // Simulate drag to (x,y) = (25,10) // // buffer: doubleClickMe dragThroughHere anotherWord - // ^ ^ ^ - // start drag finish + // ^ ^ + // start finish term.SetSelectionEnd({ 25, 10 }); // Validate selection area: "doubleClickMe dragThroughHere" still selected @@ -705,7 +721,9 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 5, 10 }); // Validate selection area - // NOTE: Pivot should be (10, 10) + // NOTES: + // - Pivot should be (10, 10) + // - though end is generally exclusive, since we moved behind the pivot, end is actually inclusive ValidateLinearSelection(term, { 5, 10 }, { 10, 10 }); } diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index d92d82d10e4..4252609e30f 100644 --- a/src/host/ut_host/TextBufferTests.cpp +++ b/src/host/ut_host/TextBufferTests.cpp @@ -2494,8 +2494,9 @@ void TextBufferTests::GetPlainText() L" 3 " }; WriteLinesToBuffer(bufferText, *_buffer); - // simulate a selection from origin to {4,4} - constexpr til::point_span selection = { { 0, 0 }, { 4, 4 } }; + // simulate a selection from origin to {5,4} + // Remember! End is exclusive! + constexpr til::point_span selection = { { 0, 0 }, { 5, 4 } }; const auto req = TextBuffer::CopyRequest{ *_buffer, selection.start, selection.end, blockSelection, includeCRLF, trimTrailingWhitespace, false }; const auto result = _buffer->GetPlainText(req); @@ -2506,31 +2507,21 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"Standard Copy to Clipboard"); - if (blockSelection) - { - expectedText += L"1234\r\n"; - expectedText += L" 34\r\n"; - expectedText += L"123\r\n"; - expectedText += L" 3\r\n"; - } - else - { - expectedText += L"12345\r\n"; - expectedText += L" 345\r\n"; - expectedText += L"123\r\n"; - expectedText += L" 3\r\n"; - } + expectedText += L"12345\r\n"; + expectedText += L" 345\r\n"; + expectedText += L"123\r\n"; + expectedText += L" 3\r\n"; } else { Log::Comment(L"UI Automation"); if (blockSelection) { - expectedText += L"1234\r\n"; - expectedText += L" 34\r\n"; - expectedText += L"123 \r\n"; - expectedText += L" 3 \r\n"; - expectedText += L" "; + expectedText += L"12345\r\n"; + expectedText += L" 345\r\n"; + expectedText += L"123 \r\n"; + expectedText += L" 3 \r\n"; + expectedText += L" "; } else { @@ -2538,7 +2529,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345 \r\n"; expectedText += L"123 \r\n"; expectedText += L" 3 \r\n"; - expectedText += L" "; + expectedText += L" "; } } } @@ -2547,31 +2538,21 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"UNDEFINED"); - if (blockSelection) - { - expectedText += L"1234"; - expectedText += L" 34"; - expectedText += L"123"; - expectedText += L" 3"; - } - else - { - expectedText += L"12345"; - expectedText += L" 345"; - expectedText += L"123"; - expectedText += L" 3"; - } + expectedText += L"12345"; + expectedText += L" 345"; + expectedText += L"123"; + expectedText += L" 3"; } else { Log::Comment(L"Shift+Copy to Clipboard"); if (blockSelection) { - expectedText += L"1234"; - expectedText += L" 34"; - expectedText += L"123 "; - expectedText += L" 3 "; - expectedText += L" "; + expectedText += L"12345"; + expectedText += L" 345"; + expectedText += L"123 "; + expectedText += L" 3 "; + expectedText += L" "; } else { @@ -2579,7 +2560,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345 "; expectedText += L"123 "; expectedText += L" 3 "; - expectedText += L" "; + expectedText += L" "; } } } @@ -2609,8 +2590,9 @@ void TextBufferTests::GetPlainText() // | | // |_____| - // simulate a selection from origin to {4,5} - constexpr til::point_span selection = { { 0, 0 }, { 4, 5 } }; + // simulate a selection from origin to {5,5} + // Remember! End is exclusive! + constexpr til::point_span selection = { { 0, 0 }, { 5, 5 } }; const auto formatWrappedRows = blockSelection; const auto req = TextBuffer::CopyRequest{ *_buffer, selection.start, selection.end, blockSelection, includeCRLF, trimTrailingWhitespace, formatWrappedRows }; @@ -2624,21 +2606,21 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"UNDEFINED"); - expectedText += L"1234\r\n"; + expectedText += L"12345\r\n"; expectedText += L"67\r\n"; - expectedText += L" 34\r\n"; + expectedText += L" 345\r\n"; expectedText += L"123\r\n"; expectedText += L"\r\n"; } else { Log::Comment(L"Copy block selection to Clipboard"); - expectedText += L"1234\r\n"; - expectedText += L"67 \r\n"; - expectedText += L" 34\r\n"; - expectedText += L"123 \r\n"; - expectedText += L" \r\n"; - expectedText += L" "; + expectedText += L"12345\r\n"; + expectedText += L"67 \r\n"; + expectedText += L" 345\r\n"; + expectedText += L"123 \r\n"; + expectedText += L" \r\n"; + expectedText += L" "; } } else @@ -2646,20 +2628,20 @@ void TextBufferTests::GetPlainText() if (trimTrailingWhitespace) { Log::Comment(L"UNDEFINED"); - expectedText += L"1234"; + expectedText += L"12345"; expectedText += L"67"; - expectedText += L" 34"; + expectedText += L" 345"; expectedText += L"123"; } else { Log::Comment(L"UNDEFINED"); - expectedText += L"1234"; - expectedText += L"67 "; - expectedText += L" 34"; - expectedText += L"123 "; - expectedText += L" "; - expectedText += L" "; + expectedText += L"12345"; + expectedText += L"67 "; + expectedText += L" 345"; + expectedText += L"123 "; + expectedText += L" "; + expectedText += L" "; } } } @@ -2683,7 +2665,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345"; expectedText += L"123 "; expectedText += L" \r\n"; - expectedText += L" "; + expectedText += L" "; } } else @@ -2704,7 +2686,7 @@ void TextBufferTests::GetPlainText() expectedText += L" 345"; expectedText += L"123 "; expectedText += L" "; - expectedText += L" "; + expectedText += L" "; } } } From 476465339833a06b54cd2fc533cccd865b2bb4f5 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 18 Dec 2024 13:36:16 -0800 Subject: [PATCH 34/34] simplify and add comment --- src/buffer/out/textBuffer.cpp | 5 ++--- src/host/selection.cpp | 25 ++++++++++--------------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 69ec7092e5c..9413b9d213e 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1927,12 +1927,10 @@ const std::vector TextBuffer::GetTextRects(til::point start { std::vector textRects; - const auto bufferSize = GetSize(); - // (0,0) is the top-left of the screen // the physically "higher" coordinate is closer to the top-left // the physically "lower" coordinate is closer to the bottom-right - const auto [higherCoord, lowerCoord] = bufferSize.CompareInBounds(start, end) <= 0 ? + const auto [higherCoord, lowerCoord] = start <= end ? std::make_tuple(start, end) : std::make_tuple(end, start); @@ -1953,6 +1951,7 @@ const std::vector TextBuffer::GetTextRects(til::point start } else { + const auto bufferSize = GetSize(); textRow.left = (row == higherCoord.y) ? higherCoord.x : bufferSize.Left(); textRow.right = (row == lowerCoord.y) ? lowerCoord.x : bufferSize.RightInclusive(); } diff --git a/src/host/selection.cpp b/src/host/selection.cpp index ff599fc3dc2..0856cc9b201 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -43,24 +43,19 @@ void Selection::_RegenerateSelectionSpans() const 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 uses an inclusive range for selection, - // but GetTextSpans() needs an exclusive range. + // 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. + // Note that Conhost defines start/end as "start was set in time before end", + // whereas above (and in Terminal) we're treating start/end as "start is physically before end". + // We want Conhost to still operate as an inclusive range. + // To make it "feel" inclusive, we need to adjust the "end" endpoint + // by incrementing it by one, so that the "end" endpoint is rendered + // and handled as selected. const auto blockSelection = !IsLineSelection(); const auto& buffer = screenInfo.GetTextBuffer(); auto startSelectionAnchor = _d->coordSelectionAnchor; - if (blockSelection) - { - auto& targetEndpoint = startSelectionAnchor.x <= endSelectionAnchor.x ? endSelectionAnchor : startSelectionAnchor; - if (targetEndpoint.x != buffer.GetSize().RightInclusive()) - { - buffer.GetSize().IncrementInBounds(targetEndpoint); - } - } - else - { - buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor); - } - + buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor.x <= endSelectionAnchor.x ? endSelectionAnchor : startSelectionAnchor); _lastSelectionSpans = buffer.GetTextSpans(startSelectionAnchor, endSelectionAnchor, blockSelection,