Broken usage of favorites_title and icon

[philippe44]

Broken usage of favorites_title and icon

[mherger]

Is this a question of Slim::Control::XMLBrowser vs. Slim::Web::XMLBrowser (not to mention Slim::Buttons::XMLBrowser...)? They will all need to be tweaked, right?

[mherger]

Oh, I thought of the other PR. Which you say wasn't the reason. Never mind...

[mherger]

Maybe all we need to do is change the order of execution in xmlbrowser.html to have the well defined case (favorites_url) executed first, falling back to the raw audio URL if nothing else is defined?

@darrell-k - what do you think?

[philippe44]

I think it's a question of Slim::Web::XMLBrowser. I've been searching for an issue on Deezer for a while now and I just gave up. Then I moved my install to 8.5 and had that regression. Removing the code in xmlbrowser.html returned things to normal but it also gave me the pointer I was missing to that issue of favs management and why only toggleFavoriteButton was called.

It's due to what happens here in Slimm::Web::XMLBrowser

# action is add/delete favorite: pop the last item off the index as we want to display the whole page not the item
	# keep item id in $favsItem so we can process it later
	if ($stash->{'action'} && $stash->{'action'} =~ /^(favadd|favdel)$/ && @index) {
		$favsItem = pop @index;
	}

and then $favsitem is not populated and then the action favs->add is not executed, so we default on hitting the "poor's man" function on the favcontrol.html page. Anyway, I still need to better understand that but I think I'm now on the right track and it's definitvely a bug because it's random: sometimes when I start LMS, I have the favs set and I have a nice title set when adding the favs, sometimes I have the menu hierarchy like 3.4.5.0. I always start with erases cache for my test system. So there is definitively sometime uncontrolled

[philippe44]

Maybe all we need to do is change the order of execution in xmlbrowser.html to have the well defined case (favorites_url) executed first, falling back to the raw audio URL if nothing else is defined?

@darrell-k - what do you think?

I'm not 100% sur yet but I suspect the issue 'what needs to be changed' is not in the html file but in the Slim::Web::XMLBrowser and making sure that favs->add [~1000) is always executed (or never) but at least consistently

[edit]: indeed, I'm almost convinced that the html file should have received the right parameters from the Slim::Web::XMLBrowser and should not be modified. In some cases, the favorites flag and favorites keys/value are not built properly for a reason that I still need to understand, but building them in the html file does not seem right (for tracks).

[darrell-k]

sometimes when I start LMS, I have the favs set and I have a nice title set when adding the favs, sometimes I have the menu hierarchy like 3.4.5.0. I always start with erases cache for my test system. So there is definitively sometime uncontrolled

This is what I was seeing prior to #1007

I put it down to caching, but likewise never fully understood.

I was hoping that my change was "light touch" but the new test IF item.type == 'audio' && item.url needs to come first because songinfo.favorites is always true in the case of adding a track favourite from the album listing, but as I said in the PR, relates to the album, not the track.

I can have a look at being more specific in the new condition, but does this:

I'm not 100% sur yet but I suspect the issue 'what needs to be changed' is not in the html file but in the Slim::Web::XMLBrowser and making sure that favs->add [~1000) is always executed (or never) but at least consistently

[edit]: indeed, I'm almost convinced that the html file should have received the right parameters from the Slim::Web::XMLBrowser and should not be modified. In some cases, the favorites flag and favorites keys/value are not built properly for a reason that I still need to understand, but building them in the html file does not seem right (for tracks).

mean that you ( @philippe44 ) are now happy with the IF condition as it is?

[darrell-k]

I've created a PR which limits the new logic to tracks from the user's database.

Tested with local tracks, Qobuz imports, Qobuz plugin, BBC Sounds and Tune In.

#1018

[philippe44]

so, I think I got it right this time, without checking the origin (db or not) of the track which I think is not the issue, it's more that songinfo shall NOT be used when the item is audio, which a more general stance. See #1019