Okay, after a review of Emodel.h, some comments:
1 - As previously said, Emodel_Eio is not ideal, it should be
Eio_Emodel and it should exist in eio's code, that will then depend on
Emodel -- as there is no reason for Emodel to know (and pull in) Eio.
(This is based on the first example in Emodel.h)
2 - Async is good, but mandate it is not. As raster pointed out
children_count_get() and similar returns nothing, that is bad. It
should return, at least, the last known value. You can emit
CHILDREN_COUNT_CHANGED if that changes and the user is expected to
listen for that. This simplifies a lot the simple cases where model
know stuff beforehand, like a model where all children are created (or
number is known) at construction time and will not change, then the
code that uses that could be simple. Generic UI should always listen
for the event, but can pre-populate based on the known number at time
(ie: start with 100, then COUNT_CHANGED emits 1000, then 10000 as new
items pop-up due some other actions -- in Eio, it could be due file
monitoring with inotify). The current API that uses a method to force
CHILDREN_COUNT_GET is not good, also remember that children count may
change without the user asking for it (funny this is what you consider
for properties, as there is prop_list but no PROP_LIST_GET).
3 - _Emodel_Property_EVT, should be Eina_Value, not a pointer to it.
Eina_Value is fixed in size and small, this should save memory
fragmentation and help cases where the value fits in the structure,
like integers, long or strings. Remember Eina_Value has no refcount,
thus you should always pass a copy to it, not share an internal value.
4 - _Emodel_Children_EVT, why that data? Where does it come from? It
says user but where? (I guess it comes from the parameter given to
children_fetch, but not clear as there it says "The child").
5 - The selection stuff really should go, it does not belong to model
(and I see your other email saying that it was an attempt to make life
simpler, it will not, will just confuse people). If this ever comes to
be a need, you can create a helper Emodel_Selection that monitors
another model and there you can set the selected items, etc.
6 - Prop list, as I said in #2, should return something. Users can
(should) always listen PROPERTIES_CHANGED to make sure they are
informed of properties appearing or vanishing. The prop_list is to get
the last version. Prop fetch is the same, if user wants to monitor for
changes, then also listen PROPERTIES_CHANGED.
7 - Prop set should receive a "const" Eina_Value *, as it is up to the
caller to release it. The Emodel code should not keep references to
it, instead it can either convert to something direct internally or
keep its own copy with eina_value_copy().
8 - child_del() is not a good name. Maybe child_remove(). If you want
to del the child, then it's better to use eo_del() in it and listen
for its death in Emodel. You can say that child_remove() will remove
it from internal keeping (list/array/db), remove Emodel's reference
and other than that it is up to the caller -- which may keep its own
reference with eo_ref() (thus children objects must be protective when
they are removed but kept alive, like NULL-ify points to DB, mark
themselves as "dangling", etc). I also don't see why we need a
parameter child_del_cb, it should be in an event CHILD_DEL.
9 - children_fetch, while here I see the need for async, I'm still not
happy. "void *data" should be "const void *data" to avoid casts on
user-side, the documentation I believe is wrong/misleading ("The
child."???).
My concern here is being hard to use, eventually the callback is
okay, as we have similar cases with hash/list foreach() that takes a
callback and calls one per element, but they are synchronous -- you
never go to main loop before they return. In this case I suppose you
want to go to main loop and call the callback whenever a children
appears. Then you'd have to monitor Emodel_Children_Evt->idx to see if
you're at the last index you asked? Which means "const void *data"
will always contain at least the "start, count" or "last"). It's
cumbersome in real life, particularly if you don't have a member in an
existing structure to store that, then you have to create another
struct { void *my_data, int last }. In that case we could give some
help to user and provide "last_idx" (or "start", "count") in
Emodel_Children_Evt?
Another alternative is to think of models with expensive children
fetch to use dummy/lazy objects. In such case you turn
children_fetch() to be synchronous, but the returned objects are dummy
and will load in background, emitting their own CHANGED events when
they get data. This puts the burden into the model implementation and
removes it from users.
Take Eio. Whenever you want a directory you do
emodel_load(my_dir). Until it reports LOADED (ie: finished), you don't
have a children_count and thus children_fetch() will return empty for
all objects. When finished, you get a new CHILDREN_COUNT_CHANGED and
you call children_fetch() which will create shallow Emodel_Eio with
only name (or type, in the case of !DT_UNKNOWN), this is cheap and can
be light in memory (see below). Proper usage says that when to use a
model, you should emodel_load() it, so until code does
emodel_load(my_dir_child), you don't need to stat() the object, you
can do it when load() is called and for it you emit a PROPERTY_CHANGED
(and CHILDREN_COUNT_CHANGED for dirs). Code for users are then much
simpler, you can return an iterator or callback synchronously like
eina_hash_foreach() does. You can also provide intermediate
updates to children count if you want, just update the variables as
Eio calls you, the code would be more responsive.
In the above example, to avoid creating one Emodel for each
children, you can avoid that and create an array of NULL pointers to
hold child objects. When children are requested, you see if they must
be created (pointer is NULL), then you get the basic information
returned by readdir() that you stored in a list of "pending" (struct
{int idx, ino_t d_ino, off_t d_off, unsigned char d_type, char
d_name[];}). As you create the children Emodel, you remove from that
pending list until it becomes NULL. This will keep it light in memory
and easy to use from outside.
However this rings a bell that we need more than emodel_load(). We
need emodel_properties_load() and emodel_children_load(), with
emodel_load() calling both. To implement a file manager we want to
show some meta information for each Emodel_Eio (type, mtime,
ctime...), but we don't need to children to be loaded -- which are
much more expensive then fstatat()).
Looking at your emodel_eio, my approach would be better as you'd
avoid multiple eio_file_direct_ls(). You'd do it once at
children_load() and then you just increase/decrease children
count/model as inotify calls you back.
10 - missing LOADED event to notify load is finally done.
11 - need to add properties_load() and children_load() as in #9.
For #2 and #6, the recommended code should be:
i- register events to be notified of new value;
ii- call getters to know the last event;
This guarantees the UI is always up to date and no empty fields will exist.
Based on emodel_eio.c:
11 - Don't use an array of properties, it's better to have a struct
that is much leaner in memory (WRT priv->properties). See
Eina_Value_Struct_Desc
(https://github.com/expertisesolutions/efl/blob/emodel-eolian/src/examples/eina/eina_value_02.c)
and old code in
https://github.com/expertisesolutions/efl/blob/emodel-eolian/src/lib/eina/eina_model.c
12 - PROP_ICON, seriously? :-D
13 - some dangerous usage of "N" variants of functions:
13.1 - strncmp(property, evt.prop, strlen(evt.prop)) is not being
safe, it is just doing prefix matching, not what you want.
13.2 - strncpy(child->fullpath, child->priv->path,
strlen(child->priv->path)), would also overflow as it is exactly the
same as strcpy(). The last argument (n) should be the maximum number
of bytes in child->fullpath). Of course this does not happen because
previously you calculated "child->fullpath" to be of the proper size,
but using strncpy() like that is not right. I suggest you to use
snprintf() or eina_str_join().
14 - as far as I know eio_file_mkdir() uses mkdir() that already
applies umask(), you don't need to do it again.
15 - _emodel_eio_emodel_children_count_get() calls direct_ls without
keeping a reference to async call, this will cause problems if count()
is still in use while the object dies.
Other than that, I suggest we add basic operations that all languages
provide to models:
- compare(): will be used by many UI/View to order elements (see
python's __cmp__, etc).
- copy(): not as useful as compare(), but good to have at least as
an optional interface for standard way to duplicate a model, ie for
transaction purposes you can work on a copy, so you can go back (not
useful in FS, but for forms it is). In Eina_Model I had deep_copy(),
also inspired in Python
- to_string(): useful for debugging, believe me :-D
- children_sort(cmp_cb): defaults to call compare() in all
children, very helpful for some UI to not replicate its sorting on its
own level. In some cases this may be provided by the internal
implementation, like in SQL you can ORDER BY.
- int children_find(start_pos, reference): returns the index of a
child in children list. Useful as we refer to index in
Emodel_Children_Evt. (The start_pos serves to incremental calls, in
that case you can save lookup time by providing the last item as
start_pos).
- children_match(start, match_cb): allows matching every children
that matches, like to provide filtering in "search as you type", etc.
All of those were used in my MVC projects, Python provides most of
that by default and they are common in Web MVC (not just Python). I
had those in Eina_Model as well as some other helpers like iterators
for children, sorted children, reversed children and filtered children
(easily created on top of the functions above).
On Wed, Jun 25, 2014 at 10:52 AM, Gustavo Sverzut Barbieri
<barbi...@gmail.com> wrote:
> did not look at it in depth now, but selection is not a model
> property, is a controller-view. If you want selection of a model to
> appear in multiple views, then you sync that elsewhere (either in a
> central controller -- more usual, or in another model that is the
> selection of target model).
>
> On Thu, Jun 19, 2014 at 12:20 PM, Carlos Carvalho
> <ccarva...@expertisesolutions.com.br> wrote:
>> Hi raster :)
>>
>> please see below.
>>
>> On Thu, Jun 19, 2014 at 4:09 AM, Carsten Haitzler <ras...@rasterman.com>
>> wrote:
>>
>>> On Mon, 16 Jun 2014 12:00:20 -0300 Felipe Magno de Almeida
>>> <felipe.m.alme...@gmail.com> said:
>>>
>>> Hey guys. just reading now. looking at emodel.eo a lot of these methods
>>> have no
>>> return.... just looking at properties for starters. also
>>> child_select_get().
>>>
>>> this needs a lot more documentation as it's really mysterious as to what
>>> this
>>> does and how it's meant to work. reading the code for the eio emodel i can
>>> see
>>> that child_select_get() will call the EMODEL_EVENT_CHILD_SELECTED on the
>>> object
>>> inside of this as long as a path is selected. what does load/unload do?
>>>
>>
>> There are child_select_get and child_select_set.
>>
>> The former is used when the application needs to know which (and if) child
>> is currently selected by the user, so application adds itself as listener
>> and call select_get and its callback will be invoked.
>>
>> The later happens when the user selects a child and every time it happens
>> an event is dispatched informing the listeners which child is selected so
>> the application can do whatever it wants with that information and can
>> navigate through the directory tree, in the case of eio, delete/create
>> files, etc..
>>
>> load/unload are actually unused because in the beginning of time we tough
>> to make them to perhaps behave like constructors and so but I don't know if
>> it makes sense and perhaps they may be removed.. ?
>>
>>
>>
>>>
>>> it REALLY needs docs there so people know what the purpose is so we can
>>> review
>>> and understand this better. :)
>>>
>>
>> Surely, I totally agree with you and this is on the way and as soon as we
>> have done I'll let you guys know.
>>
>>
>>>
>>> for some symmetry - there is a child_del in the core, but nothing for
>>> adding
>>> children. seems a bit... asymmetric to me? also the eio model overrides
>>> child_del AND has a child_del of its own. why?. they have different input
>>> params too. for the eio model, can i recommend we have a path property you
>>> can
>>> set AND get? so you can create an empty eio model and then point it to a
>>> path?
>>> cleaner. a custom constructor is fine but we eventually want to get rid of
>>> them
>>> or as many as we can.
>>>
>>
>> EIO indeed overrides child_del and implements its own, it is a kind of
>> unwanted indirection since emodel_eio deal with files and directories and
>> Emodel's child_del is kind of generic. In emodel_eio you'll see that
>> child_del is redirected to eio_child_del , the later then works directly
>> with EIO performing eio_dir_unlink (which works for simple files too). This
>> indirection may be unnecessary and can be removed if code/logic clarity is
>> improved.
>>
>> Making something like child_add perhaps has a drawback because, as far I
>> know, the application must know in advance what kind of model it is
>> constructing. In the case of emodel_eio it is filesystem, but could be
>> Database stuff or some other thing.
>>
>> Making a generic constructor I don't know if would make much sense because
>> of different implementations for emodel. But we're are very open to change
>> this if you think it is a good idea os have something in mind I don't
>> though about.
>>
>>
>>>
>>> so right now... docs docs - especially at the core like emodel. :)
>>>
>>
>> Yeah! It is on the way. Thanks for your feedback, we'll appreciate.
>>
>>
>>
>>>
>>> > Hello,
>>> >
>>> > We are implementing a model interface and a few views in elementary
>>> > and would like feedback.
>>> > The code is at
>>> https://github.com/expertisesolutions/efl/tree/emodel-eolian
>>> > and https://github.com/expertisesolutions/elementary/tree/eo_mvc
>>> >
>>> > We've designed a model interface which is tree-like (DAG) where each
>>> > child is an Eo*, which can itself be a Emodel or not) and each node
>>> > has properties. And the model is completely asynchronous.
>>> >
>>> > There is a list view and a form view in elementary and a EIO emodel in
>>> > EFL which are used in examples in elementary and for testing in EFL.
>>> >
>>> > We developed the model interface like this because we felt this
>>> > allowed more flexibility with an application being entirely defined in
>>> > a model class with each child for each frame (using a form view or a
>>> > specific implemented view for example).
>>> >
>>> > Any feedback is appreciated,
>>> > --
>>> > Felipe Magno de Almeida
>>> >
>>> >
>>> ------------------------------------------------------------------------------
>>> > HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions
>>> > Find What Matters Most in Your Big Data with HPCC Systems
>>> > Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
>>> > Leverages Graph Analysis for Fast Processing & Easy Data Exploration
>>> > http://p.sf.net/sfu/hpccsystems
>>> > _______________________________________________
>>> > enlightenment-devel mailing list
>>> > enlightenment-devel@lists.sourceforge.net
>>> > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>>> >
>>>
>>>
>>> --
>>> ------------- Codito, ergo sum - "I code, therefore I am" --------------
>>> The Rasterman (Carsten Haitzler) ras...@rasterman.com
>>>
>>>
>>
>>
>> --
>> --
>> Expertise Solutions
>> ------------------------------------------------------------------------------
>> HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions
>> Find What Matters Most in Your Big Data with HPCC Systems
>> Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
>> Leverages Graph Analysis for Fast Processing & Easy Data Exploration
>> http://p.sf.net/sfu/hpccsystems
>> _______________________________________________
>> enlightenment-devel mailing list
>> enlightenment-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
>
>
> --
> Gustavo Sverzut Barbieri
> --------------------------------------
> Mobile: +55 (19) 99225-2202
> Contact: http://www.gustavobarbieri.com.br/contact
--
Gustavo Sverzut Barbieri
--------------------------------------
Mobile: +55 (19) 99225-2202
Contact: http://www.gustavobarbieri.com.br/contact
------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
