Use artist when inserting a new album when album artist is not available
ClosedPublic

Authored by astippich on Apr 14 2019, 10:59 AM.

Details

Summary

When adding new tracks, artist was not used as fallback for inserting
new albums in case album artist is not available. This made tracks
with no album artist create albums with empty artists.

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich requested review of this revision.Apr 14 2019, 10:59 AM
astippich created this revision.

Elisa is barely usable for me without this as most of my files apparently do not have an album artist tag.
Please have a look if I correctly changed all the tests, especially the addTwoAlbumsWithDifferentPathsAndSameTracks test

mgallien requested changes to this revision.Apr 16 2019, 3:38 PM

Thanks for your work.
I believe that the way the tests are modified is correct.
The database part should instead modify the way data are read to fallback to the track artist without writing data to the database that does not really exist.
I can provide you pointers on how to achieve that if you want.

This revision now requires changes to proceed.Apr 16 2019, 3:38 PM

This is used only when adding the album (insertAlbum function). It is not added to the tracks in the database, so I believe this change is correct. This is also done the same way a few lines above when checking for albums already in the database. I will change the title to make that clear.

astippich retitled this revision from Use artist when album artist is not available when adding new tracks to Use artist when inserting a new album when album artist is not available.Apr 16 2019, 5:45 PM
astippich edited the summary of this revision. (Show Details)
mgallien accepted this revision.Apr 18 2019, 6:01 AM

This is used only when adding the album (insertAlbum function). It is not added to the tracks in the database, so I believe this change is correct. This is also done the same way a few lines above when checking for albums already in the database. I will change the title to make that clear.

I still think that inserting made up data in the database is wrong. The fact that it is already done when there are multiple artists in an album is enough for me to accept your diff.
I fear that in the long term this will have unwanted side effects.

This revision is now accepted and ready to land.Apr 18 2019, 6:01 AM

I will land this as an intermediate fix then.
Out of curiosity, how would you do it instead? When there is no album artist in the track metadata, an album with no artist at all will be created. I would say that this is the best guess available.

This revision was automatically updated to reflect the committed changes.

I will land this as an intermediate fix then.
Out of curiosity, how would you do it instead? When there is no album artist in the track metadata, an album with no artist at all will be created. I would say that this is the best guess available.

Thanks for that.
I also feel that adding 'Various Artists' in database is wrong and your patch made me realize that.
I think that it would be better to "compute" that when reading the data (like for the tracks count for example).

This way, the data are always correct given you only read the data from the tracks that reflect the data in the files on storage.