Review Request: Restore heuristics to guess whether album is a compilation

Sat Aug 18 22:56:49 UTC 2012

> On Aug. 16, 2012, 12:15 p.m., Ralf Engels wrote:
> > Just going through all the review requests again.
> > 
> > With your patch the following auto test is failing: testIdentifyCompilationInMultipleDirectories
> 
> Alexey Neyman wrote:
>     Which is, I guess, expected - since it was the purpose of this patch to detect albums as separate if they reside in separate directories. As far as I can see from the test, it sets "track artist" on each of the tracks, but not "album artists". So, the test should be adjusted to set album artist to the same value for all these tracks (see your example above vs. mine, above).
>     
>     I'll try to update the test - but I won't be able to run it, I still haven't succeeded in using google mock on Kubuntu.
> 
> Ralf Engels wrote:
>     When talking about the example:
>     
>     In my example the Album artist should be set to "Michael Jackson" (also for the audio comment from Quincy Jones).
>     In your example the Album artists should be set to "Abba" and "Chris Norman".
>     
>     How about continuing like this: add a new test case that checks if Albums with different album artists are not merged.
>     Then we change the code so that also this case works.
>     
>     If you split up albums with different track artist, then you are in principle eliminating all compilations, right?
> 
> Alexey Neyman wrote:
>     As to "eliminating all compilations": not exactly. We have the following cases when the album names match (AA = Album artist, TA = track artist):
>     1) Tracks have the same AA, same TA: these are part of the same album, album is not a compilation - [obvious case]
>     2) Tracks have the same AA, different TA: these are a part of compilation [this is your example if it's tagged properly]
>     3) Tracks have different TA: these are parts of separate albums [this is my example]
>     4) Tracks have no AA, same TA: these are part of the same album [in line with using TA as AA when AA is not set]
>     5) Tracks have no AA, different TA, in the same directory: arguable
>     6) Tracks have no AA, different TA, in different directories: arguable
>     
>     As you see, the case #2 should be still considered a single compilation album even though the TAs are different.
>     
>     As to cases #5 and #6: I would suggest detecting #5 as a single compilation album and #6 as multiple compilation albums, one per directory.
>     
>     That is, it looks like we'd need 6 test cases instead of one - for all of the above scenarios :) However, I wasn't able to make Amarok use googlemock installation on my Kubuntu 12.04. I'll try to update the system and Amarok sources tonight to see if this changed. If Amarok can use googlemock installation on Kubuntu - I'll implement these test cases and update the patch to satisfy these scenarios. If not - I won't be able to continue this, due to inability to test the changes.

Good overview. The failing test is checking 6) as far as I understand and it's currently assuming an compilation.

I agree that one can argue about that but it's working like this since the time I started hacking Amarok, I think it's a good default case and for my personal usage it is also working pretty well.
So I would like the test case to continue working.

But again, your overview is good and having test cases for all those (and maybe a short textual description for each of the tests) would prevent the behaviour from ever devolving again.

- Ralf

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104294/#review17522
-----------------------------------------------------------

On March 16, 2012, 12:13 a.m., Alexey Neyman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104294/
> -----------------------------------------------------------
> 
> (Updated March 16, 2012, 12:13 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Older amarok used the following heuristics to determine whether a particular
> album is a compilation (from a comment in 
> amarok-2.2.1/src/collection/sqlcollection/ScanResultProcessor.cpp):
> 
>     //using the following heuristics:
>     //if more than one album is in the dir, use the artist of each track as albumartist
>     //if all tracks have the same artist, use it as albumartist
>     //try to find the albumartist A: tracks must have the artist A or A feat. B (and variants)
>     //if no albumartist could be found, it's a compilation
> 
> 
> However, more recent Amarok versions started to merge different albums
> with different artist in separate directories together, as explained above.
> Amarok started to assume all albums with same name to be compilations
> (even if in separate directories) since the following commit:
> 
> dfd8b457d7094144563c51b2528afdbe23ffc344
> Ralf Engels
> Fix all collection scanner auto tests.
> 
> Now, amarok first scans all directories (sorting albums by the name)
> and then tries to process *album names*, one at a time. If it finds
> more than one instance of an album name, it assumes it to be a compilation.
> Thus, it lost the heuristics in employed before ("if more than one album
> is in the dir...").
> 
> While it is still possible to force the right behavior
> by selecting "Do not show under Various Artists" for each of the erroneous
> albums, it would still be better to restore the original heuristics as there
> may be lots of albums merged this way. I think the old heuristics made sense
> (why would albums be put into separate directories otherwise, if they are
> a single compilation album?).
> 
> The attached patch restores the following logic: If any given directory
> contains tracks that were sorted into a single album and and that album
> was not created as a compilation (i.e. it has non-empty artists), this
> album is excluded from being merged with other albums to create a "compilation".
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/db/ScanResultProcessor.cpp 4f02a16 
> 
> Diff: http://git.reviewboard.kde.org/r/104294/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alexey Neyman
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120818/63103222/attachment-0001.html>