Page MenuHomePhabricator

ecore: use efl_future_then to simplify the code logic and reduce potential bugs.
ClosedPublic

Authored by cedric on Wed, Nov 28, 9:39 AM.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cedric created this revision.Wed, Nov 28, 9:39 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

cedric requested review of this revision.Wed, Nov 28, 9:39 AM

I am a bit confused. I was under the impression that efl_promise_* and efl_future_* methods had to be replaced by eina_promise_* and eina_future_*. Moreover, you seem to be mixing them (efl_future_then() accepts an Eina_promise).

Can you clarify the situation?

I am a bit confused. I was under the impression that efl_promise_* and efl_future_* methods had to be replaced by eina_promise_* and eina_future_*. Moreover, you seem to be mixing them (efl_future_then() accepts an Eina_promise).

Can you clarify the situation?

Sure. So the old efl_promise* and efl_future* is dead. But along with eina_future_* being introduced, @barbieri introduced an efl_future_Eina_FutureXXX_then that linked the lifecycle of a future with the lifecycle of an object. That function was supposed to be renamed as soon as we got completely read of the old future, ..., which wasn't done until this serie of patch.

cedric updated this revision to Diff 17695.Thu, Nov 29, 3:10 PM
cedric added a reviewer: bu5hm4n.

No change.

Sure. So the old efl_promise* and efl_future* is dead. But along with eina_future_* being introduced, @barbieri introduced an efl_future_Eina_FutureXXX_then that linked the lifecycle of a future with the lifecycle of an object. That function was supposed to be renamed as soon as we got completely read of the old future, ..., which wasn't done until this serie of patch.

It looks to me like we still have two sets of Promise methods, EFL's and EINA's. What is the relationship between efl_future_then() and eina_future_then(), for example?

Also, I cannot apply this patch on top of latest master:

error: patch failed: src/lib/ecore/efl_model_composite_selection.c:262
cedric updated this revision to Diff 17785.Wed, Dec 5, 11:36 AM

Rebase.

cedric updated this revision to Diff 17793.Wed, Dec 5, 12:15 PM

Nothing.

It looks right from reading it - however, i cannot build this as D7332 does not build

src/lib/ecore/efl_model_composite_boolean.c
191

Why is that check not required anymore ?

src/lib/ecore/efl_model_composite_selection.c
235–236

On a unrelated note, can you seperate functional changes from formatting ?

It is a bit distracting when reviewing :)

(Just a site note :))

Okay, i dropped the problem patch. However, eio test suite times out *sometimes*.

Its like running ninja test 10 times, 1 of the runs will not be successfull.

Okay - timeout seems also to happen from time to time with master ... :(

With my comments answered this looks good

cedric updated this revision to Diff 17821.Thu, Dec 6, 9:40 AM

Nothing.

cedric added inline comments.Thu, Dec 6, 9:47 AM
src/lib/ecore/efl_model_composite_boolean.c
191

With efl_future_then you can specify the type of the value that is considered a success removing the need to do that check yourself. If the type is incorrect, efl_future_then will dispatch an error instead of the value itself. This help simplify the code.

On the same remark, error with efl_future_then are also always dispatched to the error callback and if no handler for it is provided it will just continue down the chain. If you don't need to handle the error, no need to deal with it either.

bu5hm4n accepted this revision.Fri, Dec 7, 2:20 AM
This revision is now accepted and ready to land.Fri, Dec 7, 2:20 AM

Code DOES seem to be simpler and everything builds and checks as expected.

This revision was automatically updated to reflect the committed changes.