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