Broken usage of favorites_title and icon

[philippe44]

This PR #1007 broke the usage of "favorites_title" which, when set, must take priority on what LMS would set otherwise to make favorite look more relevant. You end up with " by "

As a matter of aesthetic, I would rather have a " - " between title and artist than a " by "

[edit]: Just saw the "by' was used elsewhere so it's just personal preference, forget it. Now, this change also kills the icon setting.
I need to understand why but it feels to me that it is not executing the code in Wlim::Web::XMLBrowser around line 1000 where the feed is parsed and the favorite is added with all details. It probably only executes the Slim::Plugin::Favorites::Plugin::toggleButtonHandler which does not set icon and all. It's the poor man's of favs settings and sets only title and url.
I'm not sure why this is not executed, because the session is missing?
But I think that the IF in the xmlbrowser.html should not lead, but rather follow and probably have more parameters

[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