-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Add boundary feature to the drag gesture. #147521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a space after period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space between "whether" and "the".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like this should default to null if no drag boundary is defined. If not then users might incorrectly think that their drag is within bounds when it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I have re-examined the code here and found that it's not really necessary to call outOfBoundary, so I removed it. I put the boundary information into DragStartDetails and DragUpdateDetails, so there's no need to add some bad APIs, making it a bit simpler. Please review it again, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: end comment with a period.
df83e0e to
e7a4977
Compare
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yiiim, thank you for your patience and contribution! This is a really cool change! I left a few comments regarding limiting the surface area of the API and still accomplishing the given use-cases.
| /// See also: | ||
| /// * [DragBoundary], which defines the boundary of the drag gesture. | ||
| /// * [CreateDragBoundary], which is a callback that creates a [DragBoundary]. | ||
| CreateDragBoundary? createDragBoundary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this API is needed to accomplish the given use-cases. Are the use-cases achievable just using DragRectBoundaryProvider?
Here is the example without any API changes to DragGestureRecognizer. I don't think it is too much more code to accomplish the use-case in the example without adding to the DragGestureRecognizer API.
class DragBoundaryExampleApp extends StatefulWidget {
const DragBoundaryExampleApp({super.key});
@override
State<StatefulWidget> createState() => DragBoundaryExampleAppState();
}
class DragBoundaryExampleAppState extends State<DragBoundaryExampleApp> {
Offset _basePosition = Offset.zero;
Offset _dragPosition = Offset.zero;
late DragBoundary? boundaryInfo;
@override
Widget build(BuildContext context) {
final Offset offset = _dragPosition - _basePosition;
return MaterialApp(
home: Scaffold(
body: Padding(
padding: const EdgeInsets.all(100),
child: DragRectBoundaryProvider(
child: Stack(
children: <Widget>[
Container(
color: Colors.green,
),
Positioned(
top: offset.dy,
left: offset.dx,
child: Builder(
builder: (BuildContext context) {
return GestureDetector(
behavior: HitTestBehavior.translucent,
onPanStart: (DragStartDetails details) {
boundaryInfo = DragRectBoundaryProvider.of(context)?.getDragBoundary(context, details.globalPosition);
setState(() {
_basePosition = details.globalPosition - offset;
_dragPosition = details.globalPosition;
});
},
onPanUpdate: (DragUpdateDetails details) {
setState(() {
if (boundaryInfo == null) {
return;
}
if (boundaryInfo!.isWithinBoundary(details.globalPosition)) {
_dragPosition = details.globalPosition;
} else {
_dragPosition = boundaryInfo!.getNearestPositionWithinBoundary(details.globalPosition);
}
});
},
child: Container(
width: 100,
height: 100,
color: Colors.red,
),
);
},
),
),
],
),
),
),
),
);
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, but it's hard to cancel the gesture through the boundary without changing the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some thoughts, as you said, we don't change any APIs to implement the boundary feature, and then let the user actively control the cancellation of the gesture in another way. I will work on this, if you have any ideas, please let me know. @Renzo-Olivares
|
|
||
| /// Whether to cancel the gesture when it moves out of the boundary | ||
| /// specified by [createDragBoundary], defaults to false. | ||
| bool cancelWhenOutsideBoundary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide an example on how to use this API to fix the issue described in #146749 .
|
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. |
a174a1e to
7d95e99
Compare
|
Reimplemented and ready for review. @Renzo-Olivares |
|
Thank you @yiiim, I will take a look this week. |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience and contribution @yiiim!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This GestureController is cool! With regards to GestureDetector would it be possible to send a GestureCancelEvent to a specific recognizer? For example if a user sets both the pan gesture callbacks and tap gesture callbacks, can they cancel just the pan gesture? I didn't see a way to do this when looking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to hear more use-cases of cancelling gestures. From this example, I think a similar functionality can be accomplished with the below:
onPanUpdate: (DragUpdateDetails details) {
if (_isOverlayBlocking) {
return;
}
text = 'PanUpdate';
setState(() {});
},Where _isOverlayBlocking is set to true when the visibility of the yellow overlay is set to true. I do see the positive of not having to handle this as a user yourself and simply cancelling the gesture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will provide a new example for GestureController in a later PR, this example is indeed not good enough, I think I prefer to reset some things in the PanCancel callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider iterating through the local list of tracked pointers and calling stopTrackingPointer for each tracked pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The wording here is a little confusing. Maybe:
This widget creates a [GestureDragBoundaryProvider] that sets the boundary for a dragged rectangle. The drag rectangle is retrieved from the context provided in [GestureDragBoundaryProvider.getDragBoundary]. The boundary is defined by the rectangle bounds of the provided child widget.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider changing wording to below,
This widget creates a [GestureDragBoundaryProvider] that sets a boundary for a dragged point. The dragged point is retrieved from the [Offset] provided in [GestureDragBoundaryProvider.getDragBoundary]. The boundary is defined by the rectangle bounds of the provided child widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards this being a different PR since the drag boundary components can work without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I have removed the part about GestureController from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider stopTrackingAllPointers instead to follow the naming conventions of similar methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider putting these drag boundary components into their own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks for decoupling this feature from the gesture detail object.
54fc2eb to
e6cbf1a
Compare
e6cbf1a to
4bab7d1
Compare
|
@Renzo-Olivares Please do not forget this here. 👻 |
|
Hi @yiiim, apologies for the delayed review. I will take a look this week. |
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
| } | ||
| final RenderBox? rb = element.findRenderObject() as RenderBox?; | ||
| assert(rb != null && rb.hasSize, 'DragBoundary is not available'); | ||
| final Rect boundary = (useGlobalPosition ? rb!.localToGlobal(Offset.zero) : Offset.zero) & rb!.size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work if localToGlobal contains scaling. You might want to use Rect.fromPoints with two localToGlobals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with scaling, do you mean like this? Rect.fromPoints(rb!.localToGlobal(Offset.zero), rb.localToGlobal(rb.size.bottomRight(Offset.zero)));
If you are still online, could you provide a commit suggestion for this? I will look into scaling later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with scaling, do you mean like this?
Rect.fromPoints(rb!.localToGlobal(Offset.zero), rb.localToGlobal(rb.size.bottomRight(Offset.zero)));
Yes, exactly.
However, when I tried to demonstrate (and verify) it by creating a demo, which uses useGlobalPosition: true, I find it so hard that I start to wonder how it's going to be useful at all. If we want the GestureDetector to be on the rectangle being dragged, and drag boundary must also wrap the rectangle, which means the rectangle's position must not be global, but at least relative to the drag boundary. On the other hand, if the rectangle is positioned on a global overlay, then the drag boundary is unlikely a parent of the rectangle. The only case I can think of is when the GestureDetector is as big as the drag boundary. My point is, can you create an example when useGlobalPosition is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the contrary, useGlobalPosition: true is actually more useful, as it is not always possible to obtain the local position of the drag object within the boundary during use. For example, if this PR is used for ReorderableList, during dragging, ReorderableList needs to use the given DragBoundaryDelegate, and here useGlobalPosition: true must be used.
However, if we can ensure that we are using the local coordinates of the boundary, useGlobalPosition: false can avoid the need for two coordinate conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The PR is in general in a good shape. I'm still wondering whether the API design is sufficiently flexible or too flexible, but that's normal case for new classes like this anyway. Thank you for the continued revisions.
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
|
Update: I've asked @Renzo-Olivares for final review. |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly looks good to me, just a few nits on naming and documentation.
| final InheritedElement? element = | ||
| context.getElementForInheritedWidgetOfExactType<DragBoundary>(); | ||
| if (element == null) { | ||
| return _DragBoundaryDelegateForRect(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we return a boundary even if there is none? It feels a little redundant when we can just return null and stop any further logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for this one:
If no [DragBoundary] ancestor is found, the delegate will return a delegate that allows the drag object to move freely.
This is proposed by me so that using this method is easier and doesn't have to handle nulls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this way it may be hard for a user to differentiate between having no parent DragBoundary and having a default one that allows free movement. Maybe we can provide both methods of and maybeOf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but when do you think maybeOf will be useful? I think we can add it upon request in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its useful if anyone wants to write some fallback logic in case they don't have a DragBoundary ancestor, but i'm okay on adding on request.
examples/api/lib/widgets/gesture_detector/gesture_detector.3.dart
Outdated
Show resolved
Hide resolved
|
Updated the suggested documentation. Additionally, I set |
| final InheritedElement? element = | ||
| context.getElementForInheritedWidgetOfExactType<DragBoundary>(); | ||
| if (element == null) { | ||
| return _DragBoundaryDelegateForRect(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this way it may be hard for a user to differentiate between having no parent DragBoundary and having a default one that allows free movement. Maybe we can provide both methods of and maybeOf?
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ small nit. Thank you for the contribution!
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ small nit. Thank you for the contribution!
flutter/flutter@42132e8...fe71cad 2024-10-30 reidbaker@google.com Update CHANGELOG.md to correct ios vs macos issue (flutter/flutter#157822) 2024-10-30 tessertaha@gmail.com Add ability to customize the default `Slider` padding (flutter/flutter#156143) 2024-10-30 11473691+YeungKC@users.noreply.github.com Fix menu anchor state handling (flutter/flutter#157612) 2024-10-30 32538273+ValentinVignal@users.noreply.github.com Add test for `interactive_viewer.0.dart` (flutter/flutter#157773) 2024-10-30 32538273+ValentinVignal@users.noreply.github.com Add test for `scroll_metrics_notification.0.dart` (flutter/flutter#157768) 2024-10-30 ybz975218925@gmail.com Add boundary feature to the drag gesture. (flutter/flutter#147521) 2024-10-30 nate.w5687@gmail.com Fix `ResizeImage` documentation (flutter/flutter#157619) 2024-10-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 795b5492f1b9 to 999797a2f690 (1 revision) (flutter/flutter#157825) 2024-10-29 magder@google.com Avoid labeling all PRs as 'text-input' (flutter/flutter#157805) 2024-10-29 engine-flutter-autoroll@skia.org Roll Packages from e0c4f55 to 028027e (8 revisions) (flutter/flutter#157813) 2024-10-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 725c8e4bc379 to 795b5492f1b9 (5 revisions) (flutter/flutter#157820) 2024-10-29 matanlurey@users.noreply.github.com Fix and remove a few `no-shuffle` tags in `flutter_tools`. (flutter/flutter#157656) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Inspired by the review on #146182.
This PR adds boundary feature to the drag gestures, including
MultiDragGestureRecognizerandDragGestureRecognizer. TheGestureDetectorwidget will also benefit from this.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.