[Android] Encode the original pointer count in messages that represent Android touch events #178015
Conversation
github-actions
bot
added
platform-android
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with duplicate touch events being sent to platform views on Android. The changes introduce a more reliable way for the framework to reassemble multi-pointer touch events by encoding the original pointer count in the messages from the embedder. The implementation looks correct, with corresponding changes on both the native (Java) and framework (Dart) sides. A new test case has been added to verify that move events are no longer duplicated, ensuring the fix is effective. I have a few minor suggestions to improve maintainability by replacing magic numbers with named constants.
...e/src/flutter/shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java
Outdated
Show resolved
Hide resolved
gmackall
left a comment
gmackall
left a comment
There was a problem hiding this comment.
Mostly lgtm, just one question about the way we determine the action below this change, and also one question for my own understanding
| ); | ||
| } | ||
|
|
||
| bool isSinglePointerAction(PointerEvent event) => |
There was a problem hiding this comment.
Is this removed because it was a heuristic for determining if the change affected multiple pointers, and we no longer need a heuristic as we explicitly encode that info?
There was a problem hiding this comment.
Yes - with this change the isSinglePointerAction function is no longer needed.
Messages from multiple-pointer events will be marked with the kPointerDataFlagMultiple flag.
| if (pointerIdx != originalPointerCount - 1) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we also update the logic below this to remove the use of numPointers, and then remove that value? I would think we would also want to encode the Android action we send based off the true pointer count of the original touch event, is that not right?
There was a problem hiding this comment.
numPointers is still being used by another code path that converts pointer data messages into Android pointer up and down events. Specifically, Android's MotionEvent has separate ACTION_DOWN/ACTION_UP and ACTION_POINTER_DOWN/ACTION_POINTER_UP codes for primary versus non-primary pointers. The framework side decides whether to map a message to ACTION_DOWN or ACTION_POINTER_DOWN based on its local state indicating whether this is the first pointer down (and likewise for the last pointer up).
AFAICT there is no race or potential for duplicate messages there. Each pointer up/down message will result in exactly one Android pointer up/down event. The logic will ensure that an ACTION_DOWN is sent before any ACTION_POINTER_DOWN events (and the reverse for pointer up).
The process of trying to recover the original Android events from Flutter's internal event representation does have complexity and potential for errors.
I suspect that there are more cases where the recovered events do not accurately reflect the original events (for example, it looks like the code in toAndroidMotionEvent that calculates pointerIdx may not consistently map Flutter's pointer IDs to Android pointer IDs). But I wanted to keep this PR focused on solving only one specific known issue.
There was a problem hiding this comment.
Sounds good, if this would mean changing additional paths then makes sense to not include it
The process of trying to recover the original Android events from Flutter's internal event representation does have complexity and potential for errors.
I suspect that there are more cases where the recovered events do not accurately reflect the original events (for example, it looks like the code in toAndroidMotionEvent that calculates pointerIdx may not consistently map Flutter's pointer IDs to Android pointer IDs). But I wanted to keep this PR focused on solving only one specific known issue.
Yeah, I've personally seen mismatches specifically for this case where the reconstructed event is ACTION_POINTER_DOWN while the saved (true original) event is ACTION_DOWN, when testing #177572 and printing them both out
There was a problem hiding this comment.
@gmackall is this an area where we should do a deep technical dive to try to reproduce the types of input you are seeing fail to be reproduced correctly?
There was a problem hiding this comment.
I looked into some issues affecting Android platform view pointer up/down events and wrote it up at #178189
…t Android touch events Android touch events include updates to multiple pointers, but each pointer data message sent from the embedder to the framework represents a single pointer. So the Android embedder will send multiple messages for each touch event, and the framework's AndroidViewController will reassemble the messages and forward the resulting event to the platform view. The AndroidViewController tracks the number of active pointers in its own local state. If that state is out of sync with the event handled by the Android embedder, then the AndroidViewController may send duplicate events to the platform view. This PR encodes the Android touch event's pointer count in the messages sent to the framework. This allows the AndroidViewController to reliably determine whether it has received all of the pointer messages that originated from an event. Fixes flutter#176574
jason-simmons
force-pushed
the
bug_176574
branch
from
3de61dd to
9eb51f2
Compare
chinmaygarde
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
…t represent Android touch events (flutter/flutter#178015)
4 participants
Android touch events include updates to multiple pointers, but each pointer data message sent from the embedder to the framework represents a single pointer. So the Android embedder will send multiple messages for each touch event, and the framework's AndroidViewController will reassemble the messages and forward the resulting event to the platform view.
The AndroidViewController tracks the number of active pointers in its own local state. If that state is out of sync with the event handled by the Android embedder, then the AndroidViewController may send duplicate events to the platform view.
This PR encodes the Android touch event's pointer count in the messages sent to the framework. This allows the AndroidViewController to reliably determine whether it has received all of the pointer messages that originated from an event.
Fixes #176574