Page MenuHomePhabricator

efl theme: remove the elm legacy name of efl ui theme
ClosedPublic

Authored by herb on Apr 13 2018, 1:30 AM.

Details

Summary

remove the elm legacy name of efl ui theme

Test Plan

run elementary_test and test efl ui widget cases

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.
herb requested review of this revision.Apr 13 2018, 1:30 AM
herb created this revision.
herb updated this revision to Diff 14145.Apr 13 2018, 1:43 AM

update the code

herb updated this revision to Diff 14166.Apr 16 2018, 4:34 AM

modified more parts

herb updated this revision to Diff 14170.Apr 16 2018, 4:50 AM

update the code

Hum, are we really giving up on any name space used inside our theme ? I am still feeling like at least efl. should be used for all the part that our object are using, but I have no strong argument here.

@cedric
I understand your point. But we think more about C# part interface here.
For the "background" part interface, background.color_set() would be used.
In this case, if the edc part name is "efl.background", then it would be confusing to application developer who customizes their own edc file.

So we prefer the edc part name which is the same as the part name.

raster requested changes to this revision.Apr 18 2018, 12:27 AM

ummm cedric is right. this patch is bad. it's removing namespacing for "defined api" parts. parts that are "well known" elements that are expected to be provided because certain features or apis depend on them or use them... this i think is bad. it removed at least a clear namespace that says "this is a standard" as opposed to making it a mix of names and you don't know when you look at some edc what is pat of a standard api and what is not without also looking up a special document. if it was efl.xxxxx it'd be good imho.

This revision now requires changes to proceed.Apr 18 2018, 12:27 AM

@cedric
I understand your point. But we think more about C# part interface here.
For the "background" part interface, background.color_set() would be used.
In this case, if the edc part name is "efl.background", then it would be confusing to application developer who customizes their own edc file.

I see, well, couldn't have the efl_part API in edje actually do the conversion for efl.background when background part is requested ? Actually enforcing at the same that only the part that are in the efl. namespace are exposed/accessible via the efl_part API. What do you think of that solution ?

raster added a comment.EditedApr 18 2018, 7:23 PM

Actually enforcing at the same that only the part that are in the efl. namespace are exposed/accessible via the efl_part API. What do you think of that solution ?

it'd destroy the part interface for edje where you should be able to access any part at all with or without namespace if its generically on efl_part. it'd have to be overridden/implemented specifically in a different way for elm widgets to do this mapping. and that override is done int he c code... so how do you do this specially just for C#?

@cedric @raster

I have questions about your comments as follows.

  1. Part names
    • Should we add "efl." to all reserved part name in elementary in edc file? e.g. "efl.background", "efl.icon", "efl.text"
  1. Part object names
    • Now efl ui widget supports part objects "shadow" and "background". So is it right the part object names should not contain "efl."?
  1. Signal names
    • We were going to remove "elm." and "efl." from signal names but we were going to keep "efl" in source names. Because we were going to remove "elm." from part names. e.g. signal "state,content,set" source "efl"
    • Although we add "efl." to all reserved part name in elementary in edc file, should we remove "efl." from signal names as follows? e.g. signal "state,content,set" source "efl"

I appreciate if you give us comments on the above questions.

@cedric @raster

I have questions about your comments as follows.

  1. Part names
    • Should we add "efl." to all reserved part name in elementary in edc file? e.g. "efl.background", "efl.icon", "efl.text"

Yes.

  1. Part object names
    • Now efl ui widget supports part objects "shadow" and "background". So is it right the part object names should not contain "efl."?

Yes.

  1. Signal names
    • We were going to remove "elm." and "efl." from signal names but we were going to keep "efl" in source names. Because we were going to remove "elm." from part names. e.g. signal "state,content,set" source "efl"
    • Although we add "efl." to all reserved part name in elementary in edc file, should we remove "efl." from signal names as follows? e.g. signal "state,content,set" source "efl"

I would say we should keep efl in anything that is used by the code directly (Basically what we could define as the theme API). So if the signal is used by elementary, it should incude efl.

cedric answered already :)

@cedric @raster

Thank you for answering the questions. The answers for my 1st and 2nd questions are clear.

I have one last question about my 3rd question. (keeping "efl" in signal name)

<Legacy Elementary Theme Problem>

  1. Both "elm.swallow.content" and "content" parts exist, then "elm,state,content,visible" "elm" is emitted twice. (not distinguishable) -> This can be done by app's EDC customization and app has no idea why the same signal is emitted.
  1. Both signal and source are always together. So "elm" is duplicated in signal "elm,state,content,visible" source "elm".

<Solution only for Efl Ui Interface Theme>

  1. In efl ui interface EDC, I want to send signals with part name itself as follows. Both "elm.swallow.content" and "content" parts exist, then "state,efl.content,set" "efl" and "state,content,set" "efl" are emitted.
  1. Keep "efl" only for source. So "state,efl.content,set" "efl" is emitted instead of "efl,state,efl.content,set" "efl".

What do you think about the above?
Sorry for the long discussion.. but we should make it clear.

herb updated this revision to Diff 14244.Apr 20 2018, 4:20 AM

update source code

herb updated this revision to Diff 14245.Apr 20 2018, 5:06 AM

update source code

herb updated this revision to Diff 14248.Apr 20 2018, 5:40 AM

update the code

herb added a comment.Apr 20 2018, 6:03 AM

@cedric, @raster
I just finished removing the legacy part name and added "efl." prefix to all parts used in source code in edc/efl :)

<Solution only for Efl Ui Interface Theme>

  1. In efl ui interface EDC, I want to send signals with part name itself as follows. Both "elm.swallow.content" and "content" parts exist, then "state,efl.content,set" "efl" and "state,content,set" "efl" are emitted.
  2. Keep "efl" only for source. So "state,efl.content,set" "efl" is emitted instead of "efl,state,efl.content,set" "efl".

    What do you think about the above?

What about "state,content,set" "efl" ?

Jaehyun_Cho added a comment.EditedApr 22 2018, 8:54 PM

<Solution only for Efl Ui Interface Theme>

  1. In efl ui interface EDC, I want to send signals with part name itself as follows. Both "elm.swallow.content" and "content" parts exist, then "state,efl.content,set" "efl" and "state,content,set" "efl" are emitted.
  2. Keep "efl" only for source. So "state,efl.content,set" "efl" is emitted instead of "efl,state,efl.content,set" "efl".

    What do you think about the above?

What about "state,content,set" "efl" ?

@cedric
Do you mean "state,content,set" "efl" for "efl.content" and "state,content,set" "" for "content"?

If so, I am afraid that application developer may be confused about source...

I think they have no idea why sometimes source is "efl" and sometimes source is "".

Because these signals for content_set, text_set are emitted from efl ui widget inside logic... not by application.

Jaehyun_Cho added a comment.EditedApr 22 2018, 11:27 PM

@cedric @raster

In short, we have 4 options as follows.

Option 1

Part NameSignal NameSource Name
"efl.abc""state,efl.abc,set""efl"
"abc""state,abc,set""efl"

-> pros: easy to understand
-> cons: not neat signal name for efl part

Option 2

Part NameSignal NameSource Name
"efl.abc""state,abc,set""efl"
"abc""state,abc,set"""

-> pros: simple signal name
-> cons: hard to understand source name

Option 3

Part NameSignal NameSource Name
"efl.abc""state,abc,set""efl"
"abc""state,abc,set""efl"

-> pros: simple signal name
-> cons: cannot distinguish signal for "efl.abc" and "abc"

Option 4

Part NameSignal NameSource Name
"efl.abc""state,content,set""efl.abc"
"abc""state,content,set""abc"

-> pros: same logic with "mouse,clicked,1" signal
-> cons: different logic from existing signal and source naming rule

I think @cedric talked about option 2 or option 3.
Please answer here which one you vote for and your opinion. :)

I would vote for option 1 or option 3 because it is the easiest option to understand.

"elm,state,content,visible" "elm" is emitted twice

why? it should not be? what is doing that? that signal is for "official" content i.e. the elm.content (elm.* namespace) and nothing else.

Both signal and source are always together. So "elm" is duplicated in signal "elm,state,content,visible" source "elm".

i don't see a problem here. it makes it easier to grep for in code for strings etc. ... :)

I want to send signals with part name itself as follows

ummmm... why? these signals are only for official efl namespace parts, that means the signal can be consistent with , as spacers because it knows what these parts names should or must be and thus can handle them correctly. it can even use a generic function that converts . to , for a new signal name... so it's easy now to find with grep because all code uses the same function for official parts. the mix of . and , just looks like a mistake.

using source of abc is not good. edje uses part names as sources... so it might be confusing then. "efl" should always be the source. also "" is not good because that is for signals for the object as a while where there is no part name of course.

so i go for "non of the options". inseatd:

Part NameSignal NameSource Name
"efl.abc"state,efl,abc,set"efl"

pat bane of "abc" is out of scope for elementary to be dealing with as its a private part for the edje design.

Jaehyun_Cho added a comment.EditedApr 23 2018, 2:56 AM

@raster @cedric

raster, thank you for your feedback.

The reason, why I mentioned about emitting signals for all parts, is that efl ui layout emits signals for all parts (not for legacy). (_icon_signal_emit() in efl_ui_layout.c)

If you don't have any objection, then I would like to combine your opinion and cedric's opinion and also legacy logics (i.e. "elm,state,content,set" "elm") as follows.

Part NameSignal NameSource Name
"efl.abc""efl,state,abc,set""efl"
"abc"not supportnot support
herb updated this revision to Diff 14275.Apr 24 2018, 12:29 AM

apply efl_part name policy more

herb added a comment.EditedApr 24 2018, 1:01 AM

@raster, @cedric
applying 'efl. 'prefix for parts is finished I think. could you verify this patch?
and then I will make the new patch for new signal policy as you and jaehyun mentioned :)

herb updated this revision to Diff 14277.Apr 24 2018, 1:14 AM

remove the unnecessary function

@herb

I think there is something to modify on this patch.
Please check the patch in devs/jaehyun/efl_part_legacy_remove_final branch.

herb updated this revision to Diff 14328.Apr 25 2018, 11:40 PM

updated source code

herb updated this revision to Diff 14329.Apr 26 2018, 12:21 AM

updated source code

herb updated this revision to Diff 14330.Apr 26 2018, 12:55 AM

apply indentation

herb updated this revision to Diff 14331.Apr 26 2018, 1:49 AM

update interface scrollable code

Jaehyun_Cho accepted this revision.Apr 26 2018, 3:26 AM
Jaehyun_Cho retitled this revision from efl_part: remove the elm legacy name of efl ui theme to efl theme: remove the elm legacy name of efl ui theme.
Jaehyun_Cho edited the summary of this revision. (Show Details)
Jaehyun_Cho edited the test plan for this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Apr 26 2018, 3:41 AM
This revision was automatically updated to reflect the committed changes.