Hi,
Thanks so much for the suggestions. The Ecore_Timer was a copy/paste issue
but the others are all new to me :)
Andy
On Tue, 12 Dec 2017 at 20:51, Gustavo Sverzut Barbieri <barbi...@gmail.com>
wrote:
> Thanks for your work, few comments to improve it a little more:
>
> > +typedef struct _Ctx {
> > + Eina_Promise *p;
> > + Eina_Bool should_fail;
> > + Ecore_Timer *timer;
>
> efl_loop_timeout(loop, seconds) will return a future that resolves
> once at a given time, it's a non-recurring timer.
>
> if you want to use the Efl_Loop_Timer, then use that type or simply
> "Eo" to make clear it's about the new API.
>
> if you want to use 2 promises, maybe you should look at
> eina_future_race(). It takes futures and once one resolves, it "wins
> the race", cancelling the others. Thus if you want to impose a timeout
> to something, just race 'something' and a timeout ;-)
>
>
> > +static Ctx *
> > +_promise_ctx_new(Eina_Value *v)
> > +{
> > + Ctx *ctx;
> > + Efl_Loop *loop;
> > + ctx = calloc(1, sizeof(Ctx));
> > + EINA_SAFETY_ON_NULL_GOTO(ctx, err_ctx);
> > +
> > + loop = efl_loop_main_get(EFL_LOOP_CLASS);
> > + ctx->p = eina_promise_new(efl_loop_future_scheduler_get(loop),
> > + _promise_cancel, ctx);
> > + EINA_SAFETY_ON_NULL_GOTO(ctx->p, err_timer);
> > + ctx->value = v;
> > + return ctx;
> > + err_timer:
> > + free(ctx);
> > + err_ctx:
> > + eina_value_free(v);
> > + return NULL;
> > +}
> > +
> > +static void
> > +_timeout(void *data, const Efl_Event *event EINA_UNUSED)
> > +{
> > + Ctx *ctx = data;
> > +
> > + if (ctx->should_fail)
> > + {
> > + eina_promise_reject(ctx->p, ENETDOWN);
> > + }
> > + else
> > + {
> > + Eina_Value v;
> > + eina_value_copy(ctx->value, &v);
> > + eina_promise_resolve(ctx->p, v);
> > + eina_value_free(ctx->value);
>
> if you're going to use it only once, you don't need to copy. My
> recommendation is:
>
> - Eina_Value value; // not pointer
>
> - eina_value_flush(&ctx->value); // for cancel or error cases.
>
> - eina_pormise_resolve(ctx->p, ctx->value); // stole ownership, can
> ctx->value = EINA_VALUE_EMPTY if you wish to make clear.
>
>
> > +static Eina_Future *
> > +_future_get(Ctx *ctx)
> > +{
> > + Eina_Future *f;
> > +
> > + f = eina_future_new(ctx->p);
> > + EINA_SAFETY_ON_NULL_GOTO(f, err_future);
> > +
> > + efl_add(EFL_LOOP_TIMER_CLASS, NULL,
> > + ctx->timer = efl_added,
>
> is this recommended? I don't think so, better to use:
>
> ctx->timer = efl_add(...
>
> > +static Eina_Future *
> > +_fail_future_get(void)
> > +{
> > + Ctx *ctx = _promise_ctx_new(NULL);
> > + EINA_SAFETY_ON_NULL_RETURN_VAL(ctx, NULL);
> > + ctx->should_fail = EINA_TRUE;
> > + return _future_get(ctx);
> > +}
> > +
> > +static Eina_Future *
> > +_str_future_get(void)
> > +{
> > + Eina_Value *v = eina_value_util_string_new(DEFAULT_MSG);
>
> if you change from "Eina_Value*" to non-pointer, then you can use
> eina_value_string_init()
>
> if you keep the pointer, remove the "util_" from the name, it's legacy
> and just calls "eina_value_string_new()"
>
> > +static Eina_Value
> > +_chain_no_errors_cb(void *data EINA_UNUSED, const Eina_Value v, const
> Eina_Future *dead_future EINA_UNUSED)
> > +{
> > + int val;
> > +
> > + VALUE_TYPE_CHECK(v, EINA_VALUE_TYPE_INT);
> > + eina_value_get(&v, &val);
> > +
> > + return *eina_value_util_int_new(val * 2);
>
> this leaks the newly allocated Eina_Value, it's better to use:
> eina_value_int_init(val * 2)
>
>
> > +static Eina_Value
> > +_chain_with_error_cb(void *data EINA_UNUSED, const Eina_Value v
> EINA_UNUSED, const Eina_Future *dead_future EINA_UNUSED)
> > +{
> > + Eina_Value err;
> > + eina_value_setup(&err, EINA_VALUE_TYPE_ERROR);
> > + eina_value_set(&err, E2BIG);
>
> return eina_value_error_init(E2BIG);
>
>
> --
> Gustavo Sverzut Barbieri
> --------------------------------------
> Mobile: +55 (16) 99354-9890
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
--
http://andywilliams.me
http://ajwillia.ms
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
