-
-
Notifications
You must be signed in to change notification settings - Fork 758
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
Add fullScreen function in IFramePlayerOptions for set fs value #926
Add fullScreen function in IFramePlayerOptions for set fs value #926
Conversation
Add Genius to the apps that are using the library
Hi, thanks for the pull request. Can you explain why this is needed? |
Hi, this function need to display full screen button with normal youtube control. Default value is set to 0. |
So you want to add this function to be able to see the full screen button? as far as i know the button doesn't work on Android. Did you verify that? |
Yes, you are correct. But still if user want to have that option to display button. |
Interesting, i wasn't aware of |
Sure, i will create one sample app. Thanks for quick feedback 🙂 |
By looking at the sample Activity, it seems like you added a button to the xml, which when clicked changes the orientation of the Activity. It doesn't seem to be related to the full screen button in the IFrame player? |
override onShowCustomView and onHideCustomView for webchromeclient
In latest commit, i remove the button. In first example, i try to showcase masking example. And in second example, i try to showcase state handle example. In second example, IFrame player full screen button icon change in landscape mode as per expectation. |
Hi @PierfrancescoSoffritti , waiting for your feedback. Please let me know if above solution and suggestion looks good or not. |
Hi @PierfrancescoSoffritti, please update. Waiting for your response. |
Hi thanks for the ping, I will try to have look before the end of the week. |
@@ -14,7 +15,7 @@ interface YouTubePlayerListener { | |||
* Called every time the state of the player changes. Check [PlayerConstants.PlayerState] to see all the possible states. | |||
* @param state a state from [PlayerConstants.PlayerState] | |||
*/ | |||
fun onStateChange(youTubePlayer: YouTubePlayer, state: PlayerConstants.PlayerState) | |||
fun onStateChange(youTubePlayer: YouTubePlayer, state: PlayerConstants.PlayerState, view: View? = null) |
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.
I find this confusing, why do we need to add a nullable view?
This is problematic for a few reasons:
- Is a breaking change. Every user of the library will have to change their implementation of
onStateChange
- The API is not clear
Maybe this should be a separate method instead?
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.
Changes are made as per suggestion:
- Revert code for
onStateChange
method definition, removedview
parameter - Create separate method for handle callback of
onShowCustomView
andonHideCustomView
@@ -3,7 +3,7 @@ package com.pierfrancescosoffritti.androidyoutubeplayer.core.player | |||
class PlayerConstants { | |||
|
|||
enum class PlayerState { | |||
UNKNOWN, UNSTARTED, ENDED, PLAYING, PAUSED, BUFFERING, VIDEO_CUED | |||
UNKNOWN, UNSTARTED, ENDED, PLAYING, PAUSED, BUFFERING, VIDEO_CUED, SHOW_CUSTOM_VIEW, HIDE_CUSTOM_VIEW |
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.
These two constants are not related to the state of the player, please move them somewhere else
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.
Changes are made as per suggestion
- Remove
SHOW_CUSTOM_VIEW
andHIDE_CUSTOM_VIEW
from PlayerState - Update
SimpleFullscreenExampleActivity
code accordingly.
1. Revert code for onStateChange method defination, remove view from parameter 2. Remove SHOW_CUSTOM_VIEW and HIDE_CUSTOM_VIEW from PlayerState 3. Create seperate method for handle callback of onShowCustomView and onHideCustomView Also update SimpleFullscreenExampleActivity code accordingly
Thanks for updating the change. It is a lot more clear now. Can you please send it to the I will play with it over the weekend and maybe make some adjustments. Should be merged by Monday :) |
I've been thinking about this change. As it is it will make things more complicated, because the library already has logic to handle full screen at the view level (instead of webview level). I will need to make more explicit that the current full screen logic is at the view level and will just make the YouTubePlayerView fill its parent. After that is done we can integrate this change. So it will take a little longer. I'll keep you posted. In the meantime please send the pull request to the |
* Update readme (PierfrancescoSoffritti#875) Add Genius to the apps that are using the library * added mute option * added mute option * changed wording of new method descriptions * removed link in README.md Co-authored-by: Pierfrancesco Soffritti <[email protected]>
Add FAQ section to README with workarounds
The existing methods to handle full screen mode in `YouTubePlayerView` only change the view’s `LayoutParams` to fill its parent or wrap its content. The naming is misleading because they don’t change the state of the IFrame Player. This change is in preparation of future integration with the full screen state of the IFrame Player. List of main changes in this commit (these are also breaking changes): * Replace YouTubePlayerView#enterFullScreen with YouTubePlayerView#matchParent * Replace YouTubePlayerView#existFullScreen with YouTubePlayerView#wrapContent * Remove YouTubePlayerView#toggleFullScreen * Remove YouTubePlayerView#isFullScreen * Remove `YouTubePlayerFullScreenListener` interface
Add Genius to the apps that are using the library
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.
lgtm
Merged. Thanks @Praveen-Pable for the help! |
Thanks @PierfrancescoSoffritti for guiding me throughout the process and for valuable feedback. 🙂👍 |
Added one method
fullScreen
inIFramePlayerOptions
class for set value of fs parameter.