LunatiqueCoder / react-native-media-console

A React Native video player. Built with TypeScript ❤️
MIT License
194 stars 29 forks source link

1104-flatlist-gesture-handling-enhancement #120

Open LunatiqueCoder opened 1 month ago

LunatiqueCoder commented 1 month ago

Fixes #104

coofzilla commented 1 month ago

okay so, I tested this and just had a question, will I have to trigger the disable scroll myself? so this isn't going to be handled internally?

here is a quick demo of how I tested it.

const videoData = [
  {id: '1', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '2', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '3', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '4', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
];

export default function HomeScreen() {
  const flatListRef = useRef<FlatList>(null);
  const [scrollEnabled, setScrollEnabled] = useState<boolean>(true);

  const handleScrollEnabled = (enabled: boolean) => {
    setScrollEnabled(enabled);
  };

  return (
    <>
      <FlatList
        ref={flatListRef}
        scrollEnabled={scrollEnabled}
        data={videoData}
        keyExtractor={(item) => item.id}
        horizontal
        renderItem={({item}) => (
          <View style={styles.videoContainer}>
            <VideoPlayer
              pan={{
                parentList: {
                  ref: flatListRef,
                  scrollEnabled: scrollEnabled,
                },
              }}
              source={{uri: item.uri}}
              onSeek={() => {
                console.log('Seeking');
              }}
              muted={true}
            />
          </View>
        )}
      />
      <Button
        title={scrollEnabled ? 'Disable Scroll' : 'Enable Scroll'}
        onPress={() => handleScrollEnabled(!scrollEnabled)}
      />
    </>
  );
}

This works as in, I can disable scrolling and then seek/scroll the volume, just wondering if this is the intended flow or, if I'm missing something so that its abstracted away?

coofzilla commented 1 month ago

In other words, am I going to have to manually trigger the disable scroll, and then seek adjust the volume, then re-enable it when I want to go back to scrolling? 🤔 My apologies if this is obvious and I'm just missing something.

LunatiqueCoder commented 1 month ago

@coofzilla You don't need need to manually trigger the disable scroll. This should happen internally. Although, there might be some certain scenarios where the <FlatList /> needs to use scrollEnabled prop, and the videoplayer has to be aware of it to revert back to the value it was before seeking.

📖 Explanation:

If FlatList's scrollEnabled={false}, the videoplayer needs to know, else it will use setNativeProps({scrollEnabled: true}) after seeking is finished, leading to a huge inconsistency between the scrollEnabled state and the native props. (one if them is true, the other is false)

setNativeProps is imperative and stores state in the native layer (DOM, UIView, etc.) and not within your React components, which makes your code more difficult to reason about.

If you update a property that is also managed by the render function, you might end up with some unpredictable and confusing bugs because anytime the component re-renders and that property changes, whatever value was previously set from setNativeProps will be completely ignored and overridden.

More info here: https://reactnative.dev/docs/direct-manipulation



But as a short answer, if you don't use the scrollEnabled prop on the <FlatList />, you're good to go just by passing the flatlist ref. However, if you use scrollEnabled prop, you also have to pass the same value/state to the videoplayer in order to avoid bugs.

This works as in, I can disable scrolling and then seek/scroll the volume, just wondering if this is the intended flow or, if I'm missing something so that its abstracted away?

If you have to disable the scrolling first, then there's definitely an issue with this implementation. However it seems to be working on my side 🤔

I really hope this is not that confusing, I also found it very hard to describe it in the docs. Let me know if this answers your questions, or I can try to be more precise when explaining.

LunatiqueCoder commented 1 month ago

@coofzilla

This works as in, I can disable scrolling and then seek/scroll the volume, just wondering if this is the intended flow or, if I'm missing something so that its abstracted away?

Yeah, I could find an issue with what you described here. I'll try to fix it tomorrow. Thank you for your help! Really appreciate it!

coofzilla commented 1 month ago

hey thanks for the explanation dude! Glad you were able to see the issue I was describing, I really appreciate it. Let me know when I can test further 🫡

LunatiqueCoder commented 1 month ago

@coofzilla You can try the new changes now. Hopefully it's fine now. 🙏

coofzilla commented 1 month ago

Do you have a snippet that I can try? Its still scrolling when I try to seek/change the volume. ( I pulled the most recent changes )

https://github.com/user-attachments/assets/2f6b0ccd-824f-4291-934f-f89a24bbfbcb

const videoData = [
  {id: '1', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '2', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '3', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '4', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
];

const screenWidth = Dimensions.get('window').width;

export default function HomeScreen() {
  const flatListRef = useRef<FlatList>(null);

  return (
    <View style={{flex: 1, width: screenWidth}}>
      <FlatList
        ref={flatListRef}
        data={videoData}
        keyExtractor={(item) => item.id}
        horizontal
        pagingEnabled
        renderItem={({item}) => (
          <View style={styles.videoContainer}>
            <VideoPlayer
              pan={{
                parentList: {
                  ref: flatListRef,
                },
              }}
              source={{uri: item.uri}}
              muted={true}
            />
          </View>
        )}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  videoContainer: {
    width: screenWidth,
  },
});
LunatiqueCoder commented 1 month ago

@coofzilla Your code snippet works perfectly fine for me now 🤔 You might need to run yarn prepare inside ./packages/media-console folder. This last commit was the fix, be sure it's also there in the code: https://github.com/LunatiqueCoder/react-native-media-console/pull/120/commits/75aafc81805d8f038d65e77686b2fc92d8f6095c

coofzilla commented 1 month ago

Alrighty, I'll get that checked out probably tomorrow and report back. Sorry, I have to put out some fires. 🧯 Thank you again for the help with this!

coofzilla commented 1 month ago

LETS GO! Works Perfect!!! I didn't have the last commit 🤦

https://github.com/user-attachments/assets/c5fa2051-86cb-4a08-a42d-67c15714bf5f

LunatiqueCoder commented 1 month ago

@coofzilla Really happy it works for you as well. I still need to add it to the docs and think how to describe it so it won't create confusion while using it. Thanks for the help!

coofzilla commented 1 month ago

@coofzilla Really happy it works for you as well. I still need to add it to the docs and think how to describe it so it won't create confusion while using it. Thanks for the help!

Given the example app, I was thinking what if there was a page with like "examples" or "use cases" as a list (each item being a different case that would press and go to the example) that could just scale as various ways to implement it are thought of. Or when people want to experiment/contribute, they could like, add theirs to the list. kinda like what this repo does.

Not as a replacement for docs; but, as a supplement?

This way when someone is like ahh, I really wish I knew how to implement this as a horizontal list and didn't have the conflicting scrolling, boom, you just have it as

1. Horizontal List
2. Vertical Feed
3. Paginated List
4. Custom Icons 🫣

here is a quick demo:

image image

Maybe thats too much for the purpose of this library haha; but, just a thought :)

LunatiqueCoder commented 1 month ago

@coofzilla Actually, that's a good idea. Besides being easier to contribute and test the changes, that's the purpose of the example app. I am planning to add various examples that users can just copy paste, however, I want to take care of the critical issues first.

Nevertheless PRs are welcome :) Active contributors are eligible to receive a license for all JetBrains IDEs: https://www.jetbrains.com/ides/