[Kde-pim] Async feedback

[Daniel Vrátil]

On Thursday, December 18, 2014 12:03:01 PM Christian Mollekopf wrote:
> Hey Daniel,
> 
> I meanwhile took a look at the async code, and here's the feedback I have so
> far.

Awesome, thanks!

> 
> First off though, nice job! I think it goes very much in the right direction
> =)
> 
> So:
> * I think it would be nice to remove waitForFinished from FutureGeneric,
> offering only a callback. It keeps the baseclass simple and clean, and it's
> easy to implement a little wrapper that provides blocking waiting if
> necessary.

We have FutureWatcher for the callback, and having a "BlockingFutureWatcher" 
does not make much sense IMO.

I don't think it really matters whether the baseclass is simple or not: the 
only reason we have FutureBase is that I need to store a non-templated pointer 
to Future in ExecutorBase (which is non-templated itself), otherwise it would 
all be in Future.

> * Currently you're copying all result values to pass it on to the next step,
> which is something I hoped to avoid. I think setFinished should take the
> value and directly call the continuation (should be possible from what I've
> seen).

We can optimize this using the move semantics etc. I was just being lazy :-)

> 
> * The final value should be consumable by a continuation:
> 
> job.exec().then<QString>(
> 	//final continuation consuming the value
> )

+1, I like this. Not sure how to do it yet, but I'll figure out something :)

> 
> The continuation would in this case also replace the FutureBase::isFinished
> acessor I think.

Probably, but there's no harm in keeping it around. We need the state 
internally anyway.

> 
> If you wanted a future that stores the value you still could:
> MemoryFuture<QString> finalFuture(future2);
> 
> The memory future would internally copy the value from the continuation,
> perhaps emit a signal when done, and provide isFinished().
> 
> * We may want to add support for more convenient composition:
> 
> getJob1().then(getJob2())
> 
> Currently you have to do something like:
> 
> getJob1().then([this](const QString &arg, Async::Future<void> &future) {
>   getJob2().exec().then<void>([future]() {
> 		future.setDone();
> 	}
> });
> 
> Which is fine for now and works nicely with kjobs, but perhaps it would be
> worth it to improve on that sometime? Of course the convenient composition
> as above would only work if the result type of the first job and the
> argument type of the second job are a perfect match, so perhaps it's also
> not worth it.

Indeed. I used to have overloads for then(), each() etc. that accepted Job as 
an argument, but they got lost over the time. I'll see if they can be brought 
back.

> 
> * Error handling is so far missing. Each step should be able to provide an
> error handler, and by default errors should bubble up, so you only have to
> handle errors at the end (probably as an error handler continuation of the
> future).

Yep, I hoped to do it this week, but so far spent most time visiting relatives 
:D Will work on that.

> 
> * We probably should have a way to abort a job (if the job supports that of
> course).

ditto

> 
> * Execution is so far tied to the event-loop, and I'm wondering whether we
> should also support non-eventloop execution, i.e. to be executed in a
> thread. Can we specialized the executors for that?

Indeed that should be rather easy. We still need the eventloop though, you 
probably don't want a blocking wait for Future to finish.

> 
> Otherwise it looks really nice =) Let me know what you think about the
> points above and whether you plan on working on them, otherwise I might
> give it a try as well ;-)

It's a small codebase and since it's all template magic, even small changes 
tend to break everything :D I will first do the error handling and aborting, 
then we can move on with the remaining stuff.

Dan

> 
> Cheers,
> Christian

-- 
Daniel Vrátil | dvratil at redhat.com | dvratil on #kde-devel, #kontact, #akonadi
Software Engineer - KDE Desktop Team, Red Hat Inc.

GPG Key: 0xC59D614F6F4AE348
Fingerprint: 4EC1 86E3 C54E 0B39 5FDD B5FB C59D 614F 6F4A E348
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20141218/85172aad/attachment.sig>
-------------- next part --------------
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/