-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Feature/android pip #1776
Feature/android pip #1776
Conversation
# Conflicts: # CHANGELOG.md # android-exoplayer/build.gradle # android-exoplayer/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java # package.json
android-exoplayer/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
Show resolved
Hide resolved
android-exoplayer/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
Outdated
Show resolved
Hide resolved
android-exoplayer/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
Outdated
Show resolved
Hide resolved
android-exoplayer/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
Outdated
Show resolved
Hide resolved
Why isn't this merged yet? |
Hi ! |
Hello everyone, let me take a look at this PR again, solve the conflicts, and then we are ready for reviews and eventually for merging. Anyways I would need someone to give a green light. I think I should be able to get back to this somewhere this week. |
Hi, |
Hi all, |
@nickfujita what do you think about this pr |
How do I apply this change to my project? how do i install this update? |
How do we implement this in our react-native-video projects? It seems to be great. Why hasn't this been merged? |
I automatically closed PRs and issues that were stale. We can reopen if there is interest and resources to review and test. If you are interested in helping getting this PR through, I'm happy to reopen. Merging this would require 3 people to review, test, and approve it. |
I would, actually. I'll see if I can carve some time. 😎👍 |
Code looks good, but I am afraid this change is not really expected in react-native-video... For exemple, if tomorrow you want to do pip with react-native-youtube, (https://www.npmjs.com/package/react-native-youtube) you can have issues to make multiple pip handling bindings ... |
What about pip for ios? Wouldn't react native youtube support that as well? In my mind it would make sense to handle pip in rn video because it also supports ios. |
I agree that ios pip is already handle by react-native-video so it make sens to support it on android. |
In other words, it is very useful for quick integration, but it doesn't fullfil all expected requirements of an advanced android application :) |
What advanced requirements are you referring to? Is it something that other package can do? Maybe I just don't know enough about android pip to weigh in. |
I'll make some time to test this tomorrow ;) Let's get this reviewed and merged. |
&& packageManager | ||
.hasSystemFeature( | ||
PackageManager.FEATURE_PICTURE_IN_PICTURE)) { | ||
long videoPosition = player.getCurrentPosition(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removed. The variable is not used anywhere and causes crashes.
With the line removed above it continues on working, but the PiP floating window disappears. |
@TomMichalkevic Thanks for the review. @matejpolak Are you still interested in getting this meged? |
Hello @hueniverse! |
@paulohc there is no point to reopen it because it needs to be rebased. If you want to do the work and open a new PR with these changes against master, it will get reviewed. Someone needs to do the work. |
Here is the Java code for anyone who needs it as the instructions were written in Kotlin for the MainActivity:
|
Provide an example of how to test the change
In order to test this change, you should follow all new steps listed in readme under picureInPicture props:
Afterwards, set
pictureInPicture
prop, or press Home/Recent HW button.Describe the changes
So far PIP mode has been supported only on iPad. From now on, it should be working also on all Android Phones and Tablets running Oreo at least. This implementation includes:
pictureInPicture
prop.onPictureInPictureStatusChanged
prop callback returning PIP state same as iOS does.