Add double click and double click + drag gestures to SelectionArea - #124817

Wow pattern matching 🤩

Should these two names be the same? textGranularity and granularity.

Good point!

I think we have a method with the same name in text_selection.dart. Is it reasonable to share code here?

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.

Sounds good.

No apostrophe here: "platform's" => "platforms"

Same for below.

return isPositionAtOrBelowStartingBaseline;

Should there be spaces between the brackets and the text? I forget. Here and below, if I'm right.

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.

Sounds like it's not a big deal then, keeping it like this is fine 👍

It was probably my oversight.

Should textGranularity actually receive a default value here?

Nit: Maybe specify that the default is [TextGranularity.character] instead of saying "character by character".

Why is this needed now? (and in a few other places below)

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.

Thanks for the explanation!

Super nit: Maybe assign this offset to a variable so that it's clear where it's coming from?

I am not entirely sure what this is for can you explain?

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.

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

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.

Can you also copy the todo over?

should we document that only character and word is supported?

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

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

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?

should the rect check here also applies to handle select word? if so we could consider put all these tweak into _getWordBoundaryAtPosition

This rect check is to handle the results of SelectionUtils.adjustDragOffset which isn't used in handle select word.

This seems to be inverse of shouldSwapEdgesWhenSelectionIsNormalized except for the equal case? can the equal case be handle the same way as either one of shouldSwapEdgesWhenSelectionIsNormalized or shouldSwapEdgesWhenSelectionNotNormalized

it is also hard to interpret the name. is it swaping the existingSelectionStart and existingSelectionEnd or is it swaping the targetPosition to be wordBoundary.wordStart or wordBoundary.wordEnd? base on the code, it seems to be both.

Maybe call this willNewSelectionBeNormalized

The equal case should actually not swap, I have updated the implementation to reflect this.

Which class is synthesizing the start event to selectable?

_startNewMouseSelectionGesture is the one setting the start edge with _selectWordAt, or were you referring to something else?

I am referring to cases where the _selectEndTo reach a new selectable other than the _selectWordAt originally sent to

_SelectableRegionContainerDelegate is synthesizing the first start event on ensureChildUpdated when _selectEndTo reaches a new selectable.

I see, so it would be a selectStart with granularity character. As long as renderParagraph can handle that, that sounds fine.

watch out for ellipsized case where the targetPosition may not be able to reach range.end. basically what to todo is describing below.

The return value should always be determine by SelectionUtils.getResultBasedOnRect(_rect, localPosition); until we have a way to know what text are ellipsized

Nit: Is this any simpler?

final (wordStart: TextPosition wordStart, wordEnd: TextPosition wordEnd) = _getWordBoundaryAtPosition(edgeToKeepInPlace);

So you don't have to refer to localWordBoundary. Here and other places where you receive a record.

I mentioned this before in my old review but just to double check, it's not possible to deduplicate this code with what we have in EditableText is it? Assuming that EditableText also has similar code to extend selection by word.

The code for extending selection by word is in RenderEditable here

It's really hard to follow all of the if/else blocks below here. Would it be possible to split any of this up into smaller methods or anything like that?

I do appreciate all of the comments though!

I tried simplifying some of the cases a bit more. Hope that clears it up a bit.

I am also agree, I would suggest separating out two branches (isEnd = true/false) into two separate function. It seems like this code try too hard to deduplicate these two branchs but now it is hard to figure out which edge a variable holds

I've separated the logic into two methods, one for updating the end edge and one for updating the start edge. I think it is clearer just more LOC.

Looks like an improvement to me despite more lines 👍

I see, so it would be a selectStart with granularity character. As long as renderParagraph can handle that, that sounds fine.

It was probably my oversight.

I am also agree, I would suggest separating out two branches (isEnd = true/false) into two separate function. It seems like this code try too hard to deduplicate these two branchs but now it is hard to figure out which edge a variable holds

As long as the non-moving edge is null you have to go to the else case no?

also this may be easier if we separate out the isEnd = true/false into two separate functions