Review Request: Statistics Synchronization: final review request

Matěj Laitl matej at laitl.cz
Sun Nov 18 22:30:11 UTC 2012 


> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > Haven't run it yet, but at least it compiles okay :)
> 
> Matěj Laitl wrote:
>     Thanks for the attentive review, I didn't expect somebody to dive that deep and I'm grateful.

So, what's your "ready to be merged" opinion on this, Edward?


> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/services/lastfm/SynchronizationAdapter.cpp, lines 38-43
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94955#file94955line38>
> >
> >     This should be Qt::AutoConnection. Otherwise it will deadlock if artists() or artistTracks() are called from the main thread.
> 
> Matěj Laitl wrote:
>     Right concern, but, from Provider.h:
>     /**
>      * (...)
>      *
>      * This method is guaranteed to be called in non-main thread and is allowed
>      * to block for a longer time; it must be implemented in a reentrant manner.
>      */
>     virtual QSet<QString> artists() = 0;
>     
>     /**
>      * (...)
>      *
>      * This method is guaranteed to be called in non-main thread and is allowed
>      * to block for a longer time; it must be implemented in a reentrant manner.
>     */
>     virtual TrackList artistTracks( const QString &artistName ) = 0;
>     
>     So this is not the case unless used wrong. Calling from the main thread is therefore unsupported, more things would break (more deadlocks), no need to support it.
>
> 
> Edward Hades Toroshchin wrote:
>     "is guaranteed to be called" is incorrect sentence then. Since you're describing an API, you should write "must be called". You can't guarantee that the users of your API will even read your docs, can you? :)
>     
>     Also, since it doesn't matter which kind of connection exactly you use here, just drop QueuedConnection. Or add it to all the other providers, for consistency.
>

> "is guaranteed to be called" is incorrect sentence then. Since you're describing an API, you should write "must be called". You can't guarantee that the users of your API will even read your docs, can you? :)

Well, the method is (and should only be) called by existing StatSyncing code, the "users" of the API would be implementers of StatSyncing::Provider, thus the docs are targeted towards them. But Edward, discussing nearly synonyms in methods docs is nitpicking, right? ;) Each second discussing this would be much better spend by *adding* docs to your dangerously undocumented core interfaces.

> Also, since it doesn't matter which kind of connection exactly you use here, just drop QueuedConnection. Or add it to all the other providers, for consistency.

Well, I haven't tested that QNetworkReply works without an event loop, so perhaps it does matter. On the other hand, since the code emits the signal from other thread Qt::QueuedConnection is used implicitly and I've dropped the Qt::QueuedConnection for your satisfaction. ;)


> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/services/lastfm/SynchronizationAdapter.cpp, lines 98-101
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94955#file94955line98>
> >
> >     You go to quite some lengths to convert the non-blocking QNetworkReply approach, to a blocking one.
> >     
> >     Why don't you just readAll() from it?
> >
> 
> Matěj Laitl wrote:
>     > You go to quite some lengths to convert the non-blocking QNetworkReply approach, to a blocking one.
>     
>     Yes, I do. The reason why I do this is that the non-blocking (e.g. event-based) code would be way more ugly (IMHO) on the StatSyncing::Provider level, plus event-based approach has no advantage for a batch process like statistics synchronization.
>     
>     > Why don't you just readAll() from it?
>     
>     I do, in slot{Artists,Tracks,Tags}Received().
> 
> Edward Hades Toroshchin wrote:
>     I didn't suggest you use non-blocking approach. I just suggested that you drop the QSemaphore and his crazy friends in favor of just calling readAll() in a loop with waitForReadyRead() and isFinished().

> I didn't suggest you use non-blocking approach. I just suggested that you drop the QSemaphore and his crazy friends in favor of just calling readAll() in a loop with waitForReadyRead() and isFinished().

I believe that looping on isFinished() would be actually uglier, because waitForReadyRead() may fire multiple times and the Last.fm may require multiple requests. Plus the QSemaphore usage here is a clean producer-consumer pattern here, you must have learnt it in your CS class. Or perhaps not, but the code works well and IMO is not really convoluted, no motivation to change it on my side.


- Matěj


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


On Nov. 18, 2012, 5:12 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107348/
> -----------------------------------------------------------
> 
> (Updated Nov. 18, 2012, 5:12 p.m.)
> 
> 
> Review request for Amarok and Myriam Schweingruber.
> 
> 
> Description
> -------
> 
> This is a final review request for my whole summer project: Statistics Syncrhonization.
> 
> This is BIG and I don't expect you reading through it completely, rather please just skim the parts outside src/statsyncing. Even outside src/statsyncing, you may just check whether the interface and design is okay, I'm confident the implementation has low bug rate/divergence from documented behaviour.
> 
> Only TODO for 2.7 release: even more GUI polishing as suggested on the usability review request
> 
> Individual commits are available for your reviewing pleasure at http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.git&a=shortlog&h=gsoc
> 
> ChangeLog entries:
>     FEATURES:
>       * Track dragging support in Unique Tracks tab of the Synchronize Statistics action;
>         allows you to do a "diff" between collections and transfer missing tracks.
>       * Amarok now scrobbles tracks in streams if the stream correctly updates
>         meta-data.
>       * When scrobbling to Last.fm, Amarok announces suggested tag corrections.
>       * Ability to scrobble recently played tracks from iPod (and the like) to Last.fm.
>       * Synchronization of labels and rating between Last.fm and Amarok collections;
>         play count can be synchronized one-way from Last.fm to Amarok.
>       * Statistics synchronization between collections, supports rating, first / last
>         played time, play count and labels.
>     
>     CHANGES:
>       * Configure Amarok dialog gets new Metadata tab to grab some weight from the
>         Collection tab and to configure statistics synchronization.
>     
>     BUGFIXES:
>       * Better Last.fm scrobbling behaviour and error reporting due to rewrite,
>         should fix long-standing problems.
>       * Update stream meta-data correctly even with phonon-gstreamer back-end.
>     
>     FEATURE: 240732
>     FEATURE: 309697
>     FEATURE: 206249
>     BUG: 293320
>     BUG: 285820
>     FIXED-IN: 2.7
>     DIGEST: Amarok statistics synchronization GSoC project merged with many features
>             and improvements, please see http://strohel.blogspot.com/search/label/gsoc
>             for more info and a bunch of screen shots.
>     GUI: Added statistics synchronization dialogs, split and one more config dialog tab
> 
> 
> This addresses bug many.
>     https://bugs.kde.org/show_bug.cgi?id=many
> 
> 
> Diffs
> -----
> 
>   ChangeLog 608de6861f931808e2a435a1e9c502d871993027 
>   src/App.cpp 7915861609564a2b0475d0530751a31f04bb58bd 
>   src/CMakeLists.txt 575cd11f51ef53474837696b31538bfe4490a1da 
>   src/EngineController.h b3a228196d7aa322d001a337d040546546e02f9e 
>   src/EngineController.cpp 23e16d82869d6ff0c1201a432da25a1e143a0727 
>   src/MainWindow.h 7e83f09343f0640afb6b18d60109133e4ef262a7 
>   src/MainWindow.cpp d691951c09af3e7581c7b82920c708b8dd1268de 
>   src/configdialog/ConfigDialog.cpp 263f77db0932e700f4ebc68f8b9f6b6c51fa6381 
>   src/configdialog/dialogs/ExcludedLabelsDialog.h PRE-CREATION 
>   src/configdialog/dialogs/ExcludedLabelsDialog.cpp PRE-CREATION 
>   src/configdialog/dialogs/ExcludedLabelsDialog.ui PRE-CREATION 
>   src/configdialog/dialogs/MetadataConfig.h PRE-CREATION 
>   src/configdialog/dialogs/MetadataConfig.cpp PRE-CREATION 
>   src/configdialog/dialogs/MetadataConfig.ui PRE-CREATION 
>   src/core-impl/capabilities/multisource/MultiSourceCapabilityImpl.h 4a472c2072193f809b2c9b6dd06f7caa6df2b991 
>   src/core-impl/capabilities/multisource/MultiSourceCapabilityImpl.cpp 28152e61c93ce3cdd2d5a26f8adf33c87571e245 
>   src/core-impl/collections/ipodcollection/IpodMeta.h c74b7238eceb40e8968bdd1a6af93139ed808040 
>   src/core-impl/collections/ipodcollection/IpodMeta.cpp 572805c46692c3659884c7ceae77da288c111b1d 
>   src/core-impl/collections/umscollection/UmsCollection.h cc11b09270a58e11609fc0a975d752289dae7f65 
>   src/core-impl/collections/umscollection/UmsCollection.cpp fb326fcdfd474c7340a9f13f80e211b49a23b339 
>   src/core-impl/meta/multi/MultiTrack.h 336fbca20996e32a063302d22a0e688fc13482cc 
>   src/core-impl/meta/multi/MultiTrack.cpp 3138805d6427291143d5059b56745269a360b69e 
>   src/core-impl/meta/stream/Stream.h 253384d0b48c6b773116d12553e1976da6bdd0ec 
>   src/core-impl/meta/stream/Stream.cpp 26aed5e9894ad17ce9e27344acbdfcd0d5a8def1 
>   src/core/capabilities/MultiSourceCapability.h 819a24ff79f8d741db30c34ed42a38610885bf4c 
>   src/core/capabilities/MultiSourceCapability.cpp 4c46ff45f7f55c3460704f7058c9323a947529cc 
>   src/core/meta/Statistics.h 159b5ffcf3906f215d7327aef4ba47a527d77030 
>   src/core/meta/Statistics.cpp d991d8b1eb88f69a6fbd10255f961eda63b4f520 
>   src/core/support/Components.h 87fabb035842cb1ab5d5ba63358728cf2fbf311f 
>   src/core/support/Components.cpp 22e5de0d0d2ce9303fdca31839f186bae5fe866d 
>   src/dialogs/CollectionSetup.h 6e3d4808373df7f0584de7c59e84f0d0f0bd463f 
>   src/dialogs/CollectionSetup.cpp eea8c792ebb8169adf21e710a5dba993761eef14 
>   src/services/lastfm/CMakeLists.txt 61042a13522adb711c93a1b91c3f509b64af07f7 
>   src/services/lastfm/LastFmConfigWidget.ui 60b5bfd401c0b6e40d3251010b7c76473d916b00 
>   src/services/lastfm/LastFmService.h 48d26a3678fe583d6008d940f3e8810b1c0234b1 
>   src/services/lastfm/LastFmService.cpp 26739ba2e812f94dbf62c9c1fd81db90089d7567 
>   src/services/lastfm/LastFmServiceConfig.h 07f79d074ca1c08cd92dfe55ad5fb0435182c4d5 
>   src/services/lastfm/LastFmServiceConfig.cpp a07cbdf8e6deeb5082520e94cc0bb66de97c05dd 
>   src/services/lastfm/LastFmServiceSettings.cpp 81c1d0c3e5fd44d5e49abb1dee4ce457bf6ffc8d 
>   src/services/lastfm/ScrobblerAdapter.h 5d55ea0d075022b8d657fc54c01a55e07a150821 
>   src/services/lastfm/ScrobblerAdapter.cpp 8d119657bba1fb212d548c322f058e88b6dc8a7d 
>   src/services/lastfm/SynchronizationAdapter.h PRE-CREATION 
>   src/services/lastfm/SynchronizationAdapter.cpp PRE-CREATION 
>   src/services/lastfm/SynchronizationTrack.h PRE-CREATION 
>   src/services/lastfm/SynchronizationTrack.cpp PRE-CREATION 
>   src/statsyncing/Config.h PRE-CREATION 
>   src/statsyncing/Config.cpp PRE-CREATION 
>   src/statsyncing/Controller.h PRE-CREATION 
>   src/statsyncing/Controller.cpp PRE-CREATION 
>   src/statsyncing/Options.h PRE-CREATION 
>   src/statsyncing/Options.cpp PRE-CREATION 
>   src/statsyncing/Process.h PRE-CREATION 
>   src/statsyncing/Process.cpp PRE-CREATION 
>   src/statsyncing/Provider.h PRE-CREATION 
>   src/statsyncing/Provider.cpp PRE-CREATION 
>   src/statsyncing/ScrobblingService.h PRE-CREATION 
>   src/statsyncing/ScrobblingService.cpp PRE-CREATION 
>   src/statsyncing/Track.h PRE-CREATION 
>   src/statsyncing/Track.cpp PRE-CREATION 
>   src/statsyncing/TrackTuple.h PRE-CREATION 
>   src/statsyncing/TrackTuple.cpp PRE-CREATION 
>   src/statsyncing/collection/CollectionProvider.h PRE-CREATION 
>   src/statsyncing/collection/CollectionProvider.cpp PRE-CREATION 
>   src/statsyncing/collection/CollectionTrack.h PRE-CREATION 
>   src/statsyncing/collection/CollectionTrack.cpp PRE-CREATION 
>   src/statsyncing/jobs/MatchTracksJob.h PRE-CREATION 
>   src/statsyncing/jobs/MatchTracksJob.cpp PRE-CREATION 
>   src/statsyncing/jobs/SynchronizeTracksJob.h PRE-CREATION 
>   src/statsyncing/jobs/SynchronizeTracksJob.cpp PRE-CREATION 
>   src/statsyncing/models/CommonModel.h PRE-CREATION 
>   src/statsyncing/models/CommonModel.cpp PRE-CREATION 
>   src/statsyncing/models/MatchedTracksModel.h PRE-CREATION 
>   src/statsyncing/models/MatchedTracksModel.cpp PRE-CREATION 
>   src/statsyncing/models/ProvidersModel.h PRE-CREATION 
>   src/statsyncing/models/ProvidersModel.cpp PRE-CREATION 
>   src/statsyncing/models/SingleTracksModel.h PRE-CREATION 
>   src/statsyncing/models/SingleTracksModel.cpp PRE-CREATION 
>   src/statsyncing/ui/ChooseProvidersPage.h PRE-CREATION 
>   src/statsyncing/ui/ChooseProvidersPage.cpp PRE-CREATION 
>   src/statsyncing/ui/ChooseProvidersPage.ui PRE-CREATION 
>   src/statsyncing/ui/MatchedTracksPage.h PRE-CREATION 
>   src/statsyncing/ui/MatchedTracksPage.cpp PRE-CREATION 
>   src/statsyncing/ui/MatchedTracksPage.ui PRE-CREATION 
>   src/statsyncing/ui/TrackDelegate.h PRE-CREATION 
>   src/statsyncing/ui/TrackDelegate.cpp PRE-CREATION 
>   src/support/QSharedDataPointerMisc.h PRE-CREATION 
>   tests/core-impl/meta/multi/TestMetaMultiTrack.h 61e37627351163a7f2bbae729779900d2abb6ca9 
>   tests/core-impl/meta/multi/TestMetaMultiTrack.cpp b9d64dee9f8f0880981550aab29b882c4b4bbc31 
> 
> Diff: http://git.reviewboard.kde.org/r/107348/diff/
> 
> 
> Testing
> -------
> 
> Most of the code is tested months by me, OTOH the Last.fm features like syncing and or scrobbling from iPods is rather new. (weeks, days)
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

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

More information about the Amarok-devel mailing list