Page MenuHomePhabricator

efl: Rename efl_model_child_add to efl_model_child_append and add prepend method
AcceptedPublic

Authored by felipealmeida on Tue, Jan 8, 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.Tue, Jan 8, 11:29 PM
felipealmeida requested review of this revision.Tue, Jan 8, 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.Wed, Jan 9, 10:00 AM
This revision now requires changes to proceed.Wed, Jan 9, 10:00 AM

Added a relative child to insert after or before

Fixed examples too

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

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

This revision is now accepted and ready to land.Thu, Jan 10, 11:54 AM
SanghyeonLee added a comment.EditedFri, Jan 11, 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.