Make selection an exclusive range by carlos-zamora · Pull Request #18106 · microsoft/terminal

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.

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

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

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

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

@@ -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

crazy, but i think i understand

carlos-zamora · 2025-01-21T19:11:22Z

Bug bash 1/21

When starting the selection in the right half of a cell and dragging left it'll not select the cell you started in.
...if, on the other hand, you immediately start dragging to the right it'll select both cells.
...one option would be to represent selections in float cell coordinates and use floor(top), ceil(bottom), floor(left+0.5), and ceil(right-0.5) to turn it into integer coordinates, or something.

carlos-zamora · 2025-01-23T18:28:21Z

When starting the selection in the right half of a cell and dragging left it'll not select the cell you started in.

...if, on the other hand, you immediately start dragging to the right it'll select both cells.

@DHowett (I think you filed the bug?) I wasn't able to get the issue to repro (unless I'm misunderstanding it?). Is there a desired behavior you want instead?

DHowett

30/30, couple open comments

DHowett · 2025-01-23T23:52:49Z

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


crazy, but i think i understand

DHowett · 2025-01-23T23:55:10Z

+{
+    const auto l = static_cast<ptrdiff_t>(_sr.left);
+    const auto t = static_cast<ptrdiff_t>(_sr.top);
+    const auto w = static_cast<ptrdiff_t>(std::max(0, _sr.right - _sr.left + 2));


Did you ever make that change and validate?

DHowett · 2025-01-24T00:07:19Z

+
+    // Try to move forward, but if we hit the buffer boundary, we fail to move.
+    const bool success = bufferSize.IncrementInExclusiveBounds(pos);
+    if (success &&


if we ever have glyphs more than two cells wide, this can easily become while() (i think?)

Leaving this as-is for now. We can change it to while() and fully test it out when that's the case

lhecker

I'd not delay it too much longer and just fix it up in post as needed.

## Summary of the Pull Request Fixes a bug where VT mouse mode would round to the nearest cell when clicking the mouse button. The fix is to round to the nearest cell only when we're selecting text. The other scenarios affected are: - clicking on a hyperlink - vt mouse mode - where the context menu is anchored Really the most notable ones were the first two. So now, we use the position of the cell we clicked on. We only round for selection. ## References and Relevant Issues Follow-up to #18106 ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed Opened Midnight Commander in Ubuntu and clicked between the two panes. - Before: threshold was too early to switch between panes - After: threshold is clearly separated between the outline of the two panes --------- Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

…9259) ## Summary of the Pull Request Fixes a bug where copying and coloring selected text would be off by one. This was introduced in #18106 when selection was updated to be stored as an exclusive range. `Selection::_RegenerateSelectionSpans()` was updated then, but copying text and coloring selection didn't rely on selection spans. Copying text relies on `GetSelectionAnchors()`. This function has now been updated to increment the bottom-right point of the selection. This way, `GetTextSpans()` operates on the expected _exclusive_ range. Coloring selection relies on `TextBuffer::SearchText()`, `TextBuffer::GetTextRects` and `GetSelectionSpans()`. Both `Selection::ColorSelection()` were updated to use `rect` over `inclusive_rect` to emphasize that they are exclusive ranges. Converting between the two improves clarity and fixes the bug. ## References and Relevant Issues Introduced in #18106 ## Validation Steps Performed Copying text works in the following scenarios: ✅ single line, left-to-right and right-to-left ✅ multi-line, diagonal directions ✅ block selection Coloring text works in the following scenarios: ✅ctrl+# --> color instance ✅ctrl+shift+# --> color all instances Closes #19053

Selection in VT mouse mode has been feeling a little weird. I've made a few changes in this space to improve the overall experience when in vt mouse mode: - don't round selection positions: this means that you now highlight the cell you're on if you're going right, and the adjacent cell if you're going left - fix drag-left excluding the current cell - #9608: shift+click now clears any existing selection instead of extending it. This feels more intuitive since Shift already works as the override modifier Somewhat related to #18106 ## Validation ✅ alt selections feel consistent ✅ selecting in VTMM feels accurate (selects the cell we started on) ✅ creating new selections (aka clearing old selection) in VTMM feels intuitive (forwards and backwards) Closes #9608

Selection in VT mouse mode has been feeling a little weird. I've made a few changes in this space to improve the overall experience when in vt mouse mode: - don't round selection positions: this means that you now highlight the cell you're on if you're going right, and the adjacent cell if you're going left - fix drag-left excluding the current cell - #9608: shift+click now clears any existing selection instead of extending it. This feels more intuitive since Shift already works as the override modifier Somewhat related to #18106 ## Validation ✅ alt selections feel consistent ✅ selecting in VTMM feels accurate (selects the cell we started on) ✅ creating new selections (aka clearing old selection) in VTMM feels intuitive (forwards and backwards) Closes #9608 (cherry picked from commit 2e33056) Service-Card-Id: PVTI_lADOAF3p4s4BQX0-zgna3V8 Service-Version: 1.25

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 AI found potential problems Oct 23, 2024

View reviewed changes

Comment thread src/buffer/out/textBuffer.cpp Fixed

Comment thread src/buffer/out/textBuffer.cpp Fixed

Comment thread src/buffer/out/textBuffer.cpp Fixed

Comment thread src/buffer/out/textBuffer.cpp Fixed

This comment has been minimized.

Sign in to view

carlos-zamora commented Oct 23, 2024

View reviewed changes

Comment thread src/types/UiaTextRangeBase.cpp Outdated

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

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

carlos-zamora added the zBugBash-Consider label Jan 14, 2025

carlos-zamora added 2 commits January 17, 2025 12:31

Fix failing test & ExpandSelectionToWord

7290330

aha! That's why the .x was there!

260dc99

carlos-zamora added a commit that referenced this pull request Jan 20, 2025

PRE-MERGE #18106 Make selection an exclusive range

02f8e5c

DHowett requested changes Jan 24, 2025

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 Jan 24, 2025

lhecker approved these changes Jan 27, 2025

View reviewed changes

carlos-zamora added 2 commits January 27, 2025 13:39

remove TODO

c8c0169

Merge branch 'main' into dev/cazamor/sln/exclusive-end

7252ab9

DHowett approved these changes Jan 28, 2025

View reviewed changes

DHowett merged commit 64d4fba into main Jan 28, 2025

DHowett deleted the dev/cazamor/sln/exclusive-end branch January 28, 2025 22:54

carlos-zamora mentioned this pull request Jan 31, 2025

Bugfix: only round to the nearest cell for selection #18486

Merged

DHowett mentioned this pull request Mar 28, 2025

"Color selection" off-by-one regression: it colors 1 cell past the end #18739

Closed

carlos-zamora mentioned this pull request Apr 14, 2025

Fix color selection off-by-one error and dangling Y-beam #18798

Merged

carlos-zamora mentioned this pull request Aug 19, 2025

[Conhost] Fix off-by-1 error when copying and coloring selections #19259

Merged

carlos-zamora mentioned this pull request Mar 13, 2026

Improve UX for selection during VTMM #19973

Merged

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

Conversation

carlos-zamora commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Follow-ups

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Dec 4, 2024

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Jan 21, 2025

Bug bash 1/21

Uh oh!

carlos-zamora commented Jan 23, 2025

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlos-zamora commented Oct 23, 2024 •

edited

Loading