Ensure #1007 only applies to database tracks

[darrell-k]

At least this puts things back as they were for favourites which are not tracks in the user's database...

[michaelherger]

Can you remind me what the original issue was you tried to fix with that addition? Shouldn't we simply prefer favourite_* over the track ID alone?

[darrell-k]

I don't think I can explain any better than I did in the original PR #1007

But I can expand any specific questions. Not quite sure what you mean by:

Shouldn't we simply prefer favourite_* over the track ID alone?

[philippe44]

I don't think I can explain any better than I did in the original PR #1007

But I can expand any specific questions. Not quite sure what you mean by:

Shouldn't we simply prefer favourite_* over the track ID alone?

You're right but don't you think that the core of the issue is that the songinfo hash should not be filled with album data at this point? Which is the responsibility of Web::XMLBrowser.pm? When you're one level above, rightfully the favorite is governed by Browse.XMLBrowser.toggleFavorite() because songinfo is empty. One level below, songinfo should be filled because we're are the track level, but it should be with track data, not album data

[darrell-k]

I don't think I can explain any better than I did in the original PR #1007
But I can expand any specific questions. Not quite sure what you mean by:

Shouldn't we simply prefer favourite_* over the track ID alone?

You're right but don't you think that the core of the issue is that the songinfo hash should not be filled with album data at this point? Which is the responsibility of Web::XMLBrowser.pm? When you're one level above, rightfully the favorite is governed by Browse.XMLBrowser.toggleFavorite() because songinfo is empty. One level below, songinfo should be filled because we're are the track level, but it should be with track data, not album data

You're absolutely correct about songinfo - I guess the clue is in its name! It's why I asked the question "Is there a better way to do this" in the original PR.

I think after drilling down to a single track it does contain the correct data - at least the favorites add already worked fine from there.

Presumably the radio menus / BBC plugin are using songinfo correctly (either by luck or judgement), so yes, the "correct" approach would be to fix XMLBrowser. The question is, how to do that without breaking loads of other stuff. A question which will now keep me busy tonight.

But I think this PR is a valid and necessary interim fix given the traffic this bug has already created on the forums.

[philippe44]

I think that the code building songinfo in Web::XMLBrowser is called one level too early, around line 860

# Find special stuff that we either want to pull up into the metadata for the
# songinfo header block or which needs unfolding.

This code should be called ONLY WHEN user presses the "M" button so that we actually build all the trackinfo when we look at details of that track. In fact, if you just comment that out, you'll see that everything works as expected. On the track's list, when you press the "favorite" (hear) button, it drills down again one level with a 'favadd' or 'favdel' action in the stash and does what is expected, as it does with plugins. So the question is why is that code used here under a block {}, but without and if () ...

[darrell-k]

Doesn't it rely on that code to build the album header information when creating the album page? Or am I missing something, which is very likely?

[michaelherger]

Instead of adding another attribute we could simply check for the URL's protocol (... && item.url.match(/^file:|tmp:/) or similar.

[darrell-k]

I thought that too, but it doesn't work, as we also need to include remote tracks (Qobuz for example) which have been imported.

[michaelherger]

I'll have to look into the real issue once I find some more time.

[darrell-k]

Superceded.