-
Notifications
You must be signed in to change notification settings - Fork 6k
Add support for double tap action from Apple Pencil 2 #39267
Add support for double tap action from Apple Pencil 2 #39267
Conversation
hellohuanlin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! just some questions around the naming
lib/ui/platform_dispatcher.dart
Outdated
| panDeltaY: packet.getFloat64(kStride * offset++, _kFakeHostEndian), | ||
| scale: packet.getFloat64(kStride * offset++, _kFakeHostEndian), | ||
| rotation: packet.getFloat64(kStride * offset++, _kFakeHostEndian), | ||
| preferredAction: PointerPreferredAction.values[packet.getInt64(kStride * offset++, _kFakeHostEndian)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear what "preferredAction" or PointerPreferredAction means in this context. How about preferredStylusAuxilliaryAction and PointerPreferredStylusAuxilliaryAction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
�[0;32m[ RUN ] �[mShellTest.CanCorrectlySynthesizePointerPacket
[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: RangeError (index): Invalid value: Not in inclusive range 0..3: 1731047719413088311
#0 _Array.[] (dart:core-patch/array.dart:10:36)
#1 PlatformDispatcher._unpackPointerDataPacket (dart:ui/platform_dispatcher.dart:420:55)
#2 PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:364:9)
#3 _dispatchPointerDataPacket (dart:ui/hooks.dart:91:31)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its actually really confusing if both the actual event and the preferredAction are called PointerPreferredStylusAuxilliaryAction and PointerStylusAuxilliaryAction respectively, I got confused just now between the two as Im trying to change the names because then the only difference them is the "preferred", and its pretty verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about StylusSecondaryGesture (double tap) + ConfiguredStylusAction (the preferred action)?
lib/ui/pointer.dart
Outdated
| scale, | ||
|
|
||
| /// A stylus generated action (e.g. double tap on Apple Pencil 2) | ||
| stylusAction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment on the naming here.
| case UITouchTypeIndirect: | ||
| return flutter::PointerData::DeviceKind::kTouch; | ||
| case UITouchTypeStylus: | ||
| case UITouchTypePencil: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, UITouchTypeStylus is deprecated and is suggested https://developer.apple.com/documentation/uikit/uitouchtype/uitouchtypestylus
| #pragma clang diagnostic push | ||
|
|
||
| - (void)pencilInteractionDidTap:(UIPencilInteraction*)interaction API_AVAILABLE(ios(13.4)) { | ||
| flutter::PointerData pointer_data = [self generatePointerDataAtLastMouseLocation]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the mouse location for this pointer_data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this is not resolved - we don't need the mouse location here. Can probably just do
flutter::PointerData pointer_data;
pointer_data.Clear();
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm
Outdated
Show resolved
Hide resolved
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you have work-in-progress design doc out for this feature: https://docs.google.com/document/d/1r4P5r-jGt2Sjqro3ldCU2axUiHTpu3yhIycnI94OKQw/edit
I think you should finish that up, add the missing sections, and pass it around to the framework team before this gets merged.
lib/ui/pointer.dart
Outdated
| this.panDeltaY = 0.0, | ||
| this.scale = 0.0, | ||
| this.rotation = 0.0, | ||
| this.preferredAction = PointerPreferredAction.ignore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning: lib/ui/ui.dart PointerData.Unnamed Constructor has a different parameter length than in lib/web_ui/ui.dart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path "lib/web_ui/ui.dart" doesnt exist, but I think this is because I need to update pointer packet on the Android side too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is udner flutter/lib/ui/pointer.dart, another one is under flutter/lib/web_ui/pointer.dart
lib/ui/pointer.dart
Outdated
| } | ||
|
|
||
| /// The preferred action for stylus action | ||
| enum PointerPreferredAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this specify stylus then?
| enum PointerPreferredAction { | |
| enum PointerPreferredStylusAction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hellohuanlin suggested preferredStylusAuxilliaryAction or PointerPreferredStylusAuxilliaryAction, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have an extra "l" in "Auxiliary". But what does "Auxiliary" mean in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I view stylus touching on screen as the "primary action" and Apple Pencil double tap as "auxiliary action". If we just call it "stylus action" or "pointer action", it sounds like stylus touching on screen (rather finger tapping on pencil in our case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a good question for the framework team, I'm sure they have prior art for this problem.
|
|
||
| #pragma mark - Stylus Events | ||
|
|
||
| #pragma clang diagnostic push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this pragma push pop here for something? Can it be removed?
| if (@available(iOS 13.4, *)) { | ||
| // noop | ||
| } else { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this with the API_AVAILABLE in the method signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is in every other test in this file too, they also all have the API_AVAILABLE in the signature. should I remove it for the others too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be great, assuming they all pass. Thank you!
| XCTAssertNotNil(vc); | ||
|
|
||
| id mockPencilInteraction = OCMClassMock([UIPencilInteraction class]); | ||
| XCTAssertNotNil(mockPencilInteraction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to assert that the mock you just made isn't nil, then you're just testing that OCMock works.
| pointer_data.kind = flutter::PointerData::DeviceKind::kStylus; | ||
| pointer_data.signal_kind = flutter::PointerData::SignalKind::kStylusAuxiliaryAction; | ||
| pointer_data.preferred_auxiliary_stylus_action = | ||
| flutter::PointerData::PreferredStylusAuxiliaryAction::kIgnore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where was ignore mocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore is the default value of UIPencilInteraction.preferredAction - it's a readonly property so I'm not sure how I would mock it to be a different enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can look into how to mock static methods using ocmock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore is the default value of UIPencilInteraction.preferredAction
We wouldn't want this test to fail based on real settings on the target device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://ocmock.org/reference/#mocking-class-methods
It would be something like:
OCMStub([mockPencilInteraction preferredAction]).andReturn(UIPencilPreferredActionIgnore);And at the end of the test, reset the class method mock:
[mockPencilInteraction stopMocking];
lib/ui/pointer.dart
Outdated
| 'rotation: $rotation' | ||
| 'preferredStylusAuxiliaryAction: $preferredStylusAuxiliaryAction, ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is malformed.
| 'rotation: $rotation' | |
| 'preferredStylusAuxiliaryAction: $preferredStylusAuxiliaryAction, ' | |
| 'rotation: $rotation, ' | |
| 'preferredStylusAuxiliaryAction: $preferredStylusAuxiliaryAction' |
lib/web_ui/lib/pointer.dart
Outdated
| 'rotation: $rotation' | ||
| 'preferredStylusAuxiliaryAction: $preferredStylusAuxiliaryAction, ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'rotation: $rotation' | |
| 'preferredStylusAuxiliaryAction: $preferredStylusAuxiliaryAction, ' | |
| 'rotation: $rotation, ' | |
| 'preferredStylusAuxiliaryAction: $preferredStylusAuxiliaryAction' |
| auto packet = std::make_unique<flutter::PointerDataPacket>(1); | ||
| packet->SetPointerData(/*index=*/0, pointer_data); | ||
|
|
||
| [[[self.mockEngine verify] ignoringNonObjectArgs] dispatchPointerDataPacket:std::move(packet)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignoringNonObjectArgs is ignoring the value of the packet and accepting anything, so you aren't actually testing anything here except that dispatchPointerDataPacket was called at least once. When I swap the action to kSwitchEraser it still passes.
This doesn't pass but something like:
id mockPencilInteraction = OCMClassMock([UIPencilInteraction class]);
OCMStub([mockPencilInteraction preferredTapAction])
.andReturn(UIPencilPreferredActionShowColorPalette);
XCTestExpectation* pointerDataPacketExpectation =
[self expectationWithDescription:@"pointerDataPacket"];
OCMStub(
[self.mockEngine dispatchPointerDataPacket:std::make_unique<flutter::PointerDataPacket>(0)])
.ignoringNonObjectArgs()
.andDo(^(NSInvocation* invocation) {
flutter::PointerData packet;
[invocation getArgument:&packet atIndex:2];
XCTAssertEqual(packet.kind, flutter::PointerData::DeviceKind::kStylus);
XCTAssertEqual(packet.signal_kind,
flutter::PointerData::SignalKind::kStylusAuxiliaryAction);
XCTAssertEqual(packet.preferred_auxiliary_stylus_action,
flutter::PointerData::PreferredStylusAuxiliaryAction::kShowColorPalette);
[pointerDataPacketExpectation fulfill];
});
[vc pencilInteractionDidTap:mockPencilInteraction];
[self waitForExpectationsWithTimeout:1 handler:nil];
[mockPencilInteraction stopMocking];There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this still doesn't pass but it's closer, it needs to get the pointer data off the packet.
std::unique_ptr<flutter::PointerDataPacket> packet;
[invocation getArgument:&packet atIndex:2];
flutter::PointerData pointer_data = packet->GetPointerData(0);
XCTAssertEqual(pointer_data.kind, flutter::PointerData::DeviceKind::kStylus);
XCTAssertEqual(pointer_data.signal_kind,
flutter::PointerData::SignalKind::kStylusAuxiliaryAction);
XCTAssertEqual(pointer_data.preferred_auxiliary_stylus_action,
flutter::PointerData::PreferredStylusAuxiliaryAction::kShowColorPalette);
[pointerDataPacketExpectation fulfill];There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some research just now - it looks like there's no such OCMock/NSInvocation API for dereferencing smart pointers at all. I think it only supports dereferencing raw pointers.
I could be wrong, but I wasn't able to find any examples online. If that's the case, verify maybe the only choice here for this test.
Another idea is to break out the main logic into a helper function like getAxiliaryActionPacket() so it's easier to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong, but I wasn't able to find any examples online. If that's the case,
verifymaybe the only choice here for this test.
How do you suggest getting verify working? It would need to do some equality check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting to just verify that it is called (like this).
For the equality check, i think we can factor out a helper function getAxiliaryActionPacket() and test that instead.
|
@camsim99 would you mind taking a look at the Android java part of this PR? |
camsim99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android part looks good to me!
lib/ui/pointer.dart
Outdated
| 'panDeltaX: $panDeltaX, ' | ||
| 'panDeltaY: $panDeltaY, ' | ||
| 'scale: $scale, ' | ||
| 'rotation: $rotation' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'rotation: $rotation' | |
| 'rotation: $rotation, ' |
lib/web_ui/lib/pointer.dart
Outdated
| 'panDeltaX: $panDeltaX, ' | ||
| 'panDeltaY: $panDeltaY, ' | ||
| 'scale: $scale, ' | ||
| 'rotation: $rotation' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'rotation: $rotation' | |
| 'rotation: $rotation, ' |
hellohuanlin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting very close. just a few comments.
lib/ui/window/pointer_data.h
Outdated
| }; | ||
|
|
||
| // Must match the PreferredStylusAuxiliaryAction enum in pointer.dart. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space
| int SCROLL_INERTIA_CANCEL = 2; | ||
| int SCALE = 3; | ||
| int STYLUS_AUXILIARY_ACTION = 3; | ||
| int SCALE = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may wanna keep scale as 3 and stylus action 4?
| OCMVerify([viewControllerMock createAuxillaryStylusActionData]); | ||
|
|
||
| // Check the return value of the helper function | ||
| flutter::PointerData pointer_data = [vc createAuxillaryStylusActionData]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may wanna create a separate test function for this helper function.
also may wanna verify all cases (rather than just the color pallet)
| panDeltaY: packet.getFloat64(kStride * offset++, _kFakeHostEndian), | ||
| scale: packet.getFloat64(kStride * offset++, _kFakeHostEndian), | ||
| rotation: packet.getFloat64(kStride * offset++, _kFakeHostEndian), | ||
| preferredStylusAuxiliaryAction: PointerPreferredStylusAuxiliaryAction.values[packet.getInt64(kStride * offset++, _kFakeHostEndian)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just double check - did framework team already agree with using this naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its already in the design doc, and they didnt object - if they change their mind, I think it wouldnt be a big deal to change the naming.
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for spending so much time on this @LouiseHsu!
hellohuanlin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Great work!
| [mockPencilInteraction stopMocking]; | ||
| } | ||
|
|
||
| - (void)testPencilHelperFunction API_AVAILABLE(ios(13.4)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: testCreateAuxillaryStylusActionData
This reverts commit 99a81a8.
…120656) * 8cd648d1d Roll Dart SDK from f80c5db8736a to ea59504416a8 (1 revision) (flutter/engine#39594) * 9ac09ced1 [Impeller] Fix unsafe access for clip stencil coverage (flutter/engine#39595) * 99a81a81f Add support for double tap action from Apple Pencil 2 (flutter/engine#39267) * 89d41d13e Add unique device id for trackpad on web (flutter/engine#39260) * f7dfb2b63 remove use of SkCanvas and DLCanvasRecorder from ui.Canvas native code (flutter/engine#39599) * c2e165e36 Fix multi-function compute (flutter/engine#39603) * c4f51bc78 Revert "Add support for double tap action from Apple Pencil 2 (#39267)" (flutter/engine#39607)
* working commit * some clean up * white space fixes * whitespace * remove unused import * Addressing PR comment, fix tests, update touch packet on Andriod * formatting n whitespace again * add new field to web ui pointer.dart * update test * whitespace * fix test * PR comments * fix test * whitespace * fix test by factoring out logic into helper function * whitespace * fix malformed string * pr comments * fix type issue * sigh whitespace * revert test changes :) * pr comments + separate out tests * extra space * change test name
…r#39267)" (flutter#39607) This reverts commit 99a81a8.
This PR adds engine support for the double tap action on the Apple Pencil 2.
Addresses flutter/flutter#73172
The Apple Pencil 2 has a feature which allows the user to double tap the side of the pencil to perform one of 4 preset actions, which can be set in Settings. The double tap is treated as a pointer event, with a new field that represents the preferred action of the user.
Framework PR will come after this is merged and will be linked here :)
Pre-launch Checklist
///).