Review Request 110070: Cleaned up playlist multilevel sorting algorithm. Added album artist special case to playlist sorting.

[Matěj Laitl]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130419/014b053f/attachment.html>