Page MenuHomePhabricator

efl_ui_widget: redo disable handling
ClosedPublic

Authored by bu5hm4n on Feb 24 2019, 12:04 PM.

Details

Summary

before the disable property was a bit weird. Setting something to
disabled=true will disable all children of the widget that is changed.
However, only the update function of the children will get the false flag,
not the flag itself. Which means, to query the real disabled state, we
need to walk up the parent relations.

With this patch, every change to disabled will go through the disabled
property, which mean, a implementor can just overwrite the disabled
property, and adjust its internal state there. Just be carefull, a set
to disabled=true still might result in disabled=false. This makes the
function on_disable_update unneccesary. Which also cleans up the
Efl.Ui.Widget class.

ref T7553

Depends on D8016

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.Feb 24 2019, 12:04 PM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

bu5hm4n requested review of this revision.Feb 24 2019, 12:04 PM
woohyun accepted this revision.Feb 25 2019, 8:29 PM
woohyun added inline comments.
src/lib/elementary/elm_widget.h
356

I cannot know why this flag needs to be changed to "int".
Probably, I am missing something.

This revision is now accepted and ready to land.Feb 25 2019, 8:29 PM
woohyun requested changes to this revision.Feb 25 2019, 9:01 PM
woohyun added inline comments.
src/lib/elementary/efl_ui_widget.c
2482

If this function is called 2 times with "disabled = EINA_TRUE", then pd->disabled will be 2.
And then, if I want to change the object be "enabled", I need to call this API twice with "disabled = EINA_FALSE".

Calling the function once will not make object be enabled, because "pd->disabled" is still "1" (not zero).

I don't exactly know why this "disabled" flag needs to be operated with this way.
(Probably I'm missing something)

This revision now requires changes to proceed.Feb 25 2019, 9:01 PM

@woohyun Yes this is on purpose, for this reason:

Imagine a window with a table, that contains 6 buttons. If you now do:
efl_widget_disabled_set(win, EINA_TRUE); then the window will be disabled, and all widgets that are inside.
Now you do efl_widget_disabled_set(button5, EINA_TRUE);, the state of the window does not change.
Now you do efl_widget_disabled_set(win, EINA_FALSE); now the whole tree is enabled again except button5. Its still disabled since its explicitly disabled :)

This is why this is now an int and not a boolean flag anymore.
(btw. the behaviour above can also be done in legacy, which makes sense to me :))

A int might be a big giant, maybe a short or char is enough ?

bu5hm4n requested review of this revision.Feb 25 2019, 11:28 PM
bu5hm4n updated this revision to Diff 19689.Feb 26 2019, 2:36 AM
bu5hm4n edited the summary of this revision. (Show Details)

update & fixes

woohyun added a comment.EditedFeb 26 2019, 3:04 AM

@woohyun Yes this is on purpose, for this reason:

Imagine a window with a table, that contains 6 buttons. If you now do:
efl_widget_disabled_set(win, EINA_TRUE); then the window will be disabled, and all widgets that are inside.
Now you do efl_widget_disabled_set(button5, EINA_TRUE);, the state of the window does not change.
Now you do efl_widget_disabled_set(win, EINA_FALSE); now the whole tree is enabled again except button5. Its still disabled since its explicitly disabled :)

This is why this is now an int and not a boolean flag anymore.
(btw. the behaviour above can also be done in legacy, which makes sense to me :))

A int might be a big giant, maybe a short or char is enough ?

@bu5hm4n
But if you execute following codes, there would be difference between legacy and new codes.

elm_object_disabled_set(bt, EINA_TRUE);
elm_object_disabled_set(bt, EINA_TRUE);
elm_object_disabled_set(bt, EINA_FALSE);

ret = elm_object_disabled_get(bt);

Legacy will give EINA_FALSE, but your codes will not.

I think pd->disabled needs to be increased only when its parent is disabled newly.
But the problem is not that easy because we also need to think the case that parent is changed.

For example,

  1. a disabled button is packed to a disabled box
  2. unpacked from the box
  3. packed another box which is enabled.
woohyun accepted this revision.Feb 26 2019, 3:04 AM
This revision is now accepted and ready to land.Feb 26 2019, 3:04 AM
woohyun requested changes to this revision.Feb 26 2019, 5:07 PM
This revision now requires changes to proceed.Feb 26 2019, 5:07 PM

@woohyun but isn't it quite normal from a api POV that when you disable something twice, and enable it once, that it still will be disabled ?

The disabled state is mirrored in the sub_object code (I think I should update this anyways)

Would it be okay, to only preserve this behaviour in elm_object_disabled_set but let the efl_ui_widgt_disabled property be build up like it is now ?

bu5hm4n updated this revision to Diff 19711.Feb 26 2019, 11:57 PM

preserve old behaviour in elm_object_disabled_set / get

@woohyun This commit now preserves this behaviour for elm_object_disabled_set / get

zmike accepted this revision.Feb 27 2019, 4:19 AM

Adds a unit test, that's an A+ from me.

zmike accepted this revision.Feb 27 2019, 9:44 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 27 2019, 11:20 AM
Closed by commit rEFL0b152734ba13: efl_ui_widget: redo disable handling (authored by zmike, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.