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: onPipChange #155

Merged
merged 2 commits into from
Feb 6, 2024
Merged

feat: onPipChange #155

merged 2 commits into from
Feb 6, 2024

Conversation

Bowlerr
Copy link
Contributor

@Bowlerr Bowlerr commented Aug 21, 2023

Issue #, if available:

At the moment, we can't have the audio of the video mute when the video is in the background and picture in picture is not active as we are not aware of the state changes to Picture in Picture.

Description of changes:

Adds onPipChange prop to IVSPlayer.
Native code which tracks if picture in picture is opened or closed.

onPipChange (optional)

Callback that returns changes to the picture in picture state.
This returns false if no picture in picture is active. It returns true if picture in picture is active.

type: (isActive: boolean) => void
Available only for Android N+ and iOS 15+

Example use case to solve Issue stated:

`const IVSPlayerWithPip = () => {

    const [isInBackground, setBackgroundState] = useState(false);
    const [isInPip, setPipState] = useState(false);

    const [appState, setAppState] = useState(AppState.currentState);

    const handleAppStateChange = useCallback(
        (nextAppState: AppStateStatus) => {
            if (
                appState.match(/inactive|background/) &&
                nextAppState === 'active'
            ) {
                // App is coming back to the foreground
                // Play audio
                setBackgroundState(false);
            } else if (
                appState === 'active' &&
                nextAppState.match(/inactive|background/)
            ) {
                // App is going to the background
                // mute audio
                setBackgroundState(true);
            }

            setAppState(nextAppState);
        },
        [appState]
    );
    useEffect(() => {
        // Add an event listener to handle app state changes
        const listener = AppState.addEventListener(
            'change',
            handleAppStateChange
        );

        // Clean up the event listener when the component is unmounted
        return () => {
            listener.remove();
        };
    }, [handleAppStateChange]);

    const onPipStateChange = useCallback((isActive: boolean) => {
        setPipState(isActive);
    }, []);

    return (
            <IVSPlayer
                onPipChange={onPipStateChange}
                volume={isInBackground && !isInPip ? 0 : 1}
            />
    );
};`

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Bowlerr Bowlerr marked this pull request as ready for review August 21, 2023 16:39
@dawhitla
Copy link
Contributor

Hey @Bowlerr we are working through merging these pip related PRs, will need to resolve a conflict here.

@@ -371,6 +382,20 @@ class AmazonIvsView(private val context: ThemedReactContext) : FrameLayout(conte
private fun intervalHandler() {
val reactContext = context as ReactContext

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we lean on https://developer.android.com/develop/ui/views/picture-in-picture#handling_ui

override fun onPictureInPictureModeChanged(
     isInPictureInPictureMode: Boolean, newConfig: Configuration) {

instead of relying on polling the state?

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 believe I attempted this but couldn't get it triggering so I went with polling as a last ditch effort as I needed something working at the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However once the other PiP PR had been merged, I am happy to test it again :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Bowlerr was hoping to merge this PiP relate PR before #156 👍

Copy link
Contributor Author

@Bowlerr Bowlerr Aug 30, 2023

Choose a reason for hiding this comment

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

Hey @dawhitla,

Last time I attempted to use this, it never triggered / claimed to override nothing.

override fun onPictureInPictureModeChanged( isInPictureInPictureMode: Boolean, newConfig: Configuration) { ...

Is there something I am missing?

Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I can pull your fork and take a look at why onPictureInPictureModeChanged does not trigger 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have cleaned this up a little replacing:

if(Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && reactContext.packageManager .hasSystemFeature( PackageManager.FEATURE_PICTURE_IN_PICTURE))

with

if(pipEnabled) in the intervalHandler

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I figured out why onPictureInPictureModeChanged isn't happening for the view

It's a function of a callback on the activity itself. src/main/java/com/facebook/react/ReactActivity.java

So hooking into onPictureInPictureModeChanged would require a manual setup 😮‍💨

Here is what another RN lib did
https://github.com/adkaushik/react-native-pip-android#setup

We may need to rely on the interval check for the time being for easier DX (no manual setup step)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thank you ! @dawhitla

I believe that's where I got to before switching to the interval check. Although it isn't ideal, the interval check has been reliable in our production use case :) Check out Moar if you want to see it in action.

I am happy to look into getting this done at some point but other priorities have taken my focus for the time being. Feel free to merge as is

@Bowlerr
Copy link
Contributor Author

Bowlerr commented Sep 1, 2023

Hey @Bowlerr we are working through merging these pip related PRs, will need to resolve a conflict here.

@dawhitla just saw the conflicts and merged the latest changes into this branch. Let me know if there is anything else I can assist with.

@Bowlerr
Copy link
Contributor Author

Bowlerr commented Sep 1, 2023

Unsure why the ios test failed 🤔

@dawhitla dawhitla merged commit 1db41ee into aws:main Feb 6, 2024
4 of 5 checks passed
@dawhitla dawhitla mentioned this pull request Feb 16, 2024
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.

None yet

2 participants