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

[Matěj Laitl]

> On March 11, 2013, 2:20 p.m., Matěj Laitl wrote:
> > src/core/collections/CollectionLocation.h, lines 165-166
> > <http://git.reviewboard.kde.org/r/109369/diff/1/?file=118797#file118797line165>
> >
> >     This is not the right approach. Instead, ensure that all *callers* of prepareCopy() already pass tracks in desired order.
> 
> Anmol Ahuja wrote:
>     But prepareCopy functions are called in around 11 different files! Should I overload these functions in UmsCollectionLocationcpp and sort the tracks there?
> 
> Anmol Ahuja wrote:
>     Oh, I was wrong, sorry, fixing it
> 
> Matěj Laitl wrote:
>     > Should I overload these functions in UmsCollectionLocationcpp and sort the tracks there?
>     
>     No. Instead, go though the 11 files and check whether they pass the tracks in desired order. Many of they probably already do.
> 
> Anmol Ahuja wrote:
>     But wouldn't it still be better to overload these in UmsCollectionLocation.cpp? The order doesn't matter for the other CollectionLocations, it's just unnecessary overhead.

Well, the original point was not to *sort* the tracks somewhere, but to *keep them in original sorting*. While it is not possible everywhere, but for example when copying from playlist it is well possible and in fact the only possible way to do it. You cannot achieve correct copy order from playlist by just extending prepareCopy(), in addition to the fact that it is ugly, redundant, and just plain wrong.


- Matěj


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


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/bbaaeaa4/attachment.html>