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

Thu Nov 21 12:24:34 UTC 2013

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > cmake/modules/FindMtp.cmake, line 52
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210110#file210110line52>
> >
> >     The wording is a bit convoluted.
> >     
> >     Probably something like "Found MTP, but we want at least version ${MTP_MIN_VERSION}" would be better.
> 
> Myriam Schweingruber wrote:
>     How about: "Found MTP, but version ${MTP_MIN_VERSION} is needed"

Fixed by means of
      message(STATUS "Found MTP, but too old. Version ${MTP_MIN_VERSION} or greater is needed")

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, line 345
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line345>
> >
> >     Oh my. I believe this is one of the manifestations of MTP creator's genius?

This was actually my paranoia (that fires every time when loop "finiteness" depends on external data), but I wouldn't be surprised if there ware crappy MTP devices out there in the wild needing this.

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, lines 547-553
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line547>
> >
> >     (same as the above)

Because I want the same string to be at four (two) places and .ui doesn't allow me (AFAIK) to share these. Making the exact change at four places every time gets boring pretty quickly; it is also beneficiary for translators to keep count of unique strings low and this eliminates the possibility of the strings going out of sync.

This is in no way a strong opinion, still this tickles my internal DRY principle [1] alarm. If you know a way how to share a string in a .ui file, I'm all in.

[1] http://www.artima.com/intv/dry.html

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, line 619
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line619>
> >
> >     Do you do this just to avoid saving a pointer to the dialog, or is there any other reason?

No other reason, and now I see that I can eliminate sender() entirely by connecting dialog's deleteLater() right to SIGNAL(finished()). (Qt guarantees slot execution order)

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, lines 656-657
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line656>
> >
> >     Potential race condition: job pointee might get deleted after this check.

Not really, with current code. All jobs passed to MtpCollection::registerTransferJob() have QObject thread affinity set to the main thread, are only deleted using QObject::deleteLater() and MtpCollection::slotRegisterTransferJob() always happens in the main thread.

I've extended MtpCollection::registerTransferJob() documentation with requirements imposed on the passed job.

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, line 679
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line679>
> >
> >     Obscure comment.

Clarified: // replaces any possible leftover null pointer

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, lines 700-703
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line700>
> >
> >     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.

Yes, but addTrack( LIBMTP_track_t *mtpTrack ) is only called by ParseMtpTracksJob, and MtpCollection does its best to only have one ParstTracksJob at a time. I've extended addTrack() documentation to mention assumptions for thread safety.

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollection.cpp, lines 737-739
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210114#file210114line737>
> >
> >     (same as above)

MemoryMeta::MapChanger::removeTrack() works fine if you pass a track that is no longer in the collection, so only harm done in this unlikely race condition is a spurious emitted warning.

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/MtpCollectionFactory.cpp, lines 101-103
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210116#file210116line101>
> >
> >     I'm ordinarily all for extra paranoid checking, but I am curious, why do you have this check here and nowhere else?

This is rather a check that sender() and qobject_cast suceeded (the error message tries to infer the reason, perhaps it is a bit confusing because of it). Other methods in the factory don't depend on sender() or qobject_cast's.

> On Nov. 18, 2013, 12:49 a.m., Edward Toroshchin wrote:
> > src/core-impl/collections/mtp/TODO, lines 1-2
> > <http://git.reviewboard.kde.org/r/113666/diff/1/?file=210119#file210119line1>
> >
> >     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.

This is a kind of a left-over from this patch being a GSoC project - the plan is to resolve the 2 remaining TODO's here (ideally before 2.9 (beta?) release) and remove this file then - certainly not to add more items.

- Matěj

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

On Nov. 21, 2013, 1:24 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. 21, 2013, 1:24 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
> -----
> 
>   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 
>   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 
> 
> 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/20131121/467cce1c/attachment-0001.html>