Page MenuHomePhabricator

ecore: add an index property on Efl.Model_Composite and handle children_slice_get.
ClosedPublic

Authored by cedric on Jan 16 2019, 6:28 PM.

Details

Summary

This will enable inheriting class to not have to implement as much code. This patch
fix also all class that use Efl.Model_Composite and its test.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cedric created this revision.Jan 16 2019, 6:28 PM
segfaultxavi requested changes to this revision.Jan 17 2019, 3:00 AM
segfaultxavi added inline comments.
src/lib/ecore/efl_model_composite.c
24

The past tense of "set" is "set", not "setted" :)

src/lib/ecore/efl_model_composite.eo
8

Position of this object in the parent model.

Or did I misunderstand it?

10

"Can only be set before the object is finalized" means that this property should be inside a constructors section?

18

Index of the object in the parent model.

If you talk both about "child" and "parent" it is a bit confusing. Also is this a zero-based index? Is there a maximum value? Can multiple objects have the same index?

This revision now requires changes to proceed.Jan 17 2019, 3:00 AM
cedric updated this revision to Diff 18574.Jan 17 2019, 4:41 PM
cedric edited the summary of this revision. (Show Details)

Rebase and improve documentation.

Sorry, one last comment that I missed in the previous review. Please do not resubmit the whole stack because of this :D

src/lib/ecore/efl_model_composite.eo
5

I guess this is a "model" property, different from the object property below?
What is their relationship? Do they always have the same value?

Also, use $child.index to use monospaced font instead of quotes.

cedric updated this revision to Diff 18631.Jan 18 2019, 12:20 PM

Rebase and improve documentation.

This patch breaks the build due to duplicated index property, and is not fixed until two patches later in the stack. This will render the stack unbisectable... Maybe it's better to merge these three patches?

Also, now I understand that you wanted to keep the quotes to indicate that these are strings. In this case, I suggest the $"child.index" notation, to include the quotes in the monospaced text. Depending on the renderer, monospaced text is included inside a box, which looks horrible inside quotes :)

cedric updated this revision to Diff 18768.Jan 23 2019, 2:39 PM
cedric edited the summary of this revision. (Show Details)

Rebase and merge commit.

Updated docs

segfaultxavi accepted this revision.Jan 24 2019, 3:18 AM
segfaultxavi added a subscriber: q66.

@cedric, @q66 just told me that quotes cannot be made monospaced, so please excuse me and leave all these comments as "child.index", without monospacing. I already updated this diff, but not the others in the stack.

I have no further comments regarding the docs.

This revision is now accepted and ready to land.Jan 24 2019, 3:18 AM
cedric updated this revision to Diff 18815.Jan 24 2019, 3:38 PM

Rebase, fix naming and docs.

We are now stepping over each other's diffs so I'll propose doc changes in a later commit :)

One last comment regarding capitalization of property names. You now have different notations: "Child.Index" but "child.selected", "self.selected"...
To aid you in the decision, part names and event names are lower case :D

segfaultxavi requested changes to this revision.Jan 25 2019, 1:15 AM
This revision now requires changes to proceed.Jan 25 2019, 1:15 AM

We are now stepping over each other's diffs so I'll propose doc changes in a later commit :)

One last comment regarding capitalization of property names. You now have different notations: "Child.Index" but "child.selected", "self.selected"...
To aid you in the decision, part names and event names are lower case :D

yeah it need to be child.index I think.

It looks dependencies broken in arc patch, so it tried to cherry-pick D7649 which was already merged.
it may need to abandon some related patches I guess though still we can stack the patches with skipping dependencies.

cedric updated this revision to Diff 18898.Jan 25 2019, 2:54 PM

Rebase and rename.

segfaultxavi resigned from this revision.Jan 28 2019, 5:44 AM

Docs look good to me now.

Resigning as reviewer so somebody else can review the rest of the patch.

SanghyeonLee accepted this revision.Jan 29 2019, 1:37 AM

It looks good to me. build, check all passed.
but one thing I have a question...
we remove boolean_child and select_child,
don't we need to remove container_item also?

This revision is now accepted and ready to land.Jan 29 2019, 1:37 AM

don't we need to remove container_item also?

I haven't looked at Efl.Model_Container as it did inherit from Efl.Model_Composite, but I think you are right. I will look at this later this week and push another patch set for this.

This revision was automatically updated to reflect the committed changes.