Page MenuHomePhabricator

efl_ui : refactoring efl.part in item based classes.
ClosedPublic

Authored by SanghyeonLee on Apr 8 2019, 9:32 PM.

Details

Summary

Most of item-based class will have similar efl.part such as text, icon, end.
creating this efl part per each class will be very hard to maintaining the class
and unnecessary eo generation.
so combine them in efl.parts of efl_ui_item.
sub item classes can use this efl.part by declairing ther own eo definition.

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11691
Build 8737: arc lint + arc unit
SanghyeonLee created this revision.Apr 8 2019, 9:32 PM
SanghyeonLee requested review of this revision.Apr 8 2019, 9:32 PM

remove unnecessary po changes.

fix typo and examples

cedric requested changes to this revision.Apr 12 2019, 3:00 PM
cedric added inline comments.
src/lib/elementary/efl_ui_item_part_content_placeholder.eo
1 ↗(On Diff #21282)

All of the above class have exactly the same interface. Wouldn't it make sense to have only one class instead?

src/lib/elementary/efl_ui_list_default_item.eo
16

I am not a fan of any of those name. Why not go simple and just define : "text", "icon", "end" ?

src/lib/elementary/efl_ui_list_empty_item.eo
9

Same here, "placeholder" seems pretty good to me by itself.

This revision now requires changes to proceed.Apr 12 2019, 3:00 PM

I agree with the intention of this commit, which is putting together in the same class repeated parts from different classes.
However, I think we can go even further. Looking at the patch, it looks like the parts for Efl.Ui.List_Default_Item and Efl.Ui.Grid_Default_Item are identical. Could these parts be moved to Efl.Ui.Item, for example?

Also, I agree with @cedric, text_default, content_icon and content_end look a bit inconsistent. I would rather use no prefix (text, icon and end) or the same prefix for the 3 parts.

Finally, I do not like the end name because it says nothing about the purpose of this part. Moreover, the theme can put this part anywhere in the layout, it does not need to be "at the end". However, this part is a bit generic, so there is no good name for it... How about extra?

src/lib/elementary/efl_ui_grid_default_item.c
11

I am not a fan of committing commented-out code.

SanghyeonLee added a comment.EditedApr 17 2019, 12:08 AM

I agree with the intention of this commit, which is putting together in the same class repeated parts from different classes.
However, I think we can go even further. Looking at the patch, it looks like the parts for Efl.Ui.List_Default_Item and Efl.Ui.Grid_Default_Item are identical. Could these parts be moved to Efl.Ui.Item, for example?

yes. that is my second plan. but firstly works only efl.part at this one.

Also, I agree with @cedric, text_default, content_icon and content_end look a bit inconsistent. I would rather use no prefix (text, icon and end) or the same prefix for the 3 parts.

its because I cannot use "text" :p. my first choice was text, icon, end. but i couldn't use text because we already have text {set, get} property in efl.text and duplication of name on property / efl.part is not allowed.
other option that I could think is just binding them in "part_" prefix like part_text part_icon, part_end... but it is not a single job for this widget.
I think same problem is on the background or backwall or other efl_part which currently existed and will be implemented. we cannot using same name on efl.part and eo property, method,
so I prefer to give some prefix or namespace for them.

other problem that we could think on efl.part is... inherited child part name cannot duplicated parent part name. so if we might need to re-register some text or content in the edje layout to child widget, their efl.part name can be very confusing if their parent already have similar one.

what do you think about this? @cedric @Jaehyun_Cho

I still go with icon, extra and other name of "text" like "label" but still label, or icon is too general term and could be duplicated with their parent property name in the future.

Finally, I do not like the end name because it says nothing about the purpose of this part. Moreover, the theme can put this part anywhere in the layout, it does not need to be "at the end". However, this part is a bit generic, so there is no good name for it... How about extra?

well, I using our legacy name but we have a chance to change it now, so I think... extra, or badge could be better. let's gather others opinion and pick something good :)

SanghyeonLee added inline comments.Apr 17 2019, 12:11 AM
src/lib/elementary/efl_ui_item_part_content_placeholder.eo
1 ↗(On Diff #21282)

you meant not placeholder but using icon part as for placeholder content?
I don't have strong opinion for this, so if you think that is better I'll fallow it.

but before that, pelase give some opinion about my reply on @segfaultxavi comment.
especially, we need to discuss about efl.part namespaces seriously.

ping?
any opinion about Efl.Part? or naming?

cedric added a comment.May 2 2019, 9:25 AM

I am sorry, for whatever reason I missed tracking this differential.

Also, I agree with @cedric, text_default, content_icon and content_end look a bit inconsistent. I would rather use no prefix (text, icon and end) or the same prefix for the 3 parts.

its because I cannot use "text" :p. my first choice was text, icon, end. but i couldn't use text because we already have text {set, get} property in efl.text and duplication of name on property / efl.part is not allowed.
other option that I could think is just binding them in "part_" prefix like part_text part_icon, part_end... but it is not a single job for this widget.
I think same problem is on the background or backwall or other efl_part which currently existed and will be implemented. we cannot using same name on efl.part and eo property, method,
so I prefer to give some prefix or namespace for them.

Oh, you are right. Forgot about the potential clash. Hum, I am not so much fan of using part_ as a prefix, but it might make sense. That or either a content_ prefix. @segfaultxavi do you have an opinion here?

other problem that we could think on efl.part is... inherited child part name cannot duplicated parent part name. so if we might need to re-register some text or content in the edje layout to child widget, their efl.part name can be very confusing if their parent already have similar one.

You are absolutely right. Good catch.

I still go with icon, extra and other name of "text" like "label" but still label, or icon is too general term and could be duplicated with their parent property name in the future.

I think the right strategy here is a prefix that reflect it is a content of the object we are talking about.

Finally, I do not like the end name because it says nothing about the purpose of this part. Moreover, the theme can put this part anywhere in the layout, it does not need to be "at the end". However, this part is a bit generic, so there is no good name for it... How about extra?

well, I using our legacy name but we have a chance to change it now, so I think... extra, or badge could be better. let's gather others opinion and pick something good :)

I like extra, but I would be fine with badge. Any idea what other platform are using?

src/lib/elementary/efl_ui_item_part_content_placeholder.eo
1 ↗(On Diff #21282)

Yes, having one class for all this case. Maybe named : Efl.Ui.Item_Part_Content or something like that.

I think this can be solved very easily by making C# suffix parts with Part automatically, just like events. Then we can remove the restriction that parts cannot be named like other properties (just like events).

Let me explain:

We have different kinds of symbols in EO files: Namespaces, Classes, Methods (and Properties), Events and Parts.
In C they can have the same name, symbol clashes are not a problem, and this is why originally our hierarchy had lots of duplicates.

When we started implementing bindings, some languages had issues with the symbol clashes: Python and C# cannot have a class named like a namespace (not even changing the case, because in Python class hierarchy maps to the filesystem, and the Windows filesystem is case-insensitive). In C# a property cannot have the same name as the class, etc.
We started solving these issues in different ways, and I think this was a mistake. For example, in C#, events are automatically suffixed with Evt so clashing is resolved, but not so parts.

As a result, these are the current rules that eolian is enforcing (as far as I understand):

  • Classes cannot clash with Namespaces
  • Methods (and Properties) cannot clash with anything
  • Events cannot clash with other events
  • Parts cannot clash with anything

I think these rules work nicely for all cases except parts, which we have not used much yet. So, if the C# bindings suffix parts with Part just like events are suffixed Evt we can remove the restriction on part names and we can use more sensible names like text, icon and extra (without prefix).
Of course, names will still need to be unique inside a widget's hierarchy (a part cannot have the same name as another part up in the hierarchy).

What do you think @cedric, @SanghyeonLee, @lauromoura, @q66 ?

Additional note: We are free to change any part name we want, because no part has been marked stable yet.
In all the classes currently marked as stable, only Efl.Ui.Widget has parts. They are shadow and background, and they are both marked @beta.

I think this can be solved very easily by making C# suffix parts with Part automatically, just like events. Then we can remove the restriction that parts cannot be named like other properties (just like events).

Let me explain:

We have different kinds of symbols in EO files: Namespaces, Classes, Methods (and Properties), Events and Parts.
In C they can have the same name, symbol clashes are not a problem, and this is why originally our hierarchy had lots of duplicates.

When we started implementing bindings, some languages had issues with the symbol clashes: Python and C# cannot have a class named like a namespace (not even changing the case, because in Python class hierarchy maps to the filesystem, and the Windows filesystem is case-insensitive). In C# a property cannot have the same name as the class, etc.
We started solving these issues in different ways, and I think this was a mistake. For example, in C#, events are automatically suffixed with Evt so clashing is resolved, but not so parts.

As a result, these are the current rules that eolian is enforcing (as far as I understand):

  • Classes cannot clash with Namespaces
  • Methods (and Properties) cannot clash with anything
  • Events cannot clash with other events
  • Parts cannot clash with anything

    I think these rules work nicely for all cases except parts, which we have not used much yet. So, if the C# bindings suffix parts with Part just like events are suffixed Evt we can remove the restriction on part names and we can use more sensible names like text, icon and extra (without prefix). Of course, names will still need to be unique inside a widget's hierarchy (a part cannot have the same name as another part up in the hierarchy).

    What do you think @cedric, @SanghyeonLee, @lauromoura, @q66 ?

Tested here adding the 'Part' suffix and it is a simple patch.

SanghyeonLee added a comment.EditedMay 10 2019, 12:17 AM

@cedric @segfaultxavi thanks for nice idea.
@lauromoura thanks for testing.

it looks adding Part prefix in c# is very easy solution,
so let's hear others opinion and decide what we will go.

additionally,
I really doubt about the term "part". is it proper name to describe really purpose of these property?
forget about the edje part(Efl part already been more large set including edje part),
which term would be perfect for explain this functionality?

@Jaehyun_Cho @herb @woohyun
do you have any opinion regarding this?

SanghyeonLee added a subscriber: herb.

ping?

is it okay we go Part_XXX ?

The C# bindings now automatically add the Part suffix, so we do not need to use any prefix or suffix. label, icon and extra should be good enough names for parts now.

The problem is that Eolian is still enforcing that part names do not clash with property names. Can this restriction be removed, @q66?

Also, be careful, there's still commented-out code in efl_ui_grid_default_item.c

The C# bindings now automatically add the Part suffix, so we do not need to use any prefix or suffix. label, icon and extra should be good enough names for parts now.

The problem is that Eolian is still enforcing that part names do not clash with property names. Can this restriction be removed, @q66?

Also, be careful, there's still commented-out code in efl_ui_grid_default_item.c

oh... thank you. I'll update the code after removed eolian enforcement. :)

hmmm I have one suggestion,

the conflict is detected when validate the part name of class property hash.
I think this check is still valid thing, somehow, as property could use "part_" prefix though we have to avoid these prefix.
so instead of adding prefix in c# binding only,
what about adding prefix in eolian part->base.name directly, when it store the stringshare?

the conflict is detected when validate the part name of class property hash.
I think this check is still valid thing, somehow, as property could use "part_" prefix though we have to avoid these prefix.
so instead of adding prefix in c# binding only,
what about adding prefix in eolian part->base.name directly, when it store the stringshare?

In C there is no clash, because properties and parts use completely separate APIs (class_name_property_get() and efl_part()).
In C# though, properties and parts can both be accessed through standard C# properties, and therefore there is collision (class.Property and class.Part).

For this reason, I think the collision should be fixed by the binding that cause them. Otherwise, we introduce artificial restrictions to the other languages.

Did I understand your concerns correctly?

the conflict is detected when validate the part name of class property hash.
I think this check is still valid thing, somehow, as property could use "part_" prefix though we have to avoid these prefix.
so instead of adding prefix in c# binding only,
what about adding prefix in eolian part->base.name directly, when it store the stringshare?

In C there is no clash, because properties and parts use completely separate APIs (class_name_property_get() and efl_part()).
In C# though, properties and parts can both be accessed through standard C# properties, and therefore there is collision (class.Property and class.Part).

For this reason, I think the collision should be fixed by the binding that cause them. Otherwise, we introduce artificial restrictions to the other languages.

Did I understand your concerns correctly?

my concern is basically on C#, that is correct,
but if we may add prefix "Part_" while the binding C#, it may cause conflict that cannot detected our normal build test.
e.g. Efl.Canvas.Layout have property to change color of part, "part_color" set/get. this will be PartColor in c#

Efl.Ui.Button have part "color" which can change button color. this will be also PartColor in c#

this may not detectable in normal c/eolian build-environment which we usually tested,
unless we forcibly build every binding languages before the patch landing (I think automation of this test will be very useful regardless of this issue.)

if my understand is wrong, and it will not happen in c# binding, it may no problem.
it is little concern about specific case that may not happen mostly.

Currently the bindings add a Part suffix: a part named color generates a C# property named ColorPart.
This should only produce a conflict if you had a property named color_part, which would be very strange.
The same problem exists if you have a property named changed_evt for example...

I think the only way to be sure is building the bindings during tests.

Currently the bindings add a Part suffix: a part named color generates a C# property named ColorPart.
This should only produce a conflict if you had a property named color_part, which would be very strange.
The same problem exists if you have a property named changed_evt for example...

I think the only way to be sure is building the bindings during tests.

yeah I've checked suffix with @Jaehyun_Cho... still conflict case can be exist, but I agreed its abnormal name.
so then only eolian can pass the validation of efl part with property everything is okay.

I have submitted patch D9031 that allows parts to have the same name as methods.
@SanghyeonLee You can now update this patch to use the simpler part names.

q66 added a comment.May 28 2019, 8:21 AM

I landed the patch so this can proceed probably.

@segfaultxavi @q66 thank you! I'll update the patch as commented way asap.

update patch with change part names,
Text_Default -> Text
Content_Icon -> Icon
Content_End -> Extra
Content_Placeholder -> Content.

ListEmptyItemClass need to be renamed ListPlaceHolderItemClass as fallowing patch.

update missing theme part name changes.

update example code.

cedric accepted this revision.May 29 2019, 2:49 PM

Nice.

This revision is now accepted and ready to land.May 29 2019, 2:49 PM
SanghyeonLee requested review of this revision.May 29 2019, 9:42 PM

I think this patch is not ready to land yet,
as it get crashed on efl_ui_list_example_1.c.
please do not merge yet.

fix the example crash on text_set and icon content_set.

additionally, I can't find the reason but,
in my develop environment,
efl_file_simple_load cause undifined symbol error.
I wonder same problem is happen in other's pc also,
but when I tried hosang's pc it works well.
looks strange issue.
but after commented line,
example works perfect.

eagleeye accepted this revision.May 29 2019, 10:59 PM
This revision is now accepted and ready to land.May 29 2019, 10:59 PM
This revision was automatically updated to reflect the committed changes.