Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Feb 7, 2022

Reland of #88534 .

Because this caused odd test failures in g3, I added a static that can be used to disable it - which I think we could probably remove before another release.

Fixes #87322

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Feb 7, 2022
@jonahwilliams jonahwilliams marked this pull request as ready for review February 9, 2022 18:38
@jonahwilliams jonahwilliams changed the title [framework] use platform touchslop [framework] use platform touchslop on Android Feb 9, 2022
Copy link
Member

Choose a reason for hiding this comment

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

Ugh. Do you have plans to clean up google3 and remove this flag in the near future?

Also, should we mark this flag es deprecated to discourage new users from using it? I assume it's only meant as a temporary measure and not a long-term API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh is right. I am hoping I could roll, cleanup the flag, and then delete it. I tried to run a global presubmit and I only got scuba failures - so maybe I don't need it. WDUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it would be easier to remove after the fact too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets give it a shot without the static

Copy link
Member

Choose a reason for hiding this comment

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

I am hoping I could roll, cleanup the flag, and then delete it.

Wouldn't you have to start out with the flag being false to allow the role to go in? So it would more be like roll, set it t true within google and clean up, switch the default to true, remove the flag in google, remove the flag in open source land.

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 was thinking it would be easier to g3 fix instances of the failure ... but I dunno, if its just scubas we can just accept the new diffs. I think the test that failed prior may not exist anymore


@override
void didChangeDependencies() {
_mediaQueryData = MediaQuery.maybeOf(context);
Copy link
Member

Choose a reason for hiding this comment

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

Actually, there's no guarantee that the GestureDetectors will be updated with the new gestureSettings when didChangeDependencies fires because setCanDrag may not execute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true - I don't think there is any reason for gesture settings to change frequently, but if the gesture detectors were already created we'd need to update them. That said, scrollable doesn't have access to the recognizer instances - and the recognizer instances don't have enough context to know generally if they should use the gesture settings

Copy link
Member

Choose a reason for hiding this comment

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

The same is also true for the way is currently handles changes to physics....

I guess under the assumption that these rarely change, this is fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems likely. I don't think there is a usecase for changing gesture settings besides at startup anyway

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit ef803a2 into flutter:master Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flutter should use the platform configured touch slop value for Android

3 participants