-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[framework] use platform touchslop on Android #97971
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
[framework] use platform touchslop on Android #97971
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
7d8db3c to
37a6061
Compare
|
|
||
| @override | ||
| void didChangeDependencies() { | ||
| _mediaQueryData = MediaQuery.maybeOf(context); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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