Hi gustavo, I'd like to talk a little bit about some points Larry told we are fixing OK.
About #1 , sure, it is going to change. Dunno yet if it will be part of EIO or something else. Anyways it was created as a kind of proof of concept so we could test Emodel concepts. It ended up being a more complex thing so we can show a more advanced example application. Anyways it can be either Eio's or a separate thing but I'd like to put it aside for a while so I can focus on other issues for now. #2 is already returning the last known count value and we keep emitting the CHANGE event. #3 seems OK now, it is not a pointer to Eina_Value anymore. #4 there is no 'data' struct member anymore. It ended up breaking apart our 'ecore I/O events' notification because we shared the same structure, and it was a hack and should go anyway. Since there is no 'data' anymore I need to figure out another approach for ecore stuff, but it is not a big deal and will be fixed soon. #7 , yes, it is const now. #9, it is 'const void *' now and the documentation was indeed wrong, fixed now. #10, now we have a loaded event, shouldn't we have 'unloaded' event as well in Emodel? #11, I already started switching from simple eina array to Eina_Value_Struct_Desc in emodel_eio, however I don't know for sure if I got it. It may be leaner to use Eina_Value_Struct_Desc but we don't have many items in that array of properties it isn't much complex to deal with. Anyways I am following the eina_value_02.c already but I'd like to know if there are more implications about using Eina_Value_Struct_Desc instead of eina_array because it is basically used only internally in emodel_eio. #13.1, now make sure to both strings fully equal. #13.2. is now using eina_str_join. Agreed it is safer than using strncpy and alike by hand although I had to read eina_str_join() implementation (and it is nice to read code of course :)) to fully understand that the parameter 'size' must be strlen(a)+strlen(b)+2, the '2' here is including the separator character plus '\0' byte, if we miss that the result will be truncated by NULL terminator. 'size' cannot simply be strlen(a)+strlen(b) and perhaps it should be documented both in eina_str_join and eina_str_join_len. What do you think? See: ---snip--- char *str1 = "AAAA"; char *str2 = "BBBB"; size_t siz = strlen(str1)+strlen(str2); char *buf = calloc(1, siz); eina_str_join(buf, siz, '#', str1, str2); fprintf(stdout, "buf: %s\n", buf); free(buf); ---snip--- result is truncated: ---snip--- buf: AAAA#BB ---snip--- #14, umask() is gone. []s On Fri, Jun 27, 2014 at 7:18 PM, Larry de Oliveira Lira Jr < la...@expertisesolutions.com.br> wrote: > Hi gustavo > > thanks for your detailed review > > about item 1, no problem, initial intention with model Eio was make a "real > assync" model, to have a best case to make/tests "views" and examples, not > make a definitive model for "file objects" and therefore this choice for > name > > If nobody oppose we will fix/change first the points 1, 2, 3, 10, 15, 11.2 > (4, 7, 13 and 14 were done) > > 5, 6, 8, 9 and 11.1 are more complex and maybe demand more > discussion/clarifications > > > On Fri, Jun 27, 2014 at 4:40 PM, Carlos Carvalho < > ccarva...@expertisesolutions.com.br> wrote: > > > Hi! > > > > Some review items already pushed: 4, 7, 13.1, 14 and partially 9 (const > > void* and doc). > > > > []s > > > > > > On Thu, Jun 26, 2014 at 3:14 PM, Carlos Carvalho < > > ccarva...@expertisesolutions.com.br> wrote: > > > > > Hi Gustavo, > > > > > > First of all thank you very much for your review! > > > > > > I, personally, agree with most of your recommendations and already > pushed > > > a couple of changes into the repository, the same you pulled from > > earlier. > > > > > > Some items like '1', '2' and '5' are also about code design and will > > > inflict more fundamental changes in the code, although, so far, they > make > > > sense too, will possibly require further interaction with other people > > like > > > Felipe and Larry. > > > > > > Anyways I am still working on fixes and pushing them as soon as I > have'em > > > done. > > > > > > thanks again :) > > > > > > > > > > > > > > > > > > > > > On Wed, Jun 25, 2014 at 12:25 PM, Gustavo Sverzut Barbieri < > > > barbi...@gmail.com> wrote: > > > > > >> 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 > > >> > > > > > > > > > > > > -- > > > -- > > > Expertise Solutions > > > > > > > > > > > -- > > -- > > Expertise Solutions > > > > > ------------------------------------------------------------------------------ > > 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 > > > > ------------------------------------------------------------------------------ > 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 > -- -- Expertise Solutions ------------------------------------------------------------------------------ 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