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

allow explodePlaylist to return an OPML object #627

Merged
merged 2 commits into from Jun 30, 2021

Conversation

philippe44
Copy link
Contributor

I'm not sure on this one: I was trying to improve a bit the YT plugin by allowing LMS' favorites to contain YT playlists and channels and let user browse through these rather than simply play all or nothing (today it's a "overridePlayback" method).

It seems that "explodePlaylist is what I want, but it only accepts an array of track's url instead of potentially a complete opml hash. If I add that option, I can have the YT plugin nicely add favorites, otherwise it looks crappy as when user clicks on the favorite link, he gets a list of "youtube://..." urls.

Now, I don't understand well, to say the least, this part of LMS and maybe this does not make sense

@michaelherger
Copy link
Member

explodePlaylist was created to be able to play (or add to the queue) an album or playlist from a single URL. Eg. Spotify would provide something like spotify:playlist:12321.... But LMS needs a list of tracks.

I therefore believe returning OPML isn't the way to go. The method is not supposed to return a menu structure, or some other form of data which can be navigated (beyond getting track information). The favorited item challenge probably needs to be fixed in the Favorites plugin. We do have some code to allow browsing LMS local content (artists, albums). But nothing for remote items.

@philippe44
Copy link
Contributor Author

philippe44 commented Jun 29, 2021

Yes I guess it is going to work here in XML.pm

        if ( $handler && $handler->can('explodePlaylist') ) {
		$handler->explodePlaylist($params->{client}, $url, sub {
			my ($tracks) = @_;

			# explode playlist might return a full opml list	
			return $cb->($tracks, $params) if ref $tracks eq 'HASH';

			# if not, this is just an array of url
			return $cb->({
				'type'  => 'opml',
				'title' => '',
				'items' => [
					map {
						{
							# compatable with INPUT.Choice, which expects 'name' and 'value'
							'name'  => $_,
							'value' => $_,
							'url'   => $_,
							'type'  => 'audio',
							'items' => [],
						}
					} @{$tracks || []}
				],
			}, $params);
		});

		return;
	}

But not so much in Command.pm

	if ( $handler && $handler->can('explodePlaylist') ) {
		$handler->explodePlaylist($client, $path, sub {
			my $tracks = shift;
			$client->execute(['playlist', $cmd . 'tracks' , 'listRef', $tracks, $fadeIn]);
			$request->setStatusDone();
		});

		return;
	}

Crap...

There is code to deal with that when it's a HTTP element, but not something with a PH

@philippe44
Copy link
Contributor Author

On the other end, I was thinking about it and explodePlaylist is called is where, I think, 'browse" explode should be done. This is what I observe for playlist in favorite from Deezer & Tidal => you can browse through a Tidal favorite playlist, not because explodePlayList is called, but because, at the same place in XML.pm, there is a direct call to "explode" the playlist from SN (the Tidal favorite playlist is a regular HTTP url, so the PH's explodePlaylist is ignored.

So I can easily change Command.pm as well to build the array from the opml hash in case explodePlaylist returns that.

It would mean the objective of explodePlaylist would be to list tracks from a playlist for either insertion in the active playlist or for browsing. Its model would be to return an array of track's url or an opml list. I can also add a parameter to say what the caller would prefer, but I'm not sure this has any benefit.

@philippe44
Copy link
Contributor Author

philippe44 commented Jun 29, 2021

Something like that 15117ea

Copy link
Member

@michaelherger michaelherger left a comment

Choose a reason for hiding this comment

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

Ok, what would the convention be for the returned OPML (I see the same use case as you have for YT for Spotty et al. too...). Would this need to be a { items => [{}, {}] } style object?

@@ -1424,6 +1424,8 @@ sub playlistXitemCommand {
if ( $handler && $handler->can('explodePlaylist') ) {
$handler->explodePlaylist($client, $path, sub {
my $tracks = shift;
# transform opml list into url array if needed
$tracks = [ map { $_->{play} || $_->{url} } @{$tracks->{items}} ] if ref $tracks eq 'HASH';
Copy link
Member

Choose a reason for hiding this comment

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

Please improve readability by using an if {} block and formatting the map onto multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, you don't like my one-liner habit ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Joke aside, I'm happy to do that, but the rest of the file as a few maps like

$info[0] = join(', ', map { $_->name } Slim::Schema->search('Genre', { 'id' => { 'in' => [ split(/,/, $genre_id) ] } })->all);

(was not me 😄)

Copy link
Member

Choose a reason for hiding this comment

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

Whoever did that didn't have any reviewer trying to understand what he did 😝.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sir, done 🤕

(I scanned through LMS to figure out the use, 90% are one-liner... many former assembly code developers like me)

@philippe44
Copy link
Contributor Author

Ok, what would the convention be for the returned OPML (I see the same use case as you have for YT for Spotty et al. too...). Would this need to be a { items => [{}, {}] } style object?

Yes, as a regular array reference we used in all opml list in plugin. It seems the most logical, no?

@mherger mherger merged commit 3d5fa12 into LMS-Community:public/8.2 Jun 30, 2021
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.

None yet

3 participants