-
-
Notifications
You must be signed in to change notification settings - Fork 955
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
Update PinchGestureHandler behavior #1613
base: fix-handler-inconsistency
Are you sure you want to change the base?
Update PinchGestureHandler behavior #1613
Conversation
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 change needs one or two new examples and docs update.
// this differs from shouldRecognizeSimultaneously because this method is called to check if there | ||
// is an already active handler that prevents this from activating, while shouldRecognizeSimultaneously | ||
// is called to check which handlers to cancel when a handler becoming active | ||
// main reason behind this method is the fact that when a handler activates it cancels all other | ||
// handlers that cannot be recognized simultaneously but when another pointer is placed on screen | ||
// while it is already active it has no influence over the ones extracted from the new pointer. | ||
// we could just use shouldRecognizeSimultaneously in this case but that would break things like | ||
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to | ||
// solve is a scrollview activating when there is active PinchGH inside |
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'd move this comment to interface docs and use /**
comment.
// this differs from shouldRecognizeSimultaneously because this method is called to check if there | |
// is an already active handler that prevents this from activating, while shouldRecognizeSimultaneously | |
// is called to check which handlers to cancel when a handler becoming active | |
// main reason behind this method is the fact that when a handler activates it cancels all other | |
// handlers that cannot be recognized simultaneously but when another pointer is placed on screen | |
// while it is already active it has no influence over the ones extracted from the new pointer. | |
// we could just use shouldRecognizeSimultaneously in this case but that would break things like | |
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to | |
// solve is a scrollview activating when there is active PinchGH inside | |
// This differs from shouldRecognizeSimultaneously because this method is called to check if there | |
// is an already active handler that prevents this from activating, while shouldRecognizeSimultaneously | |
// is called to check which handlers to cancel when a handler becoming active. | |
// The main reason behind this method is the fact that when a handler activates it cancels all other | |
// handlers that cannot be recognized simultaneously but when another pointer is placed on the screen | |
// while it is already active it has no influence over the ones extracted from the new pointer. | |
// We could use shouldRecognizeSimultaneously in this case but that would break things like | |
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to | |
// solve is a scrollview activating when there is active PinchGH inside |
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.
Also I don't fully understand this part:
// We could use shouldRecognizeSimultaneously in this case but that would break things like
// separate draggable objects without `simultaneousHandlers` set, when the problem it's trying to
// solve is a scrollview activating when there is active PinchGH inside
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 meant that (because of the similar behavior suggested by name) one could think that the same effect could be accomplished by additional logic built upon simultaneousHandlers
, but that would break existing functionality. I renamed this method and updated logic to reflect that change and it should be much more understandable now. Also added the comment in 3dbec81.
// when there is an active PinchGH lifting finger up and placing it back on the screen activates | ||
// scrollview alongside PinchGH resulting in weird behavior | ||
if (handler is PinchGestureHandler) { | ||
return false | ||
} |
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.
Why it relates only to pinch? Wouldn't pan with maxPointers=2 behave in a similar way?
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 changed the name of that method to needsToPreventOtherHandlerFromActivating
and updated the logic associated with it in d3ba7a1. Now it allows to add specific check in every gesture, that allows us to update all multi-touch gestures in the future.
android/lib/src/main/java/com/swmansion/gesturehandler/PinchGestureHandler.kt
Show resolved
Hide resolved
…rHandlerFromActivating
Description
PinchGestureHandler has been working differently on iOS and on Android.
On Android PinchGestureHandler was transitioning to
BEGAN
state immediately after first pointer down and transitioning toEND
if the pointer was pulled up.On iOS it is transitioning to
BEGAN
just before it activates and was allowing to continue the gesture after one of the pointers was pulled up.This PR modifies both of the platforms to be much more consistent:
UNDETERMINED
state only toBEGAN
state and ignore others. Internally the logic stays the same but it prevents sendingUNDETERMINED
->FAILED
orCANCELLED
which IMO is the correct behavior (it wasn't required before because all handlers went toBEGAN
state on touch start, but this PR allows PinchGH to stay in theUNDETERMINED
state).canActivateAlongsideAlreadyActive
method to baseGestureHandler
class. WhileshouldRecognizeSimultaneously
is called on handler activation to cancel other handlers that cannot be recognized at the same time, when a new pointer appears on the screen it may extract previously cancelled handlers again. This method adds option to prevent a handler from activating in that case. (Fixes a bug whereScrollView
would activate whenPinchGH
was inside of it withendOnFingerRelease
set to false)endOnFingerRelease
prop to PinchGH which changes behavior of the handler to match the original Android behavior - gesture will end when any of the fingers is releasedTest plan
Tested on the Example app