Page MenuHomePhabricator

Introduce Efl.Ui.Radio_Group & Efl.Ui.Radio_Box
ClosedPublic

Authored by bu5hm4n on May 30 2019, 5:18 PM.

Details

Summary

Radio_Group is a interface that manages that radio groups can be grouped inside a
external object, the current API of radio was considered confusing in
that regard. It is implemented in the Radio_Group_Internal class which
is private to EFL, a instance of it can be found with get due to the
class function in efl_ui_radio.eo. This architecture was taken like
this, in order to have implementation and interface seperated. With
those two seperated we can inherit from regular widgets, implement the
interface, and composite attach the internal object to the regular
widget. This makes a lot of things easier.

Radio_Box is a class which is extending Efl.Ui.Box, which has an
internal Radio_Group. This is extremly usefull for cases where you just
want to have a list of radio buttons in your UI. The radio group is also
exposed using composition to the internal object. Simular things can be
done for the table.

For now i did not add API to find the group of a radio button. However,
this can be quickly added if requested.

ref T7867

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n created this revision.May 30 2019, 5:18 PM
bu5hm4n added a reviewer: YOhoho.
bu5hm4n updated this revision to Diff 22660.Jun 5 2019, 5:00 AM
bu5hm4n edited the summary of this revision. (Show Details)

Add a new example.

Add another testcase.

Fix a bug

zmike requested changes to this revision.Jun 11 2019, 8:42 AM

This needs doc cop.

Also, why are we using the word 'selected' in radio? Shouldn't it be something more like 'active'?

Overall I think this is useful, minor changes. I'd like to get @woohyun and @Jaehyun_Cho opinions.

src/lib/elementary/efl_ui_radio.eo
25

What does this mean?

src/lib/elementary/efl_ui_radio_box.c
16

Could this be changed to a function instead for ease of debugging/reading?

29

efl_content_count should be outside the loop here so that it won't be called on every iteration.

This revision now requires changes to proceed.Jun 11 2019, 8:42 AM
bu5hm4n updated this revision to Diff 22703.Jun 12 2019, 3:20 AM
  • move count_get outside the for loop
  • make macro more readable
segfaultxavi requested changes to this revision.Jun 12 2019, 4:33 AM

Only some comments regarding the efl_ui_radio_example_01 example.
Also, the example does not render correctly for me, the window has 0-size and shows this in the console:

ERR<77946>:eina_safety ../src/lib/elementary/efl_ui_radio_box.c:18 register_safe_in_group_begin() safety check failed: efl_isa(subobj, EFL_UI_RADIO_CLASS) is false

I think it worked correctly in the previous diff.

@zmike docs have been updated in a child patch (last one in this stack).

src/examples/elementary/efl_ui_radio_example_01.c
4

This will need updating.

12

Unused

34

Nice copy&paste, my friend! :D

This revision now requires changes to proceed.Jun 12 2019, 4:33 AM
zmike added a comment.Jun 12 2019, 5:38 AM

On one hand, I like the idea of radio box, but on the other hand I feel like this opens up a can of worms. If we have radio box, shouldn't we also have a check box? What about a button box? Image box?

In D9058#166856, @zmike wrote:

On one hand, I like the idea of radio box, but on the other hand I feel like this opens up a can of worms. If we have radio box, shouldn't we also have a check box? What about a button box? Image box?

You're right, but I feel radio buttons are packed together far more often than the other widgets. As the doc master, I say users will understand that radio_box exists but not the others.

bu5hm4n updated this revision to Diff 22711.Jun 12 2019, 5:29 PM

i updated the wrong revision yesterday...

  • this makes the example work again
  • example things from xavi are fixed

After thinking about this patch and Radio widget for a while, I thought that
"how about providing Efl.Ui.Radio_Group class?" (not interface)

The Efl.Ui.Radio_Group class becomes almost same as the suggested Efl.Ui.Radio_Group interface and Efl.Ui.Radio_Group class is not a widget nor canvas object.
Consequently, all the elementary containers (e.g. Efl.Ui.Box) can be used to position Efl.Ui.Radio instances.
Efl.Ui.Radio_Group just controls only one Efl.Ui.Radio instance can be selected and Efl.Ui.Radio_Group tells us which radio is selected.

To do the above,

  • Efl.Ui.Radio_Group becomes a class. (Efl.Ui.Radio_Group_Internal implementation goes to Efl.Ui.Radio_Group)
  • Efl.Ui.Radio's @class method radio_create is not needed.
  • Efl.Ui.Radio_Box is not needed.

What do you think about it?

bu5hm4n added a comment.EditedJun 12 2019, 11:26 PM

What exactly is the problem with the Efl.Ui.Radio_Box class ? It simplifies the usage of the API a lot.
Take the efl_ui_radio_example.c, and write it without Efl.Ui.Radio_Box, you will see things are getting longer, as you have to remember that you need add the radio buttons to the group. Yet you also have to create a second object by hand. I think the box class does make a lot of sense, and should stay, i don't see a problem with it. Rather one without it ...

To make things clear, you *can* also take any other container right now to position the radio buttons, you then just have to add them to the group object by hand, the Radio_Box class does that for you, but does *NOT* force you to only use the Radio_Box class.

And all of the above comments by @bu5hm4n are already explained in the updated docs in D9061 ?

segfaultxavi requested changes to this revision.Jun 13 2019, 3:34 AM

With the latest diff the window now appears and I get no console errors, but I can check ALL radio buttons, and there's no way to uncheck them :/

This revision now requires changes to proceed.Jun 13 2019, 3:34 AM

@bu5hm4n
There are 2 things I want to comment related to the Radio_Group and Radio_Box.

  1. The creation of Radio_Group instance

I think it does not look common that Radio_Group instance is created by class method of Radio class.
If the instance is unique, then it looks fine. e.g. unique default instance or unique manager class for global usage
But Radio_Group case is not like that. It seems that the only reason is for the design of the current Radio_Box class. (to avoid multi class inheritance)

  1. Extension of C# class

Let's assume C# developers want to create their customized Radio container.
It is difficult to inherit and re-implement Radio_Box becuase it is not pure C# widget.
By only using Radio_Group interface, it is not possible to implement the customized Radio container.
Although Radio_Group_Internal class is not private and it is provided to C# developers, it might be confusing.

To resolve the above, I suggested that Radio_Group becomes a class and class method group_create is removed.
To implement Radio container, I think that Radio_Group class is inherited and container's interface can be implemented with using container instance as a composite object internally.

zmike added a comment.Jun 13 2019, 6:14 AM

I'm wondering if we want to have some kind of Widget_Box class that can be used by various widgets instead of a specific Radio_Box. Maybe not all widgets would support or use this type of thing, but as I pointed out previously it does seem like there is more use case for this than just the radio widget...

it does seem like there is more use case for this than just the radio widget...

I cannot see them. Which ones do you have in mind?
The Radio_Box makes sure there's only one radio button selected. What functionality does Widget_Box offer to its children widgets?

zmike added a comment.Jun 13 2019, 9:19 AM

it does seem like there is more use case for this than just the radio widget...

I cannot see them. Which ones do you have in mind?
The Radio_Box makes sure there's only one radio button selected. What functionality does Widget_Box offer to its children widgets?

Hm that's a valid point. I was considering it more from the perspective of "this is a logical group of widgets", so e.g., a settings dialog with a bunch of checkboxes.

Yeah, creating logical groups of widgets might make sense, but I see no benefit right now for widgets other than Radio Buttons...

Related: How do you create in Elm a box around a set of widgets to visually group them? Like elementary_test does.

zmike added a comment.Jun 13 2019, 9:51 AM

Yeah, creating logical groups of widgets might make sense, but I see no benefit right now for widgets other than Radio Buttons...

Related: How do you create in Elm a box around a set of widgets to visually group them? Like elementary_test does.

You mean like using elm_frame ?

Thanks. And I see the Unified equivalent is Efl.Ui.Frame. I had never come across it.

Back on topic: I would not create logical groupings for widgets other than radio buttons, since this is the only current use case, and 20 years of elm development have not come up with other use cases :)
We can create them later should the necessity arise.

@bu5hm4n: efl_ui_radio_example_01 still does not work for me, and neither does elm_test: all radio buttons can be selected, and the api to directly select a button seems to have no effect.

zmike requested changes to this revision.Jun 14 2019, 6:51 AM

Still a minor doc nit and then I'm okay with it.

src/lib/elementary/efl_ui_radio.eo
25

Still not addressed.

segfaultxavi added inline comments.Jun 14 2019, 8:19 AM
src/lib/elementary/efl_ui_radio.eo
25

It has been addressed in D9061 with this sentence:

Creates an @Efl.Ui.Radio_Group object to handle radio button groups. The group initially empty.
zmike accepted this revision.Jun 14 2019, 9:04 AM

Oh ok.

bu5hm4n updated this revision to Diff 22787.Jun 14 2019, 5:33 PM
bu5hm4n edited the summary of this revision. (Show Details)

fix bug, remove printf

bu5hm4n planned changes to this revision.Jun 14 2019, 5:45 PM

@segfaultxavi i rebased now the whole thing and tested again, this seems to work for me.

After meeting in korea we have agreed that i should make the class Efl.Ui.Group_Internal public, so it can be extended, that will be done soon.

@cedric @segfaultxavi @Jaehyun_Cho so my actions items here are:

  • Rename efl.ui.radio_group_internal to efl.ui.radio_group_impl and make it public available, so others can inherit from it, and the instanciation is getting easier ?
  • Make the radio_state_value_set a constructor call, error in the finalizer when no state value is set (only for non-legacy, keep behavior for legacy just like right now)

After that everyone is happy ? (Sorry, before i posted to the wrong ticket)

This still does not work for me (buttons are not mutually exclusive), I am currently debugging. I would appreciate if anybody else tested this patch and told me if it works for him.

segfaultxavi accepted this revision.Jun 18 2019, 10:16 AM

The problems I was seeing disappear after adding the whole patchset, so I got no further issues with this.

bu5hm4n updated this revision to Diff 22802.Jun 18 2019, 10:56 AM

implement latest proposed changes

This revision is now accepted and ready to land.Jun 18 2019, 10:56 AM
bu5hm4n updated this revision to Diff 22805.Jun 18 2019, 10:58 AM

and here comes the forgotten files

Closed by commit rEFLc9177a9f8d75: Introduce Efl.Ui.Radio_Group & Efl.Ui.Radio_Box (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyJun 20 2019, 7:02 AM
This revision was automatically updated to reflect the committed changes.