Page MenuHomePhabricator

efl.ui.check
Closed, ResolvedPublic

Description

class Efl.Ui.Check @beta
├ (P) selected
├ (E) selected,changed
  • Remove NState from the inheritance tree
  • Rename property to check_state and event to check_state,changed
  • Make it include the Efl.Ui.Clickable mixin, and use the signals in theme.
bu5hm4n created this task.May 3 2019, 11:19 AM
bu5hm4n triaged this task as TODO priority.

IMO this should not inherit from nstate. Check is not made for displaying n-states, everything else then 2 states is going to break the displaying (same for efl.ui.check).

The whole point of Efl.Ui.Nstate is that widgets like Efl.Ui.Check inherit from it. It provides them count (number of states) and value (current state) properties.
I agree this is a bit over-engineered, but I do not see anything wrong with it.

Efl.Ui.Check is not meant to display more than 2 states. It's a particularization of Efl.Ui.Nstate that only supports 2 states (just like Efl.Ui.Button).
These two widgets should override the count.set property and show an error if anybody tries to use it.

Then what is the point of having this property, it is an abstraction that does not work. It's useless API. Additionally, due to the fact of inheriting from button, we also get things like autorepeat implemented on check, which is something we definitely do not want ... I would really just make it inherit from layout, and continue from there

Sure, I have no objection.

Do we have any real N-state widget in EFL, though? Something like a checkbox that accepts a grayed-out third state, or something like that? Looks like most of the work on this class was done by @jpeg so we cannot ask him...

zmike moved this task from Backlog to Evaluating on the efl: api board.Jun 12 2019, 7:36 AM
zmike added a comment.Jun 12 2019, 9:21 AM

I'm not aware of any kind of tristate type widget, I guess this was some kind of future-proofing? Would be good to get @woohyun or @Jaehyun_Cho to reply here.

I don't think nstate would be used in the near future. That was the reason why I hoped to cut the relation between nstate and check :)

zmike added a comment.Jun 25 2019, 9:31 AM

I think this needs some naming changes:

  • selected property should be active or a similar word since the widget only has a state of being activated or not-activated; selection is slightly different in my mind.
  • selected,changed event can then be renamed to active,changed or something, ideally using words which are not both verbs. But also is there a reason this can't just be changed?

Yeah, definitely. selected is about choosing one option among many, and we only care about on or off here.
I could go with active but it looks too similar with enabled (meaning that the widget can be interacted with)... how about check, cross, mark, stroke or check_state?

In many other places, we have used the pattern property,changed for event names whenever they are emitted upon property being changed. We should be consistent.

zmike added a comment.Jun 26 2019, 6:38 AM

I guess that's a good point.

check_state is okay? I think?

segfaultxavi updated the task description. (Show Details)Jun 26 2019, 6:43 AM
PROPERTYEVENT
check_statecheck_state,changed
checkedchecked,changed
is_checkedchecked,changed

I also agree that "selected" is weird, but I'm sorry to not give good new name.

bu5hm4n updated the task description. (Show Details)Jul 17 2019, 8:47 AM
bu5hm4n updated the task description. (Show Details)Jul 17 2019, 9:02 AM

Let's bring here the discussion in T8025 regarding whether or not Efl.Ui.Check should implement Efl.Ui.Clickable.

I think using Efl.Ui.Clickable to reuse code is OK (although I do not see the point of Checkboxes and radio buttons emitting longpressed events).
I think inheriting from Efl.Ui.Button will be too confusing... they are conceptually different widgets. I know Gtk does that, and it has always confused me :)

segfaultxavi updated the task description. (Show Details)Jul 17 2019, 9:24 AM

If we use the *not yet existing* selectable interface on this, then check_state is not there anymore, but rather the name from the selectable interface. (T8057)

@zmike @Jaehyun_Cho do we now want to have selected removed here and move to the Efl.Ui.Selectable interface ? (D9503)
Which makes it possible to implement single_select in radio group.

So - i applied the change, the revision is linked above. If the revision is landed, things do looks fine ?

bu5hm4n updated the task description. (Show Details)Aug 5 2019, 11:02 AM
bu5hm4n updated the task description. (Show Details)Aug 6 2019, 8:01 AM

Could we move this task to "Stabilized" ?

i think so :)

bu5hm4n moved this task from Evaluating to Stabilized on the efl: api board.Aug 20 2019, 4:27 AM