On Sun, Nov 18, 2012 at 10:30:11PM -0000, 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 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. > 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. > 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. > 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. -- Edward "Hades" Toroshchin dr_lepper on irc.freenode.org _______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel