<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/111804/">http://git.reviewboard.kde.org/r/111804/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thank you very much for the patch.
I understand the problem, however the solution could be improved a bit.
Please see the code comments, address them, and we'll push your patch.</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/111804/diff/1/?file=175527#file175527line73" style="color: black; font-weight: bold; text-decoration: underline;">src/playlist/PlaylistSortWidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">SortWidget::SortWidget( QWidget *parent )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">73</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">levelParts</span><span class="p">.</span><span class="n"><span class="hl">at</span></span><span class="p">(</span> <span class="mi">1</span> <span class="p">)</span> <span class="o">==</span> <span class="n">QString</span><span class="p">(</span> <span class="s">"asc"</span> <span class="p">)</span> <span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">73</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">levelParts</span><span class="p">.</span><span class="n"><span class="hl">value</span></span><span class="p">(</span> <span class="mi">1</span> <span class="p">)</span> <span class="o">==</span> <span class="n">QString</span><span class="p">(</span> <span class="s">"asc"</span> <span class="p">)</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;">If the sort token has neither "asc" nor "des" suffix, it won't be added to the sort at all.
I suggest you make the line 73 something like that:
if( levelParts.count() < 2 || levelParts.at( 1 ) == QString( "asc" ) )
This way it will assume ascending order by default, if no specifier is present.
Please check, if this indeed compiles and works. Please also check that the "Shuffle" playlist sort works after Amarok restart. (I believe it isn't in the current solution.)</pre>
</div>
<br />
<p>- Edward Hades</p>
<br />
<p>On July 30th, 2013, 8:16 p.m. UTC, Fabian Kosmale 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 Fabian Kosmale.</div>
<p style="color: grey;"><i>Updated July 30, 2013, 8:16 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;">Without this patch, Amarok would crash with
ASSERT failure in QList<T>::at: "index out of range"
when amarokrc contained
[Playlist Sorting]
SortPath=Shuffle
</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;">Compiled and started Amarok. It doesn't crash anymore.</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>src/playlist/PlaylistSortWidget.cpp <span style="color: grey">(384e4a0eaa28f82eb27665a1a0d497e018a61161)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111804/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>