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

Track motion events for reuse post gesture disambiguation #19484

Merged
merged 3 commits into from
Jul 7, 2020

Conversation

iskakaushik
Copy link
Contributor

@iskakaushik iskakaushik commented Jul 2, 2020

This change makes it so that we track all the motion events encountered by FlutterView and all of its subviews in the MotionEventTracker class, indexed by a unique MotionEventId. This identifier is then passed to the Flutter framework as seen in flutter/flutter#60930. Once the gestures take part in gesture disambiguation and are sent back to the engine, we look-up the original motion event using the MotionEventId and dispatch it to the platform.

Bug: flutter/flutter#58837

@iskakaushik iskakaushik requested a review from blasten July 2, 2020 21:41
@fluttergithubbot
Copy link
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@@ -183,6 +187,7 @@ private void addPointerForIndex(

long timeStamp = event.getEventTime() * 1000; // Convert from milliseconds to microseconds.

packet.putLong(motionEventId.getId());
Copy link

Choose a reason for hiding this comment

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

do you know if the other embeddings have to be updated as well? I wonder if adding the field to the end is safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, this field will be set to 0 for other embeddings and nothing else needs to change.

Copy link

Choose a reason for hiding this comment

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

SG

private static final AtomicLong ID_COUNTER = new AtomicLong(0);
private final long id;

private MotionEventId(long id) {
Copy link

Choose a reason for hiding this comment

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

would this wrapper object provide something that just the return value of AtomicLong doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not much really, except type safety. If you feel strongly about this, I can move to atomic long.

Copy link

Choose a reason for hiding this comment

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

Got it. I don't feel strongly about it, so your call!

@Nullable
public MotionEvent pop(MotionEventId eventId) {
// TODO(kaushikiska) do the actual timestamp based book-keeping.
return eventById.remove(eventId);
Copy link

Choose a reason for hiding this comment

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

do you plan to remove all the previous events as well? How would we make sure that the queue of gestures doesn't grow for ever?

For example:
If the motion events aren't owned by the Android view, how would we make sure that those are ignored or removed?

If there are N motion events, but only the last one is owned by the Android view, where would we delete the motion events owned by Flutter widgets?

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 class will now manage all the events encountered by the Flutter view. Given that all the platform view's events are managed by Flutter view as well, this will essentially only the events for Flutter view and its subviews.

Once we pop an event, I plan on popping all the events that occurred prior to the event and see how that looks like. If that causes drift, we might have to come up with an alternate pruning strategy (for example, periodically remove all the motion events that are more than say 3 seconds old(.

Copy link

Choose a reason for hiding this comment

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

SGTM :)

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM

@iskakaushik iskakaushik force-pushed the android-track-motion-events branch 2 times, most recently from 27015c8 to a9adee6 Compare July 6, 2020 22:16
@iskakaushik iskakaushik changed the title [WIP] Track motion events for reuse post gesture disambiguation Track motion events for reuse post gesture disambiguation Jul 6, 2020
@iskakaushik iskakaushik force-pushed the android-track-motion-events branch from a9adee6 to a03b013 Compare July 6, 2020 22:48
This change makes it so that we track all the motion events encountered by `FlutterView` and all of its subviews in the `MotionEventTracker` class, indexed by a unique `MotionEventId`. This identifier is then passed to the Flutter framework as seen in flutter/flutter#60930. Once the gestures take part in gesture disambiguation and are sent back to the engine, we look-up the original motion event using the `MotionEventId` and dispatch it to the platform.

Bug: flutter/flutter#58837
@iskakaushik iskakaushik force-pushed the android-track-motion-events branch from a03b013 to 2ff6ad8 Compare July 7, 2020 00:16
@iskakaushik iskakaushik merged commit 110a579 into flutter:master Jul 7, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 7, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 7, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 7, 2020
justinmc pushed a commit to justinmc/engine that referenced this pull request Jul 7, 2020
)

This change makes it so that we track all the motion events encountered by `FlutterView` and all of its subviews in the `MotionEventTracker` class, indexed by a unique `MotionEventId`. This identifier is then passed to the Flutter framework as seen in flutter/flutter#60930. Once the gestures take part in gesture disambiguation and are sent back to the engine, we look-up the original motion event using the `MotionEventId` and dispatch it to the platform.

Bug: flutter/flutter#58837
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 7, 2020
a-siva added a commit to a-siva/flutter that referenced this pull request Jul 7, 2020
3fe5edf Roll Skia from cf5e35f72130 to b4d60f807dbd (5 revisions) (flutter/engine#19587)
b919945 include list_libraries.dart as a snapshot for fuchsia
(flutter/engine#19567)
de0932b Manual roll of Dart 06cb010247...69aba23371 (flutter/engine#19577)
35b5aa5 switch const finder to package_config (flutter/engine#19576)
07d5090 Roll Skia from 0c0d8dd6d637 to cf5e35f72130 (13 revisions) (flutter/engine#19573)
0541502 kick build (flutter/engine#19575)
a53782f Roll Skia from 6130d5079d55 to 0c0d8dd6d637 (3 revisions) (flutter/engine#19570)
d0d6a4c Refactor Win32FlutterWindow in preparation for UWP windowing implementation (flutter/engine#18878)
110a579 Track motion events for reuse post gesture disambiguation (flutter/engine#19484)
5f8e91c Resubmit frame when the surface is switched (flutter/engine#19555)
ab0abdd Roll Fuchsia Linux SDK from TvWbh... to 1oAHN...
(flutter/engine#19566)
@xster
Copy link
Member

xster commented Jul 11, 2020

(go/tested-engine-prs nag)
This PR could use a test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants