Page MenuHomePhabricator

Clarify how the "constructor" section in eo files is to be used
Open, Incoming QueuePublic

Description

Right now, it is not clear what the constructors section means. This ticket should start a discussion so that we all know how to use that.

  • Can anything be put there?
  • Only methods or properties specified there can be used inside initialization methods?
  • Should this behavior be enforced by a runtime error? warning?
  • I understand its main purpose was to speedup theme initialization by avoiding the default theme setup. Why is this section not used in the Widget class then?

Please comment with your answers, or any other question you have, or any information you possess.

herdsman added a comment.EditedNov 19 2018, 6:02 AM

Continuing our conversation on IRC, I will express my opinions here:

Can anything be put there?

My opinion: no methods should be used. Only set properties.

Only methods or properties specified there can be used inside initialization methods?

Ideally, users should only use what the constructors block specifies.
But also, we need to consider that there should be a minimal set of properties for the initialization of the object.

A good example case (as mentioned in the following bullets) is that widgets are themed only at the finalized stage, thus operations like efl_part during construction initialization are improper.
There are many more examples, of course. The object should be "finalized" before most of its functionality be made available for the user.

Should this behavior be enforced by a runtime error? warning?

I would say ERR first (or lower), and then mitigate those slowly. That, assuming we agree with the previous point.

I understand its main purpose was to speedup theme initialization by avoiding the default theme setup. [...]

That was one example that I mentioned to you. It's not the "main purpose", but a good example nevertheless.

[...] Why is this section not used in the Widget class then?

You are referring to the style property in Efl.Ui.Widget. I am not sure why it's missing. The efl_finalized_get() check in the very first lines of style_set(), so I guess it should be specified under constructors.

raster added a subscriber: raster.Nov 19 2018, 8:37 AM
<SegFaultXavi> I don't know the internals of eo object construction so I don't understand what do you mean :)
<raster> hmmm
<raster> ok so we added this idea that when objects get constructed they call the constructor and then LATER call a finalize method to finalize construction
<raster> any class can implement a finalize method if it wants - they are like any other methods
<raster> the idea is that you "delay" expansive things until finalize
<raster> so the idea is that you can set up object properties and so on knowing nothing will be actively "done" to your object other than setting up some state
<raster> then in finalize you can do the expensive stuff
<SegFaultXavi> once you have gathered all the state info during construction
<raster> correct
<SegFaultXavi> ok, makes sense
<SegFaultXavi> then I agree only property setters should be allowed in initialization methods
<SegFaultXavi> and enforced at runtime, or users will use all kind of weird stuff in there
<raster> so everything in the constructor block is about doing that before finalize
<raster> if you put nothing there then it's finalized before eo_add returns (or the constructor call returns in other langs)
<raster> right now we dont actually optimize for this - it's a design for possible optimizations
<raster> so encourage people to use it for obj setup
<raster> the q is - what counts as obj setup?
<SegFaultXavi> what the obj decides is obj setup, by specifying it in its constructors section
<raster> sure
<raster> but we havent done that yet
<raster> and itsa class by class
<raster> so from eo base class up we have to start doing this
<raster> a lot of eo base class is safe to be @constructor methods

On the light of this, then I think in C# it makes far more sense to turn all properties in the constructors section into optional constructor parameters and remove the initialization method completely.

@raster @felipealmeida @lauromoura @Jaehyun_Cho so, what do you think of the above suggestion? If only properties in the constructors section are allowed inside the initialization callback, why don't we remove the initialization callback completely and turn those properties into optional constructor parameters?

In C that is complicated and cumbersome (to implement and to use), but I think in C# that can be pretty simple if we keep the number of constructor-only properties low (right now, I think there is only one, the style).

I am interested in this because it simplifies learning EFL, since the initialization method is a bit weird (at least, from C#).

@segfaultxavi

I agree with you :)

But as you said it would be complicated, I don't know how to support constructor parameters in C without initialization callback.

I think it would make EFL C# easier if initialization callback is removed in constructor in C# :)

We are only missing the input from the C# people who would need to implement this... Do you foresee any problem, @lauromoura ?

lauromoura added a comment.EditedNov 29 2018, 6:32 PM

We are only missing the input from the C# people who would need to implement this... Do you foresee any problem, @lauromoura ?

Given the small number of properties listed inside the constructors (see below), I think this is doable. I got this list with pyolian by just iterating through the classes and listing the constructors. As Xavi noted above, Efl.Ui.Widget.style is still missing.

  • Efl.Model_Container_Item.define : optional? No
  • Efl.Ui.Win.accel_preference : optional? No
  • Efl.Ui.Win.win_name : optional? No
  • Efl.Ui.Win.win_type : optional? No
  • Ecore.Exe.command : optional? No
  • Eldbus.Model.Arguments.custom_constructor : optional? No
  • Eldbus.Model.connection : optional? No
  • Eldbus.Model.connect : optional? No
  • Eldbus.Model.Method.method : optional? No
  • Eldbus.Model.Method.proxy : optional? No
  • Eldbus.Model.Proxy.interface : optional? No
  • Eldbus.Model.Signal.signal_constructor : optional? No
  • Elm.Code_Widget.code : optional? No
  • Elm.Glview.version_constructor : optional? No
  • Elm.View.Form.model_set : optional? No
  • Elm.View.List.genlist_set : optional? No

Cool! Most of the list above is for legacy classes which are not accessible through C# so I'll move forward and create a task to implement this in C#.
Thanks everybody for this discussion!

Moving even further, if the purpose of the constructors section is to list those properties which can only be set during construction, would it make sense to remove this section and add a @constructor_only tag to properties?
Does anybody remember why it was implemented as a new section instead of a property tag?

i really liked the lambda callback as this then also translates across many languages (js, lua, c++) too... with C being able to skip he lambda fluff and just inline the code directly without it. it's consistent for efl. this is NOT a feature you MUST use. it is optional, so i'm not sure that it is that confusing as you aren't forced into it. it's a (possible) optimization if you want to make use of it though.

I'm OK with keeping the lambda as optional (I think it is already optional in C#).
It still opens the door to using "unauthorized" methods in it, though, so we should forbid those methods at runtime somehow. Otherwise, the constructors section has no purpose, right?

Any opinion about turning the section into a property tag?

q66 moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Nov 30 2018, 7:06 AM

well it does open the door but this is something we would police at the c level anyway (or should) and at least print errors for calling unapproved calls before finalize on an obj.

I'm not sure we are doing it now, that's why I ask :)

raster added a comment.Dec 3 2018, 4:30 AM

we're not policing the calls at the C level? eolian_gn can auto-generate the checks so it's possible... :)

herdsman added a comment.EditedDec 3 2018, 9:11 AM

Just adding that the constructors block should be used to not only ALLOW (i.e. "optional" methods) setting the state of the object, but also REQUIRE setting its state.

Classes (objects) can be authored to require the user to set some of the object's state during construction.
For example, the Circle class would require the user to specify a radius, or a Point class would require to specify the x and y coordinates.

You could argue that such values can always be considered "optional", so that in the above example the radius can default to 0 if not provided during construction.
However, some authors would actually want to have REQUIRED properties/methods, and for it to be documented (and ideally, enforced). We can see such examples in OOP languages.

A suggestion from today's discussion on IRC was to do something like:

constructors {
   radius: int;
   center: Point @optional;
}

Going with @optional instead of @required would have the benefit of following the same scheme we use for method parameter declaration (i.e. all non @optional arguments are required).

Another thing discussed: To avoid changing the C efl_add API and enforce checking that could be useful regardless of language, such obligatory constructor parameters ideally would be checked (with eolian-generated checks?) if the user provided them before calling efl_finalize.

q66 added a comment.Dec 3 2018, 3:11 PM

@segfaultxavi as for tagging methods directly, well, eolian by design doesn't specifically enforce ordering of methods in their block, especially since generators typically work with Eolian_Implementation rather than Eolian_Function list directly; in a constructors section, we'd typically want the order of the fields to be strict, I'd assume.

@lauromoura i still have a concern about having either required or optional keyword within the constructors section, it will become a real mess of ambiguities that are hard to check if we do so; obviously here required/optional would have different behavior than it does for params, for params it's more about wrapping the param inside some sort of maybe type.

@q66, Good point! So, if we want to be able to specify the constructor parameters order we need to put them in a constructor section rather than tagging the properties individually. Thanks. We will also need to have some clear rule regarding the order of the inherited constructor parameters (I think it suffices to say that "inherited params come before each class params").

Regarding possible @optional or @required tags, I think we might be overcomplicating things a bit. At this point, I don't think we want Eolian to be a fully-fledged OOP language. We need it to solve our problems. Please allow me to sketch a bit the story to make sure we are all on the same page.

  • Eolian allows creating objects and setting properties on them.
  • The above order (first create, then customize), however, turned out to be inefficient in some cases. For example, setting the style of a widget is a slow operation so setting it to "default" during object creation is wasteful if we are going to set it to something else later on.
  • To improve the above situation, initialization lists were created: a set of methods to be called during object construction so the expensive properties can be set only once (let's call them construction-time properties). Initialization lists are meant to set only the expensive properties. Not all properties can be safely set during construction. That's the purpose of the constructors section, to indicate which properties can be safely set during construction.
  • However, the resulting implementation of the initialization lists is so flexible (varargs in C, or initialization methods in C#) that they allow any property to be set (and any method to be called, actually). I myself wrote all the C# tutorials thinking that the initialization method was all-purpose and abused them thoroughly, which led to all kinds of problems.
  • Therefore, finally, what we are trying to solve here is to forbid unauthorized properties to be set during construction. In C this can easily be done by making Eolian generate checks, as @raster was pointing out. In C#, we have the option to turn the authorized properties into optional constructor parameters and remove the initialization method completely. I think this is simple and removes the problem completely. Its drawbacks are a possible explosion of parameters and parameter name collision, but this does not seem to be a problem now, given the amount of construction-time properties we have.

So, yeah, we can further improve Eolian and mark construction-time properties as Optional or Required, so you must always provide them in the constructor. However, as @q66 pointed out, this adds complexity, and I do not think it is required at this point to solve the problem (or that we have available manpower).

Did I get the facts straight? Can @lauromoura continue working on T7487?

@segfaultxavi

Thank you for clear summarize.

As far as I can see, you get the facts straight.

herdsman added a comment.EditedDec 5 2018, 3:19 AM

I am not sure about the third point:

To improve the above situation, [...] so the expensive properties can be set only once [...]

Some authors may want to restrict properties so they can be set only once, indeed, but this is a specific case. In C++ we have const member, and in C#read-only (feel free to correct me here).
In any case, I am not sure if your proposal suggests that constructors properties could be set only once.

I didn't phrase that correctly. I meant that with initialization lists you have the option to set these properties only once, instead of having to set them to "default" and then to something else. I didn't mean that they cannot be further modified.

raster added a comment.Dec 6 2018, 6:06 AM

indeed ordering may be an issue - some properties may be affected by the state of other properties at the time they are set... :)

woohyun added a subscriber: woohyun.Dec 6 2018, 6:12 PM

Do we have a way something like ...

class Efl.Ui.Win ( ...
{
....

 constructors
{
    win_attribute : structure;
}

}

win_attribute structure includes win_name, win_type, and accel_preference as a structure.

If we are worrying about ordering + extension of constructor parameters, I think it would be better.
How do you think about this ?

@woohyun I do not understand what do you mean. Right now, the constructors section is just a list of properties or methods which are already defined for that class.

  • In your example above, is win_attribute an existing property, or are you defining a new one?
  • What is the content of that structure?
  • Can you provide a full example of that EO file?
  • How would you translate that example into a C# constructor?