[android] Childview will process its motion events #19662

iskakaushik · 2020-07-10T22:10:15Z

Prior to this change the parent view, i.e, FlutterView intercepts
all the MotionEvents and sent them to the framework for processing,
after this it makes it so that the ChildView processes the event.

Fixes flutter/flutter#61200

fluttergithubbot · 2020-07-10T22:10:17Z

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.

blasten · 2020-07-13T03:33:04Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

    mutatorViews.put(viewId, mutatorView);
    mutatorView.addView(platformView.getView());
    ((FlutterView) flutterView).addView(mutatorView);
  }

+  public void attachToFlutterRenderer(FlutterRenderer flutterRenderer) {
+    androidTouchProcessor = new AndroidTouchProcessor(flutterRenderer, /*trackMotionEvents=*/ true);


wdyt about making AndroidTouchProcessor a singleton, and pass this trackMotionEvents to onTouchEvent?

so for moving trackMotionEvents to AndroidTouchProcessor, the only reason i did not have the additional arg onTouch was because, we also need it for onGenericMotionEvent, so Ideally, we would have a MotionEventTrackingAndroidTouchProcessor as a delegate for AndroidTouchProcessor and have it track all events that go through that, but I added an extra constructor param for now.

blasten · 2020-07-13T03:38:38Z

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

+    packet.putDouble(event.getRawX(pointerIndex)); // physical_x
+    packet.putDouble(event.getRawY(pointerIndex)); // physical_y


As discussed, we would need a shim for getRawX and getRawY.

Yup, looking into it now.

blasten · 2020-07-13T19:55:51Z

shell/platform/android/io/flutter/embedding/engine/mutatorsstack/FlutterMutatorView.java

+  public boolean performClick() {
+    return super.performClick();


is this needed?

blasten · 2020-07-13T20:03:28Z

shell/platform/android/io/flutter/embedding/engine/mutatorsstack/FlutterMutatorView.java

  /**
   * Initialize the FlutterMutatorView. Use this to set the screenDensity, which will be used to
   * correct the final transform matrix.
   */
-  public FlutterMutatorView(@NonNull Context context, float screenDensity) {
+  public FlutterMutatorView(
+      @NonNull Context context, float screenDensity, AndroidTouchProcessor androidTouchProcessor) {


is androidTouchProcessor NonNull?

yup, added the annotation

blasten · 2020-07-13T20:05:55Z

shell/platform/android/io/flutter/embedding/engine/mutatorsstack/FlutterMutatorView.java

+  @Override
+  // Intercept the events here and do not propagate them to the child platform views.


minor nit: for consistency, what about moving the @Override after the javadoc, and using /** */?

sounds good

blasten · 2020-07-13T20:08:42Z

shell/platform/android/io/flutter/embedding/engine/mutatorsstack/FlutterMutatorView.java

+    Matrix screenMatrix = new Matrix();
+    screenMatrix.postTranslate(getLeft(), getTop());


did you intend to use getPlatformViewMatrix() here or somewhere else?

nope, it is for when we need to support mutations on top of platform views

the mutator view does rotate, translate and scale. right @cyanglaz ?

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java

blasten

LGTM. This change will be tested by the e2e android view test.

blasten · 2020-07-14T03:57:17Z

@iskakaushik I removed the waiting for tree to go green label. While updating the e2e test, I saw a strange behavior where events looked duplicated. It turns out Android mutates the MotionEvent instance, so we need to call MotionEvent.obtain here

engine/shell/platform/android/io/flutter/embedding/android/MotionEventTracker.java

Line 53 in fef0fcc

eventById.put(eventId.id, event);

The e2e test wasn't designed to capture these issues because it dispatches unique instances every time. I'm going to change the test so it fires a tap from shell.

blasten · 2020-07-14T04:41:16Z

flutter/flutter#61417 is verifying this issue for virtual displays. I will add the same for hybrid e2e. Once we address the mutation issue, then feel free to merge!

blasten · 2020-07-14T14:27:43Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

@@ -325,10 +329,12 @@ public MotionEvent toMotionEvent(
            .toArray(new PointerCoords[touch.pointerCount]);

    if (!usingVirtualDiplays && trackedEvent != null) {
+      // TODO (kaushikiska): investigate why we can not use the tracked


I think the reason is that the action was mutated while it was waiting for the reply from the framework

iskakaushik · 2020-07-14T16:52:14Z

@blasten after my conversation with the android-ui team, it sounds like this approach isn't sound as it translates things from mutator view coordinate space to screen space as opposed to FlutterView space. I'll make amends to fix that as well.

blasten

LGTM

fluttergithubbot · 2020-07-14T18:21:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

Prior to this change the parent view, i.e, FlutterView intercepts all the MotionEvents and sent them to the framework for processing, after this it makes it so that the ChildView processes the event.

There was an issue where we were getting it w.r.t screen as opposed to parent.

…ngine#19662)

Co-authored-by: Kaushik Iska <[email protected]>

iskakaushik requested a review from blasten July 10, 2020 22:10

fluttergithubbot added the platform-android label Jul 10, 2020

googlebot added the cla: yes label Jul 10, 2020

auto-assign bot requested a review from jason-simmons July 10, 2020 22:11

blasten reviewed Jul 13, 2020

View reviewed changes

iskakaushik force-pushed the consume_input_events_at_child_view branch from 1c412b5 to 62d495d Compare July 13, 2020 19:45

blasten reviewed Jul 13, 2020

View reviewed changes

iskakaushik force-pushed the consume_input_events_at_child_view branch from 62d495d to 835b2d0 Compare July 13, 2020 20:02

blasten reviewed Jul 13, 2020

View reviewed changes

shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java Show resolved Hide resolved

iskakaushik force-pushed the consume_input_events_at_child_view branch 4 times, most recently from b8a6247 to 5636064 Compare July 13, 2020 21:16

blasten approved these changes Jul 14, 2020

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 14, 2020

blasten removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 14, 2020

blasten reviewed Jul 14, 2020

View reviewed changes

iskakaushik force-pushed the consume_input_events_at_child_view branch from f2704b6 to 8e72b19 Compare July 14, 2020 17:05

iskakaushik requested a review from blasten July 14, 2020 17:27

blasten approved these changes Jul 14, 2020

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 14, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 14, 2020

iskakaushik force-pushed the consume_input_events_at_child_view branch from c3410d1 to 0b7ed4a Compare July 14, 2020 19:11

Kaushik Iska added 4 commits July 14, 2020 17:15

[android] Childview will process its motion events

c7207d1

Prior to this change the parent view, i.e, FlutterView intercepts all the MotionEvents and sent them to the framework for processing, after this it makes it so that the ChildView processes the event.

replace getRawX and getRawY with shims

57a8b35

Get transform matrix relative to parent coordinate space

840b022

There was an issue where we were getting it w.r.t screen as opposed to parent.

copy motion event and re-use it as it gets mutated later

e1aefc5

iskakaushik force-pushed the consume_input_events_at_child_view branch from 0b7ed4a to e1aefc5 Compare July 15, 2020 00:15

blasten merged commit 1832613 into flutter:master Jul 15, 2020

engine-flutter-autoroll mentioned this pull request Jul 15, 2020

Roll Engine from 7b1515ed292a to 383dfa2033b8 (9 revisions) flutter/flutter#61538

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 15, 2020

1832613 [android] Childview will process its motion events (flutter/e…

91c4d82

…ngine#19662)

pcsosinski pushed a commit to pcsosinski/engine that referenced this pull request Jul 15, 2020

[android] Childview will process its motion events (flutter#19662)

957dac3

pcsosinski pushed a commit that referenced this pull request Jul 16, 2020

[android] Childview will process its motion events (#19662) (#19776)

b15a076

Co-authored-by: Kaushik Iska <[email protected]>

		packet.putDouble(event.getRawX(pointerIndex)); // physical_x
		packet.putDouble(event.getRawY(pointerIndex)); // physical_y

		@Override
		// Intercept the events here and do not propagate them to the child platform views.

		Matrix screenMatrix = new Matrix();
		screenMatrix.postTranslate(getLeft(), getTop());

[android] Childview will process its motion events #19662

[android] Childview will process its motion events #19662

Uh oh!

Conversation

iskakaushik commented Jul 10, 2020 • edited by blasten Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fluttergithubbot commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

blasten commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blasten commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Jul 14, 2020

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Jul 14, 2020

Uh oh!

Uh oh!

iskakaushik commented Jul 10, 2020 •

edited by blasten

Loading

blasten commented Jul 14, 2020 •

edited

Loading