Review Request: Statistics Synchronization: final review request
Edward Hades Toroshchin
edward.hades at gmail.com
Sat Nov 17 19:59:00 UTC 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107348/#review22116
-----------------------------------------------------------
Haven't run it yet, but at least it compiles okay :)
src/MainWindow.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17006>
Includes are unsorted here.
src/MainWindow.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17007>
...and here.
src/configdialog/dialogs/ExcludedLabelsDialog.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17023>
unsorted headers
src/configdialog/dialogs/ExcludedLabelsDialog.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17024>
Why didn't you write a UI file for this?
src/configdialog/dialogs/ExcludedLabelsDialog.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17022>
I'd use QString()
src/configdialog/dialogs/MetadataConfig.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17025>
oh zomg
src/services/lastfm/SynchronizationAdapter.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17044>
This should be Qt::AutoConnection. Otherwise it will deadlock if artists() or artistTracks() are called from the main thread.
src/services/lastfm/SynchronizationAdapter.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17027>
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?
src/services/lastfm/SynchronizationAdapter.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17028>
magic number :(
src/services/lastfm/SynchronizationAdapter.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17029>
here as well
src/services/lastfm/SynchronizationTrack.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17033>
one one one
src/statsyncing/Controller.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17034>
include files out of order
src/statsyncing/Controller.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17035>
magic numbers :(
src/statsyncing/Provider.h
<http://git.reviewboard.kde.org/r/107348/#comment17038>
Why do you need this guarantee?
Also, why don't you Q_ASSERT it in Provider() constructor instead of in individual providers' constructors?
src/statsyncing/ScrobblingService.h
<http://git.reviewboard.kde.org/r/107348/#comment17039>
right...
src/statsyncing/ScrobblingService.h
<http://git.reviewboard.kde.org/r/107348/#comment17040>
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.
src/statsyncing/Track.h
<http://git.reviewboard.kde.org/r/107348/#comment17041>
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 :)
src/statsyncing/TrackTuple.h
<http://git.reviewboard.kde.org/r/107348/#comment17042>
Did you choose aggregation over inheritance on purpose?
src/statsyncing/collection/CollectionProvider.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17045>
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.
src/statsyncing/jobs/MatchTracksJob.cpp
<http://git.reviewboard.kde.org/r/107348/#comment17046>
Who defined it in the first place?
src/statsyncing/models/CommonModel.h
<http://git.reviewboard.kde.org/r/107348/#comment17047>
unsorted headers
- Edward Hades Toroshchin
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/20121117/2bb2d52d/attachment-0001.html>
More information about the Amarok-devel
mailing list