Review Request: Statistics Synchronization: final review request

[Matěj Laitl]

On 19. 11. 2012 Edward Hades Toroshchin wrote:
> 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.

Okay. I will for sure maintain and continue to improve it as other parts of 
Amarok allow. This for sure isn't the last chance to change how some things 
are done.

> Matěj Laitl wrote:
> > Edward 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.

I'm sorry about it. :-|

> Matěj Laitl wrote:
> > Well, the method is (and should only be) called by existing
> > StatSyncing code
> 
> Then the existing code is the user of the API. The code exists, doesn't
> mean it will stay unmodified for the rest of times.
> 
> > the "users" of the API would be implementers of StatSyncing::Provider,
> > thus the docs are targeted towards them.
> 
> Implementers of Provider aren't "users" of API, but "implementers" of
> API.

Well, I don't care that much, so I've changed it to "must be called".

> > Well, I haven't tested that QNetworkReply works without an event loop,
> > so perhaps it does matter. On the other hand, since the code emits the
> > signal from other thread Qt::QueuedConnection is used implicitly and
> 
> You contradict yourself.

Where? ;)

> > I've dropped the Qt::QueuedConnection for your satisfaction.
> 
> It's not for my satisfaction, it's for consistency. In similar parts
> of the code you use different types of connections, for no reason.

You're right here, I've used implicit (Qt::AutoConnection) connection type in 
CollectionProvider so I should be consistent in last.fm 
SynchronizationAdapter. It is a question of explicit vs. implicit now, as also 
ScrobblerAdapter actually depends on the actual signal/slot delivery to be 
queued, see below. (and I don't know what of the explicit/implicit approaches 
is preferred in Amarok)

> > I believe that looping on isFinished() would be actually uglier,
> > because waitForReadyRead() may fire multiple times and the Last.fm may
> > require multiple requests.
> 
> It would be ugly, but I wouldn't say it would be uglier. But okay, I'll
> take it.

Thanks to you I've studied it more and it turns out that:
QNetworkReplies are (according to Qt docs) only created using 
QNetwrokAccesManager (subclasses) and Qt docs say that all 
QNetworkAccessManager methods are reentrant (and not necessarily thread safe). 
All lastfm::*:getSomething() methods internally use the same 
QNetworkAccessManager instance and other Amarok code can call some of these 
any time (think of scrobbling of currently played track) from the main thread. 
Thus it is only safe to call these from the main thread in last.fm 
SynchronizationAdapter. So this is the reason why the SIGNAL/SLOT + QSemaphore 
trickery is necessary - in order to use liblastfm's QNetworkAccessManager in a 
reentrant way.

> > 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.
> 
> Well, if you document the diamond inheritance for people who have heard
> first of it, you could've documented this "pattern" here as well.

The reasons I didn't do so are two:
 a) discoverability: it suffices to read a dozen or two lines of code in 
SynchronizationAdapter.cpp in order to recognize the producer-consumer. On the 
other hand, one needs to study 3 or more .h files to discover the dreaded 
diamond problem. (OTOH, looking at it now, I don't see it anymore, perhaps 
I've factored the code somehow in between? will check later)
b) popularity: producer-consumer is a general and arguably much more known 
problem than a dreaded-diamond corner-case specific to C++ implementation of 
multiple inheritance.

Regards,
		Matěj