> On May 30, 2013, 10:56 a.m., Matěj Laitl wrote: > > 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) > > Konrad Zemek wrote: > 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. > :) > > Matěj Laitl wrote: > 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. > > Konrad Zemek wrote: > > 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.
> 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. Oh, then it was a plain old misunderstanding, sorry for the fuss and for not phrasing myself more clearly. :) - Matěj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110658/#review33423 ----------------------------------------------------------- On May 28, 2013, 9:01 p.m., Konrad Zemek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/110658/ > ----------------------------------------------------------- > > (Updated May 28, 2013, 9:01 p.m.) > > > Review request for Amarok. > > > Description > ------- > > 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 > > > Diffs > ----- > > ChangeLog 9dd0c3dfb9c2a275ef9796bce18f7c8ae7b39360 > src/playlist/PlaylistActions.h dee8793e5386d3a44c98698631f6e8dd13a7d62f > src/playlist/PlaylistActions.cpp 1decce7c3d7bef36f314b8abcc337064438a92e0 > src/playlist/PlaylistBreadcrumbItem.h > d6ff243d7b78358f3a943de9a345d694c613aec9 > src/playlist/PlaylistBreadcrumbItem.cpp > 59fc6d2a68d86e4d0cd4c1def343ac6973bc5965 > src/playlist/PlaylistBreadcrumbItemSortButton.h > c141597614ac40c3c4adaf212f3d46b9ff360293 > src/playlist/PlaylistBreadcrumbItemSortButton.cpp > 8bf861cfcd4234a0d34c3542b9fa806ce66454de > src/playlist/PlaylistBreadcrumbLevel.cpp > 185ebfbafcbc3ef8ca80d9fd9ae42b0663a59eca > src/playlist/PlaylistController.h 5047e473b7d236d5be5bd99342d8ed731913634b > src/playlist/PlaylistController.cpp > df293a994798299335aeeb3221e2231ecb357356 > src/playlist/PlaylistDefines.h 3e10b552f86b68946b6883b0ee49c226d83315f6 > src/playlist/PlaylistModel.cpp a562bdbf0789d23e5712223593d9e09de0e29520 > src/playlist/PlaylistSortWidget.cpp > 62f760cc6a201a67be65c80bf03ae696bd44444c > src/playlist/proxymodels/SortAlgorithms.h > 2eba6a7dd4c227f55d519083f9af9cf7b08f043d > src/playlist/proxymodels/SortAlgorithms.cpp > 47b468c99e37d3ff8b148704791c5058726ac812 > src/playlist/proxymodels/SortScheme.cpp > d29f9dec6b1c2ae555146853782819328ae192f0 > tests/playlist/TestPlaylistModels.h > 3751e6a2c3d56be1a6abcea742eec088e2a1123b > tests/playlist/TestPlaylistModels.cpp > ec8adb8f2c5bbb141f291989499e361d5a73381d > > Diff: http://git.reviewboard.kde.org/r/110658/diff/ > > > Testing > ------- > > Test added for the shuffle action. GUI tested manually. > > > Thanks, > > Konrad Zemek > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
