Review Request: Statistics Synchronization: final review request

[Matěj Laitl]

On 19. 11. 2012 Edward Hades Toroshchin wrote:
> On Sun, Nov 18, 2012 at 10:43:07PM -0000, Matěj Laitl wrote:
> > [wrt BlockingQueryMaker created from code in
> > StatSyncing::CollectionProvider]
> > 
> > Cannot be done. There couldn't be any "BlockingQueryMaker only created
> > in the main thread" restriction (which would make it useless) and
> > without it, the code wouldn't work.
> 
> You have this restriction in StatSync::Provider, and according to
> yourself. And it won't make it useless. Again, your Provider is in main
> thread, but it isn't useless, despite that it's essentially a blocking
> interface to QueryMaker.

Yes. The StatSyncing::Provider has a "privilege" of the additional constraint 
that it needs to be created in the main thread. The constraint is somewhat 
unnatural and you've correctly questioned its purpose earlier in the review. 
But given that StatSyncing::Providers are long-lived objects currently 
exclusively created by StatSyncing::Controller, this constraint is not really 
restrictive.

Now let's sketch what we would expect from a class named BlockingQueryMaker. I 
would like to use it like this:

class MyBatchJob : public ThreadWeaver::Job { ... ]

MyBatchJob::run()
{
    (...)
    BlockinqQueryMaker qm( m_collection /* or whatever */ );
    qm.addFilter( bla bla bla );
    Meta::TrackList foundTracks = qm.run(); // can block for longer time
    // do something with trackList...
}

^^^^^^
Would be impossible if the same constraint is applied to BlockingQueryMaker.

Okay, no problem creating BlockingQueryMaker in MyBatchJob constructor, they 
say?

class MyBatchJob2 : public ThreadWeaver::Job
{
    private:
        BlockingQueryMaker m_qm;
}

MyBatchJob2::run()
{
    Collection *coll = getCollectionToWorkOnFromSomewhere();
    m_qm.setCollection( coll );
    m_qm.addFilter( bla bla bla );
    Meta::TrackList foundTracks = m_qm.run(); // can block for longer time
    // do something with trackList...
}

^^^^^^
Still impossible with the current StatSyncing::Provider code, because setting 
collection while already in non-main tread cannot be done.

This is why I'm reluctant to author BlockingQueryMaker, as it would have 
either a sloppy API (with restrictive constraints) or would be tricky to 
implement correctly *in my humble opinion*.

On the other hand, if someone else (you?) wants to create it (perhaps reusing 
some StatSyncing::Provider code) I'm happy to review the code and use it 
instead of ad-hoc code in Provider once it proves working correctly.

P.S.: Edward, when you've already compiled the StatSyncing code, perhaps you 
can lightly test it? ;) UI in Amarok Config, scrobbling still working, 
synchronizing with Last.fm (warning: non-interactive part is time-consuming) 
or with an iPod...

Cheers,
		Matěj