----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110070/#review31279 -----------------------------------------------------------
Ship it! This looks even better, I think I'll just merge it once you resolve the nitpicks below. (as I understand the code better now) src/playlist/proxymodels/SortAlgorithms.cpp <http://git.reviewboard.kde.org/r/110070/#comment23287> nitpick: spaces around arguments per Amarok coding style src/playlist/proxymodels/SortScheme.h <http://git.reviewboard.kde.org/r/110070/#comment23288> style nitpick: we usually don't add spaces around template arguments: QStack<SortLevel> (but as you see, we were not really consistent about it) src/playlist/proxymodels/SortScheme.h <http://git.reviewboard.kde.org/r/110070/#comment23289> I think some compilers choke on "implicit template argument" like this, safer way would be: typedef QStack<SortLevel>::const_iterator ...; see http://quickgit.kde.org/?p=amarok.git&a=commitdiff&h=1140d3056af65ddf0f3999df846f1fd44970c303 OTOH, this typedef seems to be needed just to make declarations of below methods shorter, right? In that case it may not be worth it. - Matěj Laitl On April 19, 2013, 12:08 a.m., Konrad Zemek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/110070/ > ----------------------------------------------------------- > > (Updated April 19, 2013, 12:08 a.m.) > > > Review request for Amarok. > > > Description > ------- > > Cleaned up playlist multilevel sorting algorithm. Added album artist special > case to playlist sorting. > > It fixes an issue with two albums being merged on the playlist when they > happen to have the same name, and sorting by album is enabled. > Originally thought to fix bug 271105, it actually fixes some other, but > connected, one (see my comment > https://bugs.kde.org/show_bug.cgi?id=271105#c10 ). > I'll search for a bug report and submit one if I find none. > > > This addresses bug 271105. > https://bugs.kde.org/show_bug.cgi?id=271105 > > > Diffs > ----- > > src/playlist/proxymodels/SortAlgorithms.h > da18fcc2107d83515ff176746e751fe00e786362 > src/playlist/proxymodels/SortAlgorithms.cpp > c7d0df9f6b86318fd896a6de08fef6d85623215b > src/playlist/proxymodels/SortScheme.h > 0fcd3a580deff7f2b4e6e65d0551c7332e0b6c9e > src/playlist/proxymodels/SortScheme.cpp > de4f37adaeb6e8bb914eb7c91ff4803d01fc5623 > tests/mocks/MetaMock.h 03fea2aaf7f27236c43a8cb78c241078414dd39c > tests/playlist/TestPlaylistModels.cpp > 8964333517e431487b6ad4ba50d3e359f51bf3f6 > > Diff: http://git.reviewboard.kde.org/r/110070/diff/ > > > Testing > ------- > > Manual tests + relevant unittest added in testplaylistmodels > > > Thanks, > > Konrad Zemek > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
