[Carlos Zamora (carlos-zamora)]

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()
• _GetDelimiterClassRunStart()`
• _GetDelimiterClassRunEnd()

Tests were used to help ensure a regression doesn't occur. That said, there were a few modifications there too:

• Removed MoveByWord test
  • It directly called MoveToNextWord() and MoveToPreviousWord(), which no longer exist. These were helper functions for UiaTextRangeBase, and any special logic has been moved into _moveEndpointByUnitWord using the unified GetWordStart()/GetWordEnd() APIs.
  • The tested behavior is still covered by MoveToPreviousWord, MovementAtExclusiveEnd, and the generated word movement tests.
• updated GetWordBoundaries tests
  • Inclusive --> exclusive end positions for GetWordEnd(): The old _GetWordEndForSelection() returned inclusive end positions, whereas the new GetWordEnd() returns exclusive end positions. This is what GetWordEnd2() already used, so every old expected value shifted +1 in the x direction (or to {RightExclusive, y} at row boundaries) to account for that.
  • ControlChar wrap-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 from GetWordStart2().

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

[Leonard Hecker (lhecker)]

So, in essence, this is "if false, add CRLF/newlines to the wordDelemiters list", right?

[Dustin L. Howett (DHowett)]

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

[Carlos Zamora (carlos-zamora)]

I think we need to test conhost, expressly, with this change.

Fair. I'll do some side-by-side testing here and report back.

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

Super fair! FWIW I included this relevant segment in the PR body:

• Removed MoveByWord test
  • It directly called MoveToNextWord() and MoveToPreviousWord(), which no longer exist. These were helper functions for UiaTextRangeBase, and any special logic has been moved into _moveEndpointByUnitWord using the unified GetWordStart()/GetWordEnd() APIs.
  • The tested behavior is still covered by MoveToPreviousWord, MovementAtExclusiveEnd, and the generated word movement tests.

[Carlos Zamora (carlos-zamora)]

Validated on Conhost. Word selection and Narrator movement by word still works as expected.

Dustin L. Howett (@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.

[Carlos Zamora (carlos-zamora)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Carlos Zamora (carlos-zamora)]

Dustin L. Howett (@DHowett) removing this from the servicing pipeline. It's just a refactor, so I'd rather get feedback on Canary than Preview. If you disagree, you can just put it back though boss.