Remote service enhancement

[darrell-k]

Firstly, this incorporates the changes in @michaelherger 's SlimBrowse-Metadata branch. As discussed on the forum, this is work in progress and only directed towards Material Skin at present. Qobuz is used as the example remote service.

The matching Material and Qobuz changes are here:
https://github.com/darrell-k/lms-material/tree/remote-service-enhancement
https://github.com/darrell-k/SqueezeboxQobuz/tree/remote-service-enhancement

This PR is for review/comments on the approach. There is much commented out code and the new code could certainly be refactored once we have the functionality sorted out. I've also added hopefully useful comments in the code here and there.

Objective

• Use the native db interface for remote albums when browsed from the plugin, even when they are not imported into the local library.
• Ensure full play queue hyperlink and MAI functionality for these albums.

Design

• Based around a new Protocol Handler getAlbumTracks in the remote service plugin, which uses the existing plugin Importer logic to return data to LMS in the required format.
• LMS will supply the client with the remote service name and remote track numbers will be identified by being made negative. (This works for Qobuz but will need to be rethought if other remote services use non-numeric track numbers!) The client can then amend subsequent LMS requests based on these values.

Current State

• There are bugs, including multi-delete from the play queue (eg "remove release") not yet working and probably many more.
• Can't yet seem to get an image for a remote-only artist to show in the MAI view.
• Remote service "extras" (eg pdf booklet, add/remove remote service favourites) not yet implemented. I want these to be present equally whether the album is browsed via library search/browse or remote service search/browse.
• Much more to be done: default/classic skins, and the big one: ensuring backwards compatibility with whatever combinations of LMS/Material/Remote Service plugin the user is using.

@michaelherger / @CDrummond and anyone else, please review/criticise/suggest design and other changes.

[michaelherger]

I'm a bit confused about this. Bear in mind that my brain might have forgotten a lot of what we've discussed - and what I've implemented in my own suggestion.... Maybe we should have a working document which describes the goals of the change etc.? Or split things up in smaller chunks of work to keep an overview? We could even start a project to organise goals and work.

What my poor memory is: we enrich plugin integrations to not only return full menu items as expected by most of the "old" UIs and players, but additional metadata which would give Material (and potentially other UIs) more context. I think that was my approach.

In this PR you define a getAlbumTracks() hook - which is something completely different. How would that be called? There are new parameters introduced to existing queries. But wouldn't the extid cover both the service item's and the service itself? Did you plan to have more new hooks like getAlbumArtist(), getArtistAlbums() and what not? And what about services which don't work on an artist/album/track level, like podcasts, radio stations? They would be left behind, wouldn't they? And what's the advantage of a more complex tracks request over a tracks list as before, which comes with the information needed to get further information?

I understand this might become one of the biggest and most important changes in a long time. Therefore I believe it would be worth laying out the plan before you spend even more time implementing things. The bigger the PR will be, the harder it will be to get it right. It would be preferable to have smaller, more manageable changes. We could have a new integration branch where we collect the results of these smaller PRs, and which itself eventually would be merged to the main branch. But that's hard to do if the plan only exist in one head.

[darrell-k]

Maybe we should have a working document which describes the goals of the change etc.? Or split things up in smaller chunks of work to keep an overview? We could even start a project to organise goals and work.

Good idea. The goal of the change I thought I'd already described in the Objective paragraph above. Maybe it needs expansion...

What my poor memory is: we enrich plugin integrations to not only return full menu items as expected by most of the "old" UIs and players, but additional metadata which would give Material (and potentially other UIs) more context. I think that was my approach.

In this PR you define a getAlbumTracks() hook - which is something completely different. How would that be called?

Yes it is different, but it is built on top of your metadata enhancements. Rather than just provide the extra track metadata in a SlimBrowse items_loop response, this approach starts with the client identifying that a remote service album is being requested from a service that implements these changes. In that case, the client requests the album using the local library command instead.

eg in Material Skin, if item.metadata (from your changes) contains remoteAlbumId it changes the type from online album to library album:

  if (stdItem==STD_ITEM_ONLINE_ALBUM && item.metadata && item.metadata.remoteAlbumId) {
       stdItem = STD_ITEM_ALBUM;
   }

This results in Material requesting

["d0:df:9a:97:f3:c8",["tracks",0,25000,"tags:distbhz1kyuAACGPSE","sort:tracknum","qobuz.2.1.0","remote_album_id:fp2ifmf3f0vlb","service:qobuz","library_id:-1"]]

instead of

["6c:94:66:57:09:9f",["qobuz","items",0,25000,"menu:qobuz","item_id:9.2.1.0","isContextMenu:1","tags:aAbcCKWdegGiJkKloPqrRStTuEyY4"]]

When Slim::Control::Queries::titlesQuery receives this request, if the album is not in the local library (_getTagDataForTracks will first try to get the album using albums.extid), the new getAlbumTracks() routine, if implemented by the remote service plugin, will be used to return the data to titlesQuery in the expected format.

There are new parameters introduced to existing queries. But wouldn't the extid cover both the service item's and the service itself?

Quite possibly. I may have introduced the onlineService parameter to make things easier in the html templates/javascript for Default and Classic skins (I have semi-working changes for those, which I'm keeping back for now). I can't really remember at the moment.

Did you plan to have more new hooks like getAlbumArtist(), getArtistAlbums() and what not?

For artists, you will see there is code in here that, for example when an artist link with a remote service id (ie less than zero) in the play queue is clicked, the eg Qobuz plugin will try to find them in the local library first.

For albums browsed/searched for on the remote service, at present the remote service (SlimBrowse items_loop) response is still used - this is something we could think about.

And what about services which don't work on an artist/album/track level, like podcasts, radio stations? They would be left behind, wouldn't they?

For now yes. This is so far concentrating on album track listings.

And what's the advantage of a more complex tracks request over a tracks list as before, which comes with the information needed to get further information?

It means that the client doesn't need to implement separate logic for track listings of not imported albums, hopefully cutting down on maintenance overhead and ensuring consistency.

I understand this might become one of the biggest and most important changes in a long time. Therefore I believe it would be worth laying out the plan before you spend even more time implementing things. The bigger the PR will be, the harder it will be to get it right. It would be preferable to have smaller, more manageable changes. We could have a new integration branch where we collect the results of these smaller PRs, and which itself eventually would be merged to the main branch. But that's hard to do if the plan only exist in one head.

I've already cut this down from my original branch, which tries to implement for Default and Classic skins as well. Maybe we could separate further into:

• SlimBrowse metadata changes (your original work)
• Track listings
• Play Queue changes

though obviously they would be dependent on each other.

As to a higher level plan that we can collectively discuss and enhance, that would be good. I'll start to work on something that maybe expands on the objectives in end-user language, though I'm better at describing things in code!

I think you'd get a better idea about all this by installing/running it.

[darrell-k]

@michaelherger I've merged the latest public/9.1 into your SlimBrowse-Metadata branch on my fork, and then created a PR there which would pull in the changes from this PR, so you can see what I've actually changed. It looks a lot less scary!
darrell-k#19

[michaelherger]

Thanks! I'm still wondering whether what you're working on is something useful: we already can drill down to tracks. Why provide another way to do it?

We really need a document describing what challenges we're trying to tackle, otherwise I fear we end up with a lot of unnecessary code and/or changes. @CDrummond - would you mind outlining what you are looking for in a document? Not in a discussion thread where details get lost...

[darrell-k]

Thanks! I'm still wondering whether what you're working on is something useful: we already can drill down to tracks. Why provide another way to do it?

We really need a document describing what challenges we're trying to tackle, otherwise I fear we end up with a lot of unnecessary code and/or changes. @CDrummond - would you mind outlining what you are looking for in a document? Not in a discussion thread where details get lost...

As I said, the reason is in order to provide the tracks to the client in exactly the same format regardless of the source (library or remote service, imported or not) and the method (via library browse or plugin browse), so that the client does not need separate logic/different processing and the user is guaranteed a consistent experience.

It seems to me that this approach simplifies things. I did try returning a titles_loop rather than an items_loop while continuing to go down the SlimBrowse route, but as you can imagine, the complications of XMLBrowser made this very messy. It turned out to be much easier to have the client make the tracks request using the standard tracks command in all cases.

And if we continued to go down the SlimBrowse route, ensuring consistent behaviour in the play queue could turn out to be more difficult.

This change is combining the building blocks we already have in different way, particularly using the existing track import functionality to render tracks which haven't been imported to the library.

[michaelherger]

As I said, the reason is in order to provide the tracks to the client in exactly the same format regardless of the source (library or remote service, imported or not) and the method (via library browse or plugin browse), so that the client does not need separate logic/different processing and the user is guaranteed a consistent experience.

I know - but I wanted to hear from @CDrummond what he, as the frontend developer, was expecting. Eg. how do we even get to calling LMS this way? What's the navigation path, the data you receive before requesting this track list? Wouldn't the enhanced metadata as I suggested already ship all the information your implementation would return?

I fear that with your addition you'll just add one more way of returning data to what we have already. The plugin developers would have to implement one more "protocol" of returning the same data. Just the addition of new parameters which IMHO would already be covered by other parameters tells me that we're adding complexity instead of understanding the code we already have in place.

And if we continued to go down the SlimBrowse route, ensuring consistent behaviour in the play queue could turn out to be more difficult.

The frontend will still have to be aware of the type of content it's dealing with, right? How would it decide to use the album request vs. SlimBrowse? There will always be plugins which don't support the new way.

This change is combining the building blocks we already have in different way, particularly using the existing track import functionality to render tracks which haven't been imported to the library.

That OTOH sounds great!

[CDrummond]

@michaelherger I think the existing change to provide more metadata is sufficient when browsing using that service's Apps entry - however, not being a user of those services its very likely I'll have missed something. From a purely Material perspective, I do like @darrell-k 's suggested changes - as an artist is an artist, why should I care if its from a service? But I agree that that's putting more work onto the service plugins. So, the decision on which way to go there is for the author's of those plugins - as its unfair for me to expect them to do all the work.

However, I do think other parts of the integration need to be improved - pints 2 & 3 from https://forums.lyrion.org/forum/developer-forums/developers/1771548-improving-online-service-integration

[darrell-k]

As I said, the reason is in order to provide the tracks to the client in exactly the same format regardless of the source (library or remote service, imported or not) and the method (via library browse or plugin browse), so that the client does not need separate logic/different processing and the user is guaranteed a consistent experience.

...how do we even get to calling LMS this way? What's the navigation path, the data you receive before requesting this track list?

I outlined this above: sending the remote album id the SlimBrowse albums list response allows the client to switch out of Slimbrowse mode.

Wouldn't the enhanced metadata as I suggested already ship all the information your implementation would return?

Potentially, though we might need add more possible elements. But the problem I'm trying to solve here is the format of the returned data. This change guarantees that the format is the same as a local/imported album.

I fear that with your addition you'll just add one more way of returning data to what we have already. The plugin developers would have to implement one more "protocol" of returning the same data. Just the addition of new parameters which IMHO would already be covered by other parameters tells me that we're adding complexity instead of understanding the code we already have in place.

No, I'm not adding another way of returning data. I'm using the same way that we already use when browsing a remote album that has been imported into the library.

The new protocol handler code is simply the plumbing that leverages existing plugin importer code, allowing it to be used interactively by the titles query (ie tracks command) as well as in batch during the scan.

I spent a long tome understanding the code we already have in place, and also trying out different approaches. Don't get hung up on extra parameters: the client logic could probably be implemented using existing parameters but it is IMO clearer to explicitly distinguish between imported and un-imported albums in the parameters.

And if we continued to go down the SlimBrowse route, ensuring consistent behaviour in the play queue could turn out to be more difficult.

The frontend will still have to be aware of the type of content it's dealing with, right? How would it decide to use the album request vs. SlimBrowse? There will always be plugins which don't support the new way.

I've explained above how it decides. Yes, plugins that don't implement this will continue to receive simple SlimBrowse lists.

If the album name is hyperlinked in the play queue, it will always use the tracks request. Currently, the hyperlinks in the play queue are not created when browsing from the plugin Slimbrowse listings.

[darrell-k]

@michaelherger I think the existing change to provide more metadata is sufficient when browsing using that service's Apps entry - however, not being a user of those services its very likely I'll have missed something. From a purely Material perspective, I do like @darrell-k 's suggested changes - as an artist is an artist, why should I care if its from a service? But I agree that that's putting more work onto the service plugins. So, the decision on which way to go there is for the author's of those plugins - as its unfair for me to expect them to do all the work.

Either way, plugin changes are required, either to add the extra metadata to the Slimbrowse response or to implement the new protocol handler routine which would allow the plugin's existing Import code to be used to return the metadata to the "native" titles query (tracks command).

I actually think the new handler approach is cleaner and duplicates less code: adding all the required metadata to the Slimbrowse response is just one more place than would need to be kept in line with evolving client metadata requirements.

The approach here uses the SlimBrowse metatdata enhancements, but in a limited way: to identify the type of Slimbrowse response to the client and to supply the key (eg remoteAlbumId) required for subsequent requests.

However, I do think other parts of the integration need to be improved - pints 2 & 3 from https://forums.lyrion.org/forum/developer-forums/developers/1771548-improving-online-service-integration

Point 3 may already be handled in the play queue changes here.

[michaelherger]

Please provide a document describing the change with all details - incl. how Material will figure out whether it can use the regular tracks query for an album or not. And what other queries would require a similar treatment (playlists? works? artists?). A discussion thread is just too hard to follow and not forget things. With a document we can go over the specification until even I get it, and it'll still be all in the same place.

Feel free to choose between markdown here or something like G doc.

Thanks!

[darrell-k]

Haven't forgotten this: real life is interfering at the moment!