Page MenuHomePhabricator

efl_ui/layout: apply theme during constructor if widget class has been set
AbandonedPublic

Authored by zmike on Jun 13 2019, 7:38 AM.

Details

Reviewers
bu5hm4n
Summary

this fixes doing operations inside the constructor which require the use
of the theme object while avoiding a second call to theme_apply during
finalize if it previously succeeded during construction

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11770
zmike created this revision.Jun 13 2019, 7:38 AM
zmike requested review of this revision.Jun 13 2019, 7:38 AM

I really dont know about this one. Should we really support this ? Basically every single call in the efl_add thing, that is not explicitly marked as constructor, is basically illegal.

Additionally, setting the class before constructor that this code path is hit does not really work nicely. Think about the inheritance like this: There is A B C, C inherits from B, B inherits from A. Now all of them are setting the class before super-constructor calls. What will happen ? A will have the klass set as last thing, before things are applied, this is probebly not what we want, (we want to have the setting after the super-constructor call, then the last element in the inherit chain gets the value set.

TLDR: i think it is fine that things are not allowed before finalize is called. only constructors should be called there.

bu5hm4n requested changes to this revision.Jun 13 2019, 4:02 PM
This revision now requires changes to proceed.Jun 13 2019, 4:02 PM
zmike requested review of this revision.Jun 14 2019, 6:44 AM

This doesn't change any existing mechanics, it just calls theme apply at the end of construction if the widget's class has already been set. As you can see, if the theme apply fails then it maintains the current behavior; I think this fallback will only be triggered for actual error cases, however.

For widgets which inherit from layout (all widgets inherit from layout), this means that since they've probably set their class already during their own constructor they will now immediately apply the layout theme. This fixes things like calling efl_text_set on widgets during extended construction, something that should definitely be allowed.

bu5hm4n requested changes to this revision.Jun 19 2019, 3:36 AM

It changes an existing mechanism, it starts to theme the object before all neccessary information are there. The object needs to be themed after the klass / style / element is set. Right now this patch only applies the theme after klass has been set. However, the style can be changed until finalize is done. So this breaks that behavior (i think). Further more, the style is meant to be set by the user. So no way to set this before finalizer is done. Additionally, we do not want to theme a object twice, as that is quite an heavy lift..

This revision now requires changes to proceed.Jun 19 2019, 3:36 AM
zmike added a comment.Jun 19 2019, 6:33 AM

Maybe we need to approach it a bit differently then. Things like packing an object into a box or setting hints during construction should always work (and they do without this patch), but also I think it's important that e.g., efl_text_set should always work on an object. So it may be the case that I need to improve edje internal caching somehow to allow "pre-setting" text and swallow parts or something...

In D9089#167740, @zmike wrote:

Maybe we need to approach it a bit differently then. Things like packing an object into a box or setting hints during construction should always work (and they do without this patch), but also I think it's important that e.g., efl_text_set should always work on an object. So it may be the case that I need to improve edje internal caching somehow to allow "pre-setting" text and swallow parts or something...

Improving edje to do so, might be doable to some extent. We already have the infrastructure to reload on theme file set. We could just have a case when the theme has not been set before and add things like it was a reload case. The main issue with this strategy is that we won't be able to answer question if a group exist and things like that. I don't know how much of this function that probe the loaded theme are used. If it is an important case to handle, we might have to thing of a different strategy. One I can think of is to add a new loading mechanism that will only do the loading when a function that need the file make a call and use the reload infrastructure in the rest of the case. This could help, just not sure what the typical scenario are.

zmike abandoned this revision.Fri, Jul 12, 6:51 AM