> On April 18, 2013, 12:30 p.m., Matěj Laitl wrote: > > Thanks for the investigation and the patch, it looks very good and contains > > very welcome cleanups. Also kudos for including & cleaning up the test > > case. As I'm not really proficient in this part of Amarok code, let's give > > other devs a day or 2 to have s chance to review this. In the meantime you > > might solve the nitpick below. > > Konrad Zemek wrote: > Thanks for the review! (And sorry if this is a wrong way to respond, > reviewboard still makes me a little bit confused.) > These changes are in whole more of a cleanup than a refactor. The only > logic that change here is the one about comparing by album artist when album > names are the same (which solves a yet-unexisting bugreport, which I'll > create later). > I feel that what I've done here make some possibilities for more serious > changes clearer. What I noticed in particular is that we're performing the > same comparisons in multiple places, but never using quite the same logic > (e.g. comparisons done for sorting collections and comparisons done for > sorting playlist), so there's a lot of opportunity to streamline things, > "harden" logic, and make it consistent. I may be picking up on it if the time > permits. > > A side note: the lines comparing Album, AlbumArtist, Artist, are all > pretty much identical apart from the types used. This would definitely be > resolved as a part of abovementioned streamlining, but for now maybe a > templated (ducktyped) helper function would be optimal to reduce code > duplication.
> (And sorry if this is a wrong way to respond, reviewboard still makes me a > little bit confused.) This is the right way. We all fight reviewboard, I'm not happy with it, but we got it maintained for free by KDE Sysadmins... > I feel that what I've done here make some possibilities for more serious > changes clearer. What I noticed in particular is that we're performing the > same comparisons in multiple places, but never using quite the same logic > (e.g. comparisons done for sorting collections and comparisons done for > sorting playlist), so there's a lot of opportunity to streamline things, > "harden" logic, and make it consistent. I may be picking up on it if the time > permits. That would be great! This code is really a candidate for factoring out to something generic & shared by all places that need it. (see for example in-the-works https://git.reviewboard.kde.org/r/109369/ that would benefit from it ) One thing that prevents this factoring out is that Playlist::SortLevel uses playlist-specific Playlist::Column enum instead of a generetic Meta::val* field identifiers (and associated playlist-specific things). Also note that we like small and gradual (yet self-contained) patches, so 1st patch could be to use Meta::val* in SortOrder, second to move the sorting algo to a better place, third to actually use it in another place... While this is a bit more work, it really helps code review and finding regressions. > A side note: the lines comparing Album, AlbumArtist, Artist, are all pretty > much identical apart from the types used. This would definitely be resolved > as a part of abovementioned streamlining, but for now maybe a templated > (ducktyped) helper function would be optimal to reduce code duplication. Well done. - Matěj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110070/#review31241 ----------------------------------------------------------- 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
