-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[android] Fix state manager on unregistered gestures #3913
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
base: next
Are you sure you want to change the base?
Conversation
m-bert
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.
As said in the first comment, I'm not a big fan of ForManual suffix. I think we could either use overloading (where possible) or add more descriptive suffix.
...ve-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt
Outdated
Show resolved
Hide resolved
...ve-gesture-handler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandler.kt
Outdated
Show resolved
Hide resolved
...andler/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt
Outdated
Show resolved
Hide resolved
| if (orchestrator == null) { | ||
| // If the state is set manually, the handler may not have been fully recorded by the orchestrator. | ||
| hostDetectorView?.recordHandlerIfNotPresentForManual(this) | ||
| } |
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 we should throw error when both, orchestrator and hostDetectorView are null. This may happen if gesture is defined, but is not assigned to any of the detectors. In that case I believe it will still crash, but with other, less descriptive error (probably NullPointerException).
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.
Thanks, f3c1e93, I will check whether this works on other platforms
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.
On web the error is more fitting, Expected delegate view to be HTMLElement and on iOS it fails silently
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.
On web the error is more fitting, Expected delegate view to be HTMLElement
I don't think it is more fitting in that case. It doesn't tell you what you've done wrong, so it won't be obvious for someone who tries to change state of the gesture.
on iOS it fails silently
I don't think it is good either. Such behaviour should be unified between platforms.
| private fun findGestureHandlerRootView(): RNGestureHandlerRootView? { | ||
| var parent: ViewParent? = this.parent | ||
| var gestureHandlerRootView: RNGestureHandlerRootView? = null | ||
|
|
||
| while (parent != null) { | ||
| if (parent is RNGestureHandlerRootView) { | ||
| gestureHandlerRootView = parent | ||
| } | ||
| parent = parent.parent | ||
| } | ||
|
|
||
| return gestureHandlerRootView | ||
| } |
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.
Don't we have this exact method already defined somewhere? Maybe it's worth moving it to some utils, or making it an extension on 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.
Sure, I moved it to be companion object for the rootView in 8a4ffc9. Let me know what you think.
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.
Hmm, I'm not sure whether this.findGestureHandlerRootView()?... wouldn't have been cleaner, but I'll leave the final decision to you. Both work for me.
Description
The new
StateManageris global and givenhandlerTagit can manually set the states of an arbitrary gesture. This causes errors, when the gesture, which state is being set, has not been yet recorded in the orchestrator. Recording gestures in the orchestrator on android is done lazily, thus if it never received touches it is not recorded.Test plan
Tested on the following example
Details