-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Transform entire MotionEvent instead of single pointer only
#2156
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
Conversation
wood1986
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 tested in my app. it works. But I do not know other area
|
Hey @j-piasecki as this PR has been a month, May I know when you merge into master and publish a new version? Thanks |
kmagiera
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.
Most of the changes look good. Accepting to unblock. Please take a look at my comments to GestureHandlerOrchestrator.kt as some of the changes there look a bit suspicious
| } | ||
|
|
||
| event.setLocation(oldX, oldY) | ||
| event.recycle() |
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.
are you sure it is safe to recycle event here? It seem like transormEventToViewCoords may sometimes create a new event that should be recycled and other times it may reuse the one provided to it. The latter case matches the previous behavior of this method and we never called recycle here. Not saying this is bad but just look suspicious.
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.
transormEventToViewCoords never creates a new event, it always transforms the one it receives but I can see how the code of the method might've suggested otherwise. To be hones I don't remember why I've done it like that (it might've been a result of some logic that used to be there), but I've simplified it in 78c37cd so it should be much clearer that it doesn't create a new event.
That said, It should be safe to recycle there since transformEventToViewCoords receives (and returns transformed) a copy of the source event:
Line 255 in 78c37cd
| val event = transformEventToViewCoords(handler.view, MotionEvent.obtain(sourceEvent)) |
| * @param view - view to which coordinate space the event should be transformed | ||
| * @param event - event to transform | ||
| */ | ||
| fun transformEventToViewCoords(view: View?, event: MotionEvent): MotionEvent { |
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.
not sure about the naming of this method. It suggests that it mutates the provided object while sometimes it doesn't. Also, it looks like it mutates the event in a very few scenarios. Unless I don't understand this well it appears like it only mutates if the view is the wrapperView (which happens only at the very top of the view hierarchy) and when view is null which I don't think should ever happen?
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 traverses the view hierarchy upwards until it finds wrapperView (which is an instance of GestureHandlerRootView) or null (in case there is no root view we don't want to crash the app) and then comes back recursively transforming the event using the same matrices that are used to transform views.
It will not mutate the provided event in the following cases:
nullis passed as an argument forview- all the views on the path from the
wrapperViewto the provided view aren't transformed (their matrix is identity) and are positioned at the top-left corner of their parents
As for the name, I wanted to make It obvious that it doesn't return a new object but may transform the one provided as an argument so it's not safe to use if the untransformed event is also needed. Semantically I believe it's fine since the only case where the event is not transformed is when the coordinate space of the provided view is the same as the wrapper view.
android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.kt
Outdated
Show resolved
Hide resolved
kmagiera
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.
well, forgot to check 'approve' with my last comment
|
Thanks. May I know when it will be released? Is it a patch or minor or major release? |
|
@wood1986 Most probably in Gesture Handler 2.7.0, but at the moment I cannot give you a timeframe. |
|
@wood1986 Just a heads up, GH 2.7.0 has been released yesterday. |
Description
Gesture Handler on Android intercepts events via the
GestureHandlerRootView, which means that we need to transform the touch coordinated by ourselves to check if it's inside the view. Similarly, we need to transform the event when dispatching it to one of the gesture handlers in order to correctly calculate the changes and send touch events.The current method works well until there is more than one pointer at once on the screen. When there is more than one pointer, only the first one will be correctly transformed, while others will simply be moved by the offset calculated when transforming the first pointer.
This PR changes the logic when delivering events to gesture handlers - the whole event is transformed, and handlers receive both events - transformed and untransformed one. This way things like touch events and
NativeViewGestureHandlerwhich rely on the location in the coordinate space of the view keep working, and other gestures get to work in the coordinate space of the root view.Fixes #2138.
Test plan
Tested on the Example app.