> On Jan. 17, 2012, 6:40 p.m., Ralf Engels wrote: > > 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.
Yup, I agree with the conclusion. But still I'm unable to come up with something reasonable. [brainstorm] Album::prettyAlbumArtist() (the guessed one) and Album::albumArtist() (the true one)? Also, I think that Album::hasAlbumArtist() is completely redundant (I fear to trust all Amarok code to return non-null albumArtist() if hasAlbumArtist() == true) and Album::isCompilation() is virtually !Album::hasAlbumArtist(). Perhaps we could remove both and define compilations as "Album::prettyAlbumArtist() == null ptr"? [/brainstorm] Speaking about the patch I've also received favourable review by email from Maximilian Kossick that will result in some docstring clarifications and a few more comments. If noone opposes, I'll push this tomorrow or so. - Matěj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103715/#review9892 ----------------------------------------------------------- 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
