Review Request: Statistics Synchronization: final review request
Matěj Laitl
matej at laitl.cz
Sun Nov 18 17:07:38 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 :)
Thanks for the attentive review, I didn't expect somebody to dive that deep and I'm grateful.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/MainWindow.cpp, line 27
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94920#file94920line27>
> >
> > Includes are unsorted here.
fixed
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/MainWindow.cpp, line 68
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94920#file94920line68>
> >
> > ...and here.
fixed
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/configdialog/dialogs/ExcludedLabelsDialog.cpp, lines 19-23
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94923#file94923line19>
> >
> > unsorted headers
fixed
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/Controller.cpp, lines 31-32
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94961#file94961line31>
> >
> > include files out of order
fixed
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/TrackTuple.h, line 150
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94972#file94972line150>
> >
> > Did you choose aggregation over inheritance on purpose?
Yes, I don't want TrackTuple to expose all the public QMap API, because it enforces some restrictions on the map contents and using the QMap API could break them.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/jobs/MatchTracksJob.cpp, line 22
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94979#file94979line22>
> >
> > Who defined it in the first place?
Ummm, nobody. I used this line to #define VERBOSE_DEBUG as a debugging tool. The #undef VERBOSE_DEBUG now only serves my IDE to know that the code is disabled. Probably warrants a comment though.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/models/CommonModel.h, lines 23-25
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94982#file94982line23>
> >
> > unsorted headers
fixed
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/configdialog/dialogs/ExcludedLabelsDialog.cpp, lines 38-66
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94923#file94923line38>
> >
> > Why didn't you write a UI file for this?
Good idea, implemented.
(originally, I thought the dialog woudl be *real* simple so that UI file would be overkill, but over time it got more complicated)
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/configdialog/dialogs/ExcludedLabelsDialog.cpp, line 80
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94923#file94923line80>
> >
> > I'd use QString()
fixed
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/configdialog/dialogs/MetadataConfig.cpp, line 28
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94925#file94925line28>
> >
> > oh zomg
Apparently not needed any more, removed. ;)
> 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.
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.
> 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?
> >
> 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().
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/services/lastfm/SynchronizationAdapter.cpp, line 141
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94955#file94955line141>
> >
> > magic number :(
Fixed by means of documented s_entriesPerQuery.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/services/lastfm/SynchronizationAdapter.cpp, line 150
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94955#file94955line150>
> >
> > here as well
Fixed.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/services/lastfm/SynchronizationTrack.cpp, line 283
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94957#file94957line283>
> >
> > one one one
Fixed "one one" -> "only one"
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/ScrobblingService.h, line 41
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94968#file94968line41>
> >
> > right...
left ;)
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/Controller.cpp, lines 209-212
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94961#file94961line209>
> >
> > magic numbers :(
Fixed by means of (documented) s_syncingTriggerTimeout attribute.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/Provider.h, lines 35-36
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94966#file94966line35>
> >
> > Why do you need this guarantee?
> >
> > Also, why don't you Q_ASSERT it in Provider() constructor instead of in individual providers' constructors?
> Why do you need this guarantee?
When you work with event-based interfaces like QNetworkReply or QueryMakers, you need to live (in QObject sense) in a thread with an event loop in order to be able to receive signals connected by Qt::QueuedConnection (which are needed e.g. for thread safety or reentrancy). It is extremely ugly and very tricky to have an event loop in a ThreadWeaver::Thread, so rather just require being in the main thread. Additionally, most QueryMakers (incl. SqlQueryMaker AFACS) simply don't work when *created* in a non-main thread, even if they are run from a main thread. (supposedly because they perhaps depend on being in a thread with an event loop)
> Also, why don't you Q_ASSERT it in Provider() constructor instead of in individual providers' constructors?
Fixed, thanks.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/ScrobblingService.h, lines 93-99
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94968#file94968line93>
> >
> > Can't you just declare a template operator once for every QExplicitlySharedDataPointer out there in a separate header file, and then just include it everywhere you have QMap with it?
> >
> > Also, why isn't it inline? I haven't seen qt's map implementation, but it could be beneficial in theory.
> Can't you just declare a template operator once for every QExplicitlySharedDataPointer out there in a separate header file,
Good suggestion, done.
> and then just include it everywhere you have QMap with it?
Extremely error-prone, because you don't get compiler error when you use it without including the new QSharedDataPointerMisc.h: QSharedPointer<T> coerces to bool in the case and they they compare all equal. I've just included QSharedDataPointerMisc.h in ScrobblingService.h and Provider.h.
> Also, why isn't it inline? I haven't seen qt's map implementation, but it could be beneficial in theory.
Good point, fixed.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/Track.h, lines 36-44
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94970#file94970line36>
> >
> > In the implementations of StatSyncing::Track you also Q_ASSERT that they're created in the main thread, but you didn't say anything about it here in the comment.
> >
> > Of course, the questions "why" and "why not in Track()" remain :)
> In the implementations of StatSyncing::Track you also Q_ASSERT that they're created in the main thread, but you didn't say anything about it here in the comment.
This is an implementation detail of Last.fm's SynchronizationTrack. There's no Q_ASSERT in StatSyncing::Track implementation (overlook?). CollectionTrack (the other StatSyncing::Track subclass) doesn't have this restriction.
> Of course, the questions "why" and "why not in Track()" remain :)
For "why" for the Last.fm SynchronizationTrack see above, TL;DR: it uses QNetworkReply.
> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote:
> > src/statsyncing/collection/CollectionProvider.cpp, lines 126-132
> > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94975#file94975line126>
> >
> > I think this should be implemented in QueryMaker as a blocking API instead.
> >
> > Also, SqlQueryMaker supports blocking API (more or less), so it could be unified.
> I think this should be implemented in QueryMaker as a blocking API instead.
Perhaps so, but I view it as a long-term goal and not something that blocks StatSyncing merging. Also see my comments about QueryMakers not working when created in non-eventloop threads. As blocking QueryMakers simply have only sense in non-main threads, BlockingQueryMaker would be hard to implement, because it doesn't have a comfort of an QObject available in the main thread as StatSyncing::CollectionProvider does.
> Also, SqlQueryMaker supports blocking API (more or less), so it could be unified.
The SqlQUeryMaker::setBlocking() is UGLY AS HELL IMHO and would lead to more complexity implementing QueryMakers. We really want to lower the barrier for implementing QueryMakers, see Phalgun's hard time (or fear of so) implemeting NepomukQueryMaker. If ever, I would propose adding BlockingQueryMaker wrapper, which would be currently also tricky, so I'd prefer not to depend on it.
- Matěj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107348/#review22116
-----------------------------------------------------------
On Nov. 16, 2012, 6:37 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. 16, 2012, 6:37 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/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
> 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/05ed9bad/attachment-0001.html>
More information about the Amarok-devel
mailing list