Review Request: MetaFile: guess album artist as in SQL collection scanner, MemoryCollection: use also album artist as identifying key in album map (2 commits squelched just for reviewboard)

Ralf Engels ralf-engels at gmx.de
Tue Jan 17 18:40:24 UTC 2012


-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120117/fa4974af/attachment-0001.html>


More information about the Amarok-devel mailing list