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

Implemented playlists #460 #887 #1006

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

reactormonk
Copy link

@reactormonk reactormonk commented Jun 15, 2023

I think I got almost everything. Maybe I'll get to the chromecast part.

Implements #460 #887

Copy link
Owner

@PierfrancescoSoffritti PierfrancescoSoffritti left a comment

Choose a reason for hiding this comment

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

I did a first pass, will look more in details after comments are addressed.

YouTubePlayerBridge.sendPlaylistIndex(index);
}

function cuePlaylistObj(obj) {

Choose a reason for hiding this comment

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

Why do we need a separate method from cuePlaylist? I think we can do without this one.

Copy link
Author

Choose a reason for hiding this comment

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

They mirror the two ways you can cue playlists according to the API. https://developers.google.com/youtube/iframe_api_reference#Queueing_Functions_for_Playlists

@@ -166,5 +169,51 @@
player.toggleFullscreen();
}

function cuePlaylist(videos, index, startSeconds) {
player.cuePlaylist(videos, index, startSeconds);
YouTubePlayerBridge.sendVideoId(videos[index]);

Choose a reason for hiding this comment

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

Why are sendVideoId, sendVideoList and sendPlaylistIndex three separate methods? they seem to be called always together. I think we should have single method like updatePlaylist that takes multiple arguments.

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/PierfrancescoSoffritti/android-youtube-player/pull/1006/files can only handle a single string, so I wasn't sure if I should serialize the arguments & deserialize again. I figured this would be simpler.

YouTubePlayerBridge.sendPlaylistIndex(index);
}

function loadPlaylistObj(obj) {

Choose a reason for hiding this comment

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

Same as above, why do we need the obj method?

core/src/main/res/raw/ayp_youtube_player.html Show resolved Hide resolved
core/src/main/res/raw/ayp_youtube_player.html Show resolved Hide resolved
@@ -20,6 +20,23 @@ interface YouTubePlayer {
*/
fun cueVideo(videoId: String, startSeconds: Float)

fun loadPlaylist(videos: List<String>, index: Int, startSeconds: Float)

Choose a reason for hiding this comment

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

Please, improve existing documentation and add it where it's missing.

Describe what the method is for and all the params.

core/build.gradle Outdated Show resolved Hide resolved
core-sample-app/build.gradle Show resolved Hide resolved
chromecast-sender-sample-app/build.gradle Show resolved Hide resolved
chromecast-sender/build.gradle Show resolved Hide resolved
@PierfrancescoSoffritti
Copy link
Owner

Thank you for the PR, I have left a few comments.

@three678
Copy link

three678 commented Jul 9, 2023

When will this code be available in the library. Version 12.0.0. still not supporting this. Please add it sooner

@PierfrancescoSoffritti
Copy link
Owner

Hi, I was looking at this again, I was testing the cuePlaylist function and got a NumberFormatException.

I decide to implement basic playlist controls separately as I understand they are quite important.

I will need to come back to this pull request to understand exactly how the cue and load playlist methods fit in.

I am sorry for taking so long to get to this, between work and other stuff time is hard to find :)

@three678
Copy link

three678 commented Aug 6, 2023

Thank you, PierfrancescoSoffritti. We made a free Christian radio app that is not working now. Your library is the only chance to make it work again. Will be waiting, You're doing a good job.
kristijan

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.

3 participants