Skip to content

Commit

Permalink
Make selection an exclusive range (#18106)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
carlos-zamora authored Jan 28, 2025
1 parent 8e4da6e commit 64d4fba
Show file tree
Hide file tree
Showing 30 changed files with 738 additions and 278 deletions.
2 changes: 2 additions & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ CBN
cbt
Ccc
CCCBB
CCCDDD
cch
CCHAR
CCmd
Expand Down Expand Up @@ -369,6 +370,7 @@ DColor
dcommon
DComposition
dde
DDDCCC
DDESHARE
DDevice
DEADCHAR
Expand Down
28 changes: 18 additions & 10 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,19 +80,20 @@ 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 }
{
}

// If given a position (`offset`) inside the ROW's text, this function will return the corresponding column.
// 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").
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<ptrdiff_t>(charsSize - 1);
const auto lastChar = gsl::narrow_cast<ptrdiff_t>(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<til::CoordType>(clamp(offset, 0, _columnCount));
return CharToColumnMapper{ _chars.data(), _charOffsets.data(), lastChar, guessedColumn };
return CharToColumnMapper{ _chars.data(), _charOffsets.data(), lastChar, guessedColumn, _columnCount };
}

const std::optional<ScrollbarData>& ROW::GetScrollbarData() const noexcept
Expand Down
5 changes: 3 additions & 2 deletions src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
9 changes: 3 additions & 6 deletions src/buffer/out/UTextAdapter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<const TextBuffer*>(ut->context);
til::point_span ret;
Expand All @@ -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
Expand Down
Loading

0 comments on commit 64d4fba

Please sign in to comment.