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 method for getting current playback status #4229

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

seyedmostafahasani
Copy link
Collaborator

Summary

Add method for getting current playback status.

Motivation

Implement feature request #4226.

Changes

Test plan

Run the sample app, add a button get play status with the use of function getCurrentPlaybackStatus.

ios/Video/RCTVideo.swift Outdated Show resolved Hide resolved
@@ -40,6 +40,15 @@ export type ReactVideoSourceProperties = {
textTracks?: TextTracks;
};

export type TPlaybackStatus = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

PlaybackStatus would be enough ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's better to add the prefix T before type names, meaning playbackStatus would be a type. Similarly, we can add an I before interface names
FX: type TSample = {} and interface ISample = {}
WDYT?

Copy link

@IslamRustamov IslamRustamov Nov 3, 2024

Choose a reason for hiding this comment

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

As Robert Martin wrote in his "Clean Code" - T, I, Enum, List (and so on) prefixes/suffixes before types, interfaces, enums (and so on) are meaningless.

We are not necessarily need to follow what he writes in his books, but it's better to follow guidelines that are accepted in the project you are working on (for example, as I can see in this project suffixes/prefixes are not used)

@YangJonghun
Copy link
Collaborator

YangJonghun commented Oct 11, 2024

@seyedmostafahasani
It's a great idea and something I've been wanting.
But in new arch, functions called via ref can't have a return value. Current implementation of save or getCurrentPosition is tricky and may be deprecated in the future via RN Core, so using callback props may be a better approach for now.
(This is why I made it so that library authors see deprecation warning when using getReactTag.)

@seyedmostafahasani seyedmostafahasani changed the title feat: add method for getting current playback status. feat: add method for getting current playback status Oct 11, 2024
@seyedmostafahasani
Copy link
Collaborator Author

@YangJonghun
Thanks for informing me. I think we can use this method until the RN core team decides on it. If they deprecate it, we can implement a new approach using callback props.
WDYT?

@freeboub
Copy link
Collaborator

@seyedmostafahasani if it is deprecated, It is nopt a good idea to use it.
BTW a possible implementation would be to implement the event callback on native side.
Register an handler in Video.tsx and the save the last state in a Ref.
Then the getter will just return the ref value.

@YangJonghun
Copy link
Collaborator

YangJonghun commented Oct 13, 2024

Thanks for informing me. I think we can use this method until the RN core team decides on it. If they deprecate it, we can implement a new approach using callback props.
WDYT?

Same as above comment

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.

4 participants