Use gestureSettings.touchSlop in PrimaryPointerGestureRecognizer

[KyleFin]

Use gestureSettings.touchSlop in PrimaryPointerGestureRecognizer.handleEvent() unless a non-default value (including null) is specified.

This behavior will be reflected in subclasses (TapGestureRecognizer and LongPressGestureRecognizer) and enable GestureDetector tap and long-press touch slop to be configured out of the box with MediaQuery instead of providing custom gesture recognizers:

// After this PR you can do this:
child: MediaQuery(
      data: MediaQuery.of(context).copyWith(
        gestureSettings: const DeviceGestureSettings(touchSlop: 64),
      ),
      child: GestureDetector(
        onTap: widget.onTap,
        child: Container(),
      ),


// instead of this:
child: RawGestureDetector(
   gestures: <Type, GestureRecognizerFactory>{
     TapGestureRecognizer: GestureRecognizerFactoryWithHandlers<TapGestureRecognizer>(
       // Without https://github.com/flutter/flutter/pull/161541, we'll actually need to make
       // our own custom TapGestureRecognizer class to expose slop parameters too.
       () => TapGestureRecognizer(preAcceptSlopTolerance: 64, postAcceptSlopTolerance: 64, onTap: onTap),
       (TapGestureRecognizer instance) {},
     ),
   },
   child: Container(),
 ),

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.

#154215

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

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

[KyleFin]

I originally had _unsetTouchSlop = kTouchSlop, but adding a -1 magic value may be a slightly less breaking change.

With -1 behavior, default values will change from kTouchSlop to gestureSettings.touchSlop (identical unless overridden). This could lead to surprising behavior, but may actually make existing app code behave more as intended since anyone who has explicitly overridden MediaQuery.gestureSettings.touchSlop likely intends to use that touch slop and may not have realized that tap and long-press don't honor it. If that's not the behavior they want, they can set gestureSettings.touchSlop = kTouchSlop or preAcceptSlopTolerance = kTouchSlop.

With _unsetTouchSlop = kTouchSlop, this PR could inappropriately change places where preAcceptSlopTolerance was explicitly set to 18 to use gestureSettings.touchSlop (which may have been overriden in MediaQuery to a value besides 18).

[Piinks]

We generally avoid magic numbers in the framework: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-heuristics-and-magic

[KyleFin]

What would be a good way to provide an "unset" default value so users can pass a specific value or null?

[Piinks]

Usually we would use null. :)

[KyleFin]

But I don't want the default value to be null. I only want to use null if explicitly requested by the user.

[Piinks]

With tools like dart fix, it is a lot easier to make minor changes that can be easily auto-migrated for folks, which are usually a lot more tolerable to land.

[KyleFin]

Needing to pass double? preAcceptSlopTolerance and differentiate between:

• no value specified
• numeric value specified
• null specified

reminds me of copyWith methods that need to allow specifying null (since generally only non-null values are set in the new object and others retain their current value)

Two common approaches I'm familiar with in that situation are:

• passing a nullable callback that returns the new value (so a null callback is different than a callback that returns null)
• Using nullable records

Records may work well here.

So it could be something like this?

PrimaryPointerGestureRecognizer({
    this.deadline,
    this.preAcceptSlopTolerance,
    this.postAcceptSlopTolerance,
})

/// The maximum distance ...
/// Specified in a record to allow leaving `null` (use default value) OR
/// explicitly setting `(null)` to indicate that the gesture can drift for any distance.
final (double?,)? preAcceptSlopTolerance;
final (double?,)? postAcceptSlopTolerance;

Then invocations could look like this:

// Use default (from gestureSettings or kTouchSlop)
PrimaryPointerGestureRecognizer();

// Use specific value
PrimaryPointerGestureRecognizer(preAcceptSlopTolerance: (42));

// Explicitly null
PrimaryPointerGestureRecognizer(preAcceptSlopTolerance: (null));

(though I still think a sentinel value would be more intuitive for end users)

[MitchellGoodwin]

I'm not a big fan of the sentinel pattern either, but I'm struggling to think of an alternative.

One thing that I think could be improved about this approach is that I don't like how the -1 sticks around after the constructor. Both values are public, so if someone was looking at primaryPointerGestureRecognizerInstance.preAcceptSlopTolerance for some reason, they'd get back a -1 which is a little confusing and magical behavior. I think the private getters should be public. What might be a little better is something like:

PrimaryPointerGestureRecognizer({
  this.deadline,
  double? preAcceptSlopTolerance = _unsetTouchSlop,
  double? postAcceptSlopTolerance = _unsetTouchSlop,
  super.debugOwner,
  super.supportedDevices,
  super.allowedButtonsFilter,
}) : _preAcceptSlopToleranceSet = preAcceptSlopTolerance != _unsetTouchSlop;
     _postAcceptSlopToleranceSet = postAcceptSlopTolerance != _unsetTouchSlop;
     preAcceptSlopTolerance = preAcceptSlopToleranceSet ? preAcceptSlopTolerance : null;
     postAcceptSlopTolerance = postAcceptSlopToleranceSet ? postAcceptSlopTolerance : null;
     assert(
        preAcceptSlopTolerance == _unsetTouchSlop || preAcceptSlopTolerance == null || preAcceptSlopTolerance >= 0,
        'The preAcceptSlopTolerance must be unspecified, positive, or null',
      ),
      assert(
        postAcceptSlopTolerance == _unsetTouchSlop || postAcceptSlopTolerance == null || postAcceptSlopTolerance >= 0,
        'The postAcceptSlopTolerance must be unspecified, positive, or null',
      );

  bool _preAcceptSlopToleranceSet;
  bool _postAcceptSlopToleranceSet;

  double? get preAcceptSlopTolerance =>
      preAcceptSlopToleranceSet
          ? preAcceptSlopTolerance
          : _defaultTouchSlop;
   
  ...

[KyleFin]

That seems nice to make the getters public 👍

We'd also need a new private internal field since get preAcceptSlopTolerance can't return itself, right?

(For instance, we need to add an underscore at the beginning of this line:
_preAcceptSlopTolerance = preAcceptSlopToleranceSet ? preAcceptSlopTolerance : null;)

[KyleFin]

I made the public getters change in 229e880

That was a great improvement. Thanks!

[Piinks]

Hey @KyleFin thanks for contributing! Welcome!
It looks like this is still a draft, and is currently failing many tests. Would you like to continue working on this? We'd be happy to review once it is no longer a draft and marked ready for review.

[KyleFin]

Thanks @Piinks!

I'd love to get any high-level thoughts about the 2 approaches in #154215 (comment) (even just from reading that comment, not any of the code) and then I'll proceed with tests etc.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[flutter-dashboard]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

[justinmc]

@KyleFin Heads up that there are analyzer failures.

[KyleFin]

@justinmc failures are resolved after I applied @MitchellGoodwin's (great) getter suggestion and rebased on latest main.

Unless there are other ideas of how we could avoid using a sentinel, I think this PR is ready to merge.

[Piinks]

Nice! Yeah, the change here now obscures the sentinel from the user - perfect! Thank you.
This LGTM

[MitchellGoodwin]

Should call out in the documentation that the default favors gestureSettings.touchSlop now, but other than that LGTM.

[KyleFin]

/// Defaults to gestureSettings.touchSlop with a fallback of 18 logical pixels. is the wording I used. Does that sound good?

[MitchellGoodwin]

LGTM! Thank you for the change.