-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Bind SharedValues
in handler config
#3658
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
RNGestureHandlerManager *manager = [RNGestureHandlerModule handlerManagerForModuleId:_moduleId]; | ||
[manager updateGestureHandlerConfig:[NSNumber numberWithDouble:handlerTag] config:config]; |
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 wasn't sure if this should be done like this, or like in setGestureHandlerConfig
. However, if we use addOperationBlock
we have to call flushOperations
, so I think we can leave it as it is (cc @j-piasecki)
This comment was marked as outdated.
This comment was marked as outdated.
override fun setGestureHandlerConfig(handlerTagDouble: Double, config: ReadableMap) { | ||
val handlerTag = handlerTagDouble.toInt() | ||
|
||
updateGestureHandlerHelper<GestureHandler>(handlerTag, config, false) | ||
} | ||
|
||
@ReactMethod | ||
override fun updateGestureHandlerConfig(handlerTagDouble: Double, config: ReadableMap) { | ||
val handlerTag = handlerTagDouble.toInt() | ||
|
||
updateGestureHandlerHelper<GestureHandler>(handlerTag, config) | ||
updateGestureHandlerHelper<GestureHandler>(handlerTag, config, true) |
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 updateGestureHandlerHelper
can be inlined in both places. Now that handlers are no longer generic classes, the helper doesn't add much, and the result should be easier to read.
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 agree, done in 50ffbcd
RNGestureHandlerModule.dropGestureHandler(tag); | ||
RNGestureHandlerModule.flushOperations(); | ||
}; | ||
}, [type, tag]); | ||
}, [type, tag, config]); |
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 will cause the gesture to be dropped when the config object is changed. Shared value binding should be handled by a separate effect.
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.
Good catch, changed in 8d1ad9b
const sharedValues = new Map< | ||
string, | ||
{ id: number; sharedValue: SharedValue } | ||
>(); |
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 don't think there's a need to store shared values in a map.
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 in 2af12c1
{ id: number; sharedValue: SharedValue } | ||
>(); | ||
|
||
let nextSharedValueListenerID = 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.
Reanimated also numbers from 0, we need some kind of offset - pick your favorite, but not obvious (rational or rounded irrational) number. We shouldn't need that counter anyway.
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.
Reanimated also numbers from 0
Reanimated starts from 9999 😅 But I do agree that we may want to use offset.
Also, do we need to use tag + offset
? If each shared value
has its own map of listeners, it should be fine to use just offset
and then we could simply call it listenerId
.
Changed in 9a98e19, though this commit has SharedValues
in array. It was removed later on in 2af12c1.
sharedValues.set(key, { | ||
id: nextSharedValueListenerID, | ||
sharedValue: maybeSharedValue, | ||
}); |
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.
sharedValues.set(key, { | |
id: nextSharedValueListenerID, | |
sharedValue: maybeSharedValue, | |
}); |
|
||
Reanimated.runOnUI(attachListener)( | ||
maybeSharedValue, | ||
nextSharedValueListenerID, |
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.
nextSharedValueListenerID, |
key | ||
); | ||
|
||
nextSharedValueListenerID++; |
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.
nextSharedValueListenerID++; |
for (const [key, { id, sharedValue }] of sharedValues.entries()) { | ||
Reanimated.runOnUI(() => { | ||
sharedValue.removeListener(id); | ||
})(); | ||
|
||
sharedValues.delete(key); | ||
} |
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.
for (const [key, { id, sharedValue }] of sharedValues.entries()) { | |
Reanimated.runOnUI(() => { | |
sharedValue.removeListener(id); | |
})(); | |
sharedValues.delete(key); | |
} | |
for (const [key, maybeSharedValue] of Object.entries(config)) { | |
if (!Reanimated.isSharedValue(maybeSharedValue)) { | |
continue; | |
} | |
Reanimated.runOnUI(() => { | |
maybeSharedValue.removeListener(handlerTag + MAGIC_OFFSET); | |
})(); | |
} |
// This is used to obtain HostFunction that can be executed on the UI thread | ||
const { updateGestureHandlerConfig } = RNGestureHandlerModule; | ||
|
||
function maybeExtractSharedValues(config: any, tag: number) { |
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.
function maybeExtractSharedValues(config: any, tag: number) { | |
function bindSharedValues(config: any, tag: number) { |
Imo, we don't need maybe
prefix here. If there's no Reanimated, there will be no shared values - every shared value would be bound. Same, if there's Reanimated but no shared values in the config. And if there's Reanimated and shared values, they will be bound correctly.
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, done in fd6eb79
} | ||
} | ||
|
||
function maybeDetachSharedValues() { |
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.
function maybeDetachSharedValues() { | |
function unbindSharedValues() { |
Same
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 in fd6eb79
Description
This PR introduces
SharedValue
bindings to config. This will allow users to specify values in config asSharedValues
, e.g:Test plan
Tested on the following code: