Page MenuHomePhabricator

efl.ui.radio_group_impl
Closed, ResolvedPublic

Description

Just implements Efl.Ui.Radio_Group

Related Objects

bu5hm4n created this task.Jun 20 2019, 8:42 AM
bu5hm4n triaged this task as TODO priority.

Related to radio, we need to support setting radio into other containers. (e.g. Efl.Ui.List)

This is exactly the reason why @bu5hm4n separated Efl.Ui.Radio_Group into an interface and an implementation class. This separation allows attaching the Efl.Ui.Radio_Group_Impl implementation to any widget, turning the widget into a radio group manager.
This is currently in use in the Efl.Ui.Radio_Box widget, making it very easy to implement and to use.
I think you should do the same thing to include radio buttons inside a list: Create an Efl.Ui.Radio_List widget, that behaves like Efl.Ui.Radio_Box.

@segfaultxavi
I reply the comment in T7867 here.

I understand the initial purpose of making Efl.Ui.Radio_Group_Impl but I think we need to consider opening this group class publicly to let developers use this group class for their custom radio group container.
Because framework cannot provide all kinds of containers/widgets but framework should provide ways to customize containers/widgets.

Based on the current status, it seems that either 1. framework should provide all kinds of radio group containers OR 2. developers should create their own eo class with attaching Efl.Ui.Radio_Group_Impl.
I think both 1 and 2 are not easy ways.
Moreover, It would be also difficult to create the customized radio group container class in language binding levels (e.g. C#) as well.

I think the easier way for both framework and developers is to open radio group class publicly and developers put radio buttons anywhere (container) and add the radio buttons to radio group class instance.

What do you think?

I think i do not understand. What exactly are you proposing ?

@Jaehyun_Cho your proposal is already available. Anybody can instantiate an Efl.Ui.Radio_Group_Impl and register radio buttons to it. It's just a matter of finding a better name.
Options 1 and 2 exist for convenience: they simplify some common cases.

I don't understand either what are you proposing.

@bu5hm4n @segfaultxavi

The following is my initial comment in T7867 of this comment threads.. ;)

Although we have class Efl.Ui.Radio_Group_Impl and we can use it to support the above scenario but the name (Efl.Ui.Radio_Group_Impl) does not look good enough to be publicly opened to be used.

Is it OK if we discuss the new name of it?

In my comment in T8025, I explained why we need to let developers use radio group class (now it is Efl.Ui.Radio_Group_Impl).
Because we need to change the class name to let developers use radio group class. Efl.Ui.Radio_Group_Impl does not look appropriate as a public class.

Can we discuss the new name for Efl.Ui.Radio_Group_Impl? :)

OK, so we all agree, and the only discussion is about renaming Efl.Ui.Radio_Group_Impl. Good. Let's do some brainstorming.

  1. First off, I'd like to propose renaming Efl.Ui.Radio --> Efl.Ui.Radio_Button. I think it's clearer this way.
  1. Then, how about Efl.Ui.Radio_Group_Impl --> Efl.Ui.Default_Radio_Group ?
  1. Finally, we might consider putting all Radio classes in their own namespace: Efl.Ui.Radio.Button, Efl.Ui.Radio.Group and Efl.Ui.Radio.Default_Group. What do you think?

I like the 3ed the most, it would cleanup our toplevel Efl.Ui. domain a lot.

@segfaultxavi
Thank you for suggestion! :)

I really like your first suggestion!

About second suggestion, the word "default" does not look the best because there is no other types of radio group.
What do you think about Efl.Ui.Radio_Button_Group?

About third suggestion, I prefer we do not use namespace for Radio because we only have 2 classes and 1 interface here and the Efl.Ui.Radio.Button may conflict with Efl.Ui.Button with using namespace in language bindings. (e.g. using Efl.Ui; and using Efl.Ui.Radio;)

@segfaultxavi @bu5hm4n

Related to the Radio_Button, what do you think about Efl.Ui.Check inherits from Efl.Ui.Button?

I discuss about this with @SanghyeonLee and we think that it would be better if Efl.Ui.Check inherits from Efl.Ui.Button and Efl.Ui.Radio_Button inherits from Efl.Ui.Button.
i.e. Efl.Ui.Check -> Efl.Ui.Button
i.e. Efl.Ui.Radio_Button -> Efl.Ui.Button

Because both Check and Radio_Button change their state to "selected" if "clicked" event happens.
But unlike Check, Radio keeps its state as "selected" if "clicked" event happens on its "selected" state. (Check changes its state to "unselected" in the same case)

Maybe we can also introduce something like Checkable interface or CompoundButton in android.

Sorry to discuss more stuff here but it seems related to Radio_Button and it looks very important.

zmike added a comment.Jul 16 2019, 5:43 AM

Because both Check and Radio_Button change their state to "selected" if "clicked" event happens.
But unlike Check, Radio keeps its state as "selected" if "clicked" event happens on its "selected" state. (Check changes its state to "unselected" in the same case)

I'm not sure I understand this point?

Conceptually, when a button is "clicked", it enters the "pressed" state and then immediately returns to the "unpressed" state. At no point does a button remain "pressed" or, as you call it, "selected".

When a check or a radio is "clicked", it enters the "selected" state. It does not leave this state unless another click occurs. While it's true that clicking the same check again has a different effect than re-selecting a radio, typically you would consider the entire radio group as a single widget; one radio by itself is not very useful.

While it's true that check and radio do accept click events (as button does), this is just a functionality of clickable and is not specifically related to button.

Regarding the renames:

  1. It is agreed then: Efl.Ui.Radio --> Efl.Ui.Radio_Button.
  1. If Efl.Ui.Radio_Group is the interface name, Efl.Ui.Radio_Button_Group does not make any sense as the implementation name, sorry :)

    How about Efl.Ui.Radio_Group_Basic, _Simple, or _Single? These give a bit more information on what this particular implementation of the Efl.Ui.Radio_Group interface does, which is good.
  1. There would be very few classes inside the Efl.Ui.Radio namespace so I prefer not to use it.

    However, I wanted to say that avoiding name collisions between Efl.Ui.Button and Efl.Ui.Radio.Button is not a good argument. We already have multiple collisions with the Object name. In C#, when you start using multiple namespaces you know you will have collisions, and it will be your fault!
In T8025#138411, @zmike wrote:

Because both Check and Radio_Button change their state to "selected" if "clicked" event happens.
But unlike Check, Radio keeps its state as "selected" if "clicked" event happens on its "selected" state. (Check changes its state to "unselected" in the same case)

I'm not sure I understand this point?

Conceptually, when a button is "clicked", it enters the "pressed" state and then immediately returns to the "unpressed" state. At no point does a button remain "pressed" or, as you call it, "selected".

When a check or a radio is "clicked", it enters the "selected" state. It does not leave this state unless another click occurs. While it's true that clicking the same check again has a different effect than re-selecting a radio, typically you would consider the entire radio group as a single widget; one radio by itself is not very useful.

I think you understand what I meant.
The main idea is that check and radio should be able to use "clicked". Because their "selected" state comes after "clicked".

While it's true that check and radio do accept click events (as button does), this is just a functionality of clickable and is not specifically related to button.

It would be fine if check and radio implements clickable but not inherits from button.
The reason why I commented inheritance from button was because I thought check and radio can be regarded as a kind of button.
As you know, we can easily find other frameworks which design that check and radio inherit from button.

I totally agree with adding Clickable to check (that will also bring it to radio). (Not sure right now why we are discussing this here, and not in T7865)

To get back to the original topic: names.
I think we should add a namespace "Radio" and stuff everything arround Radio into this. We can add a lot of radio containers into this namespace, and it will be very cleanedup. Imagine our documentation, you click on the Efl.Ui. Namespace and get a shitload of class, everything from all the radio classes, all the item classes, all the spotlight classes. In one level, alphabetical sorted, not by meaning, which is different in a few cases. Just imagine this giant list of widgets is already driving me into the direction of "we should use more namespaces."

Regarding namespaces, @cedric raised an interesting point on IRC. The generic containers and their different managers (like Item_Container + List/Grid_Manager, or SpotLight + Scroller/Stack_Manager) will in the end be wrapped in an easy-to-use single widget (like List, or Pager, or Stack).
If we go the namespace route, then these easy-to-use widgets should go inside the namespace too, which will look weird to the users, that know nothing of the widget implementation (Item_Container.List? Spotlight.Pager???)
In the case of Radio, the user will have to instantiate Radio.Radio_Button, which is just redundant (not as weird as the others).

So, in this light, I think I prefer no namespaces. But let's continue discussing :)

However, I wanted to say that avoiding name collisions between `Efl.Ui.Button` and `Efl.Ui.Radio.Button` is not a good argument. We already have multiple collisions with the `Object` name. In C#, when you start `using` multiple namespaces you know you will have collisions, and it will be your fault!

Yes, you are right. The collisions are not the problem of the namespace.

  1. If Efl.Ui.Radio_Group is the interface name, Efl.Ui.Radio_Button_Group does not make any sense as the implementation name, sorry :)

    How about Efl.Ui.Radio_Group_Basic, _Simple, or _Single? These give a bit more information on what this particular implementation of the Efl.Ui.Radio_Group interface does, which is good.

I discussed with @SanghyeonLee and we wanted to re-organize our selectable interfaces and use them for radio group. (maybe I need to write this to the task for selectable interface)
This change makes Radio_Group class from interface.
(some details are required to be discussed more and also can be changed.)

I think this allows us to remove the difficulties for naming radio group class and also helps us to re-organize our selectable interfaces.

What do you think about it?

I like the proposal. However, Radio_Group is a interface and needs to stay a interface, but it can inherit from Efl.Ui.Single_Selectable. My only fear here is that the single / multi_selectable interface bring again the same problem of types that we have in the pack_linear interface. We need some sort of template in order to express the types correctly. But the overall concept does look nice. Can you create a separate "RFC: ..." ticket in the "efl: api" project?

bu5hm4n moved this task from Backlog to Trivial on the efl: api board.
bu5hm4n moved this task from Trivial to Evaluating on the efl: api board.

I created https://phab.enlightenment.org/T8057 for refactoring interface Efl.Ui.Selectable.

Could you tell me why Radio_Group should remain as an interface based on the proposal?

If Radio_Group is class, then we need to have Radio_Group class only.
But if Radio_Group is interface, then we need to have Radio_Group interface and Radio_Group_xxx class.

I am tired of repeating myself, and i am not going to discuss it again: https://phab.enlightenment.org/T7867#138325

@bu5hm4n

If Radio_Group becomes a class based on the proposal, your design to turn any container into radio group container works.

Because Radio_Group class instance is attached as a composite object in the same way. i.e. efl_composite_attach(obj, pd->group);

Since register() and unregister() is not required to be called by user with radio group container and all the selected methods are provided in SingleSelectable interface, it would be the same.
e.g. User calls pack() to register Radio_Button to Radio_Box. User calls select(), unselect(), and other selected methods which may not be written by me in the proposal.

Radio_Group right now:
(P) selected_object
(P) selected_value
(m) register
(m) unregister
(E) value,changed

With the multiselect interface:
(P) selected_value
(m) register
(m) unregister
(E) value,changed

So no, we still need the interface.

If you say we need interface because SingleSelectable interface does not have (P) selected_object, then we can add it to Single_Selectable interface, right?

Since register() and unregister() is not required to be called by user with radio group container and all the selected methods are provided in SingleSelectable interface, it would be the same.
User calls select(), unselect(), and other selected methods which may not be written by me in the proposal.

No, i am not talking about selected_object. I am talking about selected_value, which works with the property state_value of radio button. There is not reason to drag the property selected_value into the Single_Selectable Interface, this API only does make sense on the Radio_Group, not on a container for items, nor on a multiselectable interface.

About state_value of Efl.Ui.Radio_Button, I would like to let's think if we really need this property.

From the point of view of identifier, state_value seems a duplicated property because we already have unique id (eo object handle).

Moreover, it would be difficult to guarantee if state_value of each Efl.Ui.Radio_Button is a unique value in one Efl.Ui.Radio_Group.

So personally, I think it would be better we remove state_value from Efl.Ui.Radio_Button.

What do you think?

All in all, we cannot get rid of it, details:

  1. the only thing making a Radio_Group a Radio_Group and not just a random set of checks is the association with a numerical number.
  2. Try to take a example usage without this API, this will not be easy for any user.
  3. It is not difficult to guarantee if state_value of each Efl.Ui.Radio_Button is unique, it is already done.

I understand the convenience of state_value.

How about using index instead of state_value?

If we use index (which is the order of setting into the container) for selecting, then we don't need to set state_value for Efl.Ui.Radio_Button manually.
Moreover, select_index() can be added to SingleSelectable interface because this SingleSelectable is supported for containers and index is common way to identify contents inside a container.

What do you think?

I do not like that, that would imply that our radio group would only work for continues number ranges. Additionally, the order of radio button registration would be given by the values of an enum, which is bad IMO.

All in All, i see no way around selected_value in efl_ui_radio_group.

I think that the use case you mentioned is so special and specific case.
IMO, most of the cases, it appears that user feels no difficulties when they use continuous numbers for Radio_Buttons.

It is difficult to me to imagine the use case that those discontinuous values should be required for Radio_Buttons.
Even if the use case exists, it would be easy to implement with the index concept.

Jaehyun_Cho added a comment.EditedJul 22 2019, 3:39 AM

I don't know how important of using integer values instead of continuous indices for Radio_Buttons.

But to support that state_value, we need 1 interface + 1 class here instead of 1 class.

Unless the functionality is really important, I think it is not a better way to add one more interface for that.

bu5hm4n added a comment.EditedJul 22 2019, 3:46 AM
  1. You left out the fact, that the order of UI elements on the screen would be defined by the internal numbering of enums, which is pretty horrible
  2. A compiler can give a enum every single value it likes to have, there is no assertion over the fact that the numbers assigned to the enum fields are continues.
  3. Just rewrite a single usage of the radio group in our tree, you will see that it will be hard to understand for someone from the outside, understanding the numbering, and you will see that by the time you have discontinous ranges, you are standing infront of a unsolvable problem
  4. We even have sample usages in the tree of discontinues ranges

last but not least: i do not know how the others are seeing this, but your proposal of how this should work, sounds very irritating, and unobvious. We have it right now like the rest in the world & legacy, why would we change it to a system that can serve fewer usecases?

Additionally: If we ever want to add an additional feature to radio_group, it will NOT BE POSSIBLE because there is no public place for extending the features of radio_group.

Just as a side note, we can make radio_group.register() give buttons increasing state values if they have not been set. In this way both mechanisms work:

  • Users can just add buttons and rely on their state values being consecutive.
  • Users can give each button an individual state value. We will ensure that they are unique or error out.

A feature like xavi described here can still be added ontop of what we have right now.

IMO what we have here seems quite reasonable, it matches the feature set we had from lagacy, and has a IMO quite clear API, it is also integrated into the common interfaces, and implements the interfaces we have, so this looks fine ?

This seems okay?

Looks good to me :)

bu5hm4n moved this task from Evaluating to Stabilized on the efl: api board.Sep 2 2019, 4:54 AM