Skip to content
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

fix(core): handle GestureObservers the same as event listeners #10538

Merged
merged 1 commit into from May 7, 2024

Conversation

shirakaba
Copy link
Contributor

This PR is related to #10537, but can be merged independently. Whichever is merged first will cause minor conflicts with the other, but it's a simple rebase that I'd be happy to take care of.

PR Checklist

What is the current behavior?

Although listeners for gesture events are set up in the same way as any other events, there are subtle differences in how they get handled.

  • Gesture events accordingly have a few bugs:
    • You can add the same eventName-callback-thisArg gesture observer multiple times.
    • Removing any gesture observer removes all of the same name, regardless of callback-thisArg identity.
  • GesturesObserver reaches up into ViewCommon to remove itself from the observer map; that should instead be a responsibility of ViewCommon, as that's the agent that maintains and inserts into the map.
  • Unlike with regular events, Gesture events can be called with any thisArg, even falsy values.

What is the new behavior?

  • Gestures are now handled just like event listeners.
    • They are identified by their eventName-callback-thisArg tuple.
    • You can't add the same one twice.
    • You can remove multiple at once by omitting the callback and/or thisArg arguments, e.g. via viewCommon.removeEventListener("tap")
  • _observe is now protected rather than public. My thought is that we're exposing far too many internal APIs and it prevents us from refactoring our internals in future. This API was not used in any tests, nor was it documented, and is prefixed with _, so it's clearly internal-looking to begin with.

@shirakaba
Copy link
Contributor Author

shirakaba commented May 4, 2024

@farfromrefug @edusperoni @CatchABus Sorry to poke. Anything holding this back from approval?

farfromrefug
farfromrefug previously approved these changes May 4, 2024
NathanWalker
NathanWalker previously approved these changes May 6, 2024
@shirakaba shirakaba dismissed stale reviews from NathanWalker and farfromrefug via 46bee53 May 7, 2024 00:38
@shirakaba shirakaba force-pushed the gesture-observers-consistency branch from cb0f32b to 46bee53 Compare May 7, 2024 00:38
@NathanWalker NathanWalker merged commit d323672 into main May 7, 2024
4 checks passed
@NathanWalker NathanWalker deleted the gesture-observers-consistency branch May 7, 2024 00:50
apburgess pushed a commit to apburgess/NativeScript that referenced this pull request May 15, 2024
apburgess pushed a commit to apburgess/NativeScript that referenced this pull request May 15, 2024
apburgess pushed a commit to apburgess/NativeScript that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants