> 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.
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. :) > 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 Gotcha! :) 'lest' is correct in this sentence, while 'least' would be incorrect. > 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. 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. - Konrad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110658/#review33227 ----------------------------------------------------------- On May 27, 2013, 6:04 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:04 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
