make track artist optional in combined artist list #1186
Conversation
| @@ -299,7 +299,7 @@ sub artists { | |||
| my @artists = $self->artistsForRoles('ALBUMARTIST'); | |||
|
|
|||
| # If the user wants to use BAND as album artist, pull that. | |||
| if (scalar @artists == 0 && $prefs->get('bandInArtists')) { | |||
| if (scalar @artists == 0 && $prefs->get('bandInArtists')) { ##????? | |||
There was a problem hiding this comment.
This line came up in my grep analysis of ...InArtists.
Was wondering if this was correct, wouldn't the scanner with useTPE2AsAlbumArtist set be importing BAND to ALBUMARTIST anyway? Not sure what it's got to do with the combined list pref?
There was a problem hiding this comment.
Those roles aren't merged to AA at scan time, are they? What is really wrong here is that only bands are handled, but none of the others. Or am I missing something?
If you git blame the line you'd see that these lines are super old (18 years). The only change was a consolidation of a previous useBandAsAlbumArtist pref into bandInArtists.
There was a problem hiding this comment.
Looking at Slim::Formats::MP3, with useTPE2AsAlbumArtist, TPE2 is mapped to ALBUMARTIST, otherwise BAND, by the scanner. I'm thinking that the comment in the code If the user wants to use BAND as album artist, pull that. is something to do with that.
Looking at the whole sub, it doesn't make much sense to me that we'd prefer returning BAND rather than ARTIST just because bandInArtists is set, which is about whether to display BAND in artist lists/searches, not about the meaning of BAND.
I think this code predates the dual-use of TPE2 (which came in only 15 years ago?). Perhaps before that, we always pulled TPE2 into BAND, but gave the user the option to reinterpret it as ALBUMARTIST at runtime, rather than scan time, so to speak?
Anyway, it's not much to do with this PR, just flagging it.
There was a problem hiding this comment.
I tried to figure out where this is even being used any more. The only caller I've found is in TitleFormatter, which would be used on Classic/Transporter/Boom only AFAIK. So... even if it was "broken", it's likely not widely use. Let's ignore that.
| @@ -8785,38 +8785,38 @@ SETUP_NOROLEFILTER_DESC | |||
| SV När du bläddrar genom "Artister" kan du filtrera så att bara album och låtar som matchar den valda rollen ("ALBUMARTIST", "COMPOSER", etc.) visas. | |||
|
|
|||
| SETUP_COMPOSERINARTISTS | |||
There was a problem hiding this comment.
Heh... this used to be a single flag...
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...
You'll see I've marked a line in Slim/Schema/Album.pm: see comment I'm about to add to the code there.