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

[video][ios] Fix unable to call presentFullScreenPlayer twice #8343

Merged
merged 3 commits into from May 19, 2020

Conversation

IjzerenHein
Copy link
Contributor

Why

Calling presentFullScreenPlayer on the Video component for the second time, throws error "Fullscreen player is already being presented". This happens only when the useNativeControls prop is also set.

Fixes #8239

How

When useNativeControls is set, it re-creates the PlayerViewController every time the full-screen view-controller is closed. This in turn caused the “videoBounds” key observer handler to be called which incorrectly assumed the regular ViewController was transitioning from full-screen to non full-screen. An additional check has been added to ensure this detection is not triggered when the full-screen viewcontroller is used.

Test Plan

Steps to reproduce

  • Open NCL and go to "Components" -> "Video"
  • Enable "Native controls"
  • Press "Open fullscreen"
  • Close video-player
  • Press "Open fullscreen" again

Before

May-18-2020 13-43-47

After

May-18-2020 13-41-03

Fixes an issue when calling `presentFullScreenPlayer` more than once on iOS. This would result in the error “Fullscreen player is already being presented” when the `useNativeControls` prop also set on the `<Video>` component.

When useNativeControls is set, it re-creates the PlayerViewController every time the full-screen view-controller was closed. This caused the “videoBounds” key-value handler to be called which incorrectly assumed the regular ViewController was transitioning from full-screen to non full-screen. An additional check has been added to ensure this detection is not triggered when the full-screen viewcontroll is used.

When `useNativeControls` is set, calling `presentFullScreenPlayer` twice fails with
@IjzerenHein
Copy link
Contributor Author

cc @FiberJW

@github-actions
Copy link
Contributor

Native Component List for this branch is ready

@mczernek mczernek self-assigned this May 18, 2020
@zackify
Copy link
Contributor

zackify commented May 18, 2020

Thanks for this! I found one other issue with audio. I can make a snack if you’d like. Just haven’t had the time. It’s explained here though: https://stackoverflow.com/a/60152015/1487687

If you try ignoring the silent switch. It does not work with the video player UNLESS you turn auto play on. Very weird!

P.S expo is awesome and I appreciate the quick turn around. 🙏🏻🙏🏻

@IjzerenHein
Copy link
Contributor Author

IjzerenHein commented May 19, 2020

Thanks for this! I found one other issue with audio. I can make a snack if you’d like. Just haven’t had the time. It’s explained here though: https://stackoverflow.com/a/60152015/1487687

If you try ignoring the silent switch. It does not work with the video player UNLESS you turn auto play on. Very weird!

P.S expo is awesome and I appreciate the quick turn around. 🙏🏻🙏🏻

You're welcome :D

So please create a separate issue for the audio problem so we can keep the discussions on topic 👍

Copy link
Contributor

@mczernek mczernek left a comment

Choose a reason for hiding this comment

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

💯 🎉 💯

@IjzerenHein
Copy link
Contributor Author

@lukmccall If I merge this, would it correctly generate the CHANGELOG and what would that changelog entry be?
Would it be possible to see a preview of the changelog entry before merging?

@lukmccall
Copy link
Contributor

@IjzerenHein, unfortunately, the danger is still in the test phase :/ So, you need to add it manually.

You can see how it will look like her. It won't add anything to your code without your confirmation. Firstly, the script will generate a message with information about all missing entries - #8304 (comment). Then, it will create a new branch with data from the body of your PR. Where you can correct messages, just by adding a suggestion to generated code - #8306 (comment).

I think it will be available after the coming release. Cause, I'm still working on fixing some issues with permissions on CI.

@IjzerenHein
Copy link
Contributor Author

@lukmccall No worries, looking forward to have this automated though 😃

@IjzerenHein IjzerenHein merged commit 106e0e9 into master May 19, 2020
@IjzerenHein IjzerenHein deleted the @hein/expo-av/present-fullscreen-player branch May 19, 2020 15:10
Jamedjo pushed a commit to Jamedjo/expo that referenced this pull request Feb 4, 2021
)

Fixes an issue when calling `presentFullScreenPlayer` more than once on iOS. This would result in the error “Fullscreen player is already being presented” when the `useNativeControls` prop also set on the `<Video>` component.

When useNativeControls is set, it re-creates the PlayerViewController every time the full-screen view-controller was closed. This caused the “videoBounds” key-value handler to be called which incorrectly assumed the regular ViewController was transitioning from full-screen to non full-screen. An additional check has been added to ensure this detection is not triggered when the full-screen viewcontroller is used.
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.

[AV][Video][iOS] presentFullscreenPlayer only works once in iOS
5 participants