[js324]

Summary of the Pull Request

Added wrapping to highlighted selection when selecting a word, added tests for it

References and Relevant Issues

#4009

Detailed Description of the Pull Request / Additional comments

• Modified GetWordStart and GetWordEnd and their helpers to no longer be bounded by the right and left viewport ranges
• Kept same functionality (does not wrap) when selecting wrapped whitespace
• Added tests to TextBufferTests.cpp to include cases of wrapping text

Just a thought, we currently select the "Line" on triple-click, but would it make more sense to include the wrapping functionality to this as well?

Validation Steps Performed

• Ran locally and verified selection works properly
• Tests passed locally

PR Checklist

• Closes Make double-click at the edge select across the edge #4009
• Tests added/passed
• Documentation updated
  • If checked, please file a pull request on our docs repo and link it here: #xxx
• Schema updated (if necessary)

Sorry about closing the last PR, tried cleaning up the commits when I was merging and accidentally closed it 🤦‍♂️

[Dustin L. Howett (DHowett)]

Thanks for bringing this PR back! I'm adding it to my docket for tomorrow.

[Dustin L. Howett (DHowett)]

(As you can probably guess, we start running out of staffing near the holidays! 😆)

[Leonard Hecker (lhecker)]

Just a thought, we currently select the "Line" on triple-click, but would it make more sense to include the wrapping functionality to this as well?

Yeah I'd say so. If you triple click to select an entire line of text it should select as many rows as necessary to select one proper line with explicit line break (aka: row.WasWrapForced() == false).

[Leonard Hecker (lhecker)]

I still feel somewhat squirmish about iterating by coordinates instead of iterating over the underlying text, but I also don't really see any flaws with this and I do think it's a genuine improvement over the old code (because it's not buggy). Thank you for tackling this issue!

To be entirely frank however, I think this code deserves to be rewritten from scratch at some point to use a simpler algorithm. For instance, we now have UTextFromTextBuffer which returns an ICU UText. We can then use ubrk_open(UBRK_WORD) to get proper Unicode word-wise navigation for languages that don't use spaces (Asian languages; in the future at some point). We can also implement delimiters more easily, accurately and in a way WAY more performant manner. uset_openPattern allows is to construct an efficient LUT from a string like "\\+!:=/.<>;|&". Then, you can just call UTEXT_NEXT32 and UTEXT_PREVIOUS32 to iterate through the backing buffer and ask the USet whether it's a delimiter char.

All that however requires a bigger rewrite...

[Dustin L. Howett (DHowett)]

Thanks for doing this! I see a bunch of the work is the mechanical change away from using a row cell iterator (which doesn't wrap) to manually walking the coordinates (which does). That'll work for me!

/cc Carlos Zamora (@carlos-zamora) as an advisory since he owns selection and accessibility, but I'm gonna merge this in for Canary anyway!

[Dustin L. Howett (DHowett)]

And for:

Just a thought, we currently select the "Line" on triple-click, but
would it make more sense to include the wrapping functionality to this
as well?

If you want to send in a followup PR for this, we'd love to review it! 😄