Review Request 110070: Indented case statements

[Konrad Zemek]

> 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.

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.


- Konrad


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


On April 18, 2013, 11:17 p.m., Konrad Zemek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110070/
> -----------------------------------------------------------
> 
> (Updated April 18, 2013, 11:17 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Indented case statements
> 
> 
> Added album artist special case to playlist sorting. Fixed a logic bug.
> 
> 
> Refactored playlist multilevel sorting algorithm.
> 
> 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.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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130418/2cf4f1f7/attachment.html>