Page MenuHomePhabricator

efl: simplify logic for widget created by factory.
ClosedPublic

Authored by cedric on Sep 15 2019, 11:38 PM.

Details

Summary

In an attempt to make things more complex than they should have been,
I tried to change the inheritance tree on the fly and assume widget would
rely on autodeleting its children. This is way more complex of a solution
than to let the View actually release all the child manually and just set
the window as the default parent.h

Co-authored-by: Marcel Hollerbach <mail@marcel-hollerbach.de>

Depends on D9952

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.
cedric created this revision.Sep 15 2019, 11:38 PM
bu5hm4n requested changes to this revision.Sep 16 2019, 12:53 AM
bu5hm4n added inline comments.
src/lib/efl/interfaces/efl_interfaces_main.c
129–130

can you prefix this with EOLIAN ?

133–134

I think this should be an EINA_SAFETY ... passing a NULL factory looks like a mistake that should be verbose.

src/lib/elementary/efl_ui_caching_factory.c
414

why not just use provider find ?

419

This is a lot of events on the window object, which will make in general event emission very slow. Can we have some stragety here that we only have one cb per window, and some array that can be freed in it ?

src/lib/elementary/efl_ui_widget_factory.c
58

Why having this outside .eo ?

173

types <3

This revision now requires changes to proceed.Sep 16 2019, 12:53 AM
zmike requested changes to this revision.Sep 16 2019, 6:33 AM

Why does this have to parent everything to the win object? This doesn't seem like what we should be doing at all...

src/lib/elementary/efl_ui_caching_factory.c
247

This isn't technically correct though, is it? The window doesn't seem like it should be the parent here...

419

I don't know that we want to be parenting all the factory widgets to the window anyway unless the window is actually the parent.

src/lib/elementary/efl_ui_widget_factory.c
35

I'm not sold on this concept.

cedric added inline comments.Sep 16 2019, 8:04 AM
src/lib/elementary/efl_ui_caching_factory.c
419

It shouldn't. You have one callback per factory and one factory per collection view. I don't think we need at this point to complexity code for that.

As for the window becoming the parent, we have to as this is the only reliable parent we can use for the widgets the factory is creating. Otherwise you end up with random order of destruction on implicit shutdown and getting it right is pretty difficult. It might be possible to rely on factory direct parent being a widget, will have to try.

src/lib/elementary/efl_ui_widget_factory.c
58

It's a helper and shouldn't really be a public API.

cedric planned changes to this revision.Sep 17 2019, 4:55 PM
cedric added inline comments.
src/lib/elementary/efl_ui_caching_factory.c
247

It is a less bad option I could think of. I need some kind of widget for parenting. Maybe a generic Efl_Ui_Widget in the parent chain might also work.

SanghyeonLee added inline comments.Sep 17 2019, 5:37 PM
src/lib/elementary/efl_ui_caching_factory.c
247

mostly create will be called after factory_set on view,
so if factory can get event callback on factory_set,
arn't we just give actual parent widget instead of window?

cedric updated this revision to Diff 25196.Sep 19 2019, 1:15 PM
cedric edited the summary of this revision. (Show Details)

rebase and take comment into account.

zmike requested changes to this revision.Sep 19 2019, 1:46 PM
zmike added inline comments.
src/lib/elementary/efl_ui_caching_factory.c
243

Wrong type and variable name.

This revision now requires changes to proceed.Sep 19 2019, 1:46 PM
bu5hm4n resigned from this revision.Fri, Sep 20, 12:46 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_ui_caching_factory.c
419

My concerns are meat, thank you.

src/lib/elementary/efl_ui_widget_factory.c
58

Okay.

cedric updated this revision to Diff 25286.Fri, Sep 20, 3:23 PM

phab!!!

This needs some rebase, i cannot apply this patch anymore.

cedric updated this revision to Diff 25413.Mon, Sep 23, 4:11 PM

rebase and take comemnts into account

bu5hm4n accepted this revision.Mon, Sep 23, 11:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 24, 12:19 AM
Closed by commit rEFL892c26f906d2: efl: simplify logic for widget created by factory. (authored by cedric, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.