Update how Gesture Handler exposes setGestureState to the Reanimated UI runtime#3207
Conversation
4b6cb29 to
039e228
Compare
setGestureState to the ui runtimesetGestureState to the Reanimated UI runtime
|
Additionally, I'd like to rename |
tjzel
left a comment
There was a problem hiding this comment.
Very nice work overall, I like the removals 😎
08271fd to
12c9f11
Compare
296b706 to
d5d7233
Compare
...e-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.kt
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/android/src/main/jni/CMakeLists.txt
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/android/src/main/jni/CMakeLists.txt
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/android/src/main/jni/CMakeLists.txt
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/android/src/main/jni/RNGestureHandlerModule.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/shared/RuntimeDecorator.h
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/handlers/gestures/gestureStateManager.ts
Show resolved
Hide resolved
2e996c8 to
eeafcc9
Compare
eeafcc9 to
801acd8
Compare
There was a problem hiding this comment.
Do we need this? We have format-apple script, the only difference is path passed to glob. Maybe it would be better to rename this script and pass path as argument?
There was a problem hiding this comment.
Unified the scripts in da123f7. Let me know what you think.
...e-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.kt
Show resolved
Hide resolved
...droid/paper/src/main/java/com/swmansion/gesturehandler/NativeRNGestureHandlerModuleSpec.java
Show resolved
Hide resolved
packages/react-native-gesture-handler/android/src/main/jni/RNGestureHandlerModule.cpp
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/handlers/gestures/gestureStateManager.ts
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/shared/RNGHRuntimeDecorator.cpp
Outdated
Show resolved
Hide resolved
...e-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.kt
Show resolved
Hide resolved
|
|
||
| @ReactMethod | ||
| override fun createGestureHandler(handlerName: String, handlerTagDouble: Double, config: ReadableMap) { | ||
| override fun createGestureHandler(handlerName: String, handlerTagDouble: Double, config: ReadableMap): Boolean { |
There was a problem hiding this comment.
The createGestureHandler seems to only ever return true/1.
Is this the case, or am I missing something?
If it is the case, can we get away with returning void instead?
There was a problem hiding this comment.
When a method returns any value in the codegen spec, the generated method is synchronous. I don't think there's a way to force codegen to generate a synchronous method that returns void.
packages/react-native-gesture-handler/shared/RNGHRuntimeDecorator.cpp
Outdated
Show resolved
Hide resolved
tomekzaw
left a comment
There was a problem hiding this comment.
LGTM, thanks for all the changes!
| jsi::Runtime &rnRuntime, | ||
| std::function<void(int, int)> &&setGestureState) { | ||
| const auto runtimeHolder = | ||
| rnRuntime.global().getProperty(rnRuntime, "_WORKLET_RUNTIME"); |
There was a problem hiding this comment.
Do I get it correctly that this is added to rnRuntime by Reanimated? From which version of lib is it there? Also, can't we have some race conditions that it is not yet there when you call this method or something like that? Btw, is it safe to have some kind of pointer from one runtime to another?
There was a problem hiding this comment.
Do I get it correctly that this is added to rnRuntime by Reanimated
Yes
From which version of lib is it there
Pretty sure it was added in 2.x
Also, can't we have some race conditions that it is not yet there when you call this method or something like that
Yes - when you don't render an Animated.View, the Reanimated native module will not be initialized, and this field will not exist. This is handled on the JS side which will throw an exception about this.
is it safe to have some kind of pointer from one runtime to another
I believe so
There was a problem hiding this comment.
Just for the context, _WORKLET_RUNTIME was added a few years ago to support integration of Reanimated with Expo GL. The latter could be used from the Reanimated UI runtime so it would inject some JSI bindings into the Reanimated UI runtime.
| rnRuntime); | ||
| const auto uiRuntimeAddress = | ||
| reinterpret_cast<uintptr_t *>(&arrayBufferValue[0]); | ||
| jsi::Runtime &uiRuntime = |
There was a problem hiding this comment.
Oh ok, so the above is just a bool specifying that there will be a buffer with a pointer to memory with uiRuntime allocated there?
There was a problem hiding this comment.
I'm not sure what you mean by "the above".
runtimeHolder is a jsi value holding an array buffer. arrayBufferValue is the data held inside the buffer (the 8 bytes of the pointer to the ui runtime). uiRuntimeAddress is a pointer to the first byte of the buffer value, casted to uintptr_t which is an integer the size of a pointer. uiRuntime is a reference to the ui runtime which we get by casting the uiRuntimeAddress to jsi::Runtime * and dereferencing it.
There was a problem hiding this comment.
For the context, this is because we can't represent a 64-bit pointer as a number so we need to use ArrayBuffer for that.
Description
Changes how
setGestureStateis exposed to the UI runtime. Instead of the weird conditionally adding Reanimated as a dependency on Android and the weird cast on iOS it uses_WORKLET_RUNTIMEconst injected by Reanimated into the JS runtime. This allows Gesture Handler to decorate the UI runtime without direct dependencies between the libraries.The new approach relies on two methods being added to the global object:
_setGestureStateAsyncon the JS runtime_setGestureStateSyncon the UI runtimewhich allows for state manipulation also from the JS thread.
The basic example has been modified to easily test the new functionality.
Caution
This works only on the New Architecture (and breaks the old one)
Test plan
Test the expo example app and the modified basic example app