Review Request 110658: Playlist sort widget: reimplement Shuffle "sort" as an action.

[Matěj Laitl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110658/#review33227
-----------------------------------------------------------


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.


src/playlist/PlaylistController.cpp
<http://git.reviewboard.kde.org/r/110658/#comment24575>

    nitpick: when it improves readability, we prefer single variable declaration per line, this seems to be the case.



src/playlist/PlaylistController.cpp
<http://git.reviewboard.kde.org/r/110658/#comment24576>

    1. Nice way to checking every item is there just once! ;)
    
    2. I think that fromItems == toItems is unssecessary assumption. I think it is perfectly valid to move rows 2, 3 to places 14, 15, assuming the playlist is long enough.



tests/playlist/TestPlaylistModels.cpp
<http://git.reviewboard.kde.org/r/110658/#comment24577>

    nitpick: typo: lest



tests/playlist/TestPlaylistModels.cpp
<http://git.reviewboard.kde.org/r/110658/#comment24578>

    nitpick: a bit too much newlines for my taste, but this is just bikeshedding, feel free to keep this.


- Matěj Laitl


On May 27, 2013, 12:26 a.m., Konrad Zemek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110658/
> -----------------------------------------------------------
> 
> (Updated May 27, 2013, 12:26 a.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
> -----
> 
>   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.cpp 95d2b828be0a6fa3c73998fe6681e57743788f1d 
>   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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130527/d83f621c/attachment.html>