Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,15 @@ public class AndroidTouchProcessor {
@VisibleForTesting static final int DEFAULT_VERTICAL_SCROLL_FACTOR = 48;
@VisibleForTesting static final int DEFAULT_HORIZONTAL_SCROLL_FACTOR = 48;

// This value must match the value in framework's platform_view.dart.
// These values must match the values in the framework's platform_views.dart.
// This flag indicates whether the original Android pointer events were batched together.
private static final int POINTER_DATA_FLAG_BATCHED = 1;
// This flag indicates that this message is part of a group of messages representing
// a change that affects multiple pointers.
private static final int POINTER_DATA_FLAG_MULTIPLE = 2;

// Bit shift for encoding the pointer count when using POINTER_DATA_FLAG_MULTIPLE
private static final int POINTER_DATA_MULTIPLE_POINTER_COUNT_SHIFT = 8;

// The view ID for the only view in a single-view Flutter app.
private static final int IMPLICIT_VIEW_ID = 0;
Expand Down Expand Up @@ -212,7 +218,10 @@ public boolean onTouchEvent(@NonNull MotionEvent event, @NonNull Matrix transfor
// but it's the responsibility of a later part of the system to
// ignore 0-deltas if desired.
for (int p = 0; p < originalPointerCount; p++) {
addPointerForIndex(event, p, pointerChange, 0, transformMatrix, packet);
int pointerData =
POINTER_DATA_FLAG_MULTIPLE
| (originalPointerCount << POINTER_DATA_MULTIPLE_POINTER_COUNT_SHIFT);
addPointerForIndex(event, p, pointerChange, pointerData, transformMatrix, packet);
}
}

Expand Down
22 changes: 16 additions & 6 deletions packages/flutter/lib/src/services/platform_views.dart
Original file line number Diff line number Diff line change
Expand Up @@ -619,19 +619,32 @@ class _AndroidMotionEventConverter {
final int pointerIdx = pointers.indexOf(event.pointer);
final int numPointers = pointers.length;

// This value must match the value in engine's FlutterView.java.
// These values must match the values in the engine's AndroidTouchProcessor.java.
// This flag indicates whether the original Android pointer events were batched together.
const int kPointerDataFlagBatched = 1;
// This flag indicates that this event is part of a group of events representing a change
// that affects multiple pointers.
const int kPointerDataFlagMultiple = 2;

// Mask for extracting the flag value from the event's platformData
const int kPointerDataFlagMask = 0xff;
const int kPointerDataMultiplePointerCountShift = 8;

// Android MotionEvent objects can batch information on multiple pointers.
// Flutter breaks these such batched events into multiple PointerEvent objects.
// When there are multiple active pointers we accumulate the information for all pointers
// as we get PointerEvents, and only send it to the embedded Android view when
// we see the last pointer. This way we achieve the same batching as Android.
if (event.platformData == kPointerDataFlagBatched ||
(isSinglePointerAction(event) && pointerIdx < numPointers - 1)) {
final int platformDataFlag = event.platformData & kPointerDataFlagMask;
if (platformDataFlag == kPointerDataFlagBatched) {
return null;
}
if (platformDataFlag == kPointerDataFlagMultiple) {
final int originalPointerCount = event.platformData >> kPointerDataMultiplePointerCountShift;
if (pointerIdx != originalPointerCount - 1) {
return null;
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I looked into some issues affecting Android platform view pointer up/down events and wrote it up at #178189


final int? action = switch (event) {
PointerDownEvent() when numPointers == 1 => AndroidViewController.kActionDown,
Expand Down Expand Up @@ -697,9 +710,6 @@ class _AndroidMotionEventConverter {
},
);
}

bool isSinglePointerAction(PointerEvent event) =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes - with this change the isSinglePointerAction function is no longer needed.

Messages from multiple-pointer events will be marked with the kPointerDataFlagMultiple flag.

event is! PointerDownEvent && event is! PointerUpEvent;
}

class _CreationParams {
Expand Down
57 changes: 57 additions & 0 deletions packages/flutter/test/services/platform_views_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/gestures.dart';
import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';

Expand Down Expand Up @@ -439,6 +440,62 @@ void main() {
await viewController.setOffset(const Offset(10, 20));
expect(viewsController.offsets, equals(<int, Offset>{}));
});

testWidgets('motion event converter does not duplicate move events', (
WidgetTester tester,
) async {
final List<MethodCall> log = <MethodCall>[];
tester.binding.defaultBinaryMessenger.setMockMethodCallHandler(
SystemChannels.platform_views,
(MethodCall methodCall) async {
log.add(methodCall);
return null;
},
);

final AndroidViewController viewController = PlatformViewsService.initSurfaceAndroidView(
id: 7,
viewType: 'web',
layoutDirection: TextDirection.ltr,
);
viewController.pointTransformer = (Offset offset) => offset;

const int pointerCount = 10;
for (int i = 0; i < pointerCount; i++) {
final PointerEvent event = PointerDownEvent(
timeStamp: const Duration(milliseconds: 1),
pointer: i,
);
viewController.dispatchPointerEvent(event);
}

// Pointer event platform data constant from _AndroidMotionEventConverter
const int kPointerDataFlagMultiple = 2;

for (int i = 0; i < pointerCount; i++) {
final PointerEvent event = PointerMoveEvent(
timeStamp: const Duration(milliseconds: 2),
pointer: i,
platformData: kPointerDataFlagMultiple | (pointerCount << 8),
);
viewController.dispatchPointerEvent(event);
}

// Indexes in the list returned by AndroidMotionEvent._asList
const int kAndroidMotionEventListIndexAction = 3;
const int kAndroidMotionEventListIndexPointerCount = 4;
Comment thread
jason-simmons marked this conversation as resolved.

final List<MethodCall> moveCalls = log.where((MethodCall call) {
final List<dynamic> args = call.arguments as List<dynamic>;
return call.method == 'touch' &&
args[kAndroidMotionEventListIndexAction] == AndroidViewController.kActionMove;
}).toList();

// The _AndroidMotionEventConverter should yield one touch event containing all of the pointers.
expect(moveCalls.length, equals(1));
final List<dynamic> moveArgs = moveCalls.single.arguments as List<dynamic>;
expect(moveArgs[kAndroidMotionEventListIndexPointerCount], equals(pointerCount));
});
});

group('iOS', () {
Expand Down