Page MenuHomePhabricator

efl: Rename efl_model_child_add to efl_model_child_append and add prepend method
AcceptedPublic

Authored by felipealmeida on Jan 8 2019, 11:29 PM.

Details

Summary

Adds the possibility to either append or prepend a child in a
model. The position requested may be ignored by the data model
depending on how ordering is defined.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8752
Build 7722: arc lint + arc unit
felipealmeida created this revision.Jan 8 2019, 11:29 PM
felipealmeida requested review of this revision.Jan 8 2019, 11:29 PM

patch looks good to me, but I didn't tested this yet, so I'll check again :)
Thanks for fast feedback!

src/lib/eldbus/eldbus_model.c
167

I think after this,
it would be better to return ERROR.. or we can add NOT_SUPPORTED_ERROR type..
make it macro and re-use every not-supported eo functions could be useful.

I have been wondering for some time what should be our approach regarding append,prepend and such. My issue with this patch is that we are taking the step of adding a lot more insertion function after that. Would it be possible to have only one append and one prepend acutally with a child as a reference point? Something like :

new_child = efl_model_child_append(model, relative_to_child);
new_child = efl_model_child_prepend(model, relative_to_child);

And when passing NULL for the relative_to_child, we will get the behavior we have in this patch ? I know that this patch likely break selection and boolean, but that's ok for now will investigate that later.

cedric requested changes to this revision.Jan 9 2019, 10:00 AM
This revision now requires changes to proceed.Jan 9 2019, 10:00 AM

Added a relative child to insert after or before

Fixed examples too

cedric accepted this revision.Jan 10 2019, 11:54 AM

That looks good to me. What do you think @SanghyeonLee ?

This revision is now accepted and ready to land.Jan 10 2019, 11:54 AM
SanghyeonLee added a comment.EditedJan 11 2019, 12:19 AM

well...I think... , I like the way of having some default actions.
what I think is...

insert
: insert in input index. NULL or 0 index means prepend.
add
: default insertion, always append.

cause I think insert_before and after is mostly meaningless excepts the edges. I really don't know why we need this two API for insertion... in some specific index.. the only the they have to do is just -1. so I prefer to have basic action for adding and some special case, insert(At).

but if you think this is better, ok, its fine.

well...I think... , I like the way of having some default actions.
what I think is...

insert

: insert in input index. NULL or 0 index means prepend.

add

: default insertion, always append.

cause I think insert_before and after is mostly meaningless excepts the edges. I really don't know why we need this two API for insertion... in some specific index.. the only the they have to do is just -1. so I prefer to have basic action for adding and some special case, insert(At).

but if you think this is better, ok, its fine.

We need an API to insert content any where in a children list. I prefer something that is consistent. So if we go with append, we need prepend as a symmetrical API. Now with insert_at, we technically don't need any symmetrical API. It is good by itself. They are also equivalent, you can implement insert_at with append/prepend. And you can implement append/prepend by using insert_at. So as they are equivalent, which one should we pick. Now that you have pointed out insert_at, I have a preference for it... because it require to implement only one API in a model instead of 2. @felipealmeida what is your opinion on this?

I like the way it is.

I like the way it is.

Let me summon @segfaultxavi to get advise on the API here. @segfaultxavi as you can see we have a little debate between the two following set of API to create new Child Model :

Efl.Object *efl_model_child_insert_after(Efl.Object *parent, Efl.Object *after_this);
Efl.Object *efl_model_child_insert_before(Efl.Object *parent, Efl_Object *before_this);

Or

Efl.Object *efl_model_child_insert_at(Efl.Object *parent, int index);

Or

Efl.Object *efl_model_child_prepend(Efl.Object *parent, Efl.Object *relative_to);
Efl.Object *efl_model_child_append(Efl.Object *parent, Efl_Object *relative_to);

The goal of this API is to create a new child model attached to a parent model. Currently, we only have add, which doesn't allow for relative insertion which are necessary for all sorted use case for example. All the above proposition do provide that capability and are functionally equivalent. So the question would be, which one is the most consistent one from your point of view? You will notice that we are three and each of us have an different proposal, so your choice is critical :-)

Well, all of them are functionally equivalent so, as you say, it's just a matter of keeping API consistency.
After grepping a bit in our current API list, this is what we have. The numbers are the amount of methods with that suffix:

40 _append
26 _prepend
15 _insert
12 _insert_before
10 _insert_after
 5 _insert_at

So append and prepend would be the most consistent.

Keep in mind that all the above are for Eina and Legacy methods, apparently we haven't introduced any insertion method in the Unified API yet, so this will be the first one and we can decide to change the rules :)

ALSO, don't forget to document how to insert easily, when you don't care about the insertion point. I am assuming you will use relative_to == NULL? The current diff does not specify this.

SanghyeonLee added a comment.EditedJan 31 2019, 1:12 AM

Keep in mind that all the above are for Eina and Legacy methods, apparently we haven't introduced any insertion method in the Unified API yet, so this will be the first one and we can decide to change the rules :)

ALSO, don't forget to document how to insert easily, when you don't care about the insertion point. I am assuming you will use relative_to == NULL? The current diff does not specify this.

not exactly insertion,
but we have similar feature already in Efl.Pack_Linear. it has,
Pack (same as pack_end)
Pack_Begin
Pack_End
Pack_Before
Pack_After
Pack_At

so refer the Pack case, we need two generic action, child_append/prepend or child_add/add_begin/add_end, but I prefer the previous one.
about before and after, mostly before and after is necessary when you can get some anchor object reference and you want to insert new object before or after on that anchor.
if we go Insert_Before/After, it is more useful when the input parameter is child_model, not an index.
for an index, Insert_At is more common usage and user can expect where exactly they insert new object.

Going to add _append, _prepend (for cases without an anchoring child, so beggining and end) and _insert_at for index. Going to do that after I come back to Brazil next week.