----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
