Makes TextBoundary and its subclasses public #110367
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
ed8f224 to
9ccc496
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. Changes reported for pull request #110367 at sha 6e6d5ef01ff94ae5a46da93388a4c32612404040 |
6e6d5ef to
dc50184
Compare
ca7dd77 to
f696749
Compare
There was a problem hiding this comment.
This should be in the ui.text, putting it here just for now, will spawn another pr and remove this before merge
There was a problem hiding this comment.
previous test is testing the implementation detail of textOffsetToPosition
f696749 to
a1d16de
Compare
There was a problem hiding this comment.
The original logic here was flawed I think? The visual end of the paragraph may not be the logical end of the paragraph (when the last run in the paragraph has a different directionality from that of the paragraph)
There was a problem hiding this comment.
directionality should be fine as the skia should handle that for us? The problem I facing here is I can't differentiate whether user taps the upstream of the end of the text or far away to the right from the end of the text. I think the correct behavior is we should still select word for the former and collapse at the end for the latter.
Since we can't tell it with the current API, I made it always collapse at the end, since that is what the original behavior is
There was a problem hiding this comment.
This doc line may need to be changed now it's public
There was a problem hiding this comment.
There is really nothing else i can explain in this constructor.
There was a problem hiding this comment.
points -> points to
There was a problem hiding this comment.
Some applications don't paint the caret between characters (for instance, in some terminal emulators it's below the character it points to).
There was a problem hiding this comment.
I am not sure what description i should add here. That will be up to the application to interpret the output?
There was a problem hiding this comment.
Is the offset -1 strategy correct? If there's no bidi text or soft line wraps in a paragraph, then we have
TextPosition(x, downstream) == TextPosition(x, upstream) != TextPosition(x-1, downstream)
right?
There was a problem hiding this comment.
My understanding of codeUnitboundary is the following. If you have a text 'abc'. To describe the boundary of code unit 'b' will be
leading : TextPosition(offset: 1, affinity:TextAffinity.downStream)
trailing: TextPosition(offset: 2, affinity:TextAffinity.upStream)
The current code should capture this. It make sure any text position that point to 'b' will return that. WDYT?
There was a problem hiding this comment.
Yeah according to what's in TextPainter that looks correct
LongCatIsLooong
left a comment
There was a problem hiding this comment.
LGTM with nits/questions
There was a problem hiding this comment.
do we still need this, now that we always treat TextPosition as caret locations?
There was a problem hiding this comment.
Yes otherwise you won't be able to move to the next word if the current position is at the edge. This boundary move the position by one affinity. This may need a better name though...
a30c24b to
2e1bc5b
Compare
Make it public so it can be reused in SelectableRegion
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.