Page MenuHomePhabricator

ecore: add an helper class Efl.Model_View
ClosedPublic

Authored by cedric on Wed, Dec 19, 2:06 PM.

Details

Summary

With the advancement of our MVVM interfaces, we realize that it could be made easier,
especially for bindings, to write an Efl.Model that proxy another one without having to
necessarily implement the entire logic of propagating event and checking if the property
we are getting request for is actually handle by our own Efl.Model. To simplify this,
I introduce this class that allow to set new callback for each property you want to handle
on your object.

Depends on D7486

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.Wed, Dec 19, 2:06 PM
cedric requested review of this revision.Wed, Dec 19, 2:06 PM

@felipealmeida this is the patch that break the C++ bindings as we do not support future in C++ properly yet. Most code that would generate Eina_Future is disabled, except when a function callback specify it which is the case here. Could you look at it ? I have pushed this branch also in devs/cedric/viewmodel.

cedric updated this revision to Diff 17999.Wed, Dec 19, 2:52 PM
cedric edited the summary of this revision. (Show Details)

so this one also need to inverted name after all?

Nobody asked for my review, but when I put on my Nazi hat, I cannot stop.

src/lib/ecore/efl_model_view.c
25

*bound

30

Comments explaining the purpose of each struct and field would make reviewing much easier. Not Doxygen comments, just plain old C comments, for the reviewers.

36

All other fields are in past tense but not this one. Should it be "bound"?
A comment would help me understand its purpose and propose a better name :)

src/lib/ecore/efl_model_view.eo
3

I do not understand this sentence. The comment for EflModelViewPropertySet is much clearer.

6

Missing docs! Please add docs to all parameters and return values.

14

Probably you meant @.property.set

63

Capitalize sentences.

68

Since you are adding cross-references to the docs (nice!) did you know you can also reference events using @[Efl.Model_View.property,changed] ?

82

Then I think this property should be listed in the constructors section, right?
This has been discussed in T7477.

Will fix as requested.

src/lib/ecore/efl_model_view.eo
68

Oh, no, that's cool.

cedric updated this revision to Diff 18083.Fri, Dec 21, 12:10 PM

Rebase and improve documentation a bit.

segfaultxavi requested changes to this revision.Mon, Dec 24, 3:36 AM
segfaultxavi added a subscriber: q66.
segfaultxavi added inline comments.
src/lib/ecore/efl_model_view.c
36

*Could *Bound 😄

Also, are you sure it should not be children_bound?

src/lib/ecore/efl_model_view.eo
68

For some reason refs to events must be fully-qualified, i.e., @[Efl.Model_View.property,changed].
Otherwise, the link does not work. Maybe @q66 can help here.

102

TYPO! THIS DOES NOT BUILD!

This revision now requires changes to proceed.Mon, Dec 24, 3:36 AM

Will fix the rest.

src/lib/ecore/efl_model_view.c
36

I think it should be children_bind as it is a future action not a past one.

cedric updated this revision to Diff 18149.Fri, Dec 28, 11:13 AM

Rebase and fix according to review.

Builds for me now. I have no further comments regarding docs. Somebody else should review the code, though.

segfaultxavi accepted this revision.Sun, Dec 30, 9:16 AM
This revision is now accepted and ready to land.Sun, Dec 30, 9:16 AM
This revision was automatically updated to reflect the committed changes.