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

feat: add getCurrentPosition to component's ref #3824

Conversation

seyedmostafahasani
Copy link
Collaborator

Summary

This function retrieves and returns the precise current position of the video playback, measured in seconds.

Motivation

The goal is to provide users with the ability to access the current position of the video playback. This allows for the creation of custom controls like forward and rewind buttons, enhancing user experience.

Changes

A new function getCurrentPosition has been added to the component's ref.

Test plan

@freeboub
Copy link
Collaborator

Looks good to me, Just one note, I would prefer having the time in msec instead of sec. @KrzysztofMoch @YangJonghun what do you think of this point ?

@YangJonghun
Copy link
Collaborator

As far as I know, onProgress callback exists for the same reason. I wonder for what purpose this method was implemented.

@seyedmostafahasani
Copy link
Collaborator Author

@YangJonghun you're correct! In situations where the user only needs to get the current position when interacting with custom controls, such as the rewind and forward buttons, it's not necessary to use a callback function that is called every progress update interval.

Instead, the user can use this approach that is only called when the user clicks the rewind or forward buttons. When a button is clicked, you can get the current position and then seek forward or backward based on the button that was clicked.

@YangJonghun
Copy link
Collaborator

@seyedmostafahasani
That makes sense!
However, I think it might be a bit difficult to use it for precise work due to the limitations of old architecture, since the current time value keeps changing on playback.
It will be great later when we're done with the new architecture :)
Thanks for your explanation.👍

@seyedmostafahasani
Copy link
Collaborator Author

So now what is your opinion about the response of this function, second or millisecond?

@YangJonghun
Copy link
Collaborator

@seyedmostafahasani
Oh sorry to late response.
I like second(specifically, second as a float value).
I prefer the same format as the value returned by onProgress, and I think many developers will be familiar with the web's video.currentTime format

@seyedmostafahasani
Copy link
Collaborator Author

I completely agree with you about second.

@freeboub
Copy link
Collaborator

As C developer I prefer working with integer, but if you are ok for sec, then OK for me !

Copy link
Collaborator

@freeboub freeboub left a comment

Choose a reason for hiding this comment

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

See my comment for android

ios/Video/RCTVideo.swift Outdated Show resolved Hide resolved
@freeboub freeboub merged commit c7f4d7b into TheWidlarzGroup:master May 28, 2024
12 checks passed
@seyedmostafahasani seyedmostafahasani deleted the feat/currentPosition-functionality branch May 28, 2024 16:09
@YangJonghun
Copy link
Collaborator

@freeboub @KrzysztofMoch @seyedmostafahasani
I think we need to deprecate this feature
ref command with return value not supported by Fabric
(save command also have same problem)

@seyedmostafahasani
Copy link
Collaborator Author

seyedmostafahasani commented Jun 28, 2024

@YangJonghun I think we can use them with Fabric, but an important point is that these are deprecated methods on the native or JS side. Utilizing deprecated methods on the native or JS side may result in these functions not working correctly. I recommend checking out react-native-blurhash, as it employs Fabric. This library uses functions that return values via promises, such as the encode function, which you can observe in these URLs.
https://github.com/mrousavy/react-native-blurhash/blob/master/src/index.tsx
https://github.com/mrousavy/react-native-blurhash/blob/master/android/src/main/java/com/mrousavy/blurhash/BlurhashModule.kt

@YangJonghun
Copy link
Collaborator

@seyedmostafahasani
BlurHash is NativeModule.
TurboModule and Fabric are different. TurboModule can be return values
What I meant was that it's not possible to use a View as a ref and return it.

Currently we use save or getCurrentPosition in react-native-video is to reference a reactTag in NativeModule(VideoManager).
We are using findNodeHandle for above behavior, but findNodeHandle will be deprecated, so it is not recommended to continue using this method. We need to migrate to command method to replace it.
However command does not support return or callback functions.

@seyedmostafahasani
Copy link
Collaborator Author

@YangJonghun Oh yes, you're right. Thanks for the detailed explanation. I was referring to the deprecated method findNodeHandle. Could you please take a look at the react-native-view-shot library? I think this library could help us.

@YangJonghun
Copy link
Collaborator

@seyedmostafahasani
Thanks for give me reference
But unfortunately react-native-view-shot also using findNodeHandle now. (link)🥲

I guess I'll have to keep it for now and find a tricky way around it. Because we don't know when RN team get rid of findNodeHandle.
fyi) https://github.com/reactwg/react-native-new-architecture/blob/main/docs/enable-libraries-prerequisites.md

@seyedmostafahasani
Copy link
Collaborator Author

seyedmostafahasani commented Jun 29, 2024

@YangJonghun anytime!
Did you see this PR?

I agree with you, we can keep it for now. Also, if I find a better solution, I will share it with you.

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.

3 participants