Make selection an exclusive range #18106

carlos-zamora · 2024-10-23T20:19:57Z

Summary of the Pull Request

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).

References and Relevant Issues

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 Refactor Word Expansion in TextBuffer #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 Mouse selection should begin and end at nearest cell boundary #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
We should definitely bug bash this to get good coverage.

Follow-ups

Refactor Word Expansion in TextBuffer #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.

src/buffer/out/textBuffer.cpp

src/types/UiaTextRangeBase.cpp

lhecker · 2024-10-24T20:57:23Z

src/renderer/atlas/AtlasEngine.cpp

        _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)


BTW I think we may have made a mistake here due to my poor understanding of this code. Probably needs to be mulled over once more...

yes, be careful - this is used in TWO places. search highlights and selection rendering. they're both inclusive today. this PR only says it changes one of them.

DHowett

Have you tested...

detecting URls
selecting URLs
search highlights (with and without regexes)

src/buffer/out/UTextAdapter.cpp

src/cascadia/TerminalControl/ControlCore.cpp

DHowett · 2024-11-08T00:06:49Z

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) {


careful! this touches conhost code too. ... !

this may make selections not work properly in conhost

Testing out Host.exe, this PR does change selection to be exclusive there too. Pivoting the selection is a bit weird now as shift+clicking before the pivot highlights the selected cell, whereas doing so after the pivot highlights up to the cell the pointer is on.

I'm able to polish Conhost to work more nicely too, but is that something we want? Or should I explore how to keep Conhost the same?

zadjii-msft · 2024-11-15T12:43:50Z

selecting URLs

Method: Enter mark mode --> Shift+Tab to select the url

Was this fixed? Just wanna know if I should be making my rounds here too

carlos-zamora · 2024-11-15T17:48:12Z

selecting URLs

Method: Enter mark mode --> Shift+Tab to select the url

Was this fixed? Just wanna know if I should be making my rounds here too

Currently working on it. My goal is to get the following fixed so we can test it in the bug bash on Tuesday:

some hyperlink issues: Make selection an exclusive range #18106 (comment)
Make Conhost work like it used to

carlos-zamora · 2024-11-15T23:51:49Z

These are fixed now! Verified ConHost using GDI and Atlas. Verified hyperlink detection with cases I mentioned earlier.

DHowett · 2024-11-16T00:56:48Z

src/buffer/out/UTextAdapter.cpp

@@ -436,7 +436,7 @@ til::point_span Microsoft::Console::ICU::BufferRangeFromMatch(UText* ut, URegula
    if (utextAccess(ut, nativeIndexEnd, true))
    {
        const auto y = accessCurrentRow(ut);
-        ret.end.x = textBuffer.GetRowByOffset(y).GetTrailingColumnAtCharOffset(ut->chunkOffset);
+        ret.end.x = textBuffer.GetRowByOffset(y).GetLeadingColumnAtCharOffset(ut->chunkOffset);


The offsets are exclusive now. This means the chunkOffset now refers to the first character after the matched range. In turn, this means that if there's a wide glyph after the matched range, we want its leading half, because the exclusive end should be exactly 1 past the end (and not 2).

DHowett · 2024-11-16T00:57:07Z

src/buffer/out/Row.cpp

+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);


what's all this?

These changes are necessary so that both accessor functions work reliably with an exclusive target char offset. I suggested to rename the member to make its usage a bit easier to understand, now that it refers to the length and not length-1 anymore. I did this in collaboration with Carlos. 🙂

carlos-zamora · 2024-11-19T22:05:03Z

Feedback from Bug Bash (11/19)

Don't select the trailing whitespace
Double clicking word and moving left is weird
click and drag on the right half of a cell, frag left. flickers and then selects prior cell
Keep selection the way it is now! (word selection and delimiter runs)

DHowett · 2024-12-02T21:01:41Z

FYI - the tests failed on all 3 platforms, which indicates a real produce code failure!

lhecker · 2024-12-03T20:12:34Z

src/cascadia/TerminalCore/TerminalSelection.cpp

+        //   To remedy this, decrement the end's position by 1.
+        //   However, only do this when expanding right (it's correct
+        //   as is when expanding left).


I'm not 100% sure if I understand this code, but to me it feels like the calling code should contain the fix for this and not this function. Is this perhaps just a case of floor VS. ceil in the calling code?

Yeah, I'm not happy about this part either. Let me explain what's going on better. The code flow is:

ControlInteractivity::SetEndSelectionPoint() (link) - called on mouse drag

ControlInteractivity::_getTerminalPosition() (link): round to the nearest cell, that way, we have to drag past a cell to "select" it

ControlCore::SetEndSelectionPoint() (link): really just clamps the position to the viewport bounds

Terminal::SetSelectionEnd() (link) --> Terminal::_SetSelectionEnd() (link):

This is the important part of the code, really. It's where we update our selection state for shift+click, pivoting, and multi-clicks (+ dragging).

The issue really only arises when we're multi-clicking+dragging (think "double-click" then drag, as it should expand by word as we drag). So the issue has to do with the expansion part. Anything before that in this function wouldn't make sense to alter, imo.

Terminal::_ExpandSelectionAnchors() (link): this is where we actually expand our selection endpoints if we're multi-clicking.

Thankfully, _ExpandSelectionAnchors() is only used in this spot. I'm not sure where else the change would make sense to go though. Open to suggestions.

carlos-zamora · 2024-12-04T18:22:38Z

All the tests are good now. The only one that failed was:

Release.x64.ConPtyTests::DiesOnClose#metadataSet0 Failed

which seems unrelated, so I'm running it again. /azp run

DHowett

18/30 i have a lot of questions after reading a few tests so i'm checkpointing here

src/host/ut_host/TextBufferTests.cpp

src/host/selection.cpp

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

DHowett · 2024-12-11T22:08:40Z

src/cascadia/UnitTests_TerminalCore/SelectionTest.cpp

@@ -709,7 +709,7 @@ namespace TerminalCoreUnitTests
                ValidateLinearSelection(term, { 5, 10 }, { 10, 10 });


I don't understand why these coordinates did not become exclusive

Good catch! Looked into it more. Since we're moving behind the pivot, end is actually inclusive! (which, admittedly, is pretty confusing)

Leaving this thread unresolved in case we want to make any changes because of this

carlos-zamora and others added 17 commits October 15, 2024 10:05

[AtlasEngine] Render selection as exclusive range

aa95790

Fix any side-effects of making GetText exclusive

4697f49

Update selection markers appropriately

4364643

[Mark Mode] Move by character (includes wide-glyph handling)

014f338

[Mark Mode] Move by viewport

ba673a4

[Mark Mode] Move by buffer

cd2da0e

[Mark Mode] Move by word

ff45eb9

[Mark Mode] Handle move up/down onto middle of emoji

659cd02

Bugfix: renderer highlights lines when at x-boundary

e7ca807

Bugfix: move to next word would move past mutable bottom

7ff4838

Fix some more rendering issues

928e9a9

Fix more rendering issues w/ Leonard

9d361d2

Fix highlighting in AtlasEngine

845e43d

fix session restore

8b996a9

Ensure mouse selection scenarios work right

df89ad0

Fix UIA

9e9db42

Fix hyperlinks

eea8399

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

src/buffer/out/textBuffer.cpp Fixed Show fixed Hide fixed

src/buffer/out/textBuffer.cpp Fixed Show fixed Hide fixed

src/buffer/out/textBuffer.cpp Fixed Show fixed Hide fixed

src/buffer/out/textBuffer.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

carlos-zamora commented Oct 23, 2024

View reviewed changes

src/types/UiaTextRangeBase.cpp Outdated Show resolved Hide resolved

fix tests

7bfd647

This comment has been minimized.

Sign in to view

microsoft-github-policy-service bot added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Oct 24, 2024

lhecker reviewed Oct 24, 2024

View reviewed changes

carlos-zamora requested a review from DHowett November 7, 2024 20:00

DHowett reviewed Nov 7, 2024

View reviewed changes

src/buffer/out/UTextAdapter.cpp Show resolved Hide resolved

src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved

DHowett reviewed Nov 8, 2024

View reviewed changes

apply feedback

83d2bff

This comment was marked as resolved.

Sign in to view

zadjii-msft assigned carlos-zamora Nov 15, 2024

carlos-zamora added 3 commits November 15, 2024 13:36

Fix shift+tab to select hyperlink

6aed5f9

Fix ConHost

7a8d448

Fix 'off by 1' hyperlink detection

6b99816

carlos-zamora removed their assignment Nov 15, 2024

Mohamed198-cup mentioned this pull request Nov 15, 2024

6aed5f98f4a55ca61fad27d9e08020e2afc9509c microsoft/vscode#233965

Closed

microsoft deleted a comment from Mohamed198-cup Nov 16, 2024

DHowett reviewed Nov 16, 2024

View reviewed changes

carlos-zamora added 2 commits November 26, 2024 15:08

bugfix: don't select trailing whitespace with mouse word selection

d179e9c

bugfix: click+drag on cell should select the cell we're on

d2ed09e

carlos-zamora added 2 commits December 3, 2024 10:16

bugfix: double-click + drag would prematurely expand

37ad74f

fix tests; fix conhost block selection

dbfdf81

lhecker reviewed Dec 3, 2024

View reviewed changes

fix terminal core selection tests

bee9371

DHowett requested changes Dec 11, 2024

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 11, 2024

carlos-zamora added 2 commits December 13, 2024 17:26

fix tests and add more comments to preserve sanity

44a1e42

simplify and add comment

4764653

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make selection an exclusive range #18106

Make selection an exclusive range #18106

carlos-zamora commented Oct 23, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

lhecker Oct 24, 2024

DHowett Nov 8, 2024

DHowett left a comment

DHowett Nov 8, 2024

carlos-zamora Nov 8, 2024

This comment was marked as resolved.

zadjii-msft commented Nov 15, 2024

carlos-zamora commented Nov 15, 2024

carlos-zamora commented Nov 15, 2024

DHowett Nov 16, 2024

lhecker Nov 16, 2024 •

edited

Loading

DHowett Nov 16, 2024

lhecker Nov 16, 2024 •

edited

Loading

carlos-zamora commented Nov 19, 2024 •

edited

Loading

DHowett commented Dec 2, 2024

lhecker Dec 3, 2024

carlos-zamora Dec 3, 2024

carlos-zamora commented Dec 4, 2024

DHowett left a comment

DHowett Dec 11, 2024

carlos-zamora Dec 14, 2024

		@@ -709,7 +709,7 @@ namespace TerminalCoreUnitTests
		ValidateLinearSelection(term, { 5, 10 }, { 10, 10 });

Make selection an exclusive range #18106

Are you sure you want to change the base?

Make selection an exclusive range #18106

Conversation

carlos-zamora commented Oct 23, 2024 • edited Loading

Summary of the Pull Request

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Follow-ups

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

zadjii-msft commented Nov 15, 2024

carlos-zamora commented Nov 15, 2024

carlos-zamora commented Nov 15, 2024

Choose a reason for hiding this comment

lhecker Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

carlos-zamora commented Nov 19, 2024 • edited Loading

Feedback from Bug Bash (11/19)

DHowett commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Dec 4, 2024

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Oct 23, 2024 •

edited

Loading

lhecker Nov 16, 2024 •

edited

Loading

lhecker Nov 16, 2024 •

edited

Loading

carlos-zamora commented Nov 19, 2024 •

edited

Loading