Page MenuHomePhabricator

Support widget styles in C# bindings
Open, HighPublic

Description

Now, in C# bindings, there is no way to support widget styles unless new classes for styles are created.

The above concept requires many classes since each style requires one class.

To resolve this, using style parameter in constructor of C# bindings is suggested. (by @herb, @woohyun)

e.g. var button = new efl.ui.Button(parent, efl.ui.Button.Type.Anchor);

According to @herb, the followings are required.

1. Support style parameter in constructor by C# bindings generator

sealed public class Button : IButton
{
   ...
   //This should be defined for style strings
   public struct Type {
      public const string Default = null;
      public const string Anchor = "anchor";
   }

   //Load native fucntion symbol for changing style name
   [System.Runtime.InteropServices.DllImport(efl.Libs.Elementary)]
   public static extern bool elm_widget_theme_style_set(System.IntPtr obj, string name);

   //Add type string parameter
   public Button(efl.IObject parent = null, string type = null, ConstructingMethod init_cb = null)
   {  
      System.IntPtr klass = efl_ui_button_class_get();
      System.IntPtr parent_ptr = System.IntPtr.Zero;
      if(parent != null)
         parent_ptr = parent.raw_handle;
      handle = efl.eo.Globals._efl_add_internal_start("file", 0, klass, parent_ptr, 1, 0);
      register_event_proxies();
      if (init_cb != null) {
         init_cb(this);
      }                                                                                                                                                      

      //Set style by the given type string parameter
      //This should be done before _efl_add_end is called (before finalize)
      if (type != Type.Default)
        elm_widget_theme_style_set(handle, type);**

      handle = efl.eo.Globals._efl_add_end(handle, 1, 0);
      eina.Error.RaiseIfOccurred();
   }
}

2. Modify all widgets not to set theme before finalized

EOLIAN static void
_efl_ui_button_efl_canvas_group_group_add(Eo *obj, Efl_Ui_Button_Data *_pd EINA_UNUSED)
{
   ...
   //Delete the following code to set theme in finalize in efl_ui_layout_object.c
   /*
   if (!elm_widget_theme_object_set(obj, wd->resize_obj,
                                       elm_widget_theme_klass_get(obj),
                                       elm_widget_theme_element_get(obj),
                                       elm_widget_theme_style_get(obj)))
     CRI("Failed to set layout!");
   */
   ...
}

I need a bit of background here... what is wrong with efl.ui.Widget.SetStyle() and efl.ui.Widget.GetStyle()? Do they work?

@segfaultxavi

As far as I know, when efl interface was designed, people think that one class represents only one style.

So SetStyle() and SetTheme() works only if they are called before efl_finalize(). If SetStyle() and SetTheme() are called after efl_finalize(), then nothing happens.

As you know, efl_add() calls _efl_add_internal_start() and then calls _efl_add_end(). And _efl_add_end() calls efl_finalize().

However, now it seems that so many classes are required for the above policy.

So I think that it would be better if we support styles without creating classes.

I still do not understand what is the problem. I verified that the following code works:

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

This works exactly the same in C and C#, you have to set the style during initialization, not afterwards.

Why do you say that "so many classes are required"?

Jaehyun_Cho added a comment.EditedSep 10 2018, 1:47 AM

@segfaultxavi

As far as I know, the original purpose was that one class only represents only one style.

That is why I said many styles are required based on the original purpose.

But like you said, your code enables SetStyle() before instantiating. Is the code normally used form in C#?

If so, then your way is another candidate way for this issue.

Anyway we need to define style strings in each widget class in C# (to display style strings automatically) and to modify widget codes not to set theme twice.

That initialization code is the proposed way to configure widgets in C#, yes. Please take a look at the C# Hello World tutorials:
https://www.enlightenment.org/develop/tutorials/csharp/hello-world-cs.md
https://www.enlightenment.org/develop/tutorials/csharp/hello-world-gui-cs.md

I do not think we need to add special constructor parameters for this, as the initialization method already in place works fine.

Regarding the list of style strings, that seems useful, yes, but I do not have enough background. How does that work in C? How is the C programmer supposed to know the available style names?

OK, I have done some research. There cannot be a hardcoded list of available styles, because that depends on the theme. Each .edc file can define any number of styles and EFL does not guarantee that any style will exists in ALL themes, except default.

BUT we can add an EFL API to query which styles are available for a given widget. Then you can find out which styles are available at runtime for any widget, and that will depend on which themes you have installed. This is a new feature, though, and I suggest you create another task with this feature request.

Do you think this ticket can be closed now?

@segfaultxavi

I think we need to discus a bit more.

I understand your point. But we need to guarantee which widget styles are publicly supported. (e.g. "anchor" style is publicly supported in Efl.Ui.Button.)

This means that the publicly supported style should exist in theme as well. Otherwise, that style is not publicly supported.

Consequently, I think that if a style always exists in theme because it is publicly supported, then the style can be explicitly defined in the class as well.

@segfaultxavi

To support widget styles in a explicit way, there was an idea which provides one class for one style. (e.g. Efl.Ui.Button, Efl.Ui.Button_Anchor)

If the widget's functionality is different, then different class is fine. But in many cases, only widget look is different. So now we are considering how to support setting widget styles in a better way..

Consequently, I think that if a style always exists in theme because it is publicly supported, then the style can be explicitly defined in the class as well.

Yes, of course, if a style is guaranteed to exist, then we can hardcode its name in the class.

What I do not know is which styles are guaranteed to exist. Has this ever been discussed? Is there a list somewhere?

To support widget styles in a explicit way, there was an idea which provides one class for one style. (e.g. Efl.Ui.Button, Efl.Ui.Button_Anchor)

If the widget's functionality is different, then different class is fine. But in many cases, only widget look is different. So now we are considering how to support setting widget styles in a better way..

I was not aware of that discussion, but if there is already a working styling mechanism, I would not create new classes just to change the looks.

So, do you still need a better mechanism to set the style. Do you not like this one?

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

Yes, of course, if a style is guaranteed to exist, then we can hardcode its name in the class.
What I do not know is which styles are guaranteed to exist. Has this ever been discussed? Is there a list somewhere?

Sorry I said only based on Tizen. Because Tizen has its own publicly supported widget styles.

So, do you still need a better mechanism to set the style. Do you not like this one
var btn = new efl.ui.Button(win, (efl.ui.IButton b) => { b.SetStyle("anchor"); });

I totally understand your point (i.e. we provide a way to support SetStyle() but why do we need to provide another way?).
However, I think that this is not a common way but this is an EFL C# specific way.
So for better usability to application developer, I think it would be better if we provide constructor with style parameter. (e.g. new efl.ui.Button(parent, style);)

I agree the initialization method is very EFL-specific and a bit weird. Maybe the C# bindings can translate that into something else more C#-friendly.

If we decide to do so, though, I would rather use a generic method, that allows you to change any property of the object, not only the Style.

What about named parameters? Something like:

var button = new efl.Ui.button(win, style: "anchor", size: new eina.Size2D(360, 240), position: new eina.Size2D(20, 20));

And I would keep accepting the initialization method, for things which are not properties of the object being created (like adding a widget to a container box).

What do you think @lauromoura @vitor.sousa @felipealmeida ? Is this doable?

@segfaultxavi

I am afraid that the named parameters are not more convenient than basic API usage. (e.g. button.SetSize();)

If we input "button.", then "SetSize" will be automatically displayed and if we select "SetSize", then the argument will be automatically displayed like "eina.Size2D" and I can input the parameter easily.

But if I use the named parameter, then I don't think "size" and its parameter will be automatically displayed.

My opinion is that we provide constructor with style parameter. Then application developer can set style easily.

Because style is common property and it is the only property which is required to be set during instantiation.
(Other property methods are not required to be called during instantiation. It is absolutely fine if those property methods are called after instantiation.)

Well, I feel strongly opposed to give the style such importance when we already have a mechanism to configure widgets. I think this is a slippery slope and we will end up wanting to add more and more parameters to the constructors...

But I do not use C# regularly so maybe I am wrong. I would like to know the opinion of @lauromoura @vitor.sousa and @felipealmeida ...

There's an option we have been discussing on IRC. Currently the EO format has a constructors { } section which is unused, and it is not clear what its purpose is.

We can put inside this section a list of properties which we want added to the constructor. In C it can be ignored, just as it is now. In C#, it can do something like:

efl_ui_widget.eo

class Efl.Ui.Widget {
  @property style { ... }
  constructors {
    style
  }
}

efl_ui_widget.eo.cs

class Efl.Ui.Widget {
  public Widget(efl.IObject parent = null, string style = null, ConstructingMethod init_cb=null)

}

This is more general than adding a special case for styles, so I like it more.

@Jaehyun_Cho would you like this solution? a lot of details have to be worked out yet.

There's an option we have been discussing on IRC. Currently the EO format has a constructors { } section which is unused, and it is not clear what its purpose is.

I've added the log of the conversation to P232 for more details.

This is more general than adding a special case for styles, so I like it more.

Something like this indeed should be the way to go instead of treating style especially.

We did use "constructors" {} before for C++ and it wasn't great.

The idea in "constructors" is actually that a property or method there, then setting it before efl_finalize would be very adventageous, or maybe strictly requried (such as window type property).

That's exactly the case for style_set (if it is not in constructors {}, then it should).

However, using those as parameters for constructors was not good, because the property "names" have a lot of meaning, and using only types leads to very big constructor types with zillions of default values.

The idea, at the time, btw @q66 I think was there too, is that passing a lambda that allows setting whatever properties on constructing time was the proper way to do things. Keeping properties separate from actual constructors, but with a semantics that may get the property value fixed after construction (finalize).

For C#, if using different styles is needed and someone wants to use different class names, why not Tizen just create a ButtonAnchor class that inherits from Button and calls StyleSet itself? It would be a 5-line class definition.

class ButtonAnchor : Button
{
  ButtonAnchor () : base((efl.ui.Button b) => { b.SetStyle("anchor"); })
  {}
}

@segfaultxavi @lauromoura

efl_ui_widget.eo

class Efl.Ui.Widget {

@property style { ... }
constructors {
  style
}

}

efl_ui_widget.eo.cs

class Efl.Ui.Widget {

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

}

This is more general than adding a special case for styles, so I like it more.

@Jaehyun_Cho would you like this solution? a lot of details have to be worked out yet.

YES! This form is exactly what I want! Thank you :)

After I saw the IRC log, I realized that there was a huge misunderstanding... maybe it's because of my explanation ;)

To me it does not matter if all widget constructors contain "style" parameter or not.

What matters to me is to support constructor with "style" parameter for widgets having styles which are publicly opened. (The styles are written in eo or somewhere to also generate string definition in the class for IntelliSense.)

So your suggestion is exactly what I want! And it perfectly fulfills my purpose! :)

So your suggestion is exactly what I want! And it perfectly fulfills my purpose! :)

Good! Let's look at the comments form @felipealmeida:

We did use "constructors" {} before for C++ and it wasn't great.

I understand they are not in use now and we are free to change its usage?

However, using those as parameters for constructors was not good, because the property "names" have a lot of meaning, and using only types leads to very big constructor types with zillions of default values.

Yes, this is a problem, specially if one constructor inherits all the parameters from its parents. I was thinking on using constructor parameters only in very special cases. In fact, right now, I would only use them to solve the problem with the style parameter. style is the only constructor parameter I would add at this moment, and I would remove all other instances of the constructors section from all eo files.
Do you see any problem with this, @felipealmeida?

The idea, at the time, btw @q66 I think was there too, is that passing a lambda that allows setting whatever properties on constructing time was the proper way to do things. Keeping properties separate from actual constructors, but with a semantics that may get the property value fixed after construction (finalize).

This is already in place and works perfectly, as far as I can see. I would not touch this mechanism. Just add the option to also have constructor parameters in some very special cases, for simplicity.

For C#, if using different styles is needed and someone wants to use different class names, why not Tizen just create a ButtonAnchor class that inherits from Button and calls StyleSet itself? It would be a 5-line class definition.

This was the initial approach by @Jaehyun_Cho, but he realized he would need to do this for all current widget classes, for all styles, so he asked for a more maintainable option.

If @felipealmeida and @q66 do not foresee any problem, it looks like this could be a valid option.

@segfaultxavi @Jaehyun_Cho So you want a way to transform some very very special constructors into parameters to classes's constructors, in this case for style. There could be a way, but using constructors {} should be kept as-is IMO because it must be available for documentation purposes.

Maybe we could use a tag inside constructors to ask bindings to add that to constructors?

Regards,

segfaultxavi added a comment.EditedSep 11 2018, 7:08 AM

How does the documentation use the constructors {} section?

A tag inside constructors works for me too. Just to be sure, you mean something like this:

class Efl.Ui.Widget {
  @property style { ... }
  constructors {
    .some_method;      // This is an old constructor method for purposes yet unknown to me
    @ctor_param style; // This will add a parameter to the default constructor for easier initialization of the style.
                       // Use sparingly to avoid turning the default constructor into a monster!
  }
}

Just to be clear: @ctor_param would be the tag that we introduce for this job. It does not exists yet :)

This @ctor_param would behave just like a hint, right? So each language decides whether it should support them as explicit constructor parameters (C#) or keep it implicit through efl_added (C/C++).

Also, what would be the semantics of it?

  • Any property that we'd like to put into the constructor (@segfaultxavi's justified fear)?
  • Those special properties that can only be set inside the constructor?

@segfaultxavi I'm not sure it does yet. But it should. Otherwise how is the user suppose to know it must call the method in the passing lambda or inside efl_add, instead of just setting that after instantiation? Documentation must be explicit about it, IMO.

I think you get the gist of what I mean, your example is a good example and I actually like @ctor_param. I also think, as @lauromoura mentions, that it should be optional in a binding-by-binding choice.

Regards,

@segfaultxavi @lauromoura @felipealmeida

After discussing about this topic with @woohyun, we realized that we need to support "style" parameter for all widget constructors.

Sorry for confusing because of my last agreement with @segfaultxavi ..

The reason is that styles can be different per profile and newly supported or deprecated per version.

  • Per profile (e.g. desktop, mobile), supported styles may be different.
  • Per version, styles may be newly supported or deprecated.
  • Since we need to provide same set of APIs for all profiles, we need to support "style" parameter for all widget constructors.
  • If we provide a constructor with "style" parameter only for one style. But if the one style is deprecated, then we cannot deprecate the constructor with "style" parameter because a new style for the widget can be supported later.

In short, we believe that we need to support constructor with "style" parameter for all widgets.

@felipealmeida

class Efl.Ui.Widget {

@property style { ... }
constructors {
  .some_method;      // This is an old constructor method for purposes yet unknown to me
}

}

Do you mean the ".some_method" above is only for documentation? i.e. documentation that explains this method should be called in efl_add()?

@lauromoura

Also, what would be the semantics of it?

  • Any property that we'd like to put into the constructor (@segfaultxavi's justified fear)?
  • Those special properties that can only be set inside the constructor?

For now, "style" is the only property. Precisely, SetStyle() should be called in the constructor.

SetStyle() should be called before finalize(). It means that SetStyle() should be called during constructor because finalize() is called the end of constructor.

@lauromoura @felipealmeida:
Yes, @ctor_param would be a hint. C can ignore it. C# can use it to add extra constructor parameters.
I don't think the current docs are using the constructors {} information at all. At least I cannot see it:
https://www.enlightenment.org/develop/api/efl/model/container/item is generated from:

class Efl.Model_Container_Item (Efl.Object, Efl.Model) {
   methods {
      define { ... }
   }
   constructors {
       .define;
   }
}

@Jaehyun_Cho:
With your question I feel like we are going backwards. Let me summarize the discussion to make sure we are all on the same page:

1. You can already set the style of any widget now

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

The code above works fine. You do not need anything else to support styles, regardless of profile or version.
What we are discussing now is a mechanism to make it simpler, and that allows IntelliSense to work.

2. We are now discussing a way to flexibly add constructor parameters

We are thinking about adding this bit to efl_ui_widget.eo:

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

The C# bindings generator will then add a new parameter string style to the default constructor:

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

And will automatically call SetStyle(style); for you inside the constructor.

So, in summary, this approach adds the constructor parameter that you want, but in a more flexible way. And only in C#, the C code does not change.

Do you like this approach?

@segfaultxavi

  1. You can already set the style of any widget now

I have no argument about this case. It is totally ok.

  1. We are now discussing a way to flexibly add constructor parameters

My question here is that "Do you mean that we are going to add @ctor_param style to Efl.Ui.Widget to support "style" parameter to all widgets' constructors which inherit from Efl.Ui.Widget?"

If so, then you and I we are saying exactly the same thing. My point is that we need to support "style" parameter to all widgets' constructors.

We can refer Android's View class public constructors as follows. (https://developer.android.com/reference/android/view/View)
Due to this View's constructors, all widgets (e.g. Button, DatePicker, etc.) have constructors with same parameters. (i.e. int defStyleAttr)

  • View(Context context)
  • View(Context context, AttributeSet attrs)
  • View(Context context, AttributeSet attrs, int defStyleAttr)
  • View(Context context, AttributeSet attrs, int defStyleAttr, int defStyleRes)

If we apply above to EFL, then the form would be as follows. (All widgets which inherit from Efl.Ui.Widget have the same form of constructors.)

  • Efl.Ui.Widget(parent)
  • Efl.Ui.Widget(parent, init_cb)
  • Efl.Ui.Widget(parent, int_cb, style)

or

  • Efl.Ui.Widget(parent)
  • Efl.Ui.Widget(parent, style)
  • Efl.Ui.Widget(parent, style, int_cb)

OK, we were saying the same thing then 😄
The idea is that all widgets inheriting from Efl.Ui.Widget will have the extra style constructor parameter.

It looks like we are all in agreement!
I am tempted to ask the mailing list what they think, but since the new tag is optional it doesn't need to be implemented in C, so it will only affect C# for now.
If nobody says anything against it in 24h, I will create a task to add the @ctor_param tag (name yet to be decided).

@segfaultxavi

I guess there is no objection about your proposal :)

Could you create a task for the proposal?

@felipealmeida @lauromoura @vitor.sousa T7399 has been committed. Is that enough to start testing C# constructors with extra parameters?

lauromoura triaged this task as High priority.

From the C part, I think so. It'll need some work on the eolian_cxx library (which the C# generator uses) but we already work on both the bindings and eolian_cxx at the same time regularly.

@lauromoura

could you share the current status of supporting widget style in constructor in C#?