Sent with [ProtonMail](https://protonmail.com) Secure Email.
> -------- Original Message --------
> Subject: Re: [E-devel] [EGIT] [core/efl] master 02/04: eio: migrate
> efl.io.manager.ls to use Eina_Future.
> Local Time: September 29, 2017 5:56 AM
> UTC Time: September 29, 2017 12:56 PM
> From: barbi...@gmail.com
> To: Enlightenment developer list <enlightenment-devel@lists.sourceforge.net>
>
> 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.
The accessor already exist and is given to the callback. I just recycled it as
I prefer the EINA_ACCESSOR_FOREACH macro to the Eina_Array one.
>> +
>> +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.
Hum, I have not assumed ENOMEM on eina_future_new. Calling
eio_file_cancel(NULL) is fine, but I should just break out of the function asap
actually. Not really something we can recover from.
>>
>> 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.
This are internal function also used by the legacy code.
>> + 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".
If I start using a structure here, I multiply the number of function to
implement and increase the code size for no good reason. This is also reuse
existing infrastructure that take care of cleanup automatically for me reducing
further the code. I do not see why do you think it is an "ugly hack".
Alternative solution lead to more code and complexity.
>> 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?
This is a the first use of a function callback in .eo done properly. Obviously
there are problem that can be seen. Callbacks type are not defined in a
namespace, so if I start using a "_", they will popup as such in C and C++
which looked weird to me. I am not to sure of the right solution here.
>> +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?
It is indeed the later, will try to phrase it better somehow. As for regarding
thread, the current model in Eo make it impossible to have any kind of nice API
with thread so there is absolutely nothing nowhere in EFL interface that expose
anything done in a thread.
>> }
>> - 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).
You missunderstand the original signature. It is exactly the same. The array
was send in the progress callback which is what the second parameter of
future<> was about.
> 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.
That is how I started, but it looks ugly to use. You build your request,
connect your callbacks and finally start it. It really is way more line of code
than it feels worth it.
As our main use case is to have dynamic UI that handle large directory just
fine, I focused on it and the API reflect that. I want to preserve the callback
that allow the partial update of the UI as things goes, but instead of having
the future only hold cound, I could return the aggregated array.
Will that do for you ?
Cedric
------------------------------------------------------------------------------
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
