Review Request 109369: Bug #254404: Copy files to USB storage devices in display order by sorting tracks in CollectionLocation.cpp

[Matěj Laitl]

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


This is a preliminary review, I've checked just changes to CollectionLocation API, not much of the actual implementation.


src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21668>

    bad include style and ordering



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21667>

    This is not the right approach. Instead, ensure that all *callers* of prepareCopy() already pass tracks in desired order.



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21670>

    As opposed to a method that takes a list of tracks, QueryMaker currently cannot represent multi-level sorting information. However, this change seems rather drastic to me and I'd rather spend some thoughts on how this can be avoided. Perhaps most callers already call the variant that takes a list of tracks?



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21669>

    ditto not the right approach



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21671>

    Ditto too drastic change



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21672>

    This change is right and will be needed.



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21674>

    tracks list should be already in desired order, sortLevels parameter shouldn't be needed.



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21673>

    Right, this will be needed



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21675>

    Right



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21676>

    ditto sortLevels argument shouldn't be needed here. It should be also moved back under private slots:



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21677>

    Right.



src/core/collections/CollectionLocation.h
<http://git.reviewboard.kde.org/r/109369/#comment21678>

    Please strip unrelated cleanups/reformattings into a separate patch


- Matěj Laitl


On March 9, 2013, 6:11 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109369/
> -----------------------------------------------------------
> 
> (Updated March 9, 2013, 6:11 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> 1. Made a levelSort() function to sort tracks according to multiple parameters ( Should i just be sorting tracks belonging to the same album? )
> 2. Modified copyUrlToCollection() to use QList instead of QMap
> 3. FileView's files are being copied in the order of selection, thanks to using QLists ( is that acceptable? )
> 
> 
> Diffs
> -----
> 
>   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.cpp aa61072 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.cpp 93efe97 
>   src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h 2b06cb4 
>   src/services/ServiceCollectionLocation.cpp d1cb0d8 
>   src/core/collections/CollectionLocation.h d37ccfb 
>   src/core/collections/CollectionLocation.cpp aecc068 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.cpp e0ba0ac 
>   src/core-impl/collections/umscollection/UmsCollectionLocation.h 45ba596 
>   src/core-impl/collections/umscollection/UmsCollection.cpp 6bebd98 
>   src/core-impl/collections/support/TrashCollectionLocation.cpp 61c2e49 
>   src/core-impl/collections/support/TrashCollectionLocation.h 239a977 
>   src/core-impl/collections/support/PlaylistCollectionLocation.cpp c885046 
>   src/core-impl/collections/mtpcollection/handler/MtpHandler.cpp a8d9f52 
>   src/core-impl/collections/support/PlaylistCollectionLocation.h 10a365f 
>   src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp c1b76f5 
>   src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.h 821f1b0 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp f60aff6 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h e40529f 
>   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp 8a40c6c 
>   src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h 3c2d9f2 
>   src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp f8105f9 
>   src/core-impl/collections/ipodcollection/IpodCollectionLocation.h cc27e19 
>   src/core-impl/collections/db/sql/SqlCollectionLocation.h 0bcf244 
>   src/core-impl/collections/audiocd/AudioCdCollectionLocation.cpp be13551 
>   src/browsers/filebrowser/FileView.h df8fbee 
>   src/browsers/CollectionTreeItemModel.cpp fa194b1 
>   src/browsers/CollectionTreeView.cpp fd9fe66 
>   src/browsers/collectionbrowser/CollectionWidget.h c281f41 
> 
> Diff: http://git.reviewboard.kde.org/r/109369/diff/
> 
> 
> Testing
> -------
> 
> Seems to be copying tracks in the correct order now
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130311/32b393a5/attachment-0001.html>