Enhanced Fullscreen options for iOS

[ashnfb]

@cobarx here's the updated version for Fullscreen video options (iOS only).
Readme file updated.

[ashnfb]

@cobarx it's been a while since I heard from you ... would you have time to review / comment on this PR?

[cobarx]

Hey man, I've just been working a lot and feeling too exhausted when I get home to do much additional coding or review. I'll do my best to take a look this weekend.

[cobarx]

In general, React libraries want the props to be scalar values rather than objects. It makes the code & behavior a lot more complex doing it the other way. The only reason I did it for audio & text tracks is that they only work with multiple parameters so I couldn't think of a way to avoid it. I went ahead and refactored that code and pushed it to https://github.com/react-native-community/react-native-video/pull/new/nfb-onf-master

I'm not sure what changes you made to the urlFilePath are doing. Can you walk me through the problem and how to test the issue & the fix? In the future, please break those changes into a separate PR so it can be evaluated away from the main functionality.

I also wasn't sure why we need the auto-rotate functionality. In my experience, you either want to lock the video in a particular orientation or you want to have it auto-rotate. Having some combination of the two almost never comes up. Unless there's a good reason to have it, I'd just as soon leave out the orientation option and make it landscape, portrait, or auto-rotate. I'm open to scenarios I might not have thought of though.

[cobarx]

I just noticed in #790 that he removes all the orientation & rotation code on tvOS. Can you test that we don't need to do that for this PR (use my changes). There should be a tvOS simulator in Xcode.

[davrosull]

@cobarx The best example of the auto-rotate functionality I can think of is the YouTube app where you go from watching a video inline (in portrait mode) and pressing fullscreen will move the video to fullscreen and force it to a landscape rotation.

I believe this is the functionality trying to be achieved.

[cobarx]

@davrosull I think we can do that with just orientation. When going to fullscreen, it creates a new view controller and we are setting these options on it. Once we return to portrait view, that view controller goes away, so these options don't apply.

Tbh, I appreciate the hard work @ashnfb has done. There is a react-native-orientation project that does this already for both iOS & Android. I'm not sure why we need to duplicate that functionality.

[ashnfb]

@cobarx my bad about the changes to urlFilePath seeping into this PR. The fix was that if the filePath has "file:///" it can be passed directly as a string to NSURL. This was to fix a bug we found with locally stored videos.

if ([filepath containsString:@"file://"]) { return [NSURL URLWithString:filepath]; }

[ashnfb]

@cobarx, updates made!

• autoRotate is unneeded
• fullscreenOrientation is still useful for the case where an iOS App is portrait only, but the video player is preferred in landscape. (This is not possible with the react-native-orientation project, as it would require allowing the presenting viewController to also rotate)
• the previous contributor (PR #790) was correct that we can't use orientation for tvOS
• I added a tvOS target to the examples/basic project for future testing on tvOS

[cobarx]

@ashnfb Thanks for making all these updates and handling tvOS.

I did a bit more testing and we may need auto-rotate after all. Without it, if you set landscape and flip the device upside down, it won't rotate. We might also want to support specific orientations like landscapeLeft like the other PR did. It's not that common, but some people may want it.

Since this already works, I'm merging this PR now :) Would you mind adding auto-rotate back in in a future PR and specific orientations if you think they're worth it.

[ashnfb]

Hi @cobarx thanks for merging the PR. I know you are busy with the new gig, so I appreciate your time :)

If I find time I'll add back the autorotate. For now I left the behaviour to auto-rotate if fullscreenOrientation is not specified (or "all" is specified), and lock the rotation otherwise. But we can easily add back an override value.