----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/#review9892 -----------------------------------------------------------
Ship it! While working on the multiple-artists feature I came to the following conclusion: Amarok should only try to guess album artists (or all artists in case of a "Ralf featuring Matej") for internal usage and not try to write them back. That would mean that the album artist could still be displayed as being empty while the track artist is used in the Collection. Still, the patch is good. Ship it. - Ralf Engels On Jan. 17, 2012, 2:10 p.m., Matěj Laitl wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103715/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2012, 2:10 p.m.) > > > Review request for Amarok, Bart Cerneels and Ralf Engels. > > > Description > ------- > > MetaFile: guess album artist as in SQL collection scanner, compilations > > Call albumArtist = ArtistHelper::bestGuessAlbumArtist( albumArtist, > artist, genre, composer ); afer the data are read from file tags. > > This makes album artist field consistent for tracks in Local collection > and tracks in file browser or drag & dropped to Amarok playlist, but it > has some downsides: > * Amarok will essentially lie about what album artist is assigned to a > track, most notably will replace empty album artist by track artist > and will replace "Various Artists" by empty artist. > * Editing album artist will be strange in corner cases: in particular > if user sets album artist to "Various Artists", it will be written > to file tags as-is, but displayed as empty album artist. Other case > would be when user sets album artist to empty string, but this is > currently prevented due to a bug/feature in TagDialog. > > (every occurrence of "Various Artists" also accepts localized version > of the string) > > Above change is needed for upcoming "MemoryCollection: use also album > artist as identifying key in album map" change for UmsCollection, but > if someone opposes, similar change can be made on UmsCollection level. > > Also, implement MetaFile::FileAlbum isCompilation() which is now easy > as it can return albumArtist.isEmpty() > > DIGEST: unify album artist handling in Amarok > > > -------------------------------------------------------------------- > > > MemoryCollection: use also album artist as identifying key in album map > > This has some far-reaching consequences which I believe are only > positive, most notably Amarok is able to differentiate between 2 albums > with same name but different album artists in USB Mass Storage > collection and in "new" iPod collection. This unifies album artist > handling across MemoryCollection and SqlCollection, 2 major collection > storage implementations in Amarok. > > Technically, AlbumMap is changed from: > typedef QMap<QString, Meta::AlbumPtr> AlbumMap; > to: > class AlbumMap : public QMap<Meta::AlbumKey, Meta::AlbumPtr> > > with insert(), remove(), value(), contains() QMap methods hidden and > reimplemented for convenience and to prevent coding mistakes. (you no > longer can add album under wrong key) > > This change is tested to work with new iPod collection, UMS collection > (depends on previous "MetaFile: guess album artist as in SQL collection > scanner, compilations"), all tests still pass. > > Only area that received just very basic testing are online > service collections that are also affected by this change. > > DIGEST: Albums with same name but different album artist are now > correctly separated in USB Mass Storage, iPod and various > online service collections. > > > Diffs > ----- > > ChangeLog ed4e955deeebf6a506e87ca8973645b9eeadc67b > src/core-impl/collections/daap/daapreader/Reader.cpp > 9f749c5fe892f0cbcbacd8454d8fc80940f6bb43 > src/core-impl/collections/ipodcollection/handler/IpodHandler.h > ceccd7cc3028051d9b93201caea180a10af15ed3 > src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp > 83c5d27839dcb5f18a4233daa5367fb18962c7af > src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp > e892952ba0ede760a66e71ffa9e43c123b74a6d7 > > src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp > 46000b3f1950cc64feef8321bcf698980c2742fe > src/core-impl/collections/playdarcollection/PlaydarCollection.cpp > 1e598f0d20ea88bef4c60877eefb71a8b934218d > src/core-impl/collections/support/MemoryCollection.h > e2b28dea72393456f5c62ecf06ce7832b49e2e4f > src/core-impl/collections/support/MemoryMatcher.cpp > 624d6d4c0eb63745293d083982b936e5c050ff0a > src/core-impl/collections/support/MemoryMeta.h > 45eefb7a4c085fa32478799e296579eef117072b > src/core-impl/collections/support/MemoryMeta.cpp PRE-CREATION > src/core-impl/collections/upnpcollection/UpnpCache.h > da00fd1b11306ef49e10b703f601147a5762e18e > src/core-impl/collections/upnpcollection/UpnpCache.cpp > 58130795298911ab1849fdc64314b8ed128df3d4 > src/core-impl/meta/file/File_p.h af2dbf670e23357dca0efc48421c7d0d6f884f32 > src/core/meta/support/MetaKeys.h f4ffa0eb9889787b1cebb9ea4781930fbcf3fd01 > src/core/meta/support/MetaKeys.cpp 3e8f0fa8587badf044c6a00806230adfe257a6fd > src/services/ampache/AmpacheServiceQueryMaker.cpp > 72862b917fb397b02187d14cd97c1c2c5fa4f79d > tests/TestTrackOrganizer.cpp 75faf04b332e9e9c0ff279037ae66053e1011cd4 > tests/browsers/TestSingleCollectionTreeItemModel.cpp > 99ec36f7eae0560024669e63ca5102829d610748 > tests/synchronization/TestMasterSlaveSynchronizationJob.cpp > 707e578d042c3b13ecea7069c1d8b496896a3c5c > tests/synchronization/TestOneWaySynchronizationJob.cpp > 058611fd8fdde491e6b2eb62c5895e20324613b1 > tests/synchronization/TestUnionJob.cpp > 591d695d1ea0641951e5aa7188a697f98b988b5c > > Diff: http://git.reviewboard.kde.org/r/103715/diff/diff > > > Testing > ------- > > This change is tested to work with new iPod collection, UMS collection, all > tests still pass. Only area that received just very basic testing are online > service collections that are also affected by this change. > > > Screenshots > ----------- > > amarok-ums-collections-and-album-artists.png > http://git.reviewboard.kde.org/r/103715/s/416/ > > > Thanks, > > Matěj Laitl > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
