> 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. > >
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. :) - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100135/#review339 ----------------------------------------------------------- On 2010-11-16 11:21:17, 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:21:17) > > > 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. > Tiny changes to collection scanner: simplify albumartist handling for Album > class, and remove really strange (and mostly useless) m_isNoCompilation var > from Track. > > > 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 > >
_______________________________________________ Amarok-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/amarok-devel
