Conversation
microsoft-github-policy-service
bot
added
Issue-Task
| // - 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 | ||
| // - accessibilityMode - when true, cross non-wrapped row boundaries freely |
There was a problem hiding this comment.
So, in essence, this is "if false, add CRLF/newlines to the wordDelemiters list", right?
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
I think we need to test conhost, expressly, with this change. I'm afraid of the test values changing--because my mental model is that a refactor shouldn't also require us to change the tests (how would we ensure the refactor was correct if the original tests didn't pass?)
Fair. I'll do some side-by-side testing here and report back.
Super fair! FWIW I included this relevant segment in the PR body:
|
|
Validated on Conhost. Word selection and Narrator movement by word still works as expected. @DHowett whatcha think? Shall we include it in the coming release? The test coverage is pretty good so no issues should come up. If any issues do come up though, I'm here to fix them. |
3 participants
Summary of the Pull Request
Simplifies the word expansion functions in TextBuffer by removing the following functions:
GetWordStart2()GetWordEnd2()MoveToNextWord()MoveToPreviousWord()_GetWordStartForAccessibility()_GetWordStartForSelection()_GetWordEndForAccessibility()_GetWordEndForSelection()In favor of a simple:
GetWordStart()GetWordEnd()_GetDelimiterClassRunEnd()Tests were used to help ensure a regression doesn't occur. That said, there were a few modifications there too:
MoveByWordtestMoveToNextWord()andMoveToPreviousWord(), which no longer exist. These were helper functions forUiaTextRangeBase, and any special logic has been moved into_moveEndpointByUnitWordusing the unifiedGetWordStart()/GetWordEnd()APIs.MoveToPreviousWord,MovementAtExclusiveEnd, and the generated word movement tests.GetWordBoundariestestsGetWordEnd(): The old_GetWordEndForSelection()returned inclusive end positions, whereas the newGetWordEnd()returns exclusive end positions. This is whatGetWordEnd2()already used, so every old expected value shifted +1 in the x direction (or to {RightExclusive, y} at row boundaries) to account for that.ControlCharwrap-crossing behavior: The old_GetWordStartForSelection()had a special check at the left boundary that prevented whitespace runs from crossing wrapped row boundaries. The new_GetDelimiterClassRunStart()doesn't have this special case (it treats ControlChar the same as other delimiter classes when the row was wrap-forced). This changed one test case:GetWordStart({1, 4})in selection mode went from{0, 4}to{6, 3}(the whitespace run now crosses the wrap boundary to row 3). This matches the behavior TerminalSelection was already getting fromGetWordStart2().Validation Steps Performed
Tests passed:
✅ Conhost.Interactivity.Win32.Unit.Tests.dll
✅ UnitTests_TerminalCore\Terminal.Core.Unit.Tests.dll
Word navigation feels good for...
✅ Narrator
✅ NVDA
✅ Mouse selection
✅ Mark mode
Closes #4423