michaelherger
commented
2 years ago Is this for the browseonlineartist query only?
Open CDrummond opened 2 years ago
michaelherger
commented
2 years ago Is this for the browseonlineartist query only?
CDrummond
commented
2 years ago Well, I guess it could happen with other other queries if LMS assumes a persistent connection. However, I've only had reports about browseonlineartist.
If you mean which level on browseonlineartist - then Material calls "browseonlineartist","items",0,25000,"service_id:SERVICE","artist_id:ARTISTID","menu:1" to get the applicable list for an artist on a service. In the response to this, the actions do not have artist_id:ARTISTID - which, I think, is where the issue arises. Also, this seems to be for Qobuz, not sure about others.
As stated Material could add artist_id:ARTISTID, as it knows the ID (as its used to get the list itself). I'm just not sure if this is what should happen, or if LMS should already have this.
SamInPgh
commented
2 years ago Is there agreement that this problem needs to be fixed in LMS? Specifically that the action list returned from the initial "browseonlineartist" request should include "artist_id:ARTISTID" in its list of required parameters? This is a major problem for those of us using Qobuz with Material Skin, although it can be worked around by using the Qobuz plugin directly.
michaelherger
commented
2 years ago I'm sorry, that comment about agreement made me laugh 😁: even if all of you agree, somebody has to do the work. If I can't free time to work on this, and nobody else is stepping up, it won't happen.
So maybe you can be of more help if you provided easy to follow step-by-step instructions how to reproduce this issue? Thanks!
michaelherger
commented
2 years ago I looked at the code without even knowing how to reproduce. But unfortunately it's based on some of the most convoluted code in LMS... I'm not sure how easy it'll be to implement this...
SamInPgh
commented
2 years ago The problem is easily reproducable if you have a Qobuz subscription and Qobuz favorites integrated with your music library, and use the Material Skin client.
Select a favorited Qobuz artist from the My Music artist list to get a list of the artist's albums.
Select 'Qobuz' from the dropdown list generated by selecting the circle icon containing three dots, just to the right of the artist name.
From the resulting menu, choose any of the 5 items, such as Albums.
Nothing is returned, and the debug log shows the following message:
[22-08-22 13:34:42.0761] Slim::Web::JSONRPC::requestMethod (477) Request failed with error: Bad params!
michaelherger
commented
2 years ago Hmm... that's working as expected here. I not only get the albums, but I can also play them. What am I missing?
{
"params": [
"00:04:20:22:01:0c",
[
"browseonlineartist",
"items",
"0",
25000,
"menu:browseonlineartist",
"item_id:0"
]
],
"id": 8,
"result": {
"title": "Alben",
"window": {
"windowStyle": "icon_list"
},
"count": 20,
"item_loop": [
{
"text": "Alors On Danse\nStromae",
"params": {
"item_id": "0.0",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F24%2F25%2F0060252752524_600.jpg/image.jpg"
},
{
"text": "Cheese\nStromae",
"params": {
"isContextMenu": 1,
"item_id": "0.1"
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F25%2F71%2F0060075327125_600.jpg/image.jpg"
},
{
"type": "playlist",
"params": {
"isContextMenu": 1,
"item_id": "0.2"
},
"text": "Formidable\nStromae",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F58%2F37%2F0060253763758_600.jpg/image.jpg"
},
{
"type": "playlist",
"text": "Multitude\nStromae",
"params": {
"item_id": "0.3",
"isContextMenu": 1
},
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F4b%2Fd1%2Fwv5ey906nd14b_600.jpg/image.jpg"
},
{
"params": {
"item_id": "0.4",
"isContextMenu": 1
},
"text": "Multitude\nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fpb%2F2d%2Ff13hljeed2dpb_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F32%2F01%2F0060253760132_600.jpg/image.jpg",
"params": {
"item_id": "0.5",
"isContextMenu": 1
},
"text": "papaoutai\nStromae",
"type": "playlist"
},
{
"text": "Peace Or Violence Remixes\nStromae",
"params": {
"item_id": "0.6",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F39%2F42%2F0060252774239_600.jpg/image.jpg"
},
{
"params": {
"isContextMenu": 1,
"item_id": "0.7"
},
"text": "Racine carrée\nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F60%2F14%2F0060254701460_600.jpg/image.jpg"
},
{
"text": "Alors On Danse\nStromae",
"params": {
"item_id": "0.8",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F40%2F56%2F0060075325640_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fmb%2Fiy%2Fzk3tza7o8iymb_600.jpg/image.jpg",
"type": "playlist",
"params": {
"isContextMenu": 1,
"item_id": "0.9"
},
"text": "Alors On Danse\nStromae"
},
{
"params": {
"item_id": "0.10",
"isContextMenu": 1
},
"text": "Alors On Danse \nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fha%2F8p%2Fjp76yze4p8pha_600.jpg/image.jpg"
},
{
"type": "playlist",
"text": "Défiler\nStromae",
"params": {
"isContextMenu": 1,
"item_id": "0.11"
},
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fhc%2Fg5%2Fm6ewryerfg5hc_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F79%2F43%2F0060253744379_600.jpg/image.jpg",
"params": {
"item_id": "0.12",
"isContextMenu": 1
},
"text": "Formidable\nStromae",
"type": "playlist"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fwc%2Fxe%2Fcoladm6v2xewc_600.jpg/image.jpg",
"type": "playlist",
"text": "L’enfer\nStromae",
"params": {
"item_id": "0.13",
"isContextMenu": 1
}
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fab%2F0y%2Fn26tf8i8q0yab_600.jpg/image.jpg",
"type": "playlist",
"params": {
"item_id": "0.14",
"isContextMenu": 1
},
"text": "Mon amour\nStromae"
},
{
"text": "Papaoutai\nStromae",
"params": {
"item_id": "0.15",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F83%2F11%2F0060253741183_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fab%2Fh5%2Flfmxf3bhfh5ab_600.jpg/image.jpg",
"type": "playlist",
"params": {
"isContextMenu": 1,
"item_id": "0.16"
},
"text": "Santé\nStromae"
},
{
"type": "playlist",
"params": {
"item_id": "0.17",
"isContextMenu": 1
},
"text": "Te Quiero\nStromae",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F49%2F67%2F0060252746749_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F75%2F49%2F0060075324975_600.jpg/image.jpg",
"type": "playlist",
"text": "Up Saw Liz\nStromae",
"params": {
"isContextMenu": 1,
"item_id": "0.18"
}
},
{
"params": {
"item_id": "0.19",
"isContextMenu": 1
},
"text": "Up Saw Liz \nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F74%2F49%2F0060075324974_600.jpg/image.jpg"
}
],
"base": {
"actions": {
"go": {
"itemsParams": "params",
"params": {
"menu": "browseonlineartist"
},
"cmd": [
"browseonlineartist",
"items"
]
},
"play": {
"nextWindow": "nowPlaying",
"params": {
"menu": "browseonlineartist"
},
"cmd": [
"browseonlineartist",
"playlist",
"play"
],
"itemsParams": "params",
"player": 0
},
"playControl": {
"params": {
"artist_id": "3785",
"item_id": "0",
"passthrough": [
{
"artist_id": "3785"
}
],
"menu": "browseonlineartist",
"_quantity": "25000",
"_index": "0"
},
"cmd": [
"browseonlineartist",
"items"
],
"itemsParams": "playControlParams",
"player": 0,
"window": {
"isContextMenu": 1
}
},
"add": {
"itemsParams": "params",
"params": {
"menu": "browseonlineartist"
},
"cmd": [
"browseonlineartist",
"playlist",
"add"
],
"player": 0
},
"add-hold": {
"cmd": [
"browseonlineartist",
"playlist",
"insert"
],
"params": {
"menu": "browseonlineartist"
},
"itemsParams": "params",
"player": 0
},
"more": {
"params": {
"menu": "browseonlineartist"
},
"itemsParams": "params",
"cmd": [
"browseonlineartist",
"items"
],
"player": 0,
"window": {
"isContextMenu": 1
}
}
}
},
"offset": 0
},
"method": "slim.request"
}@CDrummond - I'm sorry I don't always follow up on all postings on the forum. But has this issue been widely reported? I'm using master (commit 83d9756b). As you can see things are working despite the missing artist_id.
SamInPgh
commented
2 years ago Just to clarify, on my system it fails only on the first request for the album list for a given artist, where nothing is returned and the error message is logged. If I then back out to the artist level and come back in a second time via the 'Qobuz" icon, the requested album list is displayed and the albums are playable despite the missing artist_id, as you observed. Additionally if, after that, another artist is selected and a request made to display that artist's albums, either the problem happens again OR the albums for the previous artist are displayed instead. Again, that can be worked around by backing out and re-requesting. The problem is very easy to recreate on my system, which is the current (Aug 22) nightly build of LMS 8.3 and the current released (master) version of Material Skin (v2.10.3). I know of only one other user who has reported the same behavior on the MS forum but I think that might be because there aren't that many Qobuz users active there.
michaelherger
commented
2 years ago Hmm... different computer, same code, different behaviour. Now I'm seeing what you're describing. Would you be able to apply the following change and test again?
diff --git a/Slim/Plugin/OnlineLibrary/BrowseArtist.pm b/Slim/Plugin/OnlineLibrary/BrowseArtist.pm
index 71dc54799..7956c4d37 100644
--- a/Slim/Plugin/OnlineLibrary/BrowseArtist.pm
+++ b/Slim/Plugin/OnlineLibrary/BrowseArtist.pm
@@ -94,7 +94,7 @@ sub cliQuery {
my $client = $request->client;
my $artist_id = $request->getParam('artist_id');
my $service_id = $request->getParam('service_id');
- my $connectionId = $request->connectionID || '';
+ my $connectionId = ($request->connectionID && $request->connectionID->clid) || $request->clientid || '';
my $feed;
if ($service_id && $artist_id) {@michaelherger - first of all, no need to appologise for anything, you do a massive amount of work for/on LMS
But, I fail to see how it can reliably work without a persistent connection (I guess the connectionID above). e.g. One user is browsing these menus with Material Skin on a desktop for Artist A, another at the same time is browsing Artist B. Seeing as Material has no persistent connection (each JSONRPC call is a new HTTP POST) how can the plugin know which subsequent call is for Artist A or Artist B ???
mherger
commented
2 years ago That's why I didn't understand it would work sometimes (and I actually still don't understand). But sometimes we seem to get a $request->connectioniID->clid, sometimes we don't. Don't ask me why.
The above workaround would fall back to using the client's MAC address. This could still fail if multiple users were interacting with the same device at the same time, though...
CDrummond
commented
2 years ago @michaelherger ...and that's why artist_id is required. Or is there a way Material can create a 'fake' connectionID? I'm pretty sure I could create a UUID for each instance and append this to all JSONRPC/HTTP calls. How does the Default skin handle this? As I assume that is not using persistent HTTP connections as well.
SamInPgh
commented
2 years ago Well, the change/workaround to BrowseArtist.pm works great for me as the only LMS user in my household. (My cats have not yet figured out the UI...) ;-)
It still seems to me that the "correct" way to handle this issue would be to populate the artist_id in the actions list returned from the previous browseonlineartist call, as stated in the original post. I personally am happy with the workaround for now though. Thanks, Michael!
SamInPgh
commented
2 years ago Let me revise my last post. I am again seeing albums from the previously viewed artist displayed sometimes, so apparently it is still using a cached artist_id if one exists.
michaelherger
commented
2 years ago How does the Default skin handle this? As I assume that is not using persistent HTTP connections as well.
As I mentioned this is going through some of the most convoluted code of LMS (XMLBrowser). A dark place I prefer to stay away from 😁. But I'm currently following that route, trying to find something.
SamInPgh
commented
2 years ago Good luck, Michael!
SamInPgh
commented
2 years ago Another fly in the ointment. The change to BrowseArtist.pm causes requests from Squeezer to fail with the following error in the log:
[22-08-26 13:38:44.6931] Slim::Control::Request::execute (1885) Error: While trying to run function coderef [Slim::Plugin::OnlineLibrary::BrowseArtist::cliQuery]: [Can't call method "clid" without a package or object reference at C:/PROGRA~2/SQUEEZ~1/server/Slim/Plugin/OnlineLibrary/BrowseArtist.pm line 98.A few hours of digging later I'm not really any smarter... What I've found, though, is that the response would have a playControl element in the base actions:
{
"id": 3,
"result": {
"base": {
"actions": {
"playControl": {
"itemsParams": "playControlParams",
"window": {
"isContextMenu": 1
},
"player": 0,
"cmd": [
"browseonlineartist",
"items"
],
"params": {
"_index": "0",
"menu": "1",
"artist_id": "3798",
"service_id": "spotify",
"_quantity": "25000"
}
},
...Could this element be of help? It has all the parameters you'd need, doesn't it?
Whether this is by chance or by design, I unfortunately don't know. I didn't even find any use of that playControl element. Neither in LMS nor Squeezeplay... But the base element is supposed to be a container for common parameters etc. So using that probably makes sense.
CDrummond
commented
2 years ago @mherger Looks like "params" is just those used for the initial request. I can update Material to always pass parameters from the previous request that are not in the current - which would then add "artist_id" here. I'm just not sure if that will break other API calls? For instance, when adding initial support for MAI I passed back all fields (artist name, artist_id, etc) but this then caused issues.
SamInPgh
commented
2 years ago I have been doing a little exploring in XMLBrowser myself and have, I believe, identified where the actions=>go=>params that are returned from the first browseonlineartist,items request (and used for subsequent specific item requests) are built. I would like to make a change to test whether adding the artist_id to these parms would fix this particular problem, ignoring for now the possible impact on other requests. However I have no experience with, and no knowledge of, how to build my own copy of LMS to test with. So for now I will just post here the one-line change to the current 8.3 version of XMLBrowser.pm that I am proposing.
Insert the line with the comment after line 1815 in subroutine _playlistControlContextMenu:
my $itemParams = {
menu => $request->getParam('menu'),
artist_id => $request->getParam('artist_id'), # this is a test
item_id => $item_id,
};If anyone can point me in the direction of how to build my own LMS to test with, I would appreciate it. I apologize for my lack of knowledge but am more than willing to learn. Thanks!
michaelherger
commented
2 years ago @mherger Looks like "params" is just those used for the initial request. I can update Material to always pass parameters from the previous request that are not in the current - which would then add "artist_id" here. I'm just not sure if that will break other API calls? For instance, when adding initial support for MAI I passed back all fields (artist name, artist_id, etc) but this then caused issues.
I don't know, really. But I believe it's what Web::XMLBrowser is doing. I tried to figure out how they handled requests differently.
michaelherger
commented
2 years ago Insert the line with the comment after line 1815 in subroutine
_playlistControlContextMenu:my $itemParams = { menu => $request->getParam('menu'), artist_id => $request->getParam('artist_id'), # this is a test item_id => $item_id, };
This might work for this particular case, where we're missing the artist_id. But it could be any other value, we don't even know the possible parameter names.
SamInPgh
commented
2 years ago @mherger As I said, I am ignoring for now the possible effects on other requests, which I believe could be handled with a conditional statement if necessary. This section of the code, however, seems to be fairly specific to the case at hand.
So how would I go about building a test version of LMS? I apologize for my ignorance in this area but I can't seem to find any information on it.
michaelherger
commented
2 years ago What OS are you on? With any but Windows you'd simply edit the file and restart the server. Windows is complicated...
SamInPgh
commented
2 years ago I'm currently running LMS on Windows 10 but I have a backup system on a RPi B+ that I can test with. Thanks!
SamInPgh
commented
2 years ago Okay, I give up. I can't find XMLBrowser.pm anywhere on the RPi. When you say "edit the file and restart the server", what file are you talking about?
michaelherger
commented
2 years ago find / -name XMLBrowser.pm. That should find three files. You'll need the one in a folder Control.
SamInPgh
commented
2 years ago Oops! I thought I had essentially done that but, upon further research, I realized that I had made an apparently common rookie mistake. The command I used was:
find / -name XMLBrowser.*
Of course, using the full file name (or quoting the wildcarded one) worked. Sorry for taking up your time with this. Thanks for your help. I'll report my findings here.
SamInPgh
commented
2 years ago After doing some testing with XMLBrowser.pm, I have the following to report:
artist_id to the list of params returned for each of the menu items in the response to the first browseonlineartist request, e.g. actions":{"go":{"params":{"artist_id":"42442","item_id":"0","menu":"browseonlineartist"},"cmd":["browseonlineartist","items"]}},"icon":"html/images/albums.png","text":"Albums"}
artist_id did not result in the JSONRPC error seen previously, it also did not return correct data. Not only that, but returning the artist_idin the params seems to have also caused Squeezer to behave in a similarly incorrect manner on its following menu requests. The only 'good' news is that, with the change, Material Skin and Squeezer both exhibited identical aberrant behavior. ;)@michaelherger I now see why you referred to the code as "convoluted", as it is nearly impossible (for me anyway) to predict what unintended affect any changes might have. I will leave that part of the code to more experienced and capable hands, of which there are many. Warning: Spending more than 8 hours in XMLBrowser.pm may cause headaches and dizziness.
@CDrummond if you decide to approach this problem from the Material Skin side, I will be glad to help in any way I can.
SamInPgh
commented
2 years ago Ignore my previous post. Lol!!! I now have Qobuz library integration working correctly in both Squeezer and Material Skin after making another small change to XMLBrowser.pm, adding the service_id to the returned params in addition to the artist_id. I understand that these changes could possibly have unintended consequences for other components of LMS and I would like to get some feedback on them, including suggestions on what type of testing to do. As Squeezer seemed to work just fine without the changes as well as with them, I am particularly interested to know whether adding these params might introduce unnecessary additional overhead with clients using persistent connections. If that is the case, maybe the code to add the params could be made conditional based on that fact. I'm open to anything at this point. Right now, however, things seem to be working very well.
Here are the changes:
pi@max2play:/usr/share/perl5/Slim/Control $ diff XMLBrowser.pm.save XMLBrowser.pm
312a313,314
> my $artist_id = $request->getParam('artist_id');
> my $service_id = $request->getParam('service_id');
1130a1133,1140
>
> if ( $service_id ) {
> $itemParams->{'service_id'} = $service_id;
> }
>
> if ( $artist_id ) {
> $itemParams->{'artist_id'} = $artist_id;
> }
Please keep us posted about potential side effects. I believe this is not the real solution, as it should be open to whatever parameter is being used. But you using it that way might give us a clue whether side effects are to be expected.
SamInPgh
commented
2 years ago Will do. I have actually narrowed down the scope of the change such that it should affect only cometd requests, thus not interfering with the caching being done by other client apps that use persistent connections and passthrough attributes for both artist_id and service_id. I have thus far confirmed that requests/responses from LMS browser, Squeezer and Squeeze Ctrl remain unchanged. Additionally, the presence of both artist_id and service_id is required in the request in order to modify the list of returned params. It is now a specific fix for a specific situation and I am confident that it will resolve the problem without introducing unwanted side effects.
Here is the latest diff:
diff XMLBrowser.pm.save XMLBrowser.pm
312a313,314
> my $artist_id = $request->getParam('artist_id');
> my $service_id = $request->getParam('service_id');
999a1002,1003
>
> main::DEBUGLOG && $log->is_debug && $log->debug("Connection ID is: " . $request->connectionID );
1130a1135,1141
>
> if ( $request->connectionID =~ /^Slim::Web::HTTP/ ) {
> if ( $service_id && $artist_id ) {
> $itemParams->{'service_id'} = $service_id;
> $itemParams->{'artist_id'} = $artist_id;
> }
> } @mherger I would like to open a pull request for this issue but I am not familiar enough with Github and how it is used with LMS to do so with any confidence. The proposed code change is contained in my previous post. At this point, I have downloaded the public/8.3 source tree to my local machine, made the change to XMLBrowser.pm, and have been testing on a RPi-based version of LMS. I would also like to test on my Windows LMS but am not sure how to proceed there. My understanding is that generating a pull request may result in feedback from developers with more experience in this area of the code. I believe that this change fixes the problem with missing service_id and artist_id params in requests using non-persistent connections without interfering with the operation of those using persistent connections, which cache these params. While I admittedly have little experience or knowledge in this area of the code, I believe that the complexity and possible pitfalls associated with creating an artificial connectionID, especially in regard to multiple concurrent users, makes this approach both simpler and less likely to result in unwanted side effects.
michaelherger
commented
2 years ago @SamInPgh I'm happy to hear that your change seems to work fine for you. But I have to admit that I don't like the idea of committing this to the code. It's a hack to get one use case working. But we should have a generic solution for the problem.
@CDrummond have you been able to work on the params integration? Is there something I could easily try myself?
CDrummond
commented
2 years ago @michaelherger Sorry, I've been busy with personal things and have not got round to looking at this. However, seeing as I don't use these services I could not really check any changes. Updating Material for this should be OK, but it also means any other future UIs need to be aware of this.
SamInPgh
commented
2 years ago @michaelherger I respectfully disagree that it's a hack. Looking for a more "generic" solution to a problem as specific as this makes no sense to me, and changing Material Skin to compensate for an obvious problem in LMS is definitely a hack. I urge you to reconsider your position or at least provide a more reasoned argument against this solution.
SamInPgh
commented
2 years ago @CDrummond Fyi, in order to work around this problem from the Material Skin side, both the artist_id AND service_id params must be specified on all browseonlineartist requests. I will be happy to help with testing any changes.
CDrummond
commented
2 years ago @SamInPgh Thanks, but I know what the issue is - just not sure on where the fix belongs. It sounds like an LMS bug to me, hence this issue report, but I'm willing to accept Material is at fault. However, I don't really want to put in a special case for 1 API.
michaelherger
commented
2 years ago Unfortunately there's no reliable documentation for SlimBrowse available. Therefore I'm not 100% sure about that params part. But I really believe it's supposed to be carrying the parameters which should be used in all the various commands.
SamInPgh
commented
2 years ago @CDrummond Understood. I don't think there's any doubt that the problem is in LMS. However, a workaround in Material would provide a stopgap until such time that an LMS solution is agreed upon. I'm puzzled that this problem is not more widely reported as, as far as I can see, it should affect all Material Skin users who subscribe to any streaming service --- not just Qobuz, which is where I am seeing it.
CDrummond
commented
2 years ago @SamInPgh I've just made a commit to Material to add this artist_id. however, I cannot actually check whether it works!
michaelherger
commented
2 years ago I believe you'd need the service_id in addition to the artist_id.
CDrummond
commented
2 years ago @michaelherger I assume you are referring to my commit? If so, the service_id should already be there - as its part of the SlimBrowse response from previous call (which is used to create the actions). However, artist_id is not part of that response (hence this issue), so my change should be adding it.
SamInPgh
commented
2 years ago Great! I'll back out my local change to XMLBrowser and test with it later today.
SamInPgh
commented
2 years ago Well, we're making progress.
Good news: The artist_id is now being sent with the browseonlineartist item_id request.
Bad news: The service_id is not. My change to XMLBrowser added both to the params returned from the previous items call as both are required and both were previously missing. If you can add the service_id as well as the artist_id, we should be good to go.
CDrummond
commented
2 years ago @SamInPgh Ah, Michael did mention that - and I, incorrectly, assumed it would be there. Please update (Material) and try again.
SamInPgh
commented
2 years ago Still not getting the service_id on the item-specific requests. No apparent change from the last update. (Yes, I emptied the cache and did a hard reload :-)
[5:27:29 PM] JSON REQ: ["b8:26:eb:c1:71:f0",["browseonlineartist","items",0,25000,"menu:browseonlineartist","item_id:2","artist_id:42877"]]
SamInPgh
commented
2 years ago I saw the change you made to line 1506 to add the service_id. Could it be that a similar change needs to be made to line 1511? Just a guess...
if (undefined!=item.id && (item.id.startsWith("artist_id") || item.id.startsWith("album_id")) && !addedParams.has(item.id)) {
cmd.params.push(item.id);
}@SamInPgh (Not really sure this issue is the best place to be discussing Material changes...) Please update and try again. I've changed the code so that it saves the paramters from the previous call to use in the next.
SamInPgh
commented
2 years ago @CDrummond That fixed (or circumvented?) the problem. As it is a more general code change than you originally planned, do you have any concerns about it impacting other areas of Material? I will keep my eyes open for any issues but, for now, everything looks good.
[11:01:33 AM] JSON REQ: ["b8:26:eb:c1:71:f0",["browseonlineartist","items",0,25000,"item_id:0","menu:browseonlineartist","service_id:qobuz","artist_id:42442"]]
As mentioned in the Material Skin thread on forums.slimdevices.com (here and here) the _artistid parameter is, I think, missing from the SlimBrowse JSONRPC responses. My understanding is that a client (e.g. Material Skin) uses the go/do/etc. actions listed in these responses when an items is clicked, and each action should list its required parameters. As stated, _artistid does not appear to be in the response, so further clicks fail.
I'm not 100% certain if the SlimBrowse response should be updated for these, or if a client should always just add an "artist_id:xxxx" parameter if the client currently has "artist_id" for its current list. (Hope that makes sense!).