Page MenuHomePhabricator

efl_ui_check: make it undependend of nstate
ClosedPublic

Authored by bu5hm4n on May 24 2019, 2:44 AM.

Details

Summary

check can only display 2 states, and is only designed to do so.
Additionally, nstate inherits from button, which
brings in autorepeat, which is hileriously broken on check and cannot
really work.

Right now there is not even support in the theme for clickable. So its a
good idea to get rid of this for now IMO.

ref T7865

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 24 2019, 2:44 AM
bu5hm4n requested review of this revision.May 24 2019, 2:44 AM
segfaultxavi requested changes to this revision.May 27 2019, 1:44 AM
segfaultxavi added inline comments.
src/lib/elementary/efl_ui_radio.eo
65

The value of the radio button in the group which should be enabled.

Also, if there is a way to indicate that no radio button is selected, it should be documented.

This revision now requires changes to proceed.May 27 2019, 1:44 AM

Also, I have serious troubles understanding this sentence :D

check can only display 2 states, and this widget will always only display three of them.

bu5hm4n edited the summary of this revision. (Show Details)May 27 2019, 1:57 AM
bu5hm4n edited the summary of this revision. (Show Details)
bu5hm4n updated this revision to Diff 22460.May 27 2019, 9:00 AM
bu5hm4n edited the summary of this revision. (Show Details)

update to xavis review, and make changed signal work again.

bu5hm4n updated this revision to Diff 22461.May 27 2019, 9:02 AM

this time for real

bu5hm4n marked an inline comment as done.May 27 2019, 9:04 AM
segfaultxavi resigned from this revision.May 28 2019, 4:07 AM

Since this is a temporary solution while radio buttons are being re-thought, there's not much point in reviewing these docs too much. They are fine with me now.

bu5hm4n updated this revision to Diff 22621.May 30 2019, 5:18 PM

rebase and rework

bu5hm4n added a reviewer: YOhoho.
bu5hm4n updated this revision to Diff 22659.Jun 5 2019, 4:59 AM
bu5hm4n edited the summary of this revision. (Show Details)

rebase

zmike requested changes to this revision.Jun 10 2019, 6:53 AM

Minor nits.

src/lib/elementary/efl_ui_check.c
64

This comment should be broken out and the explicit elm,state,check,on signal should be mentioned here since it's unclear.

The efl,state,check,on signal is not yet a defined API, so this can be removed or changed if we want.

83

Same as above.

src/lib/elementary/efl_ui_radio.eo
62

This is a very long line.

src/tests/elementary/efl_ui_test_check.c
91

What's the point of this shutdown fixture? The win and check objects will always be destroyed by the global fixture in elm_shutdown.

This revision now requires changes to proceed.Jun 10 2019, 6:53 AM
bu5hm4n updated this revision to Diff 22684.Jun 10 2019, 7:42 AM
bu5hm4n edited the summary of this revision. (Show Details)

address comment things

I am not sure if i addressed your comment regards with this ... ?

src/lib/elementary/efl_ui_radio.eo
62

It will be removed again in the next commit. This is just a temp solution ... :) i could have skipped the entire doc tbh.

src/tests/elementary/efl_ui_test_check.c
91

Mhm, we are already doing it in most testcases, i thought we also want to do it here./

I think explicitly deleting them might be a good idea, as relying on eo_shutdown or evas_shutdown for shutdown does sometimes cause weird errors (internals are freed before widgets are freed and so on)

However, i am not sure if we really should do that or not.

zmike requested changes to this revision.Jun 10 2019, 9:06 AM
zmike added inline comments.
src/tests/elementary/efl_ui_test_check.c
91

I think the only cases where it happens are ones you wrote it in for and people copied you?

Generally speaking, it should be fine to rely on the global shutdown to handle this since it tests hierarchy deletion, which is where problems would occur. Problems that we want to fix.

This revision now requires changes to proceed.Jun 10 2019, 9:06 AM
bu5hm4n updated this revision to Diff 22702.Jun 12 2019, 3:15 AM

remove teardown from tests

"I think the only cases where it happens are ones you wrote it in for and people copied you?"

Just saying this now, tests for box, grid, table have exactly the same code, i have not written them, nor does any test from me predate the existence of this piece of code ...

zmike accepted this revision.Jun 12 2019, 5:27 AM
This revision is now accepted and ready to land.Jun 12 2019, 5:27 AM
zmike added a comment.EditedJun 12 2019, 5:53 AM

argh wrong patch

Closed by commit rEFL3a5f506b0e87: efl_ui_check: make it undependend of nstate (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>, committed by zmike). · Explain WhyJun 13 2019, 6:22 AM
This revision was automatically updated to reflect the committed changes.