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)

[Matěj Laitl]

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

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/e25d42e6/attachment.html>