-
Notifications
You must be signed in to change notification settings - Fork 6k
[ios] Update gesture position on every event #55285
Conversation
Previously, gesture origin position relied on hover events. But iOS 18 screen mirroring feature sends only pan/scale gestures, but doesn't hover. So we need to check the gesture location every time.
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.
@hellohuanlin can you also take a look since you reviewed #34929
| - (flutter::PointerData)generatePointerDataAtLastMouseLocation API_AVAILABLE(ios(13.4)) { | ||
| - (flutter::PointerData)updateMousePointerDataFrom:(UIGestureRecognizer*)gestureRecognizer | ||
| API_AVAILABLE(ios(13.4)) { | ||
| CGPoint location = [gestureRecognizer locationInView:self.view]; |
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 be checking self.viewIfLoaded and not updating the location if it's not yet loaded?
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 dont know the implications of this. there wasn't such a check before. at this point we have received an event from UIKit, the view must exist?
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'm not sure the implications either, maybe there are none if the Flutter view is guaranteed to be loaded at this point. 🙂
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 be fine. this is only called in gesture callbacks, at which point the view must have already been on screen.
| API_AVAILABLE(ios(13.4)) { | ||
| CGPoint location = [gestureRecognizer locationInView:self.view]; | ||
| CGFloat scale = [self flutterScreenIfViewLoaded].scale; | ||
| _mouseState.location = {location.x * scale, location.y * scale}; |
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: since you're calculating the x and y you could put them in local variables and use it a few lines below when setting pointer_data.physical_x and y.
|
|
||
| [vc discreteScrollEvent:mockPanGestureRecognizer]; | ||
|
|
||
| // The mouse position within panGestureRecognizer should be checked |
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 it testable to check if the mouse location was also updated, with the right values?
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 couldn't figure it out. because this is a primitive value / non-Object, OCMock stub-and-check-argument can't be used.
| return YES; | ||
| } | ||
|
|
||
| - (void)hoverEvent:(UIPanGestureRecognizer*)recognizer 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.
did we use the wrong type before?
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.
Yep just realized it
| - (flutter::PointerData)generatePointerDataAtLastMouseLocation API_AVAILABLE(ios(13.4)) { | ||
| - (flutter::PointerData)updateMousePointerDataFrom:(UIGestureRecognizer*)gestureRecognizer | ||
| API_AVAILABLE(ios(13.4)) { | ||
| CGPoint location = [gestureRecognizer locationInView:self.view]; |
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 be fine. this is only called in gesture callbacks, at which point the view must have already been on screen.
…155385) flutter/engine@94f852d...fe823a9 2024-09-19 [email protected] Roll Skia from 321d7750aa07 to 8858f081d1e1 (1 revision) (flutter/engine#55292) 2024-09-18 [email protected] Roll Skia from a60a52a11763 to 321d7750aa07 (2 revisions) (flutter/engine#55290) 2024-09-18 [email protected] [ios] Update gesture position on every event (flutter/engine#55285) 2024-09-18 [email protected] Roll Dart SDK from 9164e2c0e994 to 379f0145421f (1 revision) (flutter/engine#55289) 2024-09-18 [email protected] Roll Skia from 760e37059fde to a60a52a11763 (2 revisions) (flutter/engine#55287) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Previously, gesture origin position relied on hover events. But iOS 18 screen mirroring feature sends only pan/scale gestures, but doesn't hover. So we need to check the gesture location every time.
Fixes flutter/flutter#153897
Pre-launch Checklist
///).