Keyboard text selection and wordwrap #85653
Conversation
|
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. |
There was a problem hiding this comment.
nit: this is selection.end right?
There was a problem hiding this comment.
it looks like selection can be invalid here.
There was a problem hiding this comment.
Could the problem be we're initializing a new TextPosition with the default text affinity being downstream? If text affinity is upstream then _getLineAtOffset won't go to the next line right?
|
(PR triage): @justinmc Do you still have plans for this PR? |
|
@goderbauer Yes I do! I was waiting for #87133 to be merged. Coming back to this this week now. |
|
This is blocked on flutter/engine#28008. There is a bug in the engine that should be fixed before this is fixed here, otherwise I'd be hacking around a bug. |
e5564a6 to
74b8943
Compare
|
Squashed commits in order to fix svg test. |
|
@LongCatIsLooong Ready for re-review. This bumps into the problem of not having multiple TextAffinities for a non-collapsed TextSelection, which you mentioned here and I wrote up in #88135. I think this PR works in all practical situations though, which I've covered in my test, even though we're not able to distinguish certain non-collapsed selections. |
|
|
||
| // If the lowest edge of the selection is at the start of a line, don't do | ||
| // anything. | ||
| final int lowestOffset = math.min(selection!.baseOffset, selection!.extentOffset); |
There was a problem hiding this comment.
this is start. also we're doing the same thing in 1760.
There was a problem hiding this comment.
these lines seem to be doing the same as line 1760-1762?
|
@LongCatIsLooong Ready for re-review. I've updated it to use |
| return; | ||
| } | ||
|
|
||
| final int startPoint = previousCharacter(selection!.start, _plainText, false); |
There was a problem hiding this comment.
Do we still need this? Isn't currentLine already the first line of the current selection?
There was a problem hiding this comment.
Good call, I was able to get rid of it. The previousCharacter thing seems to have been used to work around these wordwrap problems mainly when getLineBoundary didn't respect TextAffinity.
6c9df75 to
86629c6
Compare
| offset: selection!.start, | ||
| affinity: selection!.isCollapsed ? selection!.affinity : TextAffinity.downstream, | ||
| )); | ||
| if (currentLine.baseOffset == selection!.start) { |
There was a problem hiding this comment.
Do we still need to explicitly check here whether selection.start == currentLine.baseOffset? It seems 1763-1771 already covered that?
There was a problem hiding this comment.
Without this line, it would call _setSelection with a selection identical to the existing one, which can have side effects as it is written right now. I've previously been thinking about changing _setSelection so that it checks for equality and just returns if a change isn't needed, but that seems to break some things that rely on that behavior.
Expanding by line (e.g. cmd-shift-left/right) shouldn't jump between lines when hit multiple times. According to my testing on native, it should go to the start/end of the line, and subsequent presses should do nothing. This PR matches that behavior and fixes some other sketchiness around the edges of lines, thanks to the engine bugfix referenced below.
After the PR it seems to work properly:
Fixes #79291
Depends on flutter/engine#28008