On Fri, Sep 29, 2017 at 2:50 PM, Cedric Bail <ced...@ddlm.me> wrote:
> 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.
sure
>>>
>>> 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.
ok, but if they are not exposed you can change the legacy code to use it.
>>> + 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.
you had to add a lock to thread local data, and IMO it's misleading to
attach this to the thread if you just need it elsewhere.
>>> 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.
talk to q66, but the way it is is not good, ugly and non-standard.
>>> +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.
oohhhh the damn "progress" thing... ok.
>> 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.
yeah, but that's how it is with eo... you efl_add(...
efl_event_callback_add(efl_added, ...))
painful in C? sure... in other languages not so much... I'd do:
Eo *efl_io_manager_ls(Eo *manager, const char *path) // returns
child eo "processor", owned by caller
static inline Eina_Future *
efl_io_manager_ls_easy(Eo *manager, const char *path, Efl_Event_Cb
func, const void *func_data, Eina_Free_Cb func_free) {
Efl_Io_Manager_Ls_Ctx *ctx = malloc(sizeof(*ctx));
ctx->func = func;
ctx->func_data = func_data;
ctx->func_free = func_free;
ctx->p = eina_promise_new(_efl_io_manager_ls_easy_cancel, ctx);
ctx->o = efl_io_manager_ls(manager, path);
efl_event_callback_array_add(ctx->o,
{ EFL_IO_LS_EVENT_PROGRESS, func, func_data }, // proxies "func"
{ EFL_IO_LS_EVENT_FINISHED,
_efl_io_manager_ls_easy_cb_finished, ctx }, // resolve "p", call
func_free
{ EFL_EVENT_DEL, _efl_io_manager_ls_easy_del, ctx }); //
call func_free, reject "p" if still there
return efl_future_Eina_FutureXXX_then(manager,
eina_future_new(ctx->p)); // bind to manager lifecycle
}
with ls_easy() future being the accumulated paths from progress... so
if I don't want intermediate updates I only use the future, otherwise
I use the future and the event.
Anyway, your patch worries me that this "function pointer" may blow in
our face... since it's "simpler to use than events" it will happen
more and more of these cases, defeating the purpose of
"similar/uniform" that we did.
> 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 ?
I see use for both, but if you do accumulate all the paths it may
consume great deal of memory... that's why I said I'd do the object
with events + partial paths (as you did), then a future that collects
path (easy version).
--
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
