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

Make Fabric event receivers init only once #6907

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

szydlovsky
Copy link
Contributor

Summary

Fixes #6896

When adding support for fabric, we have inadvertently kept previous listeners still active for fabric. This way, we registered twice with the same NodesManager for React Native's FabricEventDispatcher, resulting in pretty much all the events doubling in number on android. I added a simple check that takes care of it.

Test plan

Paste the following into EmptyExample and run FabricExample. Check logs making sure that both start and end are logged only once per gesture.

import React, { useState } from 'react';
import { View, Text } from 'react-native';
import Animated, { useAnimatedScrollHandler } from 'react-native-reanimated';

function EmptyExample() {
  const [w, sW] = useState(0);

  const scrollHandler = useAnimatedScrollHandler({
    onBeginDrag() {
      console.log('start');
    },
    onMomentumEnd() {
      console.log('end');
    },
  });

  return (
    <View
      style={{ flex: 1 }}
      onLayout={(evt) => sW(evt.nativeEvent.layout.width)}>
      <Animated.ScrollView
        onScroll={scrollHandler}
        horizontal
        snapToInterval={w}
        decelerationRate="fast">
        {Array.from({ length: 20 })
          .fill(0)
          .map((_, i) => {
            return (
              <View key={i} style={{ width: w }}>
                <Text>{i}</Text>
              </View>
            );
          })}
      </Animated.ScrollView>
    </View>
  );
}

export default EmptyExample;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow move the event subscription bits to the same place in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjzel we could. Here, I targeted only the given problem without taking any risk in refactoring the code, so I believe we could do that in a seperate PR 😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a major refactor then leave it since we don't want to put too much effort into refactoring Paper code. If it's minor we could include it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see about it, but they are being initialized from two different contexts - paper goes through the usual initializer, fabric gets a initiWithContext method, sounds fishy

Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me, haven't tested it though, let's wait for approvals from other reviewers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: onMomentumEnd called many times
3 participants