Make TRACKARTIST a fix part of the unified artists browse list.
#1192
Conversation
This reverts PR LMS-Community#1186 in large parts, but adds some more consequent use of `Slim::Schema::Contributor->unifiedArtistsListRoles` instead. I believe the motivation for LMS-Community#1186 was to include custom roles in search result sets. This is one place where this PR does things differently than before LMS-Community#1186. `TRACKARTIST` had been in the unified artists list for > 10 years. Removing it or making it optional IMHO was our mistake. But I believe we only did so in order to use other methods compiling role lists. This change will hopefully do the best of both...
|
I haven't studied the code changes yet, but:
is incorrect. That PR had nothing to do with user-defined roles. |
| } | ||
| elsif ($prefs->get('useUnifiedArtistsList')) { | ||
| # include user-defined roles that user wants in artist list | ||
| $roles = Slim::Schema->artistOnlyRoles( Slim::Schema::Contributor->getUserDefinedRolesToInclude() ); | ||
| $roles = [ Slim::Schema::Contributor->unifiedArtistsListRoles() ]; |
There was a problem hiding this comment.
This is going to unconditionally include TRACKARTIST in the combined artist list, but see https://forums.slimdevices.com/forum/user-forums/logitech-media-server/112121-online-music-library-integration-an-observation?p=1730145#post1730145 and https://forums.slimdevices.com/forum/user-forums/logitech-media-server/112121-online-music-library-integration-an-observation?p=1730162#post1730162
There was a problem hiding this comment.
Exactly. And that's what it was for ten years. TRACKARTIST always was part of the unified list. And making this optional (or removing it) caused issues. What your PR does is basically add them back in on a case by case basis. But we only have three callers of unifiedArtistsListRoles() - and all of them would need that new treatment. So basically we would be back to where we were before, just a little more confusingly so.
There was a problem hiding this comment.
Thanks for the links: they answer the "what was the motivation again?" question. I was wrong in that discussion. I'm sorry for the confusion. We should not make TRACKARTIST optional, we should leave that as it was. Quite obviously not including it has caused problems.
There was a problem hiding this comment.
No, TRACKARTIST was never in the Combined artist list. See my comments on the other PR.
There was a problem hiding this comment.
I think we're referring to different things: I was referring to the previous callers of unifiedArtistsListRoles(), or the code we had before we added that method. They were always using the TRACKARTIST role.
The line we're commenting on here indeed was different: it only had TRACKARTIST in this case. Then you added all the user defined roles plus it in 6a4944a, to remove it/make it optional in 5d7e146 (not sure which of the two).
Now the question is what the expectation is for this line. I assumed we wanted to treat items shown for the unified list always the same. That's why I'm using unifiedArtistsListRoles here. But the expectation is that we want that role to be optional when browsing the artists list, while we'd include albums where the artist was TRACKARTIST only when browsing that artist's releases?
I think I've lost track of all the changes and their motivation 😞.
There was a problem hiding this comment.
As I tried to explain in the other PR, first, we added TRACKARTIST as a parameter to artistOnlyRoles() which added it to the combined artist list. This resolved the issue where certain artists were not appearing in the list, especially those originating from OMLI where the user is stuck with the online service's tagging.
This caused problems for users, including yourself, who didn't want artists who were only track artists cluttering up their combined lists. So, we decided to make it optional, like Composer, Conductor and Band.
This was where I made the error of removing TRACKARTIST from the roles returned by unifiedArtistsListRoles() (unless it was selected for the combined list), leading to statusQuery not returning tracks where the only artist role was TRACKARTIST, and so messing up the rendering of the playlist and current track.
The latest version of my PR, rather than simply revert unifiedArtistsListRoles(), parameterises it so it can be used in place of artistOnlyRoles() as well as where it is currently used. Also, it is renamed.
For me, this clarifies the code, so we don't wonder in the future why we have 2 functions doing roughly the same thing.
I believe that the latest push to my PR ensures that functionality compared to that before all these changes does not change, except to include TRACKARTIST in the combined artist list when the user wants it.
There was a problem hiding this comment.
Ok, let's concentrate on #1191.
This reverts PR #1186 in large parts, but adds some more consequent use of
Slim::Schema::Contributor->unifiedArtistsListRolesinstead. I believe the motivation for #1186 was to include custom roles in search result sets. This is one place where this PR does things differently than before #1186.TRACKARTISThad been in the unified artists list for > 10 years. Removing it or making it optional IMHO was our mistake. But I believe we only did so in order to use other methods compiling role lists. This change will hopefully do the best of both...