Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@mengdouer
Copy link
Contributor

@mengdouer mengdouer commented Jun 21, 2022

Fixes flutter/flutter#105229
fixes flutter/flutter#70639

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@xanahopper
Copy link

This PR will fix the following issues

But using reconstruct event from Flutter engine may cause PlatformView receive a single-touch event instead of a multi-touch event and missing some touch point infomation.

Please make sure the original behavior of multi-touch event with different view in Android.

@wangying3426
Copy link
Contributor

@iskakaushik Could you have time to look at this PR?

@chinmaygarde
Copy link
Member

From Triage: It is hard to reason about the correctness of this patch. Can you write a test or describe the issue and fix in more detail?

@mengdouer
Copy link
Contributor Author

@chinmaygarde When the gesture event comes, it is stored in the trackedEvent in addPointerForIndex. When the PlatformView wants to respond to the gesture event later, it mixes the MotionEvent synthesized from flutter and the attributes of the event saved at that time. At this time, the correctness of the ActionIndex in the synthesized Event cannot be guaranteed. For example, ActionIndex has always been 2, but pointerCount has changed from 3 to 2, so a crash will occur (in general, it will be caught but the special mechanism of chromium will throw it out)

if (trackedEvent != null) {
      return MotionEvent.obtain(
          trackedEvent.getDownTime(),
          trackedEvent.getEventTime(),
          touch.action,
          touch.pointerCount,
          pointerProperties,
          pointerCoords,
          trackedEvent.getMetaState(),
          trackedEvent.getButtonState(),
          trackedEvent.getXPrecision(),
          trackedEvent.getYPrecision(),
          trackedEvent.getDeviceId(),
          trackedEvent.getEdgeFlags(),
          trackedEvent.getSource(),
          trackedEvent.getFlags());
    }

We are also thinking about other solutions, like @xanahopper mentioned above may affect the behavior of multi-point contact, if you have any ideas, you can share them here

@chinmaygarde
Copy link
Member

Thanks for elaborating. cc @dnfield. This probably still needs a test.

@chinmaygarde chinmaygarde removed the request for review from iskakaushik July 14, 2022 20:14
@mengdouer
Copy link
Contributor Author

mengdouer commented Aug 1, 2022

Thanks for elaborating. cc @dnfield. This probably still needs a test.

Sorry for not replying for so long. Not sure if this also needs to be tested.
As another example, when two or more events are sent to the framework side, but some of the current events are processed by the framework side, for example, platformview and a Container are on the same stack, and the first event falls to the Container. It is not transparent, and the rest is sent back to platformview. When toMotionEvent, the actionIndex is still sent before, such as 2, but because some events are intercepted, the check will fail.

@mengdouer
Copy link
Contributor Author

mengdouer commented Aug 1, 2022

Below is where the error occurs

// frameworks/base/core/jni/android_view_MotionEvent.cpp
static jint android_view_MotionEvent_nativeGetPointerId(JNIEnv* env, jclass clazz,
        jlong nativePtr, jint pointerIndex) {
    MotionEvent* event = reinterpret_cast<MotionEvent*>(nativePtr);
    size_t pointerCount = event->getPointerCount();
    if (!validatePointerIndex(env, pointerIndex, pointerCount)) {
        return -1;
    }
    return event->getPointerId(pointerIndex);
}

static bool validatePointerIndex(JNIEnv* env, jint pointerIndex, size_t pointerCount) {
    if (pointerIndex < 0 || size_t(pointerIndex) >= pointerCount) {
        jniThrowException(env, "java/lang/IllegalArgumentException",
                "pointerIndex out of range");
        return false;
    }
    return true;
}

I think the action of the event handled by platformView should be consistent with the one sent from the framework side, will this cause other problems?This is a bug fix, but I saw that there is no similar test, what do I need to do?
Does anyone have time to take a look at this pr?
@chinmaygarde @iskakaushik

@zanderso zanderso requested review from dnfield and iskakaushik August 4, 2022 20:19
@dnfield
Copy link
Contributor

dnfield commented Aug 4, 2022

The test for this would be similar to itUsesActionEventTypeFromFrameworkEventForVirtualDisplays in PlatformViewsControllerTest.java. Do you think you can take that on?

@dnfield
Copy link
Contributor

dnfield commented Aug 4, 2022

/cc @stuartmorgan FYI

@mengdouer
Copy link
Contributor Author

@dnfield Thank you for your suggestion. I added the test but the test seems to fail in the compilation engine. I was successful locally, could you help me to find the problem?

@dnfield
Copy link
Contributor

dnfield commented Aug 8, 2022

Try merging or rebasing on main - it looks like there may have been changes in CI since you branched that are incompatible.

@mengdouer
Copy link
Contributor Author

@dnfield All the tests have passed, could you please review again

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

This LGTM but I must admit I'm not sure I'm fully grasping the failure modes here. At any rate, we need to keep the SDK happy about what action we're sending it.

@dnfield
Copy link
Contributor

dnfield commented Aug 9, 2022

@stuartmorgan or @iskakaushik would you mind providing a second review here?

@wangying3426
Copy link
Contributor

This fix works fine in our business, so also LGTM.

@wangying3426 wangying3426 added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests platform-android labels Aug 12, 2022
@auto-submit auto-submit bot merged commit 9c3c233 into flutter:main Aug 15, 2022
@wangying3426 wangying3426 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 Aug 15, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 15, 2022
johnmccutchan added a commit that referenced this pull request Dec 19, 2023
…inter count (#47424)

related issue flutter/flutter#111268,
flutter/flutter#106190

### Motivation:
- At flutter/flutter#111268, we found that
Android PlatformView scrolls slowly after #34182 commit

### Modification:
- Makes `PlatformViewsController` view to use `tracked event`'s action &
pointer count

### Result:
- Now PlatformView scrolls not slowly
- Close flutter/flutter#111268,
flutter/flutter#106190


- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

Co-authored-by: John McCutchan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[google_maps_flutter] Hybrid Composition throw error [webview_flutter] Scale and horizontal scroll is not working on Android on some websites

5 participants