Review Request: Statistics Synchronization: final review request

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


> 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.
> 
> Matěj Laitl wrote:
>     > 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.
> 
> Edward Hades Toroshchin wrote:
>     Can you just create a BlockingQueryMaker wrapper out of your code in the CollectionProvider?

> Can you just create a BlockingQueryMaker wrapper out of your code in the CollectionProvider?

Cannot be done. There couldn't be any "BlockingQueryMaker only created in the main thread" restriction (which would make it useless) and without it, the code wouldn't work. Yes, there is QObject::moveToThread(), but its usage is tricky and last time I tried it didn't work for me as expected.


- 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/727de2e4/attachment-0001.html>

More information about the Amarok-devel mailing list