chore: add setFullScreen to component's ref

[seyedmostafahasani]

…eenPlayer & dismissFullscreenPlayer

Summary

I added setFullScreen to handle fullScreen mode without controls.

Motivation

The user can use it to handle fullscreen mode with custom controls, specifically refactoring the codebase and fix this issue

Changes

I added a function called setFullScreen and deprecated the functions presentFullscreenPlayer and dismissFullscreenPlayer.

Test plan

[freeboub]

We cannot remove existing APIs on 6.X branch, I don't understand why you need this change. Can you please clarify ?

[seyedmostafahasani]

I think we can implement full-screen mode with just this function. We don't need two functions to handle it. Additionally, if we remove these functions, we will eliminate an unnecessary useState from the video component, which prevents re-rendering. Specifically, we will also fix another issue in the library, #3732. The main point is that we handle this logic with just one function. What is your opinion?

[seyedmostafahasani]

@freeboub what do you think about my points?

[freeboub]

@seyedmostafahasani We cannot change api on V6.X, if we integrate such api change, we will need to bump to V7 and all apps using the package will have to upgrade their integration. as the team have a lot of api changes in mind, we try to keep api interoperability as much as possible.

Here is a sample:
https://github.com/TheWidlarzGroup/react-native-video/pull/3867/files#diff-e2131d0631cdc1ddf0ba36827a27b86adf8e5b8ed162a280a53c6219f4df5a5f

I just move few props from the root of video to the source object, to avoid upgrade issue, I comment in doc that the old api is deprecated, and point to the new way to proceed.

It will allow easy upgrade for apps, we keep in mind that old api (deprecated) can be removed on V7.
Can you please do the same thing please ?
I think merging the 2 apis make sense, but we need to keep compatibility of old api some time

[seyedmostafahasani]

@seyedmostafahasani We cannot change api on V6.X, if we integrate such api change, we will need to bump to V7 and all apps using the package will have to upgrade their integration. as the team have a lot of api changes in mind, we try to keep api interoperability as much as possible.

Here is a sample: https://github.com/TheWidlarzGroup/react-native-video/pull/3867/files#diff-e2131d0631cdc1ddf0ba36827a27b86adf8e5b8ed162a280a53c6219f4df5a5f

I just move few props from the root of video to the source object, to avoid upgrade issue, I comment in doc that the old api is deprecated, and point to the new way to proceed.

It will allow easy upgrade for apps, we keep in mind that old api (deprecated) can be removed on V7. Can you please do the same thing please ? I think merging the 2 apis make sense, but we need to keep compatibility of old api some time

I completely agree with you about deprecating the old API and planning to remove it in V7. I will add deprecation notices for both presentFullscreenPlayer and dismissFullscreenPlayer. This approach ensures backward compatibility while allowing a smooth transition for users until V7, where we can fully remove the old API.

[freeboub]

@seyedmostafahasani Thank you for the fix,
last remaining point, can you please remove deprecated functions in the sample please ? :)

following code shall be simplified:

  toggleDecoration() {
    this.setState({decoration: !this.state.decoration});
    if (this.state.decoration) {
      this.video?.dismissFullscreenPlayer();
    } else {
      this.video?.presentFullscreenPlayer();
    }
  }

[seyedmostafahasani]

@freeboub I removed the controls condition in the updateFullScreenButtonVisibility function. When the user sets controls={false}, the fullScreen button disappears in fullScreen mode, requiring the user to press the physical back button to exit fullScreen mode, which may cause confusion. What is your opinion?

[YangJonghun]

I don't use native controls and fullscreen in this library, but I'm not sure if this breaking change is really necessary in this library.
It's not a duplicate function, so it doesn't have much effect on user DX as long as user pass an argument. Also, It's easy to remove useState and keep the existing API, so why we make breaking change?

[freeboub]

It is not a breaking change as previous api are still working! It is just a preparation for api clean up ! (Let say for V7)

[YangJonghun]

@freeboub
oh..then I misunderstood.
(If possible, it would be nice to have something like a new-arch only prop that works synchronously. ref.fullscreen = true)

[freeboub]

Yes I agree, Fullscreen management is one of the next challenge I think...

[KrzysztofMoch]

LGTM