<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103715/">http://git.reviewboard.kde.org/r/103715/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok, Bart Cerneels and Ralf Engels.</div>
<div>By Matěj Laitl.</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>ChangeLog <span style="color: grey">(ed4e955deeebf6a506e87ca8973645b9eeadc67b)</span></li>
<li>src/core-impl/collections/daap/daapreader/Reader.cpp <span style="color: grey">(9f749c5fe892f0cbcbacd8454d8fc80940f6bb43)</span></li>
<li>src/core-impl/collections/ipodcollection/handler/IpodHandler.h <span style="color: grey">(ceccd7cc3028051d9b93201caea180a10af15ed3)</span></li>
<li>src/core-impl/collections/ipodcollection/handler/IpodHandler.cpp <span style="color: grey">(83c5d27839dcb5f18a4233daa5367fb18962c7af)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp <span style="color: grey">(e892952ba0ede760a66e71ffa9e43c123b74a6d7)</span></li>
<li>src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp <span style="color: grey">(46000b3f1950cc64feef8321bcf698980c2742fe)</span></li>
<li>src/core-impl/collections/playdarcollection/PlaydarCollection.cpp <span style="color: grey">(1e598f0d20ea88bef4c60877eefb71a8b934218d)</span></li>
<li>src/core-impl/collections/support/MemoryCollection.h <span style="color: grey">(e2b28dea72393456f5c62ecf06ce7832b49e2e4f)</span></li>
<li>src/core-impl/collections/support/MemoryMatcher.cpp <span style="color: grey">(624d6d4c0eb63745293d083982b936e5c050ff0a)</span></li>
<li>src/core-impl/collections/support/MemoryMeta.h <span style="color: grey">(45eefb7a4c085fa32478799e296579eef117072b)</span></li>
<li>src/core-impl/collections/support/MemoryMeta.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpCache.h <span style="color: grey">(da00fd1b11306ef49e10b703f601147a5762e18e)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpCache.cpp <span style="color: grey">(58130795298911ab1849fdc64314b8ed128df3d4)</span></li>
<li>src/core-impl/meta/file/File_p.h <span style="color: grey">(af2dbf670e23357dca0efc48421c7d0d6f884f32)</span></li>
<li>src/core/meta/support/MetaKeys.h <span style="color: grey">(f4ffa0eb9889787b1cebb9ea4781930fbcf3fd01)</span></li>
<li>src/core/meta/support/MetaKeys.cpp <span style="color: grey">(3e8f0fa8587badf044c6a00806230adfe257a6fd)</span></li>
<li>src/services/ampache/AmpacheServiceQueryMaker.cpp <span style="color: grey">(72862b917fb397b02187d14cd97c1c2c5fa4f79d)</span></li>
<li>tests/TestTrackOrganizer.cpp <span style="color: grey">(75faf04b332e9e9c0ff279037ae66053e1011cd4)</span></li>
<li>tests/browsers/TestSingleCollectionTreeItemModel.cpp <span style="color: grey">(99ec36f7eae0560024669e63ca5102829d610748)</span></li>
<li>tests/synchronization/TestMasterSlaveSynchronizationJob.cpp <span style="color: grey">(707e578d042c3b13ecea7069c1d8b496896a3c5c)</span></li>
<li>tests/synchronization/TestOneWaySynchronizationJob.cpp <span style="color: grey">(058611fd8fdde491e6b2eb62c5895e20324613b1)</span></li>
<li>tests/synchronization/TestUnionJob.cpp <span style="color: grey">(591d695d1ea0641951e5aa7188a697f98b988b5c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103715/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/103715/s/416/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/01/17/amarok-ums-collections-and-album-artists_400x100.png" style="border: 1px black solid;" alt="amarok-ums-collections-and-album-artists.png" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>