-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Logic Detector #3689
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?
Logic Detector #3689
Conversation
5b65a19
to
81bf405
Compare
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.
Did a fist pass, but some stuff will likely change due to #3682 :/
Anyway, you have the hardest part figured out, now it's just a matter of polishing it up.
for (child in logicChildren) { | ||
shouldKeepLogicChild[child.key] = 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.
It's the same loop as above, and it's not modified in between. Is it needed twice?
override fun setLogicChildren(view: RNGestureHandlerDetectorView?, value: ReadableArray?) { | ||
view?.setLogicChildren(value) | ||
} |
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.
override fun setLogicChildren(view: RNGestureHandlerDetectorView?, value: ReadableArray?) { | |
view?.setLogicChildren(value) | |
} | |
override fun setLogicChildren(view: RNGestureHandlerDetectorView, value: ReadableArray?) { | |
view.setLogicChildren(value) | |
} |
I think we can assume that view will not be nullable like in other methods
@@ -4,6 +4,7 @@ export const ActionType = { | |||
JS_FUNCTION_OLD_API: 3, | |||
JS_FUNCTION_NEW_API: 4, | |||
NATIVE_DETECTOR: 5, | |||
LogicDetector: 6, |
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.
LogicDetector: 6, | |
LOGIC_DETECTOR: 6, |
handlerTag: Int32; | ||
state: Int32; | ||
handlerData: UnsafeMixed; | ||
childTag: Int32; |
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 we've talked offline - I think that childTag
shouldn't be needed by events, so you should be able to get rid of Logic
events, and use the existing ones instead.
- (facebook::react::RNGestureHandlerDetectorEventEmitter::OnGestureHandlerEvent)getNativeEvent; | ||
|
||
- (facebook::react::RNGestureHandlerDetectorEventEmitter::OnGestureHandlerLogicEvent)getNativeLogicEvent: | ||
(NSNumber *)childTag; | ||
@end | ||
|
||
@interface RNGestureHandlerStateChange (NativeEvent) | ||
|
||
- (facebook::react::RNGestureHandlerDetectorEventEmitter::OnGestureHandlerStateChange)getNativeEvent; | ||
|
||
- (facebook::react::RNGestureHandlerDetectorEventEmitter::OnGestureHandlerLogicStateChange)getNativeLogicEvent: | ||
(NSNumber *)childTag; |
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.
Then, you wouldn't need getNativeLogicEvent
here
NSNumber *parentTag = [_registry getLogicParent:@(detectorView.tag)]; | ||
RNGHUIView *parentView = [self viewForReactTag:parentTag]; |
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 should happen before this method - in the current implementation, detectorView
isn't actually a detector but whatever view the gesture is attached to.
If you move this to RNGestureHandler
, can we follow the same pattern as on Android? I.e., keep a reference to the parent detector in the gesture itself? Then you wouldn't need to go through the view registry at all.
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, done in 7fb8ea9
@@ -105,4 +105,68 @@ export function deepEqual(obj1: any, obj2: any) { | |||
return true; | |||
} | |||
|
|||
export function invokeNullableMethod( |
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'm not sure this is the way to go. Once #3682 is merged, you should be able to cleanly handle all cases in hooks it adds.
const attachedNativeHandlerTags = | ||
logicChildren.current.get(key)?.attachedNativeHandlerTags; | ||
if (attachedHandlerTags && attachedNativeHandlerTags) { | ||
detachHandlers( |
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 detach when shouldKeepLogicChild
is true?
useEffect(() => { | ||
setViewTag(findNodeHandle(viewRef.current)!); | ||
}, []); |
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.
View under the detector can change. I'm not sure if the effect will retrigger in that case.
If not, you can try with a function ref on Wrap (<Wrap ref={(ref) => console.log(ref)} ...
)
}; | ||
useEffect(() => { | ||
// TODO: set tags from parent | ||
setViewTag(Math.floor(Math.random() * Number.MAX_SAFE_INTEGER)); |
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.
You can try using the dom reference as a view tag - I think we are doing it already in some places.
Description
This PR implements new component ->
LogicDetector
. It resolves the issue of attaching gestures to inner SVG components.LogicDetector
communicates with aNativeDetector
higher in the hierarchy, which will be responsible for attaching gestures.Test plan
tested on the following code