Add double click and double click + drag gestures to SelectionArea #124817
Conversation
flutter-dashboard
bot
added
the
framework
Renzo-Olivares
force-pushed
the
selection-area-tapdrag
branch
4 times, most recently
from
8377b42 to
2d35b08
Compare
Renzo-Olivares
force-pushed
the
selection-area-tapdrag
branch
2 times, most recently
from
0bc6241 to
9ba1c10
Compare
Renzo-Olivares
changed the title
flutter-dashboard
bot
added
the
f: material design
Renzo-Olivares
changed the title
Renzo-Olivares
changed the title
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM with nits 👍
I'm always bummed that we don't get this kind of behavior for free shared between RenderEditable and RenderParagraph. But given that we can't do that yet, this is pretty clean, and I think TextGranularity makes it straightforward.
You have some test failures FYI.
There was a problem hiding this comment.
Should these two names be the same? textGranularity and granularity.
There was a problem hiding this comment.
I think we have a method with the same name in text_selection.dart. Is it reasonable to share code here?
There was a problem hiding this comment.
I think I can re-visit this once I implement triple-click gestures. Right now there are some small nuances between how this effective consecutive tap count works on static vs editable text.
There was a problem hiding this comment.
No apostrophe here: "platform's" => "platforms"
There was a problem hiding this comment.
return isPositionAtOrBelowStartingBaseline;
There was a problem hiding this comment.
Should there be spaces between the brackets and the text? I forget. Here and below, if I'm right.
There was a problem hiding this comment.
Looks like the other methods in this file with named parameters do not, so I just followed that. But I am also unsure what the preferred styling here is.
There was a problem hiding this comment.
Sounds like it's not a big deal then, keeping it like this is fine 👍
There was a problem hiding this comment.
It was probably my oversight.
There was a problem hiding this comment.
Should textGranularity actually receive a default value here?
There was a problem hiding this comment.
Nit: Maybe specify that the default is [TextGranularity.character] instead of saying "character by character".
There was a problem hiding this comment.
Why is this needed now? (and in a few other places below)
There was a problem hiding this comment.
This is needed because of the switch to PanGestureRecognizer -> TapAndPanGestureRecognizer. PanGestureRecognizer calls onDown earlier than TapAndPanGestureRecognizer. PanGestureRecognizer calls it in addAllowedPointer, and TapAndPanGestureRecognizer waits for the recognizer to declare victory.
There was a problem hiding this comment.
Thanks for the explanation!
There was a problem hiding this comment.
Super nit: Maybe assign this offset to a variable so that it's clear where it's coming from?
Renzo-Olivares
changed the title
|
Looks like the failing tests are because Offset textOffsetToPosition(RenderParagraph paragraph, int offset) {
const Rect caret = Rect.fromLTWH(0.0, 0.0, 2.0, 20.0);
final Offset localOffset = paragraph.getOffsetForCaret(TextPosition(offset: offset), caret);
return paragraph.localToGlobal(localOffset);
}
test('getWordBoundary control test failing', () {
final RenderParagraph paragraph = RenderParagraph(
const TextSpan(text: 'How are you?'),
textDirection: TextDirection.ltr,
);
layout(paragraph);
final Offset target4 = textOffsetToPosition(paragraph, 4);
final TextPosition position4 = paragraph.getPositionForOffset(target4);
final TextRange range4 = paragraph.getWordBoundary(position4);
expect(range4.textInside('How are you?'), equals('are'));
});This fails with Looks like this test passes using Opened #125582 for further investigation. |
Renzo-Olivares
force-pushed
the
selection-area-tapdrag
branch
2 times, most recently
from
cf86e53 to
ffdaecd
Compare
|
Thanks for opening that issue! |
|
|
||
| final (TextPosition start, TextPosition end) wordBoundary = _getWordBoundaryAtPosition(position); | ||
| TextPosition targetPosition = isEnd ? wordBoundary.$2 : wordBoundary.$1; | ||
| if (!_rect.contains(localPosition)) { |
There was a problem hiding this comment.
I am not entirely sure what this is for can you explain?
There was a problem hiding this comment.
I found that since SelectionUtils.adjustDragOffset returns the top left of the rect for ltr if the drag is in Area1 of a Selectable it will return TextPosition(0). When using _getWordBoundaryAtPosition(position) that ends up selecting the first word of the selectable.
This is not desired since say we have a list of Selectables and our drag starts at the first one, dragging down to the second one moves the selection to the word on the second selectable, but then when dragging back up to the first selectable the second selectables first word is still selected because _getWordBoundaryAtPosition(TextPosition(0)) = first word.
This same logic is in _updateSelectionEdge but it works there because the edge will moved to TextPosition(0) and not the word at TextPosition(0).
This was my workaround for this issue.
There was a problem hiding this comment.
Thanks for explanation. If this is the case, I think you can move this before the whole _getWordBoundaryAtPosition and short circuit it. Also would be good to add some comments about this
| localPosition, | ||
| direction: paragraph.textDirection, | ||
| ); | ||
| final TextPosition position = _clampTextPosition(paragraph.getPositionForOffset(adjustedOffset)); |
There was a problem hiding this comment.
This probably should instead clamp after _getWordBoundaryAtPosition
it is fine if the position is outside of the text range of this selectablefragment, as long as the targetPosition is in the text range.
| return SelectionResult.previous; | ||
| } | ||
|
|
||
| return SelectionUtils.getResultBasedOnRect(_rect, localPosition); |
There was a problem hiding this comment.
Can you also copy the todo over?
| /// The [globalPosition] contains the new offset of the edge. | ||
| /// | ||
| /// This event is dispatched when the framework detects [DragStartDetails] in | ||
| /// The [granularity] contains the granularity that the selection edge should move by. |
There was a problem hiding this comment.
should we document that only character and word is supported?
flutter-dashboard
bot
added
the
f: scrolling
Renzo-Olivares
force-pushed
the
selection-area-tapdrag
branch
3 times, most recently
from
012396b to
83190e9
Compare
There was a problem hiding this comment.
why do you need to cache this value? is it possible to figure out whether the you need to move the start or end based on the current selection and the newly updated selection? for example, if current selection is (start: 3, end: 6) and you receive a end update, you will know you need to swap the start if new end position is < 3
There was a problem hiding this comment.
It is hard to read the logic in these block. Can you add check isEnd in the condition? I think it would be noop in the set selection anyway
There was a problem hiding this comment.
I think you don't need to cache the _originWordSelectionStart, you just need to find the wordboundary of _textSelectionEnd/Start and move them to the other side of the word.
Also the current logc assume _originWordSelectionStart and _originWordSelectionEnd are enclosing a word. is it possible it is not the case?
Renzo-Olivares
force-pushed
the
selection-area-tapdrag
branch
2 times, most recently
from
1b1c1a8 to
52cb91b
Compare
3 participants
Adds double click to select a word.
Adds double click + drag to select word by word.
Screen.Recording.2023-04-25.at.11.03.35.AM.mov
Partially fixes #104552
Pre-launch Checklist
///).