LMS-Community / plugin-Qobuz

Squeebox plugin to stream from qobuz.com
31 stars 16 forks source link

Include direct link to booklet in main album listing, where possible … #64

Closed darrell-k closed 10 months ago

darrell-k commented 11 months ago

…- takes advantage of LMS commit https://github.com/Logitech/slimserver/commit/75674a4314c71647376f75d35d21ef33d219f810

darrell-k commented 11 months ago

Thinking about it, this needs to check LMS version otherwise it will break booklet viewing in Material for older LMSs

michaelherger commented 11 months ago

I think whatever you're trying to do with _isBrowser() should work the same if _canWeblink(). The difference indeed is that 8.4 can render weblink directly, where 8.3 and older can't. So the distinction should actually be 8.4 or not 8.4 for _canWeblink() clients, but not the _isBrowser() check. Maybe all we'd need is this small change?

diff --git a/Plugin.pm b/Plugin.pm
index 0432dc2..a6c06d1 100644
--- a/Plugin.pm
+++ b/Plugin.pm
@@ -1696,7 +1672,7 @@ sub trackInfoMenuBooklet {
        my $goodies = $remoteMeta->{goodies};
        if ($goodies && ref $goodies && scalar @$goodies) {
            # jive clients like iPeng etc. can display web content, but need special handling...
-           if ( _canWeblink($client) )  {
+           if ( Slim::Utils::Versions->compareVersions($::VERSION, '8.4.0') >= 0 && _canWeblink($client) )  {
                $item = {
                    name => cstring($client, 'PLUGIN_QOBUZ_GOODIES'),
                    itemActions => {
darrell-k commented 11 months ago

The problem is that the direct link doesn't work in Squeezer/OpenSqueeze, so I needed to distinguish between a browser client (Material or default) and those client apps.

On the version question, it's not all 8.4.0 is it? the change only went in a few days ago, so there will be 8.4.0s out there without the capability.

So I was going to check $::REVISION as well, which leads to a question - what will that be once 8.4.0 is released? will $::REVISION > nnnnnnnnn still work?

michaelherger commented 11 months ago

Would those clients return _canWeblink()? They shouldn't if they can't handle the links.

And don't care about old 8.4 builds: users running 8.4 should be used to update to the latest or go back to 8.3.

darrell-k commented 11 months ago

They are fine with the existing code which renders a cli menu containing the link, but not with a direct link on the main album listing.

                                $item = {
                                        name => cstring($client, 'PLUGIN_QOBUZ_GOODIES'),
                                        itemActions => {
                                                items => {
                                                        command  => [ 'qobuz', 'goodies' ],
                                                        fixedParams => {
                                                                goodies => to_json($goodies),
                                                        }
                                                },
                                        },
                                };
                        }

We did discuss this in LMS forum PMs last summer, at one point you said:

Hm... as I tried to find good entry points I came to realize that even if we change XMLBrowser, the clients will have to update, too: they currently don't expect such external links in those menu responses, as this hasn't existed before. Looking at Material I think it might just work. But that would be purely coincidental.

michaelherger commented 11 months ago

We did discuss this in LMS forum PMs last summer, at one point you said:

Hm... as I tried to find good entry points I came to realize that even if we change XMLBrowser, the clients will have to update, too: they currently don't expect such external links in those menu responses, as this hasn't existed before. Looking at Material I think it might just work. But that would be purely coincidental.

It's always good when people remember what I discussed before 🤦🏻. Was this on Github, via email, or in the forums? I don't remember why I came to the conclusion that this would not work...

darrell-k commented 11 months ago

It was in a Forum PM thread between Me, you and Sam, entitled "Qobuz plug-in: identification of client ".

My testing indicates that you were correct to say what you did - your recent change to XMLBrowser enables direct weblinks to work with Material, but not with Squeezer or OpenSqueeze. Hence the logic in this PR.