<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/110658/">http://git.reviewboard.kde.org/r/110658/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 27th, 2013, 3:52 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;">Looks very well, *BIG* kudos for cleaning up all places that can now be simplified, we love patches that actually remove more lines than they add while improving functionality. But perhaps Shuffle can be removed also from Playlist::Column enum and Playlist::columnForName().
BTW I've tried and it works fine, only consideration is the behavirour when the Shuffle is used for example after "Artist" - currently the effect is not visible unless you have multiple same artists, which looks confusing. Another problem is that "Shuffle" is not visible under leftmost arrow when you already have some sorting levels active. (more specifically, it is shown only under the rightmost arrow)
I suggest either
a) show Shuffle *only* under the leftmost arrow (which I prefer)
b) make Shuffle reset all sorting levels
Normally I would think whether this is important enought to be added to ChangeLog, but bug number exists, then is is simple: bug exists -> should be in ChangeLog, this one under BUGFIXES.
Also thanks for including proper tags in commit msg.</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;">I've changed it so that Shuffle is showed only under the leftmost arrow. It was surprisingly tricky, since the rightmost arrow is a separate entity from other elements (that's why 'Shuffle' was displayed only under this arrow).</pre>
<br />
<p>- Konrad</p>
<br />
<p>On May 27th, 2013, 6:04 p.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 May 27, 2013, 6:04 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;">Playlist sort widget: reimplement Shuffle "sort" as an action.
Remove now-unnecessary Shuffle handlers in sort-related functions. Additionally, PlaylistController::MoveRows has been modified to help with big move actions (validation complexity reduced).
GUI: In the playlist sort widget, the Shuffle menu entry is now separated from other entries. Activating the entry no longer results in a "Shuffle" sort level being added.
BUG: 320129
FIXED-IN: 2.8</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;">Test added for the shuffle action. GUI tested manually.</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">(9dd0c3dfb9c2a275ef9796bce18f7c8ae7b39360)</span></li>
<li>src/playlist/PlaylistActions.h <span style="color: grey">(dee8793e5386d3a44c98698631f6e8dd13a7d62f)</span></li>
<li>src/playlist/PlaylistActions.cpp <span style="color: grey">(1decce7c3d7bef36f314b8abcc337064438a92e0)</span></li>
<li>src/playlist/PlaylistBreadcrumbItem.h <span style="color: grey">(d6ff243d7b78358f3a943de9a345d694c613aec9)</span></li>
<li>src/playlist/PlaylistBreadcrumbItem.cpp <span style="color: grey">(59fc6d2a68d86e4d0cd4c1def343ac6973bc5965)</span></li>
<li>src/playlist/PlaylistBreadcrumbItemSortButton.h <span style="color: grey">(c141597614ac40c3c4adaf212f3d46b9ff360293)</span></li>
<li>src/playlist/PlaylistBreadcrumbItemSortButton.cpp <span style="color: grey">(8bf861cfcd4234a0d34c3542b9fa806ce66454de)</span></li>
<li>src/playlist/PlaylistBreadcrumbLevel.cpp <span style="color: grey">(185ebfbafcbc3ef8ca80d9fd9ae42b0663a59eca)</span></li>
<li>src/playlist/PlaylistController.cpp <span style="color: grey">(95d2b828be0a6fa3c73998fe6681e57743788f1d)</span></li>
<li>src/playlist/PlaylistDefines.h <span style="color: grey">(3e10b552f86b68946b6883b0ee49c226d83315f6)</span></li>
<li>src/playlist/PlaylistSortWidget.cpp <span style="color: grey">(62f760cc6a201a67be65c80bf03ae696bd44444c)</span></li>
<li>src/playlist/proxymodels/SortAlgorithms.h <span style="color: grey">(2eba6a7dd4c227f55d519083f9af9cf7b08f043d)</span></li>
<li>src/playlist/proxymodels/SortAlgorithms.cpp <span style="color: grey">(47b468c99e37d3ff8b148704791c5058726ac812)</span></li>
<li>src/playlist/proxymodels/SortScheme.cpp <span style="color: grey">(d29f9dec6b1c2ae555146853782819328ae192f0)</span></li>
<li>tests/playlist/TestPlaylistModels.h <span style="color: grey">(3751e6a2c3d56be1a6abcea742eec088e2a1123b)</span></li>
<li>tests/playlist/TestPlaylistModels.cpp <span style="color: grey">(ec8adb8f2c5bbb141f291989499e361d5a73381d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110658/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>