Page MenuHomePhabricator

elementary: fix legacy widget type name for backward compat
ClosedPublic

Authored by id213sin on Feb 1 2018, 3:14 AM.

Details

Summary

For example, the widget type of elm_button was "Elm_Button".
But, the object which is created by elm_button_add() will
return its widget type "Efl.Ui.Button_Legacy".
It is not legacy name. It should be fixed to return "Elm_Button".

I don't know when but eolian start to make class name with ".".
So, it should be converted to "_" for all widgets.

@fix

Test Plan

All test cases are included in this patch.
Run "make check"

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.
id213sin requested review of this revision.Feb 1 2018, 3:14 AM
id213sin created this revision.
jpeg requested changes to this revision.Feb 1 2018, 6:51 PM

Hmm. I don't get it. What's going on there? MY_CLASS_NAME_LEGACY is correctly defined as "elm_button" (and not "Elm_Button" anyway).
How did you get the wrong type name???
This change should either be done on ALL legacy widgets or there is some logic error somewhere which needs to be fixed. One way or another, this change is dubious.

This revision now requires changes to proceed.Feb 1 2018, 6:51 PM
id213sin added a comment.EditedFeb 1 2018, 7:52 PM

@jpeg

Hmm. I don't get it. What's going on there? MY_CLASS_NAME_LEGACY is correctly defined as "elm_button" (and not "Elm_Button" anyway).

The "elm_button" is supa dupa legacy. It is older than "Elm_Button".
The "Elm_Button" was appeaed and it was returned from "elm_object_widget_type_get()". The "elm_button" became leagacy and it was returned from evas_object_type_get().

Now, "Efl.Ui.Button" is appeared. So, "Elm_Button" and "elm_button" have to be legacy.
If the object is added from "elm_button_add()", it has to show legacy name just like before.
Efl.Ui.Button_Legacy is totally new name. Not Legacy.

How did you get the wrong type name???

Try elm_object_widget_type_get() function. It will return object's given class name.

This change should either be done on ALL legacy widgets or there is some logic error somewhere which needs to be fixed. One way or another, this change is dubious.

Yes, I prepared patch for asking the way to resolve the issue.
If reviewers accept this solution, I can update it as big one patch or create multiple patches for each widget.

jpeg added a subscriber: taxi2se.Feb 1 2018, 10:55 PM

@jpeg

Hmm. I don't get it. What's going on there? MY_CLASS_NAME_LEGACY is correctly defined as "elm_button" (and not "Elm_Button" anyway).

The "elm_button" is supa dupa legacy. It is older than "Elm_Button".
The "Elm_Button" was appeaed and it was returned from "elm_object_widget_type_get()". The "elm_button" became leagacy and it was returned from evas_object_type_get().

Now, "Efl.Ui.Button" is appeared. So, "Elm_Button" and "elm_button" have to be legacy.
If the object is added from "elm_button_add()", it has to show legacy name just like before.
Efl.Ui.Button_Legacy is totally new name. Not Legacy.

How did you get the wrong type name???

Try elm_object_widget_type_get() function. It will return object's given class name.

I didn't know that function. F*ck that.

This change should either be done on ALL legacy widgets or there is some logic error somewhere which needs to be fixed. One way or another, this change is dubious.

Yes, I prepared patch for asking the way to resolve the issue.
If reviewers accept this solution, I can update it as big one patch or create multiple patches for each widget.

Then this patch is correct. And thanks to @taxi2se's efforts in the "Legacy" classes we can actually do this. Can you handle the other classes as well? Then I will accept your patch.
Thanks!

id213sin updated this revision to Diff 13808.Feb 5 2018, 10:49 PM

Update to use legacy_type_table instead of changing all .eo file name and its class name.

It also includes test cases for legacy type name.

id213sin retitled this revision from elementary: fix legacy widget type of elm_button to elementary: fix legacy widget type name for backward compat.Feb 5 2018, 10:52 PM
id213sin edited the summary of this revision. (Show Details)
id213sin edited the test plan for this revision. (Show Details)
taxi2se added a comment.EditedFeb 5 2018, 11:29 PM

Until efl-1.18, eolian replaces namespace separator '.' to '_'.
So the classname would be like

static const Eo_Class_Description _elm_button_class_desc = {
     EO_VERSION,                                            
     "Elm_Button",                                          
     EO_CLASS_TYPE_REGULAR,                                 
     EO_CLASS_DESCRIPTION_OPS(_elm_button_op_desc),         
     NULL,                                                  
     sizeof(Elm_Button_Data),                               
     _elm_button_class_constructor,                         
     NULL                                                   
};

But when eolian_gen upgrades to eolian_gen2 (efl-1.19), this logic seems to be deleted, thus

static const Efl_Class_Description _elm_button_class_desc = {
   EO_VERSION,
   "Elm.Button",
   EFL_CLASS_TYPE_REGULAR,
   sizeof(Elm_Button_Data),
   _elm_button_class_initializer,
   _elm_button_class_constructor,
   NULL
};

So we have 2 legacy class names: "Elm_Button" and "Elm.Button"

@taxi2se
Thank you for comment.
I think we can ignore "Elm.XXX" legacy type name if it was merged to EFL recently.
Then, this patch will be acceptable.

jpeg requested changes to this revision.Feb 6 2018, 1:00 AM

I'm surprise it even works for you...
Other than the string compare, good job! This is a HUGE patch, so... good job :)

src/lib/elementary/efl_ui_widget.c
79

Please indicate the EFL version for which we are doing the compatibility (1.18).

3229

You can't use == to compare strings...

3232

Simpler:

if (eina_streq(ret, legacy_type_table[i][0])
  return legacy_type_table[i][1];
This revision now requires changes to proceed.Feb 6 2018, 1:00 AM
id213sin updated this revision to Diff 13811.Feb 6 2018, 2:51 AM

Update to compare using eina_streq()

raster added a comment.Feb 7 2018, 2:58 AM

almost done... just "which efl version is this for" is missing (see above review requests) :)

jpeg added a comment.Feb 7 2018, 6:44 PM
In D5782#99070, @raster wrote:

almost done... just "which efl version is this for" is missing (see above review requests) :)

In fact, see T5938, we may want to add this new startup API for "which version of EFL was this app written for?" (not compiled with, but QA'ed with). We could then switch the name based on that rather than elm_button_add vs. efl_add (note: the name has changed multiple times so there is no actual fixed value here... this compatibility patch therefore is imperfect)

@raster, @jpeg
Thank you. I forgot to add comment for EFL version. :)
I'll update it soon.

id213sin updated this revision to Diff 13831.Feb 7 2018, 10:31 PM

Add a version information about the legacy type table.

Anyway, I added a comment for EFL version information based on my poor English writing.

cedric accepted this revision.Feb 14 2018, 12:04 PM

Discovering this patch and I am amazed. Pushing it as it seems to have reached full review by everyone.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 14 2018, 12:48 PM
Closed by commit rEFL855c1886b68d: elementary: fix legacy widget type name for backward compat (authored by id213sin, committed by Cedric Bail <cedric@osg.samsung.com>). · Explain Why
This revision was automatically updated to reflect the committed changes.