Refactor Word Expansion in TextBuffer #4423
Assignees
Labels
Area-CodeHealth
Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
Issue-Task
It's a feature request, but it doesn't really need a major design.
Product-Terminal
The new Windows Terminal.
Milestone
Comments
carlos-zamora
added
Product-Terminal
ghost
added
the
Needs-Triage
DHowett-MSFT
removed
the
Needs-Triage
25 tasks
zadjii-msft
modified the milestones:
Engineering Improvements 2021,
Engineering Improvements
1 task
DHowett
pushed a commit
that referenced
this issue
Jan 28, 2025
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description of the new feature/enhancement
In #4018, word navigation for selection and accessibility is merged into this mess:
GetWordEnd(withaccessibilityModeflag)GetWordStart(withaccessibilityModeflag)MoveToNextWordMoveToPreviousWordThere has to be a way to combine all of these into just 2 functions (or 4 with an accessibilityMode flag). Unfortunately, doing so at the time could have resulted in breaking Selection for the sake of Accessibility (if one was not careful). So here's a work item to refactor that and make it look waaaaay better.
Proposed technical implementation details (optional)
There's a good amount of repeated code/concepts throughout these functions. I think we should just rely on making the function return a bool and have an in/out param.
The text was updated successfully, but these errors were encountered: