Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix album_contributor updates in new & changed scan #962

Merged
merged 2 commits into from Dec 20, 2023

Conversation

darrell-k
Copy link
Contributor

This seems to work, but I wonder if we should put some more thought into how we handle multiple Artist tags - at the moment, tracks.primary_artist is arbitrarily set to the first element in the Artists array.

Maybe if there is an ARTIST that is also an ALBUMARTIST (in the music file tags) it should take precedence? (Would also need to bear in mind that there could be multiple ALBUMARTIST tags as well).

This seems to work, but I wonder if we should put some more thought into how we handle multiple Artist tags - at the moment, tracks.primary_artist is arbitrarily set to the first element in the Artists array.

Maybe if there is an ARTIST that is also an ALBUMARTIST (in the music file tags) it should take precedence? (Would also need to bear in mind that there could be multiple ALBUMARTIST tags as well).
@@ -255,8 +255,9 @@ sub rescan {
SELECT COUNT(*) FROM contributor_track WHERE contributor = ?
} );

# There may have be more than one Artist tag in the music file, so track.primary_artist does not tell the whole story. So also check for ARTIST/ALBUMARTIST in contributor_track.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"...have been..."?

my $albumSth = $dbh->prepare_cached( qq{
SELECT COUNT(1) FROM tracks WHERE album = ? AND primary_artist = ?
SELECT COUNT(1) FROM tracks JOIN contributor_track ON tracks.id=contributor_track.track WHERE album=? AND ( tracks.primary_artist=? OR contributor_track.contributor=? AND contributor_track.role IN (1,5) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please format the query to make it more readable?

@michaelherger
Copy link
Member

I believe the primary_artist concept is wrong and should be replaced. But try to do this without a ton of side-effects and discussions...

@darrell-k
Copy link
Contributor Author

I agree. What do you think about changing its population so at least there's a chance of it matching albums.contributor? Or are there people out their with multiple Artist tags who are taking advantage of the fact that for LMS the first one in their music file is "special"?

Also made it a LEFT JOIN to cater for the unlikely situation where there are no contributor_track matches.
@michaelherger
Copy link
Member

You can expect people out there to do anything you wouldn't 😂. I think I've seen any assumption we/I did at some point proved wrong at some later point. Remove "the" from the search index? Shouldn't be a problem. Except for "The The". Any album title would have a letter or number? Along came Ed Sheeran with stuff like "÷"...

So... I'd like to keep 8.4 relatively stable, not introduce too risky changes in the next few weeks, so I could cut a 8.4 release early next year. Maybe a complete overhaul of primary_artist could go in a next version? It could get some good exposure there before being released to the masses.

@mherger
Copy link
Contributor

mherger commented Dec 20, 2023

Thanks a lot! Let's give this particular fix a try for now. That way you'll have to deal with one branch less 😉.

@mherger mherger merged commit cc57618 into LMS-Community:public/8.4 Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants