<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 30th, 2013, 10:04 p.m. UTC, <b>Edward Hades Toroshchin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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="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>
</blockquote>
<p>On July 31st, 2013, 1:20 a.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;">I think that we can safely file tokens without either "asc" or "des" as ill-formed, as with removal of the "Shuffle" special case everything is either one or the other.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">@Edward: Could you tell me how can I check if the shuffle sort actually works after restart? I'm not sure what the expected behaviour would be in that case.
Also, a final word on whether missing "asc"/"desc" is supported would be nice.</pre>
<br />
<p>- Fabian</p>
<br />
<p>On July 31st, 2013, 9:47 a.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 31, 2013, 9:47 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;">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>