<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 />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This looks even better, I think I'll just merge it once you resolve the nitpicks below. (as I understand the code better now)</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/110070/diff/4/?file=139676#file139676line224" style="color: black; font-weight: bold; text-decoration: underline;">src/playlist/proxymodels/SortAlgorithms.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">171</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">left</span><span class="o">-></span><span class="n">sortableName</span><span class="p">().</span><span class="n">compare</span><span class="p">(</span><span class="n">right</span><span class="o">-></span><span class="n">sortableName</span><span class="p">(),</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">172</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Qt</span><span class="o">::</span><span class="n">CaseInsensitive</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">nitpick: spaces around arguments per Amarok coding style</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/110070/diff/4/?file=139677#file139677line56" style="color: black; font-weight: bold; text-decoration: underline;">src/playlist/proxymodels/SortScheme.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class SortScheme</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">55</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">SortScheme</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">56</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">SortScheme</span> <span class="o">:</span> <span class="n">private</span> <span class="n">QStack</span><span class="o"><</span> <span class="n">SortLevel</span> <span class="o">></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">style nitpick: we usually don't add spaces around template arguments: QStack<SortLevel> (but as you see, we were not really consistent about it)</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/110070/diff/4/?file=139677#file139677line66" style="color: black; font-weight: bold; text-decoration: underline;">src/playlist/proxymodels/SortScheme.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class SortScheme</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">65</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nl">private:</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">66</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">typedef</span> <span class="n">QStack</span><span class="o">::</span><span class="n">const_iterator</span> <span class="n">const_iterator</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think some compilers choke on "implicit template argument" like this, safer way would be: typedef QStack<SortLevel>::const_iterator ...; see http://quickgit.kde.org/?p=amarok.git&a=commitdiff&h=1140d3056af65ddf0f3999df846f1fd44970c303
OTOH, this typedef seems to be needed just to make declarations of below methods shorter, right? In that case it may not be worth it.</pre>
</div>
<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>