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

Support for displaying controls in Android Exoplayer #1414

Merged
merged 9 commits into from Feb 11, 2019

Conversation

Projects
None yet
9 participants
@IbrahimSulai
Copy link

commented Jan 4, 2019

Added support for displaying player controls in Android Exoplayer component using PlayerControlView
Utilised the existing boolean prop 'controls' to show/hide the native controls for the player.

Sample Usage

<Video
  source={
    {uri: 'https://www.w3schools.com/html/mov_bbb.mp4'}
  }
  controls={true}
/>
@kcalmes

This comment has been minimized.

Copy link

commented Jan 8, 2019

When will this pull request be accepted?

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 8, 2019

@cobarx
could you help to review and merge.

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 14, 2019

@n1ru4l @cobarx - Could you please help to review this pull request.

@cobarx
Copy link
Collaborator

left a comment

Hi @IbrahimSulai, sorry for the delay in looking at this, I've been sick the last week so haven't gotten to put any time into the project.

Here are some minor changes that I saw by glancing over the PR. I still need to run the code and see how it works.

Thanks for putting this together! I had no idea that ExoPlayer included built-in controls so this makes it so much easier to implement this feature. Nice work!

Go ahead and make the changes, I will test this out in the next day or two.

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

@cobarx - Thanks for your valuable review comments. I have resolved the same. Please review the changes.

I hope you are doing good regarding your health. Take care.

@AndriiUhryn

This comment has been minimized.

Copy link

commented Jan 21, 2019

Is there any news about this PR? @cobarx

@mgezkaya

This comment has been minimized.

Copy link

commented Jan 22, 2019

Hey @cobarx I have tried this PR on Android in my local and it's really works. It has some issues that needs to be solved although but I think it's still better than nothing. I hope you can merge it when you are available. And @IbrahimSulai thanks for your attention but can you apply a change for fullScreen on control, too? Because i couldn't see and i really need it. Thank you both.

@felipemillhouse

This comment has been minimized.

Copy link

commented Jan 23, 2019

Please @cobarx merge it!!!!! 🎉🎉🎉 thank you @IbrahimSulai

@cobarx

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2019

Ok I tested this and am seeing a weird behavior where when I press pause, the play button doesn't appear there is just a blank space. Any idea what's going on here? Here is a screenshot:
no play

Also, if I start the video with controls={true} then change it to false, the controls don't disappear. I looked at the code and this should be possible, can you update it to handle this?

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 24, 2019

@cobarx - I will look into the above UI issue and also will handle the controls props.

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 25, 2019

Ok I tested this and am seeing a weird behavior where when I press pause, the play button doesn't appear there is just a blank space. Any idea what's going on here? Here is a screenshot:
no play

Also, if I start the video with controls={true} then change it to false, the controls don't disappear. I looked at the code and this should be possible, can you update it to handle this?

@cobarx - I have fixed the UI player control UI issue. Also handled the controls prop state. Could you please review it.

@mgezkaya

This comment has been minimized.

Copy link

commented Jan 25, 2019

@IbrahimSulai Can you apply a button for fullscreen if you can, please?

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 25, 2019

@IbrahimSulai Can you apply a button for fullscreen if you can, please?

@mgezkaya , Sure. Once @cobarx merge this, will add the full screen feature as a separate PR. Thanks.

@mgezkaya

This comment has been minimized.

Copy link

commented Jan 25, 2019

@IbrahimSulai Can you apply a button for fullscreen if you can, please?

@mgezkaya , Sure. Once @cobarx merge this, will add the full screen feature as a separate PR. Thanks.

Thanks for return

@xstable

This comment has been minimized.

Copy link

commented Jan 28, 2019

@IbrahimSulai didn't work for me on Android 9.
Play-Button still disappear after pressing Pause.

But if I put the App into background, and get it back to foreground, it autoplays the video, and "Pause" button is visible.
If I start the Video with "paused", I see the Play-Button. And If I press on it, the "pause"Button disappear.

Here is my patch-file: https://hastebin.com/jugowesova.diff

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 28, 2019

@IbrahimSulai didn't work for me on Android 9.
Play-Button still disappear after pressing Pause.

But if I put the App into background, and get it back to foreground, it autoplays the video, and "Pause" button is visible.

Here is my patch-file: https://hastebin.com/ximupiwexe.diff

@xstable
I tested in Android version 9 and lower versions (Models: Google Pixel, OnePlus5T, and Nexus), the player works as expected. Can you share more details (like Device Model) on the issue you are facing?

I’m not sure about the patch package you are using, so can you also please try to use the changes AS IS from the PR instead of trying via any patch/patch packages.

PFB, the recording of the player.
exoplayercontrols

@xstable

This comment has been minimized.

Copy link

commented Jan 29, 2019

@IbrahimSulai The Patch is my own one, I've created with patch-package. It contains the diff between default "react-native-video" and my changes.

My Device is: Xiaomi Mi A1 & Samsung S4

Maybe I've missed something, or have an Issue in my code, so here I've create a package of my changed source-code from android-explayer so that you can have a look inside.
android-exoplayer.tar.gz

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 29, 2019

@xstable
I have tested the app in Samsung S4, it is working as expected. Do you have a specific scenario where this is happening since I am not able to reproduce this.

Could you please share your mail id. So, that we can communicate in person in the below slack channel to solve this.

https://ibrahim-6ic2441.slack.com/messages/CFS1PQ3HQ

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

@IbrahimSulai The Patch is my own one, I've created with patch-package. It contains the diff between default "react-native-video" and my changes.

My Device is: Xiaomi Mi A1 & Samsung S4

Maybe I've missed something, or have an Issue in my code, so here I've create a package of my changed source-code from android-explayer so that you can have a look inside.
android-exoplayer.tar.gz

I can able to reproduce the issue with your patch file. I debugged the issue, the player states are working exactly fine. But, there is a UI glitch with the play/pause button. I am working on it to find the root cause for the same, will get back with the solution ASAP.

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Feb 4, 2019

@IbrahimSulai The Patch is my own one, I've created with patch-package. It contains the diff between default "react-native-video" and my changes.
My Device is: Xiaomi Mi A1 & Samsung S4
Maybe I've missed something, or have an Issue in my code, so here I've create a package of my changed source-code from android-explayer so that you can have a look inside.
android-exoplayer.tar.gz

I can able to reproduce the issue with your patch file. I debugged the issue, the player states are working exactly fine. But, there is a UI glitch with the play/pause button. I am working on it to find the root cause for the same, will get back with the solution ASAP.

@xstable
The issue is related to the open bug in react-native: facebook/react-native#17968

I have fixed the above issue by updating the layout of the playerViewControl.

Also, I have created a working sample for "react-native-video with player control" in the below repo. I have tested the same. It's working as expected.
https://github.com/IbrahimSulai/ReactNativeVideoSample/

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Feb 4, 2019

@n1ru4l @cobarx - I have fixed the player control UI issue. Now everything is working fine as expected. Could you please help to review and merge this pull request.

@cobarx
Copy link
Collaborator

left a comment

When I test this on my test harness, the play / pause button is displaying properly. However the progress bar doesn't update the position as the video plays and when I press the pause button it doesn't pause the video.

You can download my test harness from:
https://github.com/cobarx/react-native-video-test

Thank you for finding a link and resolution to the layout bug. I have encountered it before but never knew how to fix it.

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Feb 5, 2019

@cobarx @n1ru4l

I have fixed the issue observed in the test harness.

Changed the execution order of initializePlayerControl method in order to align with the player state. Since the createView is executed before the "setControls" props method gets called. so the values of the playerControl are not aligned with the player state values.

Could you please review and merge the same.

cobarx added some commits Feb 11, 2019

@cobarx

cobarx approved these changes Feb 11, 2019

@cobarx

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2019

Very nice, this is working wonderfully for me in the test harness. I'm merging now. There are going to be a lot of happy people using this!

Adding the controls exposes a lot of new functionality that you could develop further (if you have the time and interest). If you want to do another PR, it would be great to have onPause & onPlay events so that the app can be notified that the play/pause state changed due a click within the controls. It might also be good to implement some kind of auto-hide behavior so the controls disappear after a certain amount of time, configurable by a prop.

Keep up the good work!

@cobarx cobarx merged commit e0fd69e into react-native-community:master Feb 11, 2019

@felipemillhouse

This comment has been minimized.

Copy link

commented Feb 11, 2019

Thank you guys! I was wanting this merge!

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

Very nice, this is working wonderfully for me in the test harness. I'm merging now. There are going to be a lot of happy people using this!

Adding the controls exposes a lot of new functionality that you could develop further (if you have the time and interest). If you want to do another PR, it would be great to have onPause & onPlay events so that the app can be notified that the play/pause state changed due a click within the controls. It might also be good to implement some kind of auto-hide behavior so the controls disappear after a certain amount of time, configurable by a prop.

Keep up the good work!

Thank you @cobarx and others in the community who supported for this feature to get merged.
Sure, I will be focusing on the other key features related to this.

@AndriiUhryn

This comment has been minimized.

Copy link

commented Feb 11, 2019

@IbrahimSulai Is there an ability to make video fullScreen on Android?

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

@IbrahimSulai Is there an ability to make video fullScreen on Android?

Will add the full-screen feature in upcoming PR.

@xstable

This comment has been minimized.

Copy link

commented Feb 26, 2019

@cobarx
Isn't that wrong... now after you've merged @IbrahimSulai commit... and as we can activate video-controls by a property now ?

As you don't have to create your own controls, but you are "able to do".
BTW: Because react-native-video-player is depending on react-native-video ^2.0.0 I would remove the suggestion to use it in the docs.

@xstable

This comment has been minimized.

Copy link

commented Feb 28, 2019

Because I hardly needed the fullscreen-functionality, I forked and upgraded react-native-video-player so that it now support react-native-video 4.4.0. Feel free to give it a try.

But I still looking forward to @IbrahimSulai Solution to have the whole functionality inside of react-native-video. So you can take my solution as temporary workaround, till @IbrahimSulai is done with it.

@shaunsantacruz

This comment has been minimized.

Copy link

commented Mar 14, 2019

Thank you so much for adding controls! Would it be possible to make them more accessible for dpad/keyboard users? My app is being rejected from the play store due to this.

@IbrahimSulai

This comment has been minimized.

Copy link
Author

commented Mar 15, 2019

Thank you so much for adding controls! Would it be possible to make them more accessible for dpad/keyboard users? My app is being rejected from the play store due to this.

Currently working on the full-screen functionality for the Android Exoplayer. Once completed, will check on this. Thanks

@datzun

This comment has been minimized.

Copy link

commented May 1, 2019

Thank you so much for adding controls! Would it be possible to make them more accessible for dpad/keyboard users? My app is being rejected from the play store due to this.

Currently working on the full-screen functionality for the Android Exoplayer. Once completed, will check on this. Thanks

@IbrahimSulai thanks for adding control support. Any updates on fullscreen functionality?

AndrewSouthpaw added a commit to flow-typed/flow-typed that referenced this pull request May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.