> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote: > > Haven't run it yet, but at least it compiles okay :) > > Matěj Laitl wrote: > Thanks for the attentive review, I didn't expect somebody to dive that > deep and I'm grateful. > > Matěj Laitl wrote: > So, what's your "ready to be merged" opinion on this, Edward?
I don't like it, but just go ahead, there doesn't seem to be anything that could be improved without improving all the rest of Amarok first. > 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? > > > > Matěj Laitl wrote: > > 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(). > > Edward Hades Toroshchin wrote: > I didn't suggest you use non-blocking approach. I just suggested that you > drop the QSemaphore and his crazy friends in favor of just calling readAll() > in a loop with waitForReadyRead() and isFinished(). > > Matěj Laitl wrote: > > I didn't suggest you use non-blocking approach. I just suggested that > you drop the QSemaphore and his crazy friends in favor of just calling > readAll() in a loop with waitForReadyRead() and isFinished(). > > I believe that looping on isFinished() would be actually uglier, because > waitForReadyRead() may fire multiple times and the Last.fm may require > multiple requests. Plus the QSemaphore usage here is a clean > producer-consumer pattern here, you must have learnt it in your CS class. Or > perhaps not, but the code works well and IMO is not really convoluted, no > motivation to change it on my side. I gave you no reason to be rude. - Edward Hades ----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
