<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 17th, 2012, 6:40 p.m., <b>Ralf Engels</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On January 17th, 2012, 2:10 p.m., Matěj Laitl wrote:</p>
<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>
<p style="color: grey;"><i>Updated Jan. 17, 2012, 2:10 p.m.</i></p>
<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>