Page MenuHomePhabricator

eolian: Add @ctor_param hint to constructors section
Closed, ResolvedPublic

Description

Description

As discussed in T7375, it would be handy if some classes's constructors had additional parameters to initialize commonly-used properties.

Not ALL properties should be initializable through constructor parameters, though, because that would create an explosion of parameters.
Instead a new tag is proposed, @ctor_param to mark those properties which COULD be initialized from a constructor parameter. This is just a hint, it is up to the eolian generator for the language to honor it.

abstract Efl.Ui.Widget ( ... ) {
  constructors {
    @ctor_param style;
  }
}

The tag should be inheritable, so in the above example, the constructor for any class inheriting from Efl.Ui.Widget should have the style parameter.

Purpose

The plan is to ignore this new tag in C, since nobody has expressed interest in it, but in C#, instead of having to write:

var btn = new efl.ui.Button(win, (efl.ui.IButton b) => {
  b.SetStyle("anchor");
});

You could write:

var btn = new efl.ui.Button(win, "anchor");

As an added benefit, the parameter is fully typed and could benefit by the IDE's IntelliSense.

The initialization method would still be available. In fact, the new signature would be something like:

public Widget(efl.IObject parent = null, ConstructingMethod init_cb=null, string style = null);

Warning

If we abuse this tag we'll end up with an explosion of constructor parameters. Right now, there's been only a request to add the style property from the Efl.Ui.Widget class, so nothing else should be added as a constructor parameter.

segfaultxavi triaged this task as TODO priority.
q66 added a comment.Oct 4 2018, 6:27 AM

I said this before but I'm not at all happy with the constructor system as it is, so I don't think it's right to just keep extending it and make things even harder for generators. Right now things are difficult and don't map well to pretty much any OO language, on the other hand I'm not entirely sure how to deal with it either :/

bu5hm4n added a subscriber: bu5hm4n.Oct 4 2018, 6:32 AM

Okay - but if you aren't happy with the current situation, then things are likely to be changed anyways. With this we have a solution for the constructor things for now. If things change this will get reimplemented in a different way. But in the meantime we have a solution ... :)

@q66

I agree with the opinion of @bu5hm4n if the way that can make you happy can not be realized soon.
If you have some idea about how to fix (or improve) the bad construrctor system, could you share your idea here ?

@q66 ping?

@q66 @woohyun @segfaultxavi @bu5hm4n
If there is no new idea on this, then I think we need to support @segfaultxavi 's suggestion with @bu5hm4n 's suggestion.
i.e. supporting @ctor_param in constructors section to generate C# constructor function with the parameter (@segfaultxavi 's suggestion)
i.e. supporting @property case only for @ctor_param and supporting @optional or @essential for @ctor_param to generate combination of constructor functions with the parameters (@bu5hm4n 's suggestion)

Yes, I think we can move forward with adding @ctor_param. From eolian's viewpoint, it's just adding an optional tag, which only the C# bindings will use for now.
Moreover, only one eo class will use it (Efl.Ui.Widget to allow setting the style property from the constructor) so if we later decide to use a different approach, only one eo file will need to be changed.

If I remember correctly, the rest of the discussion regarding adding even more tags (@optional and @essential), was to prevent generating ambiguous constructors (where the actual parameter being supplied could not be inferred from the parameter's type). I suggest we do not complicate eolian any more at this point. If the C# generator produces ambiguous constructors, then the code will not compile and whoever tried to add the new @ctor_param can raise the issue and we discuss again.

@segfaultxavi
I totally agree with you. For now, we have only one case (i.e. @ctor_param style) so we don't need to consider @optional or @essential.

q66 added a comment.Oct 30 2018, 4:45 AM

I still don't think that this will actually solve any real problem, but feel free to go ahead if you think it'll help, the constructor system is unlikely to stay in long term anyway

just put me in reviews whenever you have the code

q66 closed this task as Resolved.Oct 30 2018, 6:49 PM
bu5hm4n moved this task from Backlog to Done on the efl: language bindings board.Dec 18 2018, 4:30 AM
bu5hm4n raised the priority of this task from TODO to Normal.