I found this commit too problematic, you can read my review bottom-up
to save some time... if you kept the same future signature it would be
much simpler.
On Thu, Sep 28, 2017 at 10:31 PM, Cedric BAIL <cedric.b...@free.fr> wrote:
> static void
> +_future_file_done_cb(void *data, Eio_File *handler)
> +{
> + Eina_Promise *p = data;
> +
> + eina_promise_resolve(p, eina_value_uint64_init(handler->length));
> +}
> +
> +static void
> +_future_file_error_cb(void *data,
> + Eio_File *handler EINA_UNUSED,
> + int error)
> +{
> + Eina_Promise *p = data;
> +
> + eina_promise_reject(p, error);
> +}
> +
> +static void
> _no_future(void *data, const Efl_Event *ev EINA_UNUSED)
> {
> Eio_File *h = data;
> @@ -154,6 +172,31 @@ _cleanup_info_progress(void *data)
> }
>
> static void
> +_future_string_cb(void *data EINA_UNUSED, Eio_File *handler, Eina_Array
> *gather)
> +{
> + EflIoPath paths = ecore_thread_local_data_find(handler->thread, ".paths");
> + void *paths_data = ecore_thread_local_data_find(handler->thread,
> ".paths_data");
> + Eina_Accessor *access;
> + unsigned int count;
> + Eina_Stringshare *s;
> +
> + if (!paths)
> + {
> + eina_array_free(gather);
> + return ;
> + }
> +
> + access = eina_array_accessor_new(gather);
> + paths(paths_data, access);
> +
> + // Cleanup strings, accessor and array
> + EINA_ACCESSOR_FOREACH(access, count, s)
> + eina_stringshare_del(s);
> + eina_accessor_free(access);
> + eina_array_free(gather);
this is weird... why do you create an acceessor to clean gather here,
but above you clean gather without deleting stringshares? It's not
clear to me why do we need 2 code blocks.
> +
> +static void
> _file_string_cb(void *data, Eio_File *handler, Eina_Array *gather)
> {
> Efl_Promise *p = data;
> @@ -320,31 +363,36 @@ _efl_io_manager_stat_ls(Eo *obj,
> return NULL;
> }
>
> -static Efl_Future *
> +static Eina_Future *
> _efl_io_manager_ls(Eo *obj,
> Efl_Io_Manager_Data *pd EINA_UNUSED,
> - const char *path)
> + const char *path,
> + void *paths_data, EflIoPath paths, Eina_Free_Cb
> paths_free_cb)
> {
> - Efl_Promise *p;
> + Eina_Promise *p;
> + Eina_Future *future;
> Eio_File *h;
>
> - Eo *loop = efl_loop_get(obj);
> - p = efl_add(EFL_PROMISE_CLASS, loop);
> + p = eina_promise_new(efl_loop_future_scheduler_get(obj),
> + _efl_io_manager_future_cancel, NULL);
> if (!p) return NULL;
> + future = eina_future_new(p);
create future later, since if this fails (ie: ENOMEM), it will call
_efl_io_manager_future_cancel(NULL) since promise_data_set(p, h) isn't
done.
>
> h = _eio_file_ls(path,
> - _file_string_cb,
> - _file_done_cb,
> - _file_error_cb,
> + _future_string_cb,
> + _future_file_done_cb,
> + _future_file_error_cb,
> p);
it would be nice to convert this to use the Promise/Future interface,
would simplify things a lot, basically just a creator helper and
nothing else needs to be done.
> + ecore_thread_local_data_add(h->thread, ".paths", paths, NULL, EINA_TRUE);
> + ecore_thread_local_data_add(h->thread, ".paths_data", paths_data,
> paths_free_cb, EINA_TRUE);
this is an ugly hack... come on! Just add a plain struct to
_eio_file_ls() that would contain "p", "paths" and "paths_data".
> static Eina_Future *
> _efl_io_manager_xattr_set(Eo *obj,
> Efl_Io_Manager_Data *pd EINA_UNUSED,
> diff --git a/src/lib/eio/efl_io_manager.eo b/src/lib/eio/efl_io_manager.eo
> index 8ad5443f01..b8ed9002e2 100644
> --- a/src/lib/eio/efl_io_manager.eo
> +++ b/src/lib/eio/efl_io_manager.eo
> @@ -7,17 +7,32 @@ struct Eio.Data
> size: uint; [[Size of private data]]
> }
>
> +function EflIoPath {
why not "_" between elements like in every other EFL function name:
Efl_Io_Path... and maybe use a more descriptive name?
> +function EflIoDirectInfo {
name here, use "_" and more descriptive name, this looks like a regular type...
> + paths: EflIoPath; [[Callback called for each packet of files found]]
packet? not too descriptive to me... is this called for each file? for
all files? Or non-accumulative partial results that were found by the
scanner thread? (I think the later, if so, try to explain a bit more
and explain how the user is supposed to handle that, such as "if you
need the full list, then walk the accessor and copy each item to your
own array or list"). Also, is it called from a thread or main loop?
> }
> - return: future<uint64, const(array<string>)>; [[List of entries in
> path]]
> + return: ptr(Eina.Future) @owned; [[Amount of files found during the
> listing of the directory]]
I rather keep the original signature, why not? Rather than exposing
the callback (which as I said above is confusing), just return an
EINA_VALUE_TYPE_ARRAY of EINA_VALUE_TYPE_STRING... as
eina_array_count() is O(1), there is no need to expose the uint64
(count).
With your new signature it just throw away the usefulness of Future,
making it behave much more like an "event".
While I can still see usefulness for partial results, like to
pre-populate an UI, I'd *NOT* do as you did, with a plain callback...
rather return an Processor object which emits: "results" events (your
callback) and "finished".
The future/promise could then be a wrapper over this object, waiting
for "finished" and then resolving with the full array.
To me it would be cleaner...
--
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
