Page MenuHomePhabricator

efl_ui : rename model connect and factory connect to bind property and factory.
ClosedPublic

Authored by SanghyeonLee on Jan 2 2019, 3:20 AM.

Details

Summary

As we discussed in T7469 with V40,

efl_ui_model_connect
efl_ui_factory_model_connect

need to be renamed to

efl_ui_bind_property
efl_ui_bind_factory

for this work,
Efl.Ui.Model.Connect interface is changed as Efl.Ui.Bind,
and bind_property and bind_factory both method is supported by this interface.

bind_factory need Efl.Ui.Factory,
and Efl.Ui.Factory inheritted Efl.Ui.Bind,
so I little bit concerned about circular referencing, but so far, it works well.

Test Plan

tested by existed examples about model connect.
WARNING : if there are some application which using model connect,
it must be renamed also, as this API is not stable version.
I've briefly checked some efl application like terminology,
and couldn't find any usage of this API yet.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8635
Build 7658: arc lint + arc unit
SanghyeonLee created this revision.Jan 2 2019, 3:20 AM
SanghyeonLee requested review of this revision.Jan 2 2019, 3:20 AM
cedric requested changes to this revision.Jan 2 2019, 11:15 AM
cedric added inline comments.
src/lib/elementary/efl_ui_layout.eo
4

Do we still need the Efl.Ui.Factory inheritance here ?

This revision now requires changes to proceed.Jan 2 2019, 11:15 AM

Nope. you are correct! thanks.

update efl_ui_layout.eo

Need to fix indent errors.

src/lib/elementary/efl_ui_layout_factory.c
99

Indent

update indent errors and change connects hash to binds

segfaultxavi requested changes to this revision.Jan 7 2019, 1:37 AM
segfaultxavi added a subscriber: segfaultxavi.

I see at least three problems with the new EO file efl_ui_bind.eo:

  • Normally objects have nouns as names, and Bind is a verb.
  • Bind is too generic and this interface seems to deal specifically with properties.
  • The file is not documented at all (sorry but saying that the interface Efl.Ui.Bind is a "Efl UI Property Bind interface", or that bind_property does "Bind property" is not proper documentation).

For me to propose a better name, I need to understand what this interface does (that's why documentation is important!). Please help me:
This interfaces exposes two methods: bind_property and bind_factory. Does this mean that this interface is implemented both by factories and by the objects produced by these factories? In this case, wouldn't it make more sense to have two separate interfaces?
An example of how this interface should be used would be a great help, and should definitely be present in the documentation.

This revision now requires changes to proceed.Jan 7 2019, 1:37 AM
SanghyeonLee added a comment.EditedJan 7 2019, 9:47 PM

I understanding about your point but,

First of all, cedric and I made a task T7469 and vote V40, and wait to gathering community opinions but only few people have look at this so I have to fallow the opinion which only I have in the task and vote.
also this is renaming patch, so I don't want to touch anything more, like document or comments, it should be updated another patch I think.

Anyway, It's really happy that you give interesting about this, so I can get more opinions. we were in lack of opinions..
model_connect or bind_property is the object[or factory] will bind the given property name of model with appointed strings.
so we can say that 'object' bind_property 'to' something(mostly text part).
what object will do by that binding is hidden area for user, though they might guess it.
(e.g. property filename will be bound label which is Efl.Part of an item object)

model_factory_connect or bind_factory is the object[or factory] will bind the given property name of model with given factory object,
so let factory create object by model data from that property.(e.g property is filepath of an image and factory is Image_Factory)
this w ill be 'object' bind_property 'to' factory' and do something(swallow mostly).

previously, this model_factory_connect was sub-method of factory, but I think those two have almost same feature to bind model property,
so I let them in one interface.
still many action is hidden area, so I want to make it more clear to know what will happen by changing name,
so your suggest is very welcome.

One proposal to make it clear what action it takes.

PropertyBind.bindPart(Efl_Part *part, char * prop)
PropertyBind.bindFactory(Efl_Part *part, char *prop, Efl_Ui_Factory *fac)
PropertyBind.bindSingal(char *signal, char *prop)
PropertyBind.bind(char* string, char *prop) //general case

Well, I have no idea how models work in EFL... is there any explanation or example I can look at? I have tried src/examples/elementary/efl_ui_list_view_example_* and src/examples/elementary/layout_model_connect but they do not seem to work for me. I only get some very small garbled text.

Sooner or later I will need to write a tutorial about this topic.

One proposal to make it clear what action it takes.

PropertyBind.bindPart(Efl_Part *part, char * prop)
PropertyBind.bindFactory(Efl_Part *part, char *prop, Efl_Ui_Factory *fac)

This should be Widget.bindFactory(Efl_Part *part, Efl_Ui_Factory *fac);

PropertyBind.bindSingal(char *signal, char *prop)

This should also really be Widget.bindSignal(Efl_Part *part, char *property);

PropertyBind.bind(char* string, char *prop) //general case

widget.bindPart is as general use case as widget.bind. An efl_part provide an object that match a part name from a string. So defining an additional key for a part seems to me unlogical in our current pattern. Maybe you have counter example where it makes sense to split the key from the part name and have kind of a double key.

Well, I have no idea how models work in EFL... is there any explanation or example I can look at? I have tried src/examples/elementary/efl_ui_list_view_example_* and src/examples/elementary/layout_model_connect but they do not seem to work for me. I only get some very small garbled text.

Sooner or later I will need to write a tutorial about this topic.

Yes, it has been evolving a lot over the year and is still. The basic idea is that you have a Model class that expose Properties (as Eina_Value) and Children (Which are also Model). This Model can then be connected to a View that will use the Properties to define its content and parameters. The goal of the design pattern is to be able to be able to make a declarative description of the UI and its link with the data set.

Of course all data from a data source don't directly come with the right value and you usually have logic between the data and the property that a UI use to display that convert this input data into color, text and action that make sense for the widget. Because of that we introduced the concept of a Proxy Model, a Model that take data from another Model, but modify them or expose new Property. We later discovered that this was the pattern used in MVVM (before the C# road was even taken) and that Proxy Model is named ViewModel in C# (So we are trying to adopt that nomenclature, but sometime still use the old one).

So once your Property are expressed with String name, it become possible to link them in a declarative way between your Model and your View. This is what Bind, discussed here, does. Still, this is not enough as your View could be more dynamic and create Widget (sub-view) on the fly, like ListView, GridView or TreeView does. For that purpose, we have introduced a Factory interface and a BindFactory, that allow to define a source for this dynamic subView in a declarative way. The idea being that you will define all the link on the Factory and the Factory will automatically create a new View and Bind it with the given Model (which is a child of the main View Model).

This is a high level description of the logic of our MVVM architecture at this point.

Thanks @cedric, I will take a deep look once you guys consider it done.
Anyway, I still think Efl.Ui.Bind is not a good name (too generic, and it's a verb).
Also, there's a lot of missing docs. Keep in mind that only what you write in the EO files goes to the final reference documentation, so everything should be explained there.

So this is interface about Properties on the model so how about,

interface : Efl.Ui.Property
: property_bind; generic function for binding property with input key string.
: property_bind_part : binding property with given Efl.Part.
: property_bind_signal : binding property with given signal string, and emit the signal when model ready.
: property_bind_factory : binding property with given factory and create object by factory and swallow in gvien Efl.part.

go this way?
I don't know which is right way..

Oh, nice, I like this proposal!

interface : Efl.Ui.Property
: property_bind; generic function for binding property with input key string.
: property_bind_part : binding property with given Efl.Part.
: property_bind_signal : binding property with given signal string, and emit the signal when model ready.
: property_bind_factory : binding property with given factory and create object by factory and swallow in gvien Efl.part.

go this way?

Please drop property_bind_part as per current Efl interface convention, input key string have to be a part, so property_bind should take an efl_part. I am ok, with the rest, I think.

rebase the patch with resolving merge conflict.
this patch need to be fixed yet.

@cedric I know its same but we can have this specific function for explicit declaring on Efl.Part controls. so this will let user/widget both understand what action will happen by API calls.
if you not a fan of this idea, okay, I can drop it.

and.. about

efl_ui_bind_property(imgf, "", "path"); //connect to "path" property
efl_ui_bind_factory(factory, "efl.icon", imgf);

we could make it one line calls,
efl_ui_property_bind_factory(factory, "efl.icon", "path", imgf);
the goal is alway bind the property data with given subfactory and sawllow in the content, so I think make it one line is more natural I think.
any opinion?

and.. about

efl_ui_bind_property(imgf, "", "path"); //connect to "path" property
efl_ui_bind_factory(factory, "efl.icon", imgf);

we could make it one line calls,
efl_ui_property_bind_factory(factory, "efl.icon", "path", imgf);
the goal is alway bind the property data with given subfactory and sawllow in the content, so I think make it one line is more natural I think.
any opinion?

I think that will be more confusing and maybe also limiting (How do you bind signal for example?). I think it fine to split. Also with efl_part we will have:

efl_ui_bind_property(imgf, "path"); // connect default value of the factory to the "path" property of the model.
efl_ui_bind_factory(efl_part(factory, "efl.icon"), imgf); // I am not sure we will want to use efl.icon instead of simply icon here.

In C#, JS, whatever modern language

imgf.bind_property("path");
factory.icon.bind_factory(imgf);

proposal interface eo.
the code is not working yet.
If property_bind_part is too ambigiuous, we can go with property_bind_text.

can you check the efl_ui_property.eo?
about the factory,
we all know the purpose of factory bind, so it need to be called always these two API as the pair.
so why not making it one API?
porperty bind will be internally handled.

we do this because,
factory.icon.bind_factory(imgf);
is unable with current list item factory design.

factory cannot distinguish Efl.Part depending on each item classes,
so we vote the way and we drop it as I remembered in the Efl.Part task.

we do this because,
factory.icon.bind_factory(imgf);
is unable with current list item factory design.

factory cannot distinguish Efl.Part depending on each item classes,
so we vote the way and we drop it as I remembered in the Efl.Part task.

Uh, I think we concluded with @felipealmeida it was completely doable to have dynamic Efl.Part in C# (because without that there is a lot of Efl.Part design in the new interface that are broken).

can you check the efl_ui_property.eo?
about the factory,
we all know the purpose of factory bind, so it need to be called always these two API as the pair.
so why not making it one API?
porperty bind will be internally handled.

Other additional case, you have multiple property set on the factory that you are binding. Technically, it could be turtle all the way down (Having a factory binded on a factory that is also binded). For example an Image widget could support remote object that take a username/password to access the remote data. So you would have :

imgf.bind_property("uri");
imgf.user.bind_property("login");
imgf.password.bind_property("passwd");
factory.icon.bind_factory(imgf);
SanghyeonLee added a comment.EditedJan 15 2019, 6:25 PM

well about those thing,
we need more detail explain about @felipealmeida , so I'll request his comment when he come to the office :)
but felipe was on the side of not using dynamic class...
okay I stop to update the patch for now...
and let's get clear conclusion regarding this matter.

imgf.bind_property("uri");
imgf.user.bind_property("login");
imgf.password.bind_property("passwd");
factory.icon.bind_factory(imgf);

and I didn't think about that usage, still it looks very rare case and that makes normal case worse... but the reason why it need to be there make sense..
still I don't like the factory bind without property, cause its unclear to me what will be binded... especially if we go this interface name on Efl.Ui."Property" :p
maybe this need to be moved factory side again..

cedric added inline comments.Jan 15 2019, 6:35 PM
src/lib/efl/interfaces/efl_ui_property.eo
18 ↗(On Diff #18441)

I think we can drop this function completely as we do have property_bind that does exactly the same.

19 ↗(On Diff #18441)

It is also not just text, but could be a color, a size or anything. We are carrying Eina_Value which have a lot of flexibility in how we can bind them to a part.

25 ↗(On Diff #18441)

Typo.

27 ↗(On Diff #18441)

This is difficult to understand what will happen and how the value of the model property will be used to generate an event. My expectation would be that the emission part use eina_value_to_string from the Eina_Value associated with property provided by the model. While the source part specified in this call is logically going to be defining the source of the signal.

SanghyeonLee marked an inline comment as done.Jan 15 2019, 6:46 PM
SanghyeonLee added inline comments.
src/lib/efl/interfaces/efl_ui_property.eo
19 ↗(On Diff #18441)

honestly that is the problem what I think.
you call the function and you cannot expect what will happen and all it depends on widget and data types.. how can user control this?
the reason why I brought this is to reduce that ambiguity at least for one case which mostly used.

27 ↗(On Diff #18441)

isn't it still possible to use signal associated with property? I saw the examples in the code.. even before we have only one string input like efl,state,%s, but I don't know how this can be controlled in other binded languages... is Eina_Value can solve this problem?

cedric added inline comments.Jan 15 2019, 7:19 PM
src/lib/efl/interfaces/efl_ui_property.eo
19 ↗(On Diff #18441)

This doesn't reduce anything as you still get an Eina_Value from the Model (which the user control what it is) and the View call eina_value_to_string on it. The user control the Eina_Value, not the other side, because the other side, the View, is the API.

I understand your concern, as the API is defined by the Class you set on the factory, not by the View itself, the user has to understand that link. I think this is really core to any MVVM interface, and we can not avoid, but document it clearly. I do not see how we could avoid this, except by removing all the automatism and letting the user do everything manually which I think make things harder. Basically it is a balance of automation and understanding of what is under the hood.

I think we should bring @segfaultxavi on the topic and see what he things once he try making a tutorial out of it.

27 ↗(On Diff #18441)

I like the idea of having this string accepting the Eina_Value as a %something and it is much more general purpose. Ok, let's keep it with string and document the use of printf syntax.

I don't think we have an eina_value_printf or something like that, but would make sense to add it and refer to it for the documentation of this function. I would expect all the %something trigger the proper conversion on the Eina_Value and generate the string from it.

@cedric @SanghyeonLee I can write a simple example using this new proposed API so we can decide if it makes sense.
However, you have to point me to some example to begin with. The ones currently in src/examples/elementary print a lot of errors and none of them seem to work for me.

SanghyeonLee marked an inline comment as done.Jan 16 2019, 3:02 AM

yes as I mentioned,
it won't be fixed at all yet,
only I updated efl_ui_property.eo file.

today I discussed about this with @felipealmeida and I think we could have few doable candidates which is not exactly what we want.
I hope he will update those candidates till end of this week, so let's vote and decide the way what we go.

renew the patches.
adding new two interfaces,

Efl.Ui.Property_Bind
Efl.Ui.Factory_Bind

note that this is just only rename patches and whole bind feature need to be refactored as
the task T7668

cedric accepted this revision.Feb 11 2019, 11:05 AM

This is a good simple first step. Let's go with this.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 11 2019, 11:06 AM
This revision was automatically updated to reflect the committed changes.