Review Request: Album Artist. Further support improvements

Tue Nov 16 19:51:53 CET 2010

> On 2010-11-15 15:33:36, Ralf Engels wrote:
> > Regarding the noCompilation of the collection scanner.
> > Actually this makes sense.
> > 
> > The collection scanner has a lot of heuristics for detecting if an album is a compilation.
> > However we also have a compilation tag.
> > If the tag claims that a track is definitely not in a compilation, then we should respect this and not try to auto-detect the compilation status.
> > 
> > I just noticed that I was not following this rule myself, but neighter should the noCompilation variable be removed unless you can argue why this doesn't make sense.
> >
> 
> Sergey Ivanov wrote:
>     Track could only has 2 states: belong_to_compilation or belong_to_regular_album. But to indicate this 1bit information is currently used 2 mutually exclusive boolean variables. Initially they all are false, if track is a part of compilation isCompilation becomes true, if not - isNoCompilation. ATM there is no any way to get both of them true or false simultaneously, so I don't see any reason to use both.
>     This "a lot of heuristics" contains 2 checks in Directory: 1 directory AND 1 album but several artists - this is compilation; 1 directory AND empty album name AND less then 20 tracks - directory name is album name. ( isCompilation isn't used ) and 2 twins-functions isCompilation and isNoCompilation in Album class, which checks is isCompilation and isNoCompilation flags and compare album artist name with "Various Artists". Since isCompilation and isNoCompilation are mutually exclusive all this "heuristic" fold up to 1 line compact check. :)
> 
> Ralf Engels wrote:
>     No. 
>     That's not it.
>     An album is a not a compilation if all tracks have the same artists, no artist or not "various artist" as album artist.
>     It is also not a compilation if one of it's tracks have the corresponding tag.
>     It is a compilation if one of it's tracks says so.
>     
>     So, you see. The twenty lines of artist name checking aren't there for show.
>     The three states that a track can have are: in a compilation, not in a compilation, depends on the artists.
>     Actually I made a mistake in Directory.cpp. There I will throw tracks together into one compilation album independent of their compilation state.
>     I should not throw them together if one of the tracks says "noCompilation". That's where you need the flag again.
>     
>     "isCompilation" and "noCompilation" are only mutually exclusive at the very end, when you actually know all the tracks you want to put into an album.
>     
>
> 
> Sergey Ivanov wrote:
>     Ok. Reverted collection scanner changes for this patch. But still disagree with current way of handling collections and Album Artists. Probably It will be another patch, with more deep changes. :)

You are welcome.
But I propose the following process:
1. I write some sensible auto tests that generate a test collection with all the different corner cases.
2. We decide on the way each of this corner cases should be handled
3. We fix the collection scanner until it passes all the tests.

- Ralf

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

On 2010-11-16 11:38:35, Sergey Ivanov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100135/
> -----------------------------------------------------------
> 
> (Updated 2010-11-16 11:38:35)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> Add new QueryType - AlbumArtist.
> Add new "level" to collection browser.
> Add Album Artist field to TagDialog.
> Add Album Artist to TagGuesser and Organize Collection dialog.
> 
> 
> Diffs
> -----
> 
>   src/amarokurls/NavigationUrlGenerator.cpp 5c48e73 
>   src/amarokurls/NavigationUrlRunner.cpp 2d6384b 
>   src/browsers/CollectionTreeItemModelBase.h 23a189f 
>   src/browsers/CollectionTreeItemModelBase.cpp 7015cd4 
>   src/browsers/CollectionTreeView.cpp 2bb894c 
>   src/browsers/collectionbrowser/CollectionWidget.cpp 8623079 
>   src/core-impl/collections/db/sql/SqlMeta.cpp ff899fe 
>   src/core-impl/collections/db/sql/SqlQueryMaker.h 680050f 
>   src/core-impl/collections/db/sql/SqlQueryMaker.cpp ca105e8 
>   src/core-impl/collections/db/sql/SqlQueryMakerInternal.cpp 72cd07d 
>   src/core-impl/collections/proxycollection/ProxyCollectionQueryMaker.h 63d6c2c 
>   src/core-impl/collections/proxycollection/ProxyCollectionQueryMaker.cpp c17c3fd 
>   src/core-impl/collections/support/MemoryQueryMaker.h 72ddaf1 
>   src/core-impl/collections/support/MemoryQueryMaker.cpp 81bf9b6 
>   src/core-impl/collections/support/MemoryQueryMakerInternal.h db94d8b 
>   src/core-impl/collections/support/MemoryQueryMakerInternal.cpp eeffd77 
>   src/core-impl/collections/support/XmlQueryReader.cpp b5518f0 
>   src/core-impl/collections/support/XmlQueryWriter.h 5caf7bc 
>   src/core-impl/collections/support/XmlQueryWriter.cpp 3c9cf41 
>   src/core/collections/MetaQueryMaker.h 799825a 
>   src/core/collections/MetaQueryMaker.cpp 10ae2a7 
>   src/core/collections/QueryMaker.h 9c15659 
>   src/core/collections/QueryMaker.cpp 78c6ba7 
>   src/core/meta/support/MetaConstants.h ba56248 
>   src/core/meta/support/MetaUtility.cpp 5cf4519 
>   src/dialogs/FilenameLayoutDialog.h 2e20c77 
>   src/dialogs/FilenameLayoutDialog.cpp 9b572a9 
>   src/dialogs/FilenameLayoutDialog.ui b65b6f1 
>   src/dialogs/MusicBrainzTagger.cpp a9fc8a2 
>   src/dialogs/TagDialog.h fbc89ae 
>   src/dialogs/TagDialog.cpp fa0b33a 
>   src/dialogs/TagDialogBase.ui 816ecb5 
>   src/dialogs/TagGuesser.h 476b16d 
>   src/dialogs/TagGuesser.cpp bd3820d 
>   src/services/ServiceSqlQueryMaker.h 5591ecc 
>   src/services/ServiceSqlQueryMaker.cpp 197f2cf 
>   src/services/ampache/AmpacheServiceQueryMaker.cpp 7dee8c8 
>   src/services/mp3tunes/Mp3tunesServiceQueryMaker.cpp 4dc8fed 
>   src/services/scriptable/ScriptableServiceQueryMaker.cpp 76faca2 
> 
> Diff: http://git.reviewboard.kde.org/r/100135/diff
> 
> 
> Testing
> -------
> 
> Tested with SqlCollection. Seems works for me. 
> 
> 
> Screenshots
> -----------
> 
> Only one album "Once". As It should be.
>   http://git.reviewboard.kde.org/r/100135/s/17/
> 2 albums, and 3rd is somewhere below.
>   http://git.reviewboard.kde.org/r/100135/s/18/
> 
>   http://git.reviewboard.kde.org/r/100135/s/19/
> 
> 
> Thanks,
> 
> Sergey
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101116/acbace95/attachment-0001.htm 