Page MenuHomePhabricator

efl_ui_radio: cleanup API
Needs ReviewPublic

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

Details

Summary

This is a bit of a giant commit. However, the problem here is that
cleaning up the API went hand in hand with rewriting most of the usages.
In the process of renewing and removing old API the following was done:

  • Legacy API testcases have been ported back to smart callbacks
  • EO-API testcases have been ported to efl_add syntax
  • weird event #defines have been removed
  • Wrong constructor usage has been removed
  • Ported to the new box object introduced before
  • removed legacy API from efl_ui_radio -> no more ptr(int) q66 will do jumps of happiness -> no more ununderstandable group_add methods -> Seperated code in blocks only for legacy, and blocks only for

non-legacy

To verify this commit, you can check all the tests that have been
touched here. Additionally, the cxx example has been adjusted

ref T7867

Depends on D9059

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/check_work
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11832
bu5hm4n created this revision.Thu, May 30, 5:18 PM
bu5hm4n requested review of this revision.Thu, May 30, 5:18 PM
bu5hm4n updated this revision to Diff 22662.Wed, Jun 5, 5:00 AM
bu5hm4n edited the summary of this revision. (Show Details)

rebase

zmike added a comment.Wed, Jun 12, 5:53 AM

Why is there so much nstate stuff in this patch? Can that be split into another patch?

I don't know, it was honestly very hard to form this commit alone, as this involves a lot of refactoring, the NSTATE thing is only 1 mistake, and a lot of event changes, where the events still were emitted by radio before

bu5hm4n updated this revision to Diff 22707.Wed, Jun 12, 7:00 AM

Fehler wurden begangen,

example is now back and working

Copy e pasta is removed.

zmike added inline comments.Wed, Jun 12, 9:18 AM
src/bin/elementary/test_bg.c
436

I'm confused by all these changes; why did you change the constructor usage here? This seems like it should work?

bu5hm4n added inline comments.Wed, Jun 12, 5:26 PM
src/bin/elementary/test_bg.c
436

No - this should have never worked, the only reason this worked is that we used legacy API to theme the object in nstate.

In the new API the theme is applied in the finalizer in layout. this means, every call in the constructor that is required to have a workinig theme, does not work correctly. And is required to happen after the finalizer. (also applies to button, text, (and likely others)

zmike added a comment.Thu, Jun 13, 6:10 AM

I'll check some things out. Other than the constructor usage this seems okay.

src/bin/elementary/test_bg.c
436

Hm. Which calls here require theme_apply? I guess I can look into this...

zmike requested changes to this revision.Thu, Jun 13, 7:38 AM

I've fixed this behavior with D9089, so all those changes can be removed since they should work as expected once that patch lands.

This revision now requires changes to proceed.Thu, Jun 13, 7:39 AM
bu5hm4n requested review of this revision.Thu, Jun 13, 4:04 PM

I think i was not clear enough: I think the way, that we require here to have things after finalize, is good. We should stop calling properties / functions before finalizing the object. Those APIs are not constructors! Xavi already also fixed all the examples for text and so on.

Well, this is what you get when things are not clearly documented. We already agreed that properties in the constructors section can only be set during construction, however, we never specified if other properties can be set there.
In C# this is enforced (only constructor properties can be set during construction, simply because we removed initialization lists), but in C you can still do it.
There used to be a task for this (T7477), which didn't reach any conclusion.

zmike added a comment.Fri, Jun 14, 6:47 AM

One of the main points of the efl_add extended construction mechanics was to allow things like this, where you could set all the properties during construction. The only thing that perhaps was not clearly defined from here would be e.g., whether packing the widget into a box during construction is allowed, but probably we did want that functionality too so that every function can be called during construction.

cedric requested changes to this revision.Fri, Jun 14, 11:00 AM

I am actually wondering if it wouldn't make sense to make efl_ui_radio_state_value_set and efl_ui_radio_group_register actually @constructor as I don't think we want to deal with changing them once the object are created?

src/examples/elementary/efl_ui_list_example_1.c
73

Hum?

This revision now requires changes to proceed.Fri, Jun 14, 11:00 AM

Good point! Agreed!

In D9060#167031, @zmike wrote:

One of the main points of the efl_add extended construction mechanics was to allow things like this, where you could set all the properties during construction. The only thing that perhaps was not clearly defined from here would be e.g., whether packing the widget into a box during construction is allowed, but probably we did want that functionality too so that every function can be called during construction.

You want to tell me the point of efl_add was to make c different from every other language ? Think about it for a second what it means if we allow *all* calls directly after constructor, that means that our finalizer is not usefull at all anymore, the object must be in c perfectly workable right after the constructor is called. And then how are we going to deal with the fact that you can set text / content, just before you change the style of the widget, which would trigger a retheme, and the loss of the setted properties.
When we stabelized layout we made theme changes after finalizer impossible, but now we are allowing text/content setting *before* finalizer ? This feels just like we are opening a giant can of worms, for nothing. I really hesitate to claim that c API does and will work differently than in all other languages.

bu5hm4n updated this revision to Diff 22790.Fri, Jun 14, 5:42 PM
bu5hm4n marked an inline comment as done.

remove asdf

bu5hm4n added a comment.EditedSat, Jun 15, 2:56 AM

EDIT: I posted to the wrong ticket please answer to this in (https://phab.enlightenment.org/D9058#167361)