Review Request: Statistics Synchronization: final review request

Sun Nov 18 17:12:41 UTC 2012

-----------------------------------------------------------
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.

Changes
-------

Address nearly all Edward's remarks. Thanks for these, they've improved the code quality.

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 (updated)
-----

  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/619a2f94/attachment.html>