Song/track/handler for url refactoring attempt #612

philippe44 · 2021-06-06T20:53:40Z

I decided to give it a try. There is fair bit of changes, some are more cosmetics, I realize, but they help making the separation between $song and $track more clear as it seems that the concept has evolved with LMS. For example, in Slim;;Player::Playlist.pm it was the same object at the beginning.

I've probably made more harm than good but well, the idea was that if I try something, I'll go all the way. Reading the evolution of LMS accross time, I think that distinction between $handler and $songHandler was necessary a long while ago (7.3) but there was a time when new firmware came that seems to have made that not required.

The idea is to use handlerFromUrl only when there is no $song and the $song->currentTrackHandler otherwise. If, after a scanUrl call, the track is modified and the currentTrackHandler is re-set because scanUrl has really changed the url (like unwrapped it to a different PH) then it will change as of before otherwise it will continue to call the PH set at creation of $song. With a modification I've made in the podcast PR, the PH has the possibility to keep control of the track if it wants to as LMS asks if it can change PH after the scanUrl. Now, if you decide to go further with that direction, the podcats PR will be need to be merged.

Now, this clearly reaches the limit of having no test harness and so I have no idea how much I've broken things.

michaelherger

Wow... this looks almost too simple and clean to be true. I certainly missed a detail, but overall this looks like a very nice cleanup. I'd be happy to merge this to 8.2 and see what happens. Anything you wanted to add before I go ahead?

I should have another pass to see whether we're still using handlerFromUrl where we should no longer be (you didn't do this, did you?). Certainly for my plugins, too. But that can be done after a merge.

Slim/Player/Playlist.pm

philippe44 · 2021-06-08T02:21:54Z

Wow... this looks almost too simple and clean to be true. I certainly missed a detail, but overall this looks like a very nice cleanup. I'd be happy to merge this to 8.2 and see what happens. Anything you wanted to add before I go ahead?

I should have another pass to see whether we're still using handlerFromUrl where we should no longer be (you didn't do this, did you?). Certainly for my plugins, too. But that can be done after a merge.

Unfortunately, I tripped again and there was more than what I saw initially (surprise!). Both the songHandler and handler were needed because some methods need to exist even if the ancestor of the $song's PH is not a HTTP (or a RemoteStream).

For example, if a custom PH wants to autorise direct streaming and for that returns a plain HTTP url in canDirectStream but this custom PH is not a subclass of HTTP/RemoteStream, then such requestString does not exist in that PH, although it exists in S:P::P::HTTP, which is the PH for the true HTTP direct stream url. Now, if the custom PH wants to redefine requestString even for direct streaming, it should be able to (see mixcloud plugin). So the general rule is that the song's protocol handler is tried first and if it does not have the methods, then the url's one is tried.

At the end, this PR contains much less real changes than I was afraid of. I don't know if it's good or bad, but "la montagne a accouché d'une souris 😄". Most changes are really cosmetics and if you don't like them, we can remove these. I just thought it would help future developers as cosmetic is mainly for consistency (although there are a few cases where it was purely my preference...). For example, I've modified SongStreamController a lot, but there is nothing new. I was just surprised that it was not bases on an accessor and I could not see why it was manually doing the exact same thing an accessor does.

I also took the liberty to add a DEVELOPERS.txt file where I thought we could add some documentation on features we add that can be reused in plugins or can simply help future contributors.

Now, I think I need to continue looking at these changes as I feel there is more to review

philippe44 · 2021-06-08T02:30:16Z

Slim/Player/Squeezebox.pm

-		if ( !$format && $handler->can("getFormatForURL") ) {
-			$format = $handler->getFormatForURL($url);
-		}
+		my $methodHandler = $songHandler->can('getFormatForURL') ? $songHandler : $handler;


We need to give priority to $song's PH (that can always default to url's handler if needs to)

philippe44 · 2021-06-08T02:32:02Z

Slim/Player/Squeezebox.pm

@@ -773,7 +772,8 @@ sub stream_s {

 		main::INFOLOG && logger('player.streaming.direct')->info("SqueezePlay direct stream: $url");

-		$request_string = $songHandler->requestString($client, $url, undef, $params->{'seekdata'});


If the custom PH allowed a directs stream and is not a subclass of HTTP, then it does not have a requestString method and that would crash if we only use songHandler. But, that PH might also want to overload requestString, so try it first

philippe44 · 2021-06-08T02:32:35Z

Slim/Player/Squeezebox2.pm

@@ -458,19 +458,17 @@ sub songElapsedSeconds {
 }

 sub canDirectStream {


Mostly cosmetic

philippe44 · 2021-06-08T02:35:09Z

Slim/Player/SongStreamController.pm

@@ -1,6 +1,5 @@
 package Slim::Player::SongStreamController;


Mostly cosmetic to make it inherit from accessor. Also, I've changed protocolHandler to urlHandler so that we have consistent naming between songHandler (for $song), streamHandler (for $sock) and urlHandler (for $song->streamUrl)

This is breakdown of LMS-Community#612 in a purely cosmetic part and a part than contains the handlers modification

This is breakdown of #612 in a purely cosmetic part and a part than contains the handlers modification

They are both needed (surprise...) when PH's don't descend from HTTP. Now, the requestString needed to be updated

This is un-necesseary as the objective was to call SUPER::canDirectStream. Now that we have a S::P::P::HTTP(S)::canDirectStreamSong, it will take care of that

philippe44 · 2021-06-14T01:39:25Z

ok, with the #611 PR, I think it's ready to see what will be user's feedback. Note that I've now changed a few PH to remove CanDirectStreamSong as it was just transferring control to ancestor's canDirectStream. Now that we have a canDirectStreamSong in their ancestors, it is no longer necessary but should help making things more clear for future PH developers, I hope

michaelherger

Let's get this done. I've added a few comments. But these can be addressed after the fact.

DEVELOPERS.txt

michaelherger · 2021-06-14T07:17:33Z

DEVELOPERS.txt

+When a $url from an OPML item is added to current playlist, a $track object is created 
+and its image is obtained by calling handlerForUrl($url)->getMetadataFor($url). 
+If this protocol handler is HTTP, it needs to find the image just by using this $url, 
+there is no other information. 


This part is not limited to OPML items, is it? That would apply to any track added/played, wouldn't it? The OPML special sauce is below mentioned calling of setRemoteMetadata.

Probably, but this part of LMS is really a big unkown to me

DEVELOPERS.txt

michaelherger · 2021-06-16T04:06:09Z

Slim/Player/Protocols/HTTP.pm

+	my $current = $song->currentTrack->url eq $url if $song;
+
+	if ( $current ) {
 		if ( my $meta = $song->pluginData('wmaMeta') ) {


I have a plugin which uses wmaMeta to override the metadata shown. This no longer work, as $current is not true. Where shall I start the investigation?

I've updated the PR with what I think is the solution: need to check for the track and currentTrack as well

Oh, the new one, of course.

michaelherger approved these changes Jun 7, 2021

View reviewed changes

Slim/Player/Playlist.pm Outdated Show resolved Hide resolved

philippe44 commented Jun 8, 2021

View reviewed changes

Slim/Player/Squeezebox2.pm

@@ -458,19 +458,17 @@ sub songElapsedSeconds {

}

sub canDirectStream {

Copy link

Contributor Author

philippe44 Jun 8, 2021

Choose a reason for hiding this comment

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

Mostly cosmetic

philippe44 commented Jun 8, 2021

View reviewed changes

philippe44 mentioned this pull request Jun 8, 2021

improve podcast + fix volatile url in scan #611

Merged

philippe44 added a commit to philippe44/slimserver that referenced this pull request Jun 8, 2021

change song to track when appropriate

8a446ba

This is breakdown of LMS-Community#612 in a purely cosmetic part and a part than contains the handlers modification

philippe44 mentioned this pull request Jun 8, 2021

change song to track when appropriate (cosmetic PR) #613

Closed

mherger pushed a commit that referenced this pull request Jun 9, 2021

change song to track when appropriate

33e55ab

This is breakdown of #612 in a purely cosmetic part and a part than contains the handlers modification

philippe44 added 6 commits June 9, 2021 11:23

attempt to refactor handlerForUrl & song vs track naming

7b30413

fix open items

b0b071a

move back to songHandler & urlHandler

828675e

They are both needed (surprise...) when PH's don't descend from HTTP. Now, the requestString needed to be updated

use $song in getMetadataFor only for current song

1f58ad6

undo cosmetic changes

749dfca

use songhandler core, not track

cb1cc4e

philippe44 force-pushed the song/track/handlerForUrl-refactoring-attempt branch from 8b3f803 to cb1cc4e Compare June 9, 2021 18:31

philippe44 added 5 commits June 9, 2021 11:37

fixes

68b9cd3

use currentTrack when possible

b647d71

oversight: streamUrl must be used in canDirectStreamSong

016847d

a few more

8f2184f

remove canDirectStreamSong in PH

d6267c1

This is un-necesseary as the objective was to call SUPER::canDirectStream. Now that we have a S::P::P::HTTP(S)::canDirectStreamSong, it will take care of that

adding some random comments

853c4cb

michaelherger approved these changes Jun 14, 2021

View reviewed changes

mherger merged commit a6f1371 into LMS-Community:public/8.2 Jun 14, 2021

michaelherger reviewed Jun 16, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Song/track/handler for url refactoring attempt #612

Song/track/handler for url refactoring attempt #612

philippe44 commented Jun 6, 2021

michaelherger left a comment

philippe44 commented Jun 8, 2021 •

edited

philippe44 Jun 8, 2021 •

edited

philippe44 Jun 8, 2021

philippe44 Jun 8, 2021

philippe44 Jun 8, 2021

philippe44 commented Jun 14, 2021 •

edited

michaelherger left a comment

michaelherger Jun 14, 2021

philippe44 Jun 14, 2021

michaelherger Jun 16, 2021

philippe44 Jun 16, 2021

michaelherger Jun 16, 2021

michaelherger Jun 16, 2021

		@@ -773,7 +772,8 @@ sub stream_s {

		main::INFOLOG && logger('player.streaming.direct')->info("SqueezePlay direct stream: $url");

		$request_string = $songHandler->requestString($client, $url, undef, $params->{'seekdata'});

		@@ -458,19 +458,17 @@ sub songElapsedSeconds {
		}

		sub canDirectStream {

Song/track/handler for url refactoring attempt #612

Song/track/handler for url refactoring attempt #612

Conversation

philippe44 commented Jun 6, 2021

michaelherger left a comment

Choose a reason for hiding this comment

philippe44 commented Jun 8, 2021 • edited

philippe44 Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippe44 commented Jun 14, 2021 • edited

michaelherger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippe44 commented Jun 8, 2021 •

edited

philippe44 Jun 8, 2021 •

edited

philippe44 commented Jun 14, 2021 •

edited