Page MenuHomePhabricator

elm/check: fix emission of legacy "changed" callback
ClosedPublic

Authored by zmike on Sep 3 2019, 11:58 AM.

Details

Summary

legacy "check" and "toggle" widgets operate differently:

  • check emits only the "toggle" event
  • toggle emits "toggle", "on", "off"

legacy also must not emit events when the widget's state is changed
programmatically

to handle this effectively, check whether the event has been emitted for
each state when the signal is emitted from the theme, and track this
for subsequent uses to ensure that exactly one event is triggered
when it should be

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 13106
zmike created this revision.Sep 3 2019, 11:58 AM

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/

jsuya added a subscriber: jsuya.

I think this patch can cause legacy widget compatibility issues. Isn't it?
elementary_test -> check -> Second check button

zmike planned changes to this revision.Sep 5 2019, 6:45 AM

Hm good catch. Will add unit test for this...

zmike updated this revision to Diff 24777.Sep 5 2019, 7:57 AM
zmike edited the summary of this revision. (Show Details)

rework this completely

bu5hm4n added inline comments.
src/lib/elementary/efl_ui_check.c
63

Why not just setting a simple boolean flag and checking that instead of << | & stuff ?

zmike added inline comments.Wed, Sep 18, 7:17 AM
src/lib/elementary/efl_ui_check.c
63

I'd need 2 bool flags since it needs to check for changed events on select and unselect.

bu5hm4n added inline comments.Wed, Sep 18, 7:46 AM
src/lib/elementary/efl_ui_check.c
63

It would still be less than a char. And it would improve readability IMO as its not really quite clear on the first look what this thing here is doing.

Additionally, in line 61. shouldnt this additional value be calculated before calling the callback? I am not sure this works correctly when the callback attached to a "changed" cb changes the selected state ...

zmike planned changes to this revision.Wed, Sep 18, 8:50 AM
zmike added inline comments.
src/lib/elementary/efl_ui_check.c
63

I suppose this is true

zmike updated this revision to Diff 25151.Wed, Sep 18, 9:14 AM

do stuff different

bu5hm4n accepted this revision.Wed, Sep 18, 9:15 AM
This revision is now accepted and ready to land.Wed, Sep 18, 9:15 AM
Closed by commit rEFL7767ce884abc: elm/check: fix emission of legacy "changed" callback (authored by zmike, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyWed, Sep 18, 9:19 AM
This revision was automatically updated to reflect the committed changes.