> 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
