<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 30th, 2013, 10:56 a.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 good, although I've spotted last error:
1. add a normal sorting, e.g. Album
2. expand the leftmost arrow down, click Shuffle
3. the Album sorting is not reset (as it should be; other "normal" sorting items correctly reset the Album sorting)</pre>
</blockquote>
<p>On May 30th, 2013, 6:33 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;">Actually, this is not a bug; that's how I designed it.
This is my very use-case: I have my playlist sorted by album at all times, yet I like to shuffle tracks around (that's especially useful for my various-artists albums). I feel that
a) my sorting choice should not be reset by shuffle action - this just annoys me because I have to set my whole sorting path again, and
b) having shuffle action only under the leftmost arrow, I am given a hint that it works on a more general scope, and having it separated from the sorting levels I am given a hint that it works differently.
These two lead me to the point, that is: although it may be surprising on the first use (what isn't?), the GUI setup makes this behavior feel natural or, at least, not awkward. And I feel that it's useful - it sure is for me. So while I am not hell-bent on making it stay, I stand for my design choice. :)</pre>
</blockquote>
<p>On May 30th, 2013, 6:56 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;">This is absolutely confusing, you also should have mentioned it in the description instead of trying to pursue it without us knowing (despite me explicitly suggesting the behaviour to rest the sorting). The UI must never be inconsistent, at least never intentionally without proper rationale. Clicking sorting button at level N must always replace existing sorting at level N. Period.
In this regard and your use-case, I actually think that something closer to the original behaviour would be better: have Shuffle at each level. (so that clicking on left-most would reset all levels, clicking on right-most would do what you want for your use-case)
The goal of Amarok is to be usable by "ordinary" users (think of your girlfirend), I know that most of us came to it as scratching its own itch, but we should really think about it becore introducing $next_supper_witty_beahviour or $suboption_of_already_a_microption.</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;">> This is absolutely confusing, you also should have mentioned it in the description instead of trying to pursue it without us knowing (despite me explicitly suggesting the behaviour to rest the sorting)
Actually, this was natural enough to me, that I understood your "show Shuffle *only* under the leftmost arrow" suggestion as exactly this: show shuffle under the leftmost arrow, and make it not reset the state. Especially since you presented this option against "make Shuffle reset all sorting levels", which by contrast reassured me that we were thinking about the same thing. I wasn't trying to introduce a supper witty behavior behind anyone's back.
That being said I understand perfectly what you're getting at here. I think that the behavior you described above is exactly how it should be, for consistency's sake. That's how I wanted it to be at the beginning, but as I noted above, I gravely misunderstood your suggestion. So sorry for the hassle, I'll get it fixed.</pre>
<br />
<p>- Konrad</p>
<br />
<p>On May 28th, 2013, 9:01 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 28, 2013, 9:01 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.h <span style="color: grey">(5047e473b7d236d5be5bd99342d8ed731913634b)</span></li>
<li>src/playlist/PlaylistController.cpp <span style="color: grey">(df293a994798299335aeeb3221e2231ecb357356)</span></li>
<li>src/playlist/PlaylistDefines.h <span style="color: grey">(3e10b552f86b68946b6883b0ee49c226d83315f6)</span></li>
<li>src/playlist/PlaylistModel.cpp <span style="color: grey">(a562bdbf0789d23e5712223593d9e09de0e29520)</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>