> On May 28, 2013, 6:43 p.m., Edward Hades Toroshchin wrote: > > src/playlist/PlaylistActions.cpp, line 355 > > <http://git.reviewboard.kde.org/r/110658/diff/4/?file=146653#file146653line355> > > > > Why not QVector? > > Matěj Laitl wrote: > Well, PlaylistController::moveRows() accepts QLists, so you'd have to > convert them.
One reason is that moveRows accepts references to QLists, and I didn't want to modify the signature (although, looking back, I'm not sure that it should necessarily take non-const refs, thus requiring lvalues). I feel this is acceptable, as vector-type container does not hold any significant advantage over deque-type one in this usecase. > On May 28, 2013, 6:43 p.m., Edward Hades Toroshchin wrote: > > src/playlist/PlaylistActions.cpp, line 357 > > <http://git.reviewboard.kde.org/r/110658/diff/4/?file=146653#file146653line357> > > > > Do you really need a nested block here? > > Matěj Laitl wrote: > I see it as a logical section to perform one task, with an advantage that > it self-documents difference between "auxiliary" and "important" variables > (so that "auxiliary" variables don't "leak" somewhere where they've lost > their meaning), so I'm personally fine with this. Doing this, I had in mind exactly what Mat?j said above. :) - Konrad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110658/#review33315 ----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
