VCS Interfaces, round 3

[Andreas Pakulat]

On 04.05.07 15:12:21, Matthew Woehlke wrote:
> Andreas Pakulat wrote:
> > On 04.05.07 13:43:22, Matthew Woehlke wrote:
> >> Andreas Pakulat wrote:
> >>> On 04.05.07 11:24:46, Matthew Woehlke wrote:
> >>>> Come to think about it, it would be ideal if VcsJob provided a way to 
> >>>> return results incrementally.
> >>> We can follow QProcess here and emit resultsReady() then the user of the
> >>> job can fetch the results and finished() would be emitted when the job
> >>> is completely done.
> >> Sounds good. I assume resultsReady() is fired 0 or more times? (I.e. 
> >> keep firing it at results come in, possibly rate-limited? The rate limit 
> >> would also be nice if the operation completes really fast, you would 
> >> skip right to finished().)
> > 
> > No, resultsReady would always emit at least once, unless there are no
> > results on an action (think add, commit and so on).
> 
> So it is (potentially) redundant with finished()? Ok, I can live with 
> that. :-)

Uhm no. I'd reduce finished() to not carry any result info at all. I'm
thinking about this workflow:

VcsJob* job = iface->log(KUrl("somehwere"));
connect(job,SIGNAL(resultsReady()), this, SLOT(slotResultReady()));
connect(job,SIGNAL(finished()), this, SLOT(slotFinished()));
job->start();

slotResultReady()
VcsLog = job->result().value<VcsLog>();

slotFinished()
delete job;

Most probably with a VcsJob* parameter in the signals to make it easier
for the using code to connect and know which job emitted the signal.

> > However there we have a problem, when the job is really fast we run into
> > trouble because it might finish before the signals are connected. So
> > should we maybe have a "start" slot in the VcsJob to initiate the start
> > from the "user"?
> 
> Good catch, we'd have that no matter what, wouldn't we? start() would be 
> fine, I'm not sure it should actually "start" the job, just flush any 
> pending signals (i.e. the job would run right away but queue signals 
> until start() is called).

That would make the job class quite more complex, i.e. it always has to
check wether "isStarted" is set to know wether a resultsReady() or
finished() signal can be sent. IMHO start() should rather start the job
and thus the job implementation would be much lighter and easier.

> Or rather, it should be /allowed/ to do that, depending on which way
> the plugin finds easier.

Hmm, I guess the above doesn't really matter for the user of the API, so
yeah. Let the plugins decide what they want to do. The important thing
to document is that no signal should be emitted until start() has been
called, but they may be emitted from inside start().

> >> Do we really need a canceled() signal?
> > 
> > Hmm, well that depends. Does the user care about the job after having
> > cancelled it? That introduces a new question: Who takes care of deleting
> > the job instance when its not needed anymore - the user? the plugin? I
> > think the user needs to do this and then cancelled is needed, you can't
> > just delete the job after calling cancel() else there might be uncleaned
> > resources...
> 
> Hmm... ok, that makes sense, I guess it may be that the job can't be 
> canceled, i.e. a synchronous cancel() would block for a while.

As I said to have a nice flow and a common point to cleanup point in
user-code we should emit 3 signals when the job is done, that is:

a) finished(VcsJob*) - the job has finished sucessfully and all results
are available (resultsReady signals haven been emitted accordingly).
b) error(VcsJob*) - an error occured
c) cancelled(VcsJob*) - the job was cancelled and is done with any
cleanup it had to do due to the cancelling

Andreas

-- 
Try the Moo Shu Pork.  It is especially good today.