From 64d4fbab17f181c19b502d3170f36f8e16c594b1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Tue, 28 Jan 2025 14:54:49 -0800 Subject: [PATCH] Make selection an exclusive range (#18106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Selection is generally stored as an inclusive start and end. This PR makes the end exclusive which now allows degenerate selections, namely in mark mode. This also modifies mouse selection to round to the nearest cell boundary (see #5099) and improves word boundaries to be a bit more modern and make sense for degenerate selections (similar to #15787). Closes #5099 Closes #13447 Closes #17892 ## Detailed Description of the Pull Request / Additional comments - Buffer, Viewport, and Point - Introduced a few new functions here to find word boundaries, delimiter class runs, and glyph boundaries. - 📝These new functions should be able to replace a few other functions (i.e. `GetWordStart` --> `GetWordStart2`). That migration is going to be a part of #4423 to reduce the risk of breaking UIA. - Viewport: added a few functions to handle navigating the _exclusive_ bounds (namely allowing RightExclusive as a position for buffer coordinates). This is important for selection to be able to highlight the entire line. - 📝`BottomInclusiveRightExclusive()` will replace `EndExclusive` in the UIA code - Point: `iterate_rows_exclusive` is similar to `iterate_rows`, except it has handling for RightExclusive - Renderer - Use `iterate_rows_exclusive` for proper handling (this actually fixed a lot of our issues) - Remove some workarounds in `_drawHighlighted` (this is a boundary where we got inclusive coords and made them exclusive, but now we don't need that!) - Terminal - fix selection marker rendering - `_ConvertToBufferCell()`: add a param to allow for RightExclusive or clamp it to RightInclusive (original behavior). Both are useful! - Use new `GetWordStart2` and `GetWordEnd2` to improve word boundaries and make them feel right now that the selection an exclusive range. - Convert a few `IsInBounds` --> `IsInExclusiveBounds` for safety and correctness - Add `TriggerSelection` to `SelectNewRegion` - 📝 We normally called `TriggerSelection` in a different layer, but it turns out, UIA's `Select` function wouldn't actually update the renderer. Whoops! This fixes that. - TermControl - `_getTerminalPosition` now has a new param to round to the nearest cell (see #5099) - UIA - `TermControlUIAProvider::GetSelectionRange` no need to convert from inclusive range to exclusive range anymore! - `TextBuffer::GetPlainText` now works on an exclusive range, so no need to convert the range anymore! ## Validation Steps Performed This fundamental change impacts a lot of scenarios: - ✅Rendering selections - ✅Selection markers - ✅Copy text - ✅Session restore - ✅Mark mode navigation (i.e. character, word, line, buffer) - ✅Mouse selection (i.e. click+drag, shift+click, multi-click, alt+click) - ✅Hyperlinks (interaction and rendering) - ✅Accessibility (i.e. get selection, movement, text extraction, selecting text) - [ ] Prev/Next Command/Output (untested) - ✅Unit tests ## Follow-ups - Refs #4423 - Now that selection and UIA are both exclusive ranges, it should be a lot easier to deduplicate code between selection and UIA. We should be able to remove `EndExclusive` as well when we do that. This'll also be an opportunity to modernize that code and use more `til` classes. --- .github/actions/spelling/expect/expect.txt | 2 + src/buffer/out/Row.cpp | 28 +- src/buffer/out/Row.hpp | 5 +- src/buffer/out/UTextAdapter.cpp | 9 +- src/buffer/out/textBuffer.cpp | 320 ++++++++++++++++-- src/buffer/out/textBuffer.hpp | 10 + .../out/ut_textbuffer/UTextAdapterTests.cpp | 6 +- src/cascadia/TerminalControl/ControlCore.cpp | 8 +- .../TerminalControl/ControlInteractivity.cpp | 38 ++- .../TerminalControl/ControlInteractivity.h | 2 +- src/cascadia/TerminalCore/Terminal.cpp | 13 +- src/cascadia/TerminalCore/Terminal.hpp | 2 +- .../TerminalCore/TerminalSelection.cpp | 156 +++++---- .../TerminalCore/terminalrenderdata.cpp | 6 + .../UnitTests_Control/ControlCoreTests.cpp | 20 +- .../ControlInteractivityTests.cpp | 27 +- .../UnitTests_TerminalCore/SelectionTest.cpp | 159 +++++---- src/host/selection.cpp | 29 +- src/host/ut_host/ClipboardTests.cpp | 2 +- src/host/ut_host/SearchTests.cpp | 2 +- src/host/ut_host/SelectionTests.cpp | 10 +- src/host/ut_host/TextBufferTests.cpp | 24 +- src/inc/til/point.h | 36 ++ src/renderer/atlas/AtlasEngine.api.cpp | 4 +- src/renderer/atlas/AtlasEngine.cpp | 8 +- src/renderer/base/renderer.cpp | 3 +- src/types/TermControlUiaProvider.cpp | 8 +- src/types/UiaTextRangeBase.cpp | 13 +- src/types/inc/viewport.hpp | 6 + src/types/viewport.cpp | 60 ++++ 30 files changed, 738 insertions(+), 278 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index b77cf969f49..1a896e0ced6 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 @@ -369,6 +370,7 @@ DColor dcommon DComposition dde +DDDCCC DDESHARE DDevice DEADCHAR diff --git a/src/buffer/out/Row.cpp b/src/buffer/out/Row.cpp index 7a46215b6fc..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; } @@ -1114,6 +1119,9 @@ 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(); @@ -1219,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 ff0861ce54d..717d97812aa 100644 --- a/src/buffer/out/UTextAdapter.cpp +++ b/src/buffer/out/UTextAdapter.cpp @@ -411,16 +411,13 @@ 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) { 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; @@ -439,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/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index a920854b732..df30a3ffb23 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 @@ -1132,6 +1140,213 @@ 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, bool includeWhitespace, 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): + // 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. (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 (!includeWhitespace || 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); + } + return pos; +} + +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()) }; + + 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): + // 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. (includeWhitespace) if the next delimiter class run is a ControlChar, go forward one more delimiter class run + pos = _GetDelimiterClassRunEnd(pos, wordDelimiters); + if (!includeWhitespace || 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; + } + + if (const auto nextDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; + nextDelimClass == DelimiterClass::ControlChar) + { + 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(); pos = nextPos) + { + 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; + } + } + 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(); + const auto initialDelimClass = bufferSize.IsInBounds(pos) ? _GetDelimiterClassAt(pos, wordDelimiters) : DelimiterClass::ControlChar; + for (auto nextPos = pos; nextPos != bufferSize.BottomInclusiveRightExclusive(); pos = nextPos) + { + 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, + // apply the move and return + return nextPos; + } + } + return pos; +} + // Method Description: // - Get the til::point for the beginning of the word you are on // Arguments: @@ -1520,13 +1735,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 +1764,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 +1827,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 +1884,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 @@ -1660,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); @@ -1686,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(); } @@ -1710,7 +1976,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. @@ -1780,31 +2046,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; } } @@ -1821,8 +2073,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 @@ -1851,7 +2103,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 { @@ -1860,7 +2112,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. @@ -1905,7 +2157,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/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index c97de9e7523..775417caab0 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -170,9 +170,15 @@ 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; + + 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; @@ -180,6 +186,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; @@ -322,6 +330,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/buffer/out/ut_textbuffer/UTextAdapterTests.cpp b/src/buffer/out/ut_textbuffer/UTextAdapterTests.cpp index be9e941c757..81974fe83ab 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, 16) }; actual = buffer.SearchText(L"ネコ", SearchFlag::None); VERIFY_ARE_EQUAL(expected, actual); } diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index b46da5236af..72a8e85ad55 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; } @@ -1217,8 +1217,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation return; } + // clamp the converted position to be within the viewport bounds + // x: allow range of [0, RightExclusive] + // GH #18106: right exclusive needed for selection to support exclusive end til::point terminalPosition{ - std::clamp(position.x, 0, _terminal->GetViewport().Width() - 1), + std::clamp(position.x, 0, _terminal->GetViewport().Width()), std::clamp(position.y, 0, _terminal->GetViewport().Height() - 1) }; @@ -2722,7 +2725,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/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 1da3b89ab1d..f4c0feed2a3 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -241,7 +241,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 }, true); const auto altEnabled = modifiers.IsAltPressed(); const auto shiftEnabled = modifiers.IsShiftPressed(); @@ -338,7 +338,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; @@ -372,7 +372,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 })); + 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; @@ -428,7 +440,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)) { @@ -475,7 +487,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. // @@ -662,7 +674,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; } @@ -672,12 +684,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 18d55b53cfa..dcf2c811d02 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 67a1523e99b..9e6350df044 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -478,7 +478,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) @@ -502,12 +502,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, true); } } else @@ -544,7 +540,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: @@ -1466,7 +1462,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)); } diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 0ff2cb1955e..1459c39fc3c 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -478,7 +478,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) 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 6a800f329dd..9501f64a85e 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -93,14 +93,21 @@ 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 // 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 }; @@ -112,14 +119,21 @@ til::point Terminal::SelectionStartForRendering() const til::point Terminal::SelectionEndForRendering() const { auto pos{ _selection->end }; - const auto bufferSize{ GetTextBuffer().GetSize() }; - if (pos.x != bufferSize.RightInclusive()) + const auto& buffer = GetTextBuffer(); + const auto bufferSize{ buffer.GetSize() }; + if (bufferSize.IsInBounds(pos) && buffer.GetCellDataAt(pos)->DbcsAttr() == DbcsAttribute::Trailing) { - // In general, we need to draw the marker one after the - // end of the selection. + // 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 + // 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 skip this step. - bufferSize.IncrementInBounds(pos); + // 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 }; @@ -157,7 +171,7 @@ 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; _multiClickSelectionMode = expansionMode; @@ -179,7 +193,7 @@ 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; _multiClickSelectionMode = SelectionExpansion::Char; @@ -198,7 +212,7 @@ void Terminal::SetSelectionEnd(const til::point viewportPos, std::optional newExpansionMode) { wil::hide_name _selection; @@ -209,7 +223,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 @@ -248,7 +267,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 @@ -273,17 +292,31 @@ std::pair Terminal::_ExpandSelectionAnchors(std::pair _selection->pivot) + { + bufferSize.DecrementInExclusiveBounds(end); + } + end = buffer.GetWordEnd2(end, _wordDelimiters, false); break; + } case SelectionExpansion::Char: default: // no expansion is necessary @@ -376,9 +409,9 @@ void Terminal::ExpandSelectionToWord() const auto& buffer = _activeBuffer(); auto selection{ _selection.write() }; wil::hide_name _selection; - selection->start = buffer.GetWordStart(selection->start, _wordDelimiters); + selection->start = buffer.GetWordStart2(selection->start, _wordDelimiters, false); selection->pivot = selection->start; - selection->end = buffer.GetWordEnd(selection->end, _wordDelimiters); + selection->end = buffer.GetWordEnd2(selection->end, _wordDelimiters, false); // if we're targeting both endpoints, instead just target "end" if (WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::Start) && WI_IsFlagSet(_selectionEndpoint, SelectionEndpoint::End)) @@ -529,7 +562,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; } @@ -625,7 +657,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: @@ -695,12 +727,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: { @@ -714,7 +746,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; } } @@ -722,61 +754,45 @@ void Terminal::_MoveByChar(SelectionDirection direction, til::point& pos) void Terminal::_MoveByWord(SelectionDirection direction, til::point& pos) { + const auto& buffer = _activeBuffer(); switch (direction) { 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 + auto nextPos = pos; + nextPos = buffer.GetWordStart2(nextPos, _wordDelimiters, true); + 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, true); } + pos = nextPos; break; } case SelectionDirection::Right: { - const auto wordEndPos{ _activeBuffer().GetWordEnd(pos, _wordDelimiters) }; - if (_activeBuffer().GetSize().CompareInBounds(pos, _selection->pivot) < 0) + const auto mutableViewportEndExclusive = _GetMutableViewport().BottomInclusiveRightExclusive(); + auto nextPos = pos; + nextPos = buffer.GetWordEnd2(nextPos, _wordDelimiters, true, mutableViewportEndExclusive); + if (nextPos == pos) { - // 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; + // didn't move because we're already at the end of a word, + // so move to the end of the next word + buffer.GetSize().IncrementInExclusiveBounds(nextPos); + nextPos = buffer.GetWordEnd2(nextPos, _wordDelimiters, true, mutableViewportEndExclusive); } + pos = nextPos; break; } case SelectionDirection::Up: _MoveByChar(direction, pos); - pos = _activeBuffer().GetWordStart(pos, _wordDelimiters); + pos = buffer.GetWordStart2(pos, _wordDelimiters, true); break; case SelectionDirection::Down: _MoveByChar(direction, pos); - pos = _activeBuffer().GetWordEnd(pos, _wordDelimiters); + pos = buffer.GetWordEnd2(pos, _wordDelimiters, true); break; } } @@ -790,7 +806,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 +820,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; } } @@ -821,7 +837,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; } } @@ -901,13 +917,17 @@ Terminal::TextCopyData Terminal::RetrieveSelectedTextFromBuffer(const bool singl // - convert viewport position to the corresponding location on the buffer // Arguments: // - viewportPos: a coordinate on the viewport +// - allowRightExclusive: if true, clamp to the right exclusive boundary of the buffer. +// Careful! This position doesn't point to any data in the buffer! // Return Value: // - the corresponding location on the buffer -til::point Terminal::_ConvertToBufferCell(const til::point viewportPos) const +til::point Terminal::_ConvertToBufferCell(const til::point viewportPos, bool allowRightExclusive) const { const auto yPos = _VisibleStartIndex() + viewportPos.y; til::point bufferPos = { viewportPos.x, yPos }; - _activeBuffer().GetSize().Clamp(bufferPos); + const auto bufferSize = _activeBuffer().GetSize(); + bufferPos.x = std::clamp(bufferPos.x, bufferSize.Left(), allowRightExclusive ? bufferSize.RightExclusive() : bufferSize.RightInclusive()); + bufferPos.y = std::clamp(bufferPos.y, bufferSize.Top(), bufferSize.BottomInclusive()); return bufferPos; } @@ -917,7 +937,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/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index 006d87aabe5..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); @@ -210,6 +215,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/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..282b1995da7 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -438,8 +438,9 @@ 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, @@ -449,10 +450,11 @@ namespace ControlUnitTests 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 diff --git a/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp b/src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp index 11f01c75297..1585fef905f 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) @@ -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{ 8, 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++; @@ -414,7 +428,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 }, { gsl::narrow(4 + text.size()), 10 }); } TEST_METHOD(DoubleClick_Delimiter) @@ -432,7 +446,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 +471,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 }); + ValidateLinearSelection(term, { 15, 10 }, { 16, 10 }); } TEST_METHOD(DoubleClickDrag_Right) @@ -489,7 +503,7 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }); // Validate selection area - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } TEST_METHOD(DoubleClickDrag_Left) @@ -502,7 +516,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 +527,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 }, { 33, 10 }); } TEST_METHOD(TripleClick_GeneralCase) @@ -532,7 +546,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 +563,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 +579,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 +593,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 }, { 17, 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 +616,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) // @@ -615,10 +629,10 @@ namespace TerminalCoreUnitTests term.SetSelectionEnd({ 21, 10 }, Terminal::SelectionExpansion::Word); // Validate selection area: "doubleClickMe dragThroughHere" selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 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,60 +642,62 @@ 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 }, { 100, 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 + // 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 - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 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 // - // buffer: doubleClickMe dragThroughHere | - // ^ ^ - // start finish (boundary) + // buffer: doubleClickMe dragThroughHere anotherWord + // ^ ^ + // start finish term.SetSelectionEnd({ 35, 10 }); - // Validate selection area: "doubleClickMe dragThroughHere..." selected - ValidateLinearSelection(term, { 4, 10 }, { 99, 10 }); + // Validate selection area: "doubleClickMe dragThroughHere anotherWord" selected + ValidateLinearSelection(term, { 4, 10 }, { 45, 10 }); } - // Step 6: Drag back to "dragThroughHere" + 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 - // ^ ^ ^ - // start drag finish + // buffer: doubleClickMe dragThroughHere anotherWord + // ^ ^ + // start finish term.SetSelectionEnd({ 21, 10 }); // Validate selection area: "doubleClickMe dragThroughHere" selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 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 finish term.SetSelectionEnd({ 25, 10 }); // Validate selection area: "doubleClickMe dragThroughHere" still selected - ValidateLinearSelection(term, { 4, 10 }, { 32, 10 }); + ValidateLinearSelection(term, { 4, 10 }, { 33, 10 }); } } @@ -691,25 +707,27 @@ 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 }); // 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 }); } - // Step 3: Drag back to (20,10) + Log::Comment(L"Step 3: Drag back to (20,10)"); { term.SetSelectionEnd({ 20, 10 }); @@ -718,7 +736,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 +745,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 }); } } }; diff --git a/src/host/selection.cpp b/src/host/selection.cpp index 300068d9671..d7dcf21ebe2 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -43,11 +43,32 @@ 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 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(); - _lastSelectionSpans = screenInfo.GetTextBuffer().GetTextSpans(_d->coordSelectionAnchor, - endSelectionAnchor, - blockSelection, - false); + const auto& buffer = screenInfo.GetTextBuffer(); + auto startSelectionAnchor = _d->coordSelectionAnchor; + if (blockSelection) + { + // Compare x-values when we're in block selection! + buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor.x <= endSelectionAnchor.x ? endSelectionAnchor : startSelectionAnchor); + } + else + { + // General comparison for line selection. + buffer.GetSize().IncrementInExclusiveBounds(startSelectionAnchor <= endSelectionAnchor ? endSelectionAnchor : startSelectionAnchor); + } + _lastSelectionSpans = buffer.GetTextSpans(startSelectionAnchor, + endSelectionAnchor, + blockSelection, + false); _lastSelectionGeneration = _d.generation(); } 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..d37c3556d40 100644 --- a/src/host/ut_host/SearchTests.cpp +++ b/src/host/ut_host/SearchTests.cpp @@ -60,7 +60,7 @@ class SearchTests const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); auto coordEndExpected = coordStartExpected; - coordEndExpected.x += 1; + coordEndExpected.x += 2; VERIFY_IS_TRUE(s.SelectCurrent()); VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor()); diff --git a/src/host/ut_host/SelectionTests.cpp b/src/host/ut_host/SelectionTests.cpp index 62eac812053..f314e8dcc9e 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); } } } @@ -141,15 +141,17 @@ class SelectionTests } } - void VerifyGetSelectionSpans_LineMode(const til::point start, const til::point end) + void VerifyGetSelectionSpans_LineMode(const til::point inclusiveStart, const til::point inclusiveEnd) { const auto selectionSpans = m_pSelection->GetSelectionSpans(); if (VERIFY_ARE_EQUAL(1u, selectionSpans.size())) { auto& span{ selectionSpans[0] }; - VERIFY_ARE_EQUAL(start, span.start, L"start"); - VERIFY_ARE_EQUAL(end, span.end, L"end"); + VERIFY_ARE_EQUAL(inclusiveStart, span.start, L"start"); + + const til::point exclusiveEnd{ inclusiveEnd.x + 1, inclusiveEnd.y }; + VERIFY_ARE_EQUAL(exclusiveEnd, span.end, L"end"); } } diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp index 2d6c1d1628e..4252609e30f 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) @@ -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); @@ -2589,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 }; 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/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 0de92b62373..90b9083b5d3 100644 --- a/src/renderer/atlas/AtlasEngine.cpp +++ b/src/renderer/atlas/AtlasEngine.cpp @@ -427,13 +427,13 @@ 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 // 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,10 +448,10 @@ 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) + 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..31ab5a410e3 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -342,9 +342,8 @@ 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; - max += 1; // Selection spans are inclusive (still) min <<= shift; max <<= shift; til::rect r{ min, row, max, row + 1 }; diff --git a/src/types/TermControlUiaProvider.cpp b/src/types/TermControlUiaProvider.cpp index 34bd94f9404..3e4091bd963 100644 --- a/src/types/TermControlUiaProvider.cpp +++ b/src/types/TermControlUiaProvider.cpp @@ -157,14 +157,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..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{}; @@ -987,13 +987,12 @@ 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)); + auto isValid = [&](const til::point& point) { + return bufferSize.IsInExclusiveBounds(point) || point == bufferSize.EndExclusive(); + }; + THROW_HR_IF(E_FAIL, !isValid(_start) || !isValid(_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) 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 30c5307dd14..ce4b38565eb 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 <= BottomInclusive(); +} + +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.