Deprecate GestureDetector.kind in favor of new supportedDevices
#81858
Conversation
| #TODO(Piinks): update these with PR number once opened | ||
|
|
||
| # Changes made in https://github.com/flutter/flutter/pull/TBD | ||
| - title: "Migrate to 'supportedDevices'" |
|
Oh, there are a few more classes I should apply this change to, marking WIP while I add those in. |
GestureDetector.kind in favor of new supportedDevicesGestureDetector.kind in favor of new supportedDevices
GestureDetector.kind in favor of new supportedDevicesGestureDetector.kind in favor of new supportedDevices
|
Well that escalated quickly. 😜 |
| }); | ||
|
|
||
| testWidgets('ForecePressGestureRecognizer asserts when kind and supportedDevices are both set', (WidgetTester tester) async { | ||
| try { |
There was a problem hiding this comment.
This pattern seems unsafe. Consider if ForcePressGestureRecognizer accidentally had the asset deleted. Then there would be no error to catch and the test would just pass as is.
consider:
expect(() => ForcePressGestureRecognizer(
kind: PointerDeviceKind.touch,
supportedDevices: <PointerDeviceKind>{ PointerDeviceKind.touch },
), throwsA(isA<AssertionError>().having((e) => e.toString(), 'description', contains('kind == null || supportedDevices == null'))There was a problem hiding this comment.
Ah I forgot to add the fail state at the end of the try statement. I like yours better. Maybe we should update other tests that are like this, I've forgotten the fail state before, and seen others do the same. Thanks! 🙏
There was a problem hiding this comment.
@a14n Is this pattern lintable to suggest the throwsA pattern over the error-prone try/catch pattern in tests?
There was a problem hiding this comment.
Running such a lint on flutter doesn't detect any similar case. So I don't think it's common enough to justify the existence/maintenance of this lint.
There was a problem hiding this comment.
My bad, I made a mistake in my test. There are actually cases where it triggers.
There was a problem hiding this comment.
Actually I found several cases (a PR will be sent soon) but there are false positives that are hard to detect. So the lint should not be that useful.
| this.dragStartBehavior = DragStartBehavior.down, | ||
| }) : assert(dragStartBehavior != null), | ||
| super(debugOwner: debugOwner, kind: kind); | ||
| assert(kind == null || supportedDevices == null), |
There was a problem hiding this comment.
Can you rely on the assert in the superclass by passing the arguments straight through (unmodified) to super()?
(here and elsewhere)
There was a problem hiding this comment.
Error messages (and accompanying stack traces) may be slightly clearer if the assert happens as early as possible.
There was a problem hiding this comment.
I was going back and forth on exactly that. Earlier error/stack trace or simpler code... I'm happy to do either. Curious what folks think about the either/or.
Since the parameters are the same for the super and sub classes (kind/supportedDevices), maybe an error on the super class would not be that obscure @goderbauer WDYT?
|
I've updated with the feedback re: tests and assertions. PTAL. :) |
|
This pull request is not suitable for automatic merging in its current state.
|
|
Confirmed the G testing failure is a flake, setting the check green manually. |
Follow up to #81569
This is not a breaking change, but as it involves deprecating something, I will be following up with a migration guide. :)
This deprecates the old
PointerDeviceKindfilter that has been replaced with thesupportedDevicesset. This allows folks to filter allowed gestures for more than one type. This also adds dart fixes for all of the associated classes to assist migration.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.