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

[Matěj Laitl]

> On May 27, 2013, 3:52 p.m., Matěj Laitl wrote:
> > src/playlist/PlaylistController.cpp, line 450
> > <http://git.reviewboard.kde.org/r/110658/diff/2/?file=146497#file146497line450>
> >
> >     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.
> 
> Konrad Zemek wrote:
>     Actually, that's the logic that was there to begin with. I'm very hestitant to change it, as moveRows is not necessarily equipped to deal with the scenario you outline. It's actually more a 'swapRows' when you think about is; maybe we should change the name instead of changing the logic. :)

Oh I see then, so please go ahead and rename it, perhaps document the fact that set(from) must equal set(to).


> On May 27, 2013, 3:52 p.m., Matěj Laitl wrote:
> > tests/playlist/TestPlaylistModels.cpp, line 54
> > <http://git.reviewboard.kde.org/r/110658/diff/2/?file=146503#file146503line54>
> >
> >     nitpick: typo: lest
> 
> Konrad Zemek wrote:
>     Gotcha! :) 'lest' is correct in this sentence, while 'least' would be incorrect.

Oh, you're right, I didn't know the word "lest" exists, now I've learnt it. :-) I thought this was a typo for 'lets'.


> On May 27, 2013, 3:52 p.m., Matěj Laitl wrote:
> > tests/playlist/TestPlaylistModels.cpp, lines 245-253
> > <http://git.reviewboard.kde.org/r/110658/diff/2/?file=146503#file146503line245>
> >
> >     nitpick: a bit too much newlines for my taste, but this is just bikeshedding, feel free to keep this.
> 
> Konrad Zemek wrote:
>     I like to separate logical steps by a newline; I find it easier to process code when it's written like this. It so happens, that the steps here are one-liners, but I'll keep it if you wouldn't mind.

Fine with me, it makes sense.


- Matěj


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


On May 27, 2013, 6:21 p.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, 6:21 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.cpp 95d2b828be0a6fa3c73998fe6681e57743788f1d 
>   src/playlist/PlaylistDefines.h 3e10b552f86b68946b6883b0ee49c226d83315f6 
>   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/20130528/2bfbd513/attachment-0001.html>