Review Request 113666: GSoC: MTP (Android) Collection Rewrite

Edward Toroshchin edward.hades at gmail.com
Sun Nov 17 23:49:14 UTC 2013 

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



cmake/modules/FindMtp.cmake
<http://git.reviewboard.kde.org/r/113666/#comment31503>

    The wording is a bit convoluted.
    
    Probably something like "Found MTP, but we want at least version ${MTP_MIN_VERSION}" would be better.



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31504>

    Oh my. I believe this is one of the manifestations of MTP creator's genius?



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31505>

    Why don't you set this in the ui?



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31506>

    (same as the above)



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31507>

    Do you do this just to avoid saving a pointer to the dialog, or is there any other reason?



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31508>

    Potential race condition: job pointee might get deleted after this check.



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31509>

    Nice class name!



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31510>

    Obscure comment.



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31511>

    I believe you should acquire a write lock for the whole transaction here. Otherwise you get a race condition, if two threads try to add the same track.



src/core-impl/collections/mtp/MtpCollection.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31512>

    (same as above)



src/core-impl/collections/mtp/MtpCollectionFactory.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31513>

    (as discussed in IRC)
    
    Construct cache dir path once for the whole lifetime of MtpCollectionFactory.



src/core-impl/collections/mtp/MtpCollectionFactory.cpp
<http://git.reviewboard.kde.org/r/113666/#comment31514>

    I'm ordinarily all for extra paranoid checking, but I am curious, why do you have this check here and nowhere else?



src/core-impl/collections/mtp/TODO
<http://git.reviewboard.kde.org/r/113666/#comment31502>

    Are you sure it is a good idea to have per-folder TODO files? I'd personally keep them in bugzilla or in "// TODO" comments.


- Edward Toroshchin


On Nov. 5, 2013, 11:34 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113666/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2013, 11:34 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> -------
> 
> This is my full GSoC project to rewrite MTP collection.
> 
> Notes:
>  * Individual commits can be seen at http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.git&a=shortlog&h=gsoc
>  * First commit to remove the old implementation is not included here for brevity
>  * Project description can be seen at https://google-melange.appspot.com/gsoc/project/google/gsoc2013/strohel/32001
>  * More info, progress reports and final report available at http://strohel.blogspot.com/search/label/gsoc
> 
> 
> Diffs
> -----
> 
>   cmake/modules/FindMtp.cmake a6b7a0e 
>   src/core-impl/collections/CMakeLists.txt 4785158 
>   src/core-impl/collections/mtp/CMakeLists.txt PRE-CREATION 
>   src/core-impl/collections/mtp/MtpCollection.h PRE-CREATION 
>   src/core-impl/collections/mtp/MtpCollection.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/MtpCollectionFactory.h PRE-CREATION 
>   src/core-impl/collections/mtp/MtpCollectionFactory.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/MtpCollectionLocation.h PRE-CREATION 
>   src/core-impl/collections/mtp/MtpCollectionLocation.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/TODO PRE-CREATION 
>   src/core-impl/collections/mtp/amarok-mtp-manage-music.desktop PRE-CREATION 
>   src/core-impl/collections/mtp/amarok_collection-mtp.desktop PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/CopyMtpTracksJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/CopyMtpTracksJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/DeleteMtpTracksJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/DeleteMtpTracksJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/DownloadMtpTrackJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/DownloadMtpTrackJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/InitMtpDeviceJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/InitMtpDeviceJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/ListMtpStorageJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/ListMtpStorageJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/MtpTransferJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/MtpTransferJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/ParseMtpTracksJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/ParseMtpTracksJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/SetMtpDeviceNameJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/SetMtpDeviceNameJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/UpdateMtpTrackJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/UpdateMtpTrackJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/UploadMtpTrackJob.h PRE-CREATION 
>   src/core-impl/collections/mtp/jobs/UploadMtpTrackJob.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/meta/MtpAlbum.h PRE-CREATION 
>   src/core-impl/collections/mtp/meta/MtpAlbum.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/meta/MtpEntity.h PRE-CREATION 
>   src/core-impl/collections/mtp/meta/MtpTrack.h PRE-CREATION 
>   src/core-impl/collections/mtp/meta/MtpTrack.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpDeviceConfiguration.ui PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpHelpers.h PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpHelpers.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpTranscodeCapability.h PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpTranscodeCapability.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpTransferJanitor.h PRE-CREATION 
>   src/core-impl/collections/mtp/support/MtpTransferJanitor.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/support/OneToOneMap.h PRE-CREATION 
>   src/core-impl/collections/mtp/support/OneToOneMap.cpp PRE-CREATION 
>   src/core-impl/collections/mtp/support/forward_declarations.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/113666/diff/
> 
> 
> Testing
> -------
> 
> Works well with my Samsung Android 4.1 phone; testing with greater variety of devices needed.
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

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

More information about the Amarok-devel mailing list