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

[expo-av] Fix fullscreen events not emitted on iOS #9323

Merged
merged 3 commits into from Jul 21, 2020

Conversation

IjzerenHein
Copy link
Contributor

@IjzerenHein IjzerenHein commented Jul 20, 2020

Why

Fixes #8393. When using the native full-screen controls on iOS, the full-screen events are not (always) emitted.

How

As of iOS 12, full-screen detection can be achieved using the AVPlayerViewControllerDelegate delegate.

  • Added AVPlayerViewControllerDelegate methods for detecting the full-screen mode change
  • Kept legacy view-bounds detection code as a fallback for iOS 11 or earlier
  • Extended NCL to log full-screen events to console

Test Plan

  • Verified that it worked locally
  • Verified that it does not emit events twice when using presentFullScreen..

fullscreen-events

@github-actions
Copy link
Contributor

Native Component List for this branch is ready

Copy link
Contributor

@bbarthec bbarthec left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Definitely it's better to base on some actual callbacks than some by-hand-calculation when it comes to fullscreen detection 😉

@@ -52,7 +52,9 @@ export default class VideoPlayer extends React.Component<

_handleVideoMount = (ref: Video) => (this._video = ref);

_updateStateToStatus = (status: any) => this.setState(status);
_handlePlaybackStatusUpdate = (status: any) => this.setState(status);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_handlePlaybackStatusUpdate = (status: any) => this.setState(status);
_handlePlaybackStatusUpdate = (status: SomeMeaningfulType) => this.setState(status);

Maybe it would help understanding what state parameters are actually changing? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. I'll have at improving the example in a separate PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NLC update PR #9335

@IjzerenHein IjzerenHein reopened this Jul 21, 2020
@IjzerenHein IjzerenHein merged commit e6e34f6 into master Jul 21, 2020
@IjzerenHein IjzerenHein deleted the @hein/expo-av/lifecycle-events branch July 21, 2020 15:25
Jamedjo pushed a commit to Jamedjo/expo that referenced this pull request Feb 4, 2021
* [ncl] Log full-screen events

* [expo-av] Fix fullscreen events not emitted on iOS

* [expo-av] Update changelog
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.

Expo video onFullScreenUpdate is not triggered as expected using the native controls in ios
2 participants