<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/110070/">http://git.reviewboard.kde.org/r/110070/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 18th, 2013, 12:30 p.m. UTC, <b>Matěj Laitl</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;">Thanks for the investigation and the patch, it looks very good and contains very welcome cleanups. Also kudos for including & cleaning up the test case. As I'm not really proficient in this part of Amarok code, let's give other devs a day or 2 to have s chance to review this. In the meantime you might solve the nitpick below.</pre>
</blockquote>
<p>On April 18th, 2013, 11:39 p.m. UTC, <b>Konrad Zemek</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;">Thanks for the review! (And sorry if this is a wrong way to respond, reviewboard still makes me a little bit confused.)
These changes are in whole more of a cleanup than a refactor. The only logic that change here is the one about comparing by album artist when album names are the same (which solves a yet-unexisting bugreport, which I'll create later).
I feel that what I've done here make some possibilities for more serious changes clearer. What I noticed in particular is that we're performing the same comparisons in multiple places, but never using quite the same logic (e.g. comparisons done for sorting collections and comparisons done for sorting playlist), so there's a lot of opportunity to streamline things, "harden" logic, and make it consistent. I may be picking up on it if the time permits.
A side note: the lines comparing Album, AlbumArtist, Artist, are all pretty much identical apart from the types used. This would definitely be resolved as a part of abovementioned streamlining, but for now maybe a templated (ducktyped) helper function would be optimal to reduce code duplication.</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;">> (And sorry if this is a wrong way to respond, reviewboard still makes me a little bit confused.)
This is the right way. We all fight reviewboard, I'm not happy with it, but we got it maintained for free by KDE Sysadmins...
> I feel that what I've done here make some possibilities for more serious changes clearer. What I noticed in particular is that we're performing the same comparisons in multiple places, but never using quite the same logic (e.g. comparisons done for sorting collections and comparisons done for sorting playlist), so there's a lot of opportunity to streamline things, "harden" logic, and make it consistent. I may be picking up on it if the time permits.
That would be great! This code is really a candidate for factoring out to something generic & shared by all places that need it. (see for example in-the-works https://git.reviewboard.kde.org/r/109369/ that would benefit from it ) One thing that prevents this factoring out is that Playlist::SortLevel uses playlist-specific Playlist::Column enum instead of a generetic Meta::val* field identifiers (and associated playlist-specific things). Also note that we like small and gradual (yet self-contained) patches, so 1st patch could be to use Meta::val* in SortOrder, second to move the sorting algo to a better place, third to actually use it in another place... While this is a bit more work, it really helps code review and finding regressions.
> A side note: the lines comparing Album, AlbumArtist, Artist, are all pretty much identical apart from the types used. This would definitely be resolved as a part of abovementioned streamlining, but for now maybe a templated (ducktyped) helper function would be optimal to reduce code duplication.
Well done.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On April 19th, 2013, 12:08 a.m. UTC, Konrad Zemek wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Konrad Zemek.</div>
<p style="color: grey;"><i>Updated April 19, 2013, 12:08 a.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;">Cleaned up playlist multilevel sorting algorithm. Added album artist special case to playlist sorting.
It fixes an issue with two albums being merged on the playlist when they happen to have the same name, and sorting by album is enabled.
Originally thought to fix bug 271105, it actually fixes some other, but connected, one (see my comment https://bugs.kde.org/show_bug.cgi?id=271105#c10 ).
I'll search for a bug report and submit one if I find none.</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;">Manual tests + relevant unittest added in testplaylistmodels</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=271105">271105</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/playlist/proxymodels/SortAlgorithms.h <span style="color: grey">(da18fcc2107d83515ff176746e751fe00e786362)</span></li>
<li>src/playlist/proxymodels/SortAlgorithms.cpp <span style="color: grey">(c7d0df9f6b86318fd896a6de08fef6d85623215b)</span></li>
<li>src/playlist/proxymodels/SortScheme.h <span style="color: grey">(0fcd3a580deff7f2b4e6e65d0551c7332e0b6c9e)</span></li>
<li>src/playlist/proxymodels/SortScheme.cpp <span style="color: grey">(de4f37adaeb6e8bb914eb7c91ff4803d01fc5623)</span></li>
<li>tests/mocks/MetaMock.h <span style="color: grey">(03fea2aaf7f27236c43a8cb78c241078414dd39c)</span></li>
<li>tests/playlist/TestPlaylistModels.cpp <span style="color: grey">(8964333517e431487b6ad4ba50d3e359f51bf3f6)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110070/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>