-
Notifications
You must be signed in to change notification settings - Fork 6k
Add support for double tap action from Apple Pencil 2 #39267
Changes from 6 commits
a945890
04b8708
6b7061a
3818c77
1b6c8f5
9c31bd1
c2450d4
0317a34
61296be
2b5a6b9
c18f4e9
9f780ce
25f6607
2ec9688
a50f554
a27ba22
1cdf0ec
b348378
4e14bbf
c8a8cd7
69d1035
b162cbc
79b790f
912a4cc
1393fae
9527282
b09a991
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -133,10 +133,21 @@ enum PointerSignalKind { | |||||
| /// A pointer-generated scale event (e.g. trackpad pinch). | ||||||
| scale, | ||||||
|
|
||||||
| /// A stylus generated action (e.g. double tap on Apple Pencil 2) | ||||||
| stylusAction, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment on the naming here. |
||||||
|
|
||||||
| /// An unknown pointer signal kind. | ||||||
| unknown | ||||||
| } | ||||||
|
|
||||||
| /// The preferred action for stylus action | ||||||
| enum PointerPreferredAction { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this specify stylus then?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hellohuanlin suggested preferredStylusAuxilliaryAction or PointerPreferredStylusAuxilliaryAction, what do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
| ignore, | ||||||
| showColorPalette, | ||||||
| switchEraser, | ||||||
| switchPrevious | ||||||
|
LouiseHsu marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
|
|
||||||
| /// Information about the state of a pointer. | ||||||
| class PointerData { | ||||||
| /// Creates an object that represents the state of a pointer. | ||||||
|
|
@@ -176,6 +187,7 @@ class PointerData { | |||||
| this.panDeltaY = 0.0, | ||||||
| this.scale = 0.0, | ||||||
| this.rotation = 0.0, | ||||||
| this.preferredAction = PointerPreferredAction.ignore, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One is udner |
||||||
| }); | ||||||
|
|
||||||
| /// Unique identifier that ties the [PointerEvent] to embedder event created it. | ||||||
|
|
@@ -374,6 +386,11 @@ class PointerData { | |||||
| /// The current angle of the pan/zoom in radians, with 0.0 as the initial angle. | ||||||
| final double rotation; | ||||||
|
|
||||||
| /// For events with signal kind of stylusAction | ||||||
| /// | ||||||
| /// The current preferred action for stylusAction, with ignore as the default. | ||||||
| final PointerPreferredAction preferredAction; | ||||||
|
|
||||||
| @override | ||||||
| String toString() => 'PointerData(x: $physicalX, y: $physicalY)'; | ||||||
|
|
||||||
|
|
@@ -414,6 +431,7 @@ class PointerData { | |||||
| 'panDeltaY: $panDeltaY, ' | ||||||
| 'scale: $scale, ' | ||||||
| 'rotation: $rotation' | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| 'preferredAction: $preferredAction, ' | ||||||
| ')'; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,7 +55,9 @@ | |
| // This is left a FlutterBinaryMessenger privately for now to give people a chance to notice the | ||
| // change. Unfortunately unless you have Werror turned on, incompatible pointers as arguments are | ||
| // just a warning. | ||
| @interface FlutterViewController () <FlutterBinaryMessenger, UIScrollViewDelegate> | ||
| @interface FlutterViewController () <FlutterBinaryMessenger, | ||
| UIScrollViewDelegate, | ||
| UIPencilInteractionDelegate> | ||
| @property(nonatomic, readwrite, getter=isDisplayingFlutterUI) BOOL displayingFlutterUI; | ||
| @property(nonatomic, assign) BOOL isHomeIndicatorHidden; | ||
| @property(nonatomic, assign) BOOL isPresentingViewControllerAnimating; | ||
|
|
@@ -93,7 +95,7 @@ @interface FlutterViewController () <FlutterBinaryMessenger, UIScrollViewDelegat | |
| // Trackpad rotating | ||
| @property(nonatomic, retain) | ||
| UIRotationGestureRecognizer* rotationGestureRecognizer API_AVAILABLE(ios(13.4)); | ||
|
|
||
| @property(nonatomic, retain) UIPencilInteraction* pencilInteraction API_AVAILABLE(ios(13.4)); | ||
|
LouiseHsu marked this conversation as resolved.
|
||
| /** | ||
| * Creates and registers plugins used by this view controller. | ||
| */ | ||
|
|
@@ -709,6 +711,10 @@ - (void)viewDidLoad { | |
| [self createTouchRateCorrectionVSyncClientIfNeeded]; | ||
|
|
||
| if (@available(iOS 13.4, *)) { | ||
| _pencilInteraction = [[UIPencilInteraction alloc] init]; | ||
| _pencilInteraction.delegate = self; | ||
| [_flutterView addInteraction:_pencilInteraction]; | ||
|
|
||
| _hoverGestureRecognizer = | ||
| [[UIHoverGestureRecognizer alloc] initWithTarget:self action:@selector(hoverEvent:)]; | ||
| _hoverGestureRecognizer.delegate = self; | ||
|
|
@@ -956,7 +962,7 @@ - (void)goToApplicationLifecycle:(nonnull NSString*)state { | |
| case UITouchTypeDirect: | ||
| case UITouchTypeIndirect: | ||
| return flutter::PointerData::DeviceKind::kTouch; | ||
| case UITouchTypeStylus: | ||
| case UITouchTypePencil: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this change intended?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| return flutter::PointerData::DeviceKind::kStylus; | ||
| case UITouchTypeIndirectPointer: | ||
| return flutter::PointerData::DeviceKind::kMouse; | ||
|
|
@@ -1211,6 +1217,40 @@ - (void)invalidateTouchRateCorrectionVSyncClient { | |
| _touchRateCorrectionVSyncClient = nil; | ||
| } | ||
|
|
||
| #pragma mark - Stylus Events | ||
|
|
||
| #pragma clang diagnostic push | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this pragma push pop here for something? Can it be removed? |
||
|
|
||
| - (void)pencilInteractionDidTap:(UIPencilInteraction*)interaction API_AVAILABLE(ios(13.4)) { | ||
| flutter::PointerData pointer_data = [self generatePointerDataAtLastMouseLocation]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need the mouse location for this pointer_data?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| switch (UIPencilInteraction.preferredTapAction) { | ||
| case UIPencilPreferredActionIgnore: | ||
| pointer_data.preferred_action = flutter::PointerData::PreferredAction::kIgnore; | ||
| break; | ||
| case UIPencilPreferredActionShowColorPalette: | ||
| pointer_data.preferred_action = flutter::PointerData::PreferredAction::kShowColorPalette; | ||
| break; | ||
| case UIPencilPreferredActionSwitchEraser: | ||
| pointer_data.preferred_action = flutter::PointerData::PreferredAction::kSwitchEraser; | ||
| break; | ||
| case UIPencilPreferredActionSwitchPrevious: | ||
| pointer_data.preferred_action = flutter::PointerData::PreferredAction::kSwitchPrevious; | ||
| default: | ||
| break; | ||
|
LouiseHsu marked this conversation as resolved.
|
||
| } | ||
|
|
||
| pointer_data.device = reinterpret_cast<int64_t>(_pencilInteraction); | ||
| pointer_data.kind = flutter::PointerData::DeviceKind::kStylus; | ||
| pointer_data.signal_kind = flutter::PointerData::SignalKind::kStylusAction; | ||
|
|
||
| auto packet = std::make_unique<flutter::PointerDataPacket>(1); | ||
| packet->SetPointerData(/*index=*/0, pointer_data); | ||
| [_engine.get() dispatchPointerDataPacket:std::move(packet)]; | ||
| } | ||
|
|
||
| #pragma clang diagnostic pop | ||
|
|
||
| #pragma mark - Handle view resizing | ||
|
|
||
| - (void)updateViewportMetrics { | ||
|
|
@@ -1680,7 +1720,7 @@ - (void)pressesCancelled:(NSSet<UIPress*>*)presses | |
|
|
||
| #pragma mark - Orientation updates | ||
|
|
||
| - (void)onOrientationPreferencesUpdated:(NSNotification*)notification { | ||
| - (void)onOrientationPreferencesUpdated:(NSNotification*)notification API_AVAILABLE(ios(12.1)) { | ||
|
LouiseHsu marked this conversation as resolved.
Outdated
|
||
| // Notifications may not be on the iOS UI thread | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| NSDictionary* info = notification.userInfo; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -121,6 +121,7 @@ - (void)performOrientationUpdate:(UIInterfaceOrientationMask)new_preferences; | |||
| - (void)handlePressEvent:(FlutterUIPressProxy*)press | ||||
| nextAction:(void (^)())next API_AVAILABLE(ios(13.4)); | ||||
| - (void)discreteScrollEvent:(UIPanGestureRecognizer*)recognizer; | ||||
| - (void)pencilInteractionDidTap:(UIPencilInteraction*)interaction; | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The interface can implement the delegate protocol instead: @interface FlutterViewController (Tests) <UIPencilInteractionDelegate>
Suggested change
|
||||
| - (void)updateViewportMetrics; | ||||
| - (void)onUserSettingsChanged:(NSNotification*)notification; | ||||
| - (void)applicationWillTerminate:(NSNotification*)notification; | ||||
|
|
@@ -1479,6 +1480,27 @@ - (void)testMouseSupport API_AVAILABLE(ios(13.4)) { | |||
| dispatchPointerDataPacket:std::make_unique<flutter::PointerDataPacket>(0)]; | ||||
| } | ||||
|
|
||||
| - (void)testPencilSupport API_AVAILABLE(ios(13.4)) { | ||||
| if (@available(iOS 13.4, *)) { | ||||
| // noop | ||||
| } else { | ||||
| return; | ||||
| } | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need this with the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that would be great, assuming they all pass. Thank you! |
||||
|
|
||||
| FlutterViewController* vc = [[FlutterViewController alloc] initWithEngine:self.mockEngine | ||||
| nibName:nil | ||||
| bundle:nil]; | ||||
| XCTAssertNotNil(vc); | ||||
|
|
||||
| id mockPencilInteraction = OCMClassMock([UIPencilInteraction class]); | ||||
| XCTAssertNotNil(mockPencilInteraction); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||
|
|
||||
| [vc pencilInteractionDidTap:mockPencilInteraction]; | ||||
|
|
||||
| [[[self.mockEngine verify] ignoringNonObjectArgs] | ||||
| dispatchPointerDataPacket:std::make_unique<flutter::PointerDataPacket>(0)]; | ||||
|
LouiseHsu marked this conversation as resolved.
Outdated
|
||||
| } | ||||
|
|
||||
| - (void)testFakeEventTimeStamp { | ||||
| FlutterViewController* vc = [[FlutterViewController alloc] initWithEngine:self.mockEngine | ||||
| nibName:nil | ||||
|
|
||||
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
PointerPreferredActionmeans in this context. How aboutpreferredStylusAuxilliaryActionandPointerPreferredStylusAuxilliaryActionThere 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.
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8790504096978029009/+/u/test:_Host_Tests_for_host_debug_unopt/stdout
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)?