Page MenuHomePhabricator

THEMES USAGE IS BROKEN
Closed, ResolvedPublic

Description

It's apparent that the patchset which included rEFLdd4467505ea29d6120e5e7d467d76836a6630ff4 was not thoroughly tested as it breaks the loading of some legacy widgets. Do not reenable loading of interface widget themes until the new functionality has been tested thoroughly for 100% of widget groups and styles.

zmike created this task.Jan 5 2018, 8:45 AM
taxi2se added a subscriber: taxi2se.Jan 7 2018, 7:59 PM

Can you tell me which legacy widget is broken?

jpeg added a comment.Jan 7 2018, 10:19 PM

@zmike thanks for the report, but we need more information. I tested elementary_test with other theme files (edj files, not compiled by me). I didn't test all the test cases of course, but I definitely tried that.
Are you talking about RE-loading theme on the fly? I didn't test this at the time (though a very quick test right didn't end up in "completely broken" situation - but finger size was somehow wrong).

zmike added a comment.Jan 8 2018, 4:16 AM

From what I can tell, anything which passes style="base" to the referenced function should be broken when the interface version is loaded since interface theme groups do not contain "base". There is an attempt to work around this in elm_widget.c by checking for "base" and replacing it with NULL, but this will fail for any widget which does not pass through the interface layers (ie. any direct caller of _elm_theme_object_set). Currently, it seems that only entry and tooltip are affected.

I will admit that I didn't thoroughly check to see what the scope of this was last week considering that after a rebuild, 100% of the things that I tested had been completely broken during the time that I was away.

jpeg reassigned this task from jpeg to taxi2se.Jan 8 2018, 6:36 PM
jpeg added a subscriber: jpeg.

Okay that's a bit better... _elm_theme_object_set appears only a few times in the code (tooltip and entry as you say). And only tooltip has a call with "base":

if (!_elm_theme_object_set(tt->tt_win ? NULL : tt->owner, tt->tooltip,
                          "tooltip", "base", style))

Can you actually reproduce specific issues now? I'm testing with f003c67 which is the patch just before yours and with Radiance.edj but I can't see any particular issue (beyond a floor of ERR messages, I think due to an imperfect theme implementation).

@taxi2se should the remaining internal calls to _elm_theme_object_set be replaced by your interface call?

Hi, any progress here?

I tried to apply new group name policy only to new widgets which has a prefix "efl_ui_".
That is why I left elm_entry and elm_tooltip because I thought they are legacy_only widgets.
Tooltip should be fixed because it may have a parent which is not legacy_only, which is the case I passed over.
And I don't think elm_entry should be changed because it will remain legacy_only.
I will patch tooltip soon.
Please give me any comment that if there are more that I must consider.

D5725, D5739
D5739 is for elm_tooltip, which fixes case that new EO widgets use tooltip by elm_object_tooltip_set()
But I am not sure whether new EO widget should fully support for legacy APIs. (Tooltip does not have EO apis)
Also, I am not sure this is the case that @zmike mentioned.

For the widgets, which are added by elm_legacy_add(), themes should be found even without this patch.
@zmike can you specify or check whether theme break happens to legacy widgets?

jpeg added a comment.Jan 16 2018, 8:30 AM

@taxi2se can you ping me again so we check these patches together? thanks

jpeg added a comment.Jan 16 2018, 9:04 PM

Two small patches merged:
fe346d2ee2 elm_datetime: use legacy elm_button
1375705168 panes: Expose parts in EO file

jpeg closed this task as Resolved.Jan 16 2018, 10:28 PM

Please reopen if there are remaining issues. And please explain the test scenario (not necessarily the code path) :)