> On Nov. 17, 2012, 7:59 p.m., Edward Hades Toroshchin wrote: > > src/services/lastfm/SynchronizationAdapter.cpp, lines 38-43 > > <http://git.reviewboard.kde.org/r/107348/diff/1/?file=94955#file94955line38> > > > > This should be Qt::AutoConnection. Otherwise it will deadlock if > > artists() or artistTracks() are called from the main thread. > > Matěj Laitl wrote: > Right concern, but, from Provider.h: > /** > * (...) > * > * This method is guaranteed to be called in non-main thread and is > allowed > * to block for a longer time; it must be implemented in a reentrant > manner. > */ > virtual QSet<QString> artists() = 0; > > /** > * (...) > * > * This method is guaranteed to be called in non-main thread and is > allowed > * to block for a longer time; it must be implemented in a reentrant > manner. > */ > virtual TrackList artistTracks( const QString &artistName ) = 0; > > So this is not the case unless used wrong. Calling from the main thread > is therefore unsupported, more things would break (more deadlocks), no need > to support it. >
"is guaranteed to be called" is incorrect sentence then. Since you're describing an API, you should write "must be called". You can't guarantee that the users of your API will even read your docs, can you? :) Also, since it doesn't matter which kind of connection exactly you use here, just drop QueuedConnection. Or add it to all the other providers, for consistency. > 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(). 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(). > 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. Can you just create a BlockingQueryMaker wrapper out of your code in the CollectionProvider? - 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
