Page MenuHomePhabricator

efl_ui : refactoring efl.part in item based classes.
Needs RevisionPublic

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 10801
Build 8390: 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

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

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.Thu, May 2, 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

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.EditedFri, May 10, 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