-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve interop touches by using UIScrollView-like strategy #1440
Conversation
- (void)handleStateChange { | ||
switch (self.state) { | ||
case UIGestureRecognizerStateBegan: | ||
NSLog(@"state = Began"); |
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 seems like logs should be removed
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.
Indeed
@protocol CMPGestureRecognizerHandler <NSObject> | ||
|
||
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.
Sry, it looks like here some formatting issues with spaces.
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.
Fixed
_scheduledFailureBlock = dispatchBlock; | ||
|
||
// Calculate the delay time in dispatch_time_t | ||
dispatch_time_t delay = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.15 * NSEC_PER_SEC)); |
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.
Please, move 0.15
to constants. Or add some meaningful comment because it looks like it require some explanation.
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.
Moved to a local variable with an explanation
* [CMPGestureRecognizer.scheduleFailure], which will pass intercepted touches to the interop | ||
* views if the gesture recognizer is not recognized within a certain time frame | ||
* (UIScrollView reverse-engineered 150ms is used). | ||
* The similar approach is used by [UIScrollView](https://developer.apple.com/documentation/uikit/uiscrollview?language=objc) |
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.
Link suggestion: https://developer.apple.com/documentation/uikit/uiscrollview
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.
Aha, changed
* scroll and compose horizontal scroll with different fingers) | ||
* | ||
* 4. Those are not the first touches in the sequence. A gesture is not recognized. | ||
* See if centroid of the tracked touches has moved enough to recognize the gesture. |
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 worry regarding this scenario. The second touch will cause significant offset of centroid, and with high probability it will trigger pan gesture, instead of multi-touch gesture, like pinch zoom. Just wonder if it will be a problem on interop maps.
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.
If second touch lands after the gesture was failed, it's not even delivered to it, so the gesture will properly be recognised by maps, for example.
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 afraid this might break the behavior of detectTransformGestures. On 1.7.0-alpha02, it seemed detectTransformGestures always call onGesture with zoom = 1.0f on iOS.
I have create an issue on youtrack: https://youtrack.jetbrains.com/issue/CMP-5879/detectTransformGestures-doesnt-work-properly-on-iOS-with-Compose-Multiplatform-1.7.0-alpha02
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.
Hi, thanks, already looking into it.
* location to null. | ||
*/ | ||
private fun stopTrackingTouches(touches: Set<*>) { | ||
onTouchesCountChanged(-touches.size) |
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 would move this call on after we actually remove touches from trackedTouches
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.
They are functionally equivalent now, because effects of this call are dispatched later on main thread by OS itself (display link changes). But let's do it for sake of consistency with onFailure
.
* @return `true` if the touches are initial, `false` otherwise. | ||
*/ | ||
private fun startTrackingTouches(touches: Set<*>): Boolean { | ||
onTouchesCountChanged(touches.size) |
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 would move this call on after we actually remove touches from trackedTouches
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.
Done
*/ | ||
var gestureRecognizer: CMPGestureRecognizer? = null | ||
|
||
private var state: UIGestureRecognizerState |
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.
Name suggestion: gestureRecognizerState
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.
Done
val initialLocation = initialLocation ?: return false | ||
val centroidLocation = trackedTouchesCentroidLocation ?: return false | ||
|
||
val slop = 10.0 |
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.
As far as I remember we already had such constant. Can it be reused?
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.
Extracted to CUPERTINO_PAN_GESTURE_SLOP_VALUE
val dx = centroidLocation.useContents { x - initialLocation.useContents { x } } | ||
val dy = centroidLocation.useContents { y - initialLocation.useContents { y } } | ||
|
||
return dx * dx + dy * dy > slop * slop |
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.
A good candidate for utility/helper function
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.
Comparing squared lengths?
I doubt it happens again, let it be here for now.
* that it can be used later to determine if the gesture recognizer should be recognized | ||
* or failed. | ||
*/ | ||
private fun rememberHitTestResult(hitTestBlock: () -> UIView?): UIView? { |
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.
rememberX
has very specific semantic in Compose, please rename to avoid confusions
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.
Done
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.
The main change/delay logic LGTM, I just have a couple of nitpicks
/** | ||
* iOS default value in scale-independent points for the pan gesture slop. | ||
*/ | ||
internal const val CUPERTINO_PAN_GESTURE_SLOP_VALUE = 10 |
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.
Let's make it typed and align naming (it's used not only in "pan" gestures)
internal const val CUPERTINO_PAN_GESTURE_SLOP_VALUE = 10 | |
internal const val DefaultCupertinoTouchSlop = 10.dp |
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.
No DP here, new logic uses CGFloat(aka Double)
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 get raw value from dp there - we need to keep such things typed
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 doesn't work well here.
Dp is not primitive - turns into just val
instead of const val
.
Let's leave it as it is. Renamed to CUPERTINO_TOUCH_SLOP
. It's not Default
, it's what it is.
// 150ms is a timer delay for notifying a handler that the gesture was failed to recognize. | ||
// `handler` implementtion is responsible for cancelling this via calling `cancelFailure` and transitioning | ||
// this gesture recognizer to a proper state. | ||
double failureInterval = 0.15; |
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.
Nitpick: mixing ms
and s
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.
Nitpick indeed
compose/ui/ui-uikit/src/uikitMain/objc/CMPUIKitUtils/CMPUIKitUtils/CMPGestureRecognizer.m
Outdated
Show resolved
Hide resolved
compose/ui/ui-uikit/src/uikitMain/objc/CMPUIKitUtils/CMPUIKitUtils/CMPGestureRecognizer.m
Outdated
Show resolved
Hide resolved
compose/ui/ui-uikit/src/uikitMain/objc/CMPUIKitUtils/CMPUIKitUtils/CMPGestureRecognizer.m
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/InteractionUIView.uikit.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitMain/kotlin/androidx/compose/ui/window/InteractionUIView.uikit.kt
Outdated
Show resolved
Hide resolved
…tils/CMPGestureRecognizer.m Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>
…InteractionUIView.uikit.kt Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>
…InteractionUIView.uikit.kt Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>
Description
In this approach
CMPGestureRecognizer
delaystouchesBegan
until explicitly failed, and is required to fail byUIGestureRecognizer
of children views (aka interop views). Failure happens after the first touch started and no motion above scroll slop happens within 150ms. If this happens, intercepted touches are delivered to children views (and their gesture recognisers), Compose itself gets all tracked touches as cancelled and ignores them until the touch sequence ends (imposed by UIKit).This behavior is inspired by
UIScrollView
implementation.Screen.Recording.2024-07-11.at.13.46.48.mov
Fixes
Improves behavior of touches in certain scenarios
Testing
This should be tested by QA
Release Notes
iOS - Features