Skip to content

Conversation

@yiiim
Copy link
Member

@yiiim yiiim commented Apr 29, 2024

Inspired by the review on #146182.

This PR adds boundary feature to the drag gestures, including MultiDragGestureRecognizer and DragGestureRecognizer. The GestureDetector widget will also benefit from this.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: gestures flutter/packages/flutter/gestures repository. labels Apr 29, 2024
@yiiim yiiim added the c: new feature Nothing broken; request for a new capability label Apr 29, 2024
@Renzo-Olivares Renzo-Olivares self-requested a review April 30, 2024 22:17
Copy link
Contributor

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.

Copy link
Contributor

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".

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

@yiiim yiiim force-pushed the boundary_for_drag_gesture branch from df83e0e to e7a4977 Compare May 8, 2024 05:40
@github-actions github-actions bot removed the f: scrolling Viewports, list views, slivers, etc. label May 8, 2024
@yiiim yiiim requested a review from Renzo-Olivares May 14, 2024 13:58
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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;
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares May 14, 2024

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,
                        ),
                      );
                    },
                  ),
                ),
              ],
            ),
          ),
        ),
      ),
    );
  }
}

Copy link
Member Author

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.

Copy link
Member Author

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;
Copy link
Contributor

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 .

@yiiim yiiim marked this pull request as draft May 17, 2024 05:38
@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@yiiim yiiim force-pushed the boundary_for_drag_gesture branch from a174a1e to 7d95e99 Compare May 23, 2024 14:34
@yiiim yiiim marked this pull request as ready for review May 23, 2024 15:19
@yiiim
Copy link
Member Author

yiiim commented May 23, 2024

Reimplemented and ready for review. @Renzo-Olivares

@yiiim yiiim requested a review from Renzo-Olivares May 23, 2024 15:24
@Renzo-Olivares
Copy link
Contributor

Thank you @yiiim, I will take a look this week.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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!

Copy link
Contributor

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.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares Jun 3, 2024

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@yiiim yiiim force-pushed the boundary_for_drag_gesture branch from 54fc2eb to e6cbf1a Compare June 4, 2024 15:02
@yiiim yiiim force-pushed the boundary_for_drag_gesture branch from e6cbf1a to 4bab7d1 Compare June 4, 2024 15:03
@yiiim yiiim requested a review from Renzo-Olivares June 4, 2024 15:49
@yiiim
Copy link
Member Author

yiiim commented Jun 20, 2024

@Renzo-Olivares Please do not forget this here. 👻

@Renzo-Olivares
Copy link
Contributor

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;
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@dkwingsmt dkwingsmt left a 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>
@dkwingsmt
Copy link
Contributor

Update: I've asked @Renzo-Olivares for final review.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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);
Copy link
Contributor

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.

Copy link
Contributor

@dkwingsmt dkwingsmt Oct 25, 2024

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

@yiiim
Copy link
Member Author

yiiim commented Oct 25, 2024

Updated the suggested documentation. Additionally, I set useGlobalPosition to true by default. We should use global coordinates by default and only use local coordinates when it is confirmed to be within the boundary locally.

final InheritedElement? element =
context.getElementForInheritedWidgetOfExactType<DragBoundary>();
if (element == null) {
return _DragBoundaryDelegateForRect(null);
Copy link
Contributor

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?

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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!

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a 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!

@yiiim yiiim added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024
@auto-submit auto-submit bot merged commit c051b69 into flutter:master Oct 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 30, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 30, 2024
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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App c: new feature Nothing broken; request for a new capability d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants