Skip to content
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

Make DragGestureRecognizer abstract methods public #151627

Conversation

angelosilvestre
Copy link
Contributor

@angelosilvestre angelosilvestre commented Jul 12, 2024

Resolves #151446

DragGestureRecognizer defines several private abstract methods that are implemented by its subclasses.
In the super_editor package, we'd like to extend PanGestureRecognizer to make it more aggressive, so it can win the gesture arena when placed inside a CustomScrollview. However, since we can't override private methods, tweaking this single function would involve copying the entire DragGestureRecognizer interface and its PanGestureRecognizer implementation.


Methods that were updated in this PR:

Method Rationale
_hasSufficientGlobalDistanceToAccept This is the most important method for us. Overriding this method allows tweaking the PanGestureRecognizer to be more aggressive.
_considerFling In super_editor we use the PanGestureRecognizer, but we want the fling gesture to behave as if it was a VerticalDragRecognizer. We'll use the fling gesture just to scroll vertically.
_finalPosition I added a getter to be able to access it inside _considerFling.
_globalDistanceMoved I added a getter to be able to access it inside _hasSufficientGlobalDistanceToAccept.

@flutter-dashboard

This comment was marked as resolved.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: gestures flutter/packages/flutter/gestures repository. labels Jul 12, 2024
@angelosilvestre
Copy link
Contributor Author

I'm not sure which tests could be added here. Since this PR does not change any functionality maybe it doesn't need a test?

@nate-thegrate nate-thegrate self-requested a review July 13, 2024 01:33
Copy link
Member

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @angelosilvestre, thanks for making this contribution!

I gave a bit of feedback, so take a look when you have a chance 🙂

packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
Copy link
Member

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for the update, this PR is looking fantastic!

I'll be happy to approve once we get a testing plan sorted out 👍

Copy link
Member

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM

Thanks for putting in the work here!

Co-authored-by: Nate Wilson <nathan.wilson1232@gmail.com>
@nate-thegrate
Copy link
Member

nate-thegrate commented Jul 14, 2024

It looks like there are a couple of test failures: one of them was caused by changes outside this branch, and the other is a dependency loop (that would have gone undetected if we'd kept the relative import).

Using the Axis enum in DragGestureRecognizer has caused gestures.dart and painting.dart to be codependent—in fact, I bet this is why _DragDirection was created in the first place!

In order to resolve this, we could revert to using a public DragDirection enum (or maybe just bool horizontal).

Or we could migrate some code to prevent the loop. Since painting/basic_types.dart is already exporting a type from the foundation package, I think it'd be super straightforward to just move the enums in that file into foundation/basic_types.dart.


@angelosilvestre Sorry that the act of making a few things public is turning out to be more complex than anticipated. I don't think there's anything you need to do at this point; I'll reach out to some folks tomorrow and hopefully we can come up with a solid action plan.

@angelosilvestre
Copy link
Contributor Author

@nate-thegrate Thanks for the update. Please let me know if there is anything I should do.

@nate-thegrate
Copy link
Member

I created #151771 in order to hopefully unblock this PR. I guess we'll see how it goes!

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such methods include _considerFling, _hasSufficientGlobalDistanceToAccept, among others.

Can you update the PR description to list all of the methods that are being made public, and why? I am not sure all of them need to be exposed in this way.

packages/flutter/lib/src/gestures/monodrag.dart Outdated Show resolved Hide resolved
/// uses.
class _EagerPanGestureRecognizer extends PanGestureRecognizer {
@override
bool hasSufficientGlobalDistanceToAccept(PointerDeviceKind pointerDeviceKind, double? deviceTouchSlop) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about all of the other methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the test simpler I didn't included the other methods. This test doesn't care about the fling gesture, so it wouldn't help here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea behind this comment was: each method that we make public should be included in the test framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'll update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

For our use-case, primaryDragAxis could be kept private, since it has a default implementation and we don't need to change it. However, if someone needs to write a custom VerticalDragRecognizer or a HorizontalDragRecognizer they will need to override primaryDragAxis.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if those methods should be re-privatized, a default implementation should be provided. Otherwise, it won't be possible to subclass DragGestureRecognizer outside of the flutter package.

Copy link
Member

@nate-thegrate nate-thegrate Jul 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I can see that as a reason to keep them as public.

…or, we could pull a pro gamer move and turn DragGestureRecognizer into a sealed class. This could help streamline the logic within the framework, and since extending DragGestureRecognizer directly would no longer be possible, developers wouldn't waste any time trying to do it and could instead extend its subclasses (or the base OneSequenceGestureRecognizer class).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to extend a class that extends a sealed class? I've never done that. If it works, it works for me that way, we could extend PanGestureRecognizer. I'd like to hear @Piinks opinion on that before making this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to extend a class that extends a sealed class?

Yes sir! The main goal of a sealed class is to enable exhaustive pattern-matching:

void foo(DragGestureRecognizer recognizer) {
  switch (recognizer) {
    case HorizontalDragGestureRecognizer():
      // ...
    case VerticalDragGestureRecognizer():
      // ...
    case PanGestureRecognizer():
      // ...
  }
}

Fortunately, passing a subtype of PanGestureRecognizer doesn't break this behavior!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll give it a try.

@angelosilvestre angelosilvestre requested a review from Piinks July 16, 2024 00:05
@nate-thegrate nate-thegrate changed the title Make DragGestureRecognizer abstract methods public (Resolves #151446) Make DragGestureRecognizer abstract methods public Jul 18, 2024
@nate-thegrate nate-thegrate changed the title Make DragGestureRecognizer abstract methods public Make DragGestureRecognizer abstract methods public Jul 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 28, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 29, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 31, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 1, 2024
…7259)

Manual roll requested by stuartmorgan@google.com

flutter/flutter@031dc3d...4d12197

2024-07-26 andrewrkolos@gmail.com further shard Mac tool_integration_tests from 4 to 5 shards (flutter/flutter#152399)
2024-07-26 magder@google.com Change flutter_build_with_compilation_error_test to check stdout or stderr (flutter/flutter#152404)
2024-07-26 victorsanniay@gmail.com [cupertino/icons.dart] Replace ligature references with characters corresponding to codepoints (flutter/flutter#152387)
2024-07-26 737941+loic-sharma@users.noreply.github.com Update minimum macOS version as needed in Swift package (flutter/flutter#152347)
2024-07-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.14 to 3.25.15 (flutter/flutter#152401)
2024-07-26 49699333+dependabot[bot]@users.noreply.github.com Bump ossf/scorecard-action from 2.3.3 to 2.4.0 (flutter/flutter#152400)
2024-07-26 31859944+LongCatIsLooong@users.noreply.github.com Update text_painter.dart (flutter/flutter#152398)
2024-07-26 737941+loic-sharma@users.noreply.github.com Fix some tests that fail with Swift Package Manager enabled (flutter/flutter#152267)
2024-07-26 bkonyi@google.com Reland "Launch DDS from Dart SDK and prepare to serve DevTools from DDS (#146593)" (flutter/flutter#152386)
2024-07-26 angelosilvestre.ccp@gmail.com Make `DragGestureRecognizer` abstract methods public (flutter/flutter#151627)
2024-07-26 koji.wakamiya@gmail.com Fix cursor position when Unicode Zs category is entered in TextField (flutter/flutter#152215)
2024-07-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 354abf2800a0 to e28f8755e25b (2 revisions) (flutter/flutter#152388)

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 rmistry@google.com,stuartmorgan@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
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
Resolves flutter#151446

`DragGestureRecognizer` defines several private abstract methods that are implemented by its subclasses.
In the **super_editor** package, we'd like to extend `PanGestureRecognizer` to make it more aggressive, so it can win the gesture arena when placed inside a `CustomScrollview`. However, since we can't override private methods, tweaking this single function would involve copying the entire `DragGestureRecognizer` interface and its `PanGestureRecognizer` implementation.

<br>

Methods that were updated in this PR:

| Method | Rationale |
|---|---|
| `_hasSufficientGlobalDistanceToAccept` | This is the most important method for us. Overriding this method allows tweaking the PanGestureRecognizer to be more aggressive. |
| `_considerFling` | In **super_editor** we use the PanGestureRecognizer, but we want the fling gesture to behave as if it was a VerticalDragRecognizer. We'll use the fling gesture just to scroll vertically. |
| `_finalPosition` | I added a getter to be able to access it inside `_considerFling`. |
| `_globalDistanceMoved` | I added a getter to be able to access it inside `_hasSufficientGlobalDistanceToAccept`. |
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
Resolves flutter#151446

`DragGestureRecognizer` defines several private abstract methods that are implemented by its subclasses.
In the **super_editor** package, we'd like to extend `PanGestureRecognizer` to make it more aggressive, so it can win the gesture arena when placed inside a `CustomScrollview`. However, since we can't override private methods, tweaking this single function would involve copying the entire `DragGestureRecognizer` interface and its `PanGestureRecognizer` implementation.

<br>

Methods that were updated in this PR:

| Method | Rationale |
|---|---|
| `_hasSufficientGlobalDistanceToAccept` | This is the most important method for us. Overriding this method allows tweaking the PanGestureRecognizer to be more aggressive. |
| `_considerFling` | In **super_editor** we use the PanGestureRecognizer, but we want the fling gesture to behave as if it was a VerticalDragRecognizer. We'll use the fling gesture just to scroll vertically. |
| `_finalPosition` | I added a getter to be able to access it inside `_considerFling`. |
| `_globalDistanceMoved` | I added a getter to be able to access it inside `_hasSufficientGlobalDistanceToAccept`. |
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 f: gestures flutter/packages/flutter/gestures repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make DragGestureRecognizer abstract methods public
4 participants