Page MenuHomePhabricator

edje: Expose loading APIs to be referenced by edje_cc
Needs ReviewPublic

Authored by conr2d on May 2 2018, 4:28 AM.

Details

Diff Detail

Repository
rEFL core/efl
Branch
import-edj
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6111
conr2d created this revision.May 2 2018, 4:28 AM
conr2d requested review of this revision.May 2 2018, 4:28 AM
cedric added a project: efl.
zmike added a comment.May 2 2018, 2:43 PM

I'll post my overall thoughts for the patchset on this revision.

I don't understand the reasoning for adding this? I guess the idea is that you have a base theme (like "default") and you want to inherit e.g., a scroller theme into your app theme file and then modify it?

It would be good to get more info on this to determine what the use case is.

conr2d added a comment.May 2 2018, 9:59 PM

@zmike

This feature has been demanded on Tizen side since the year before last, however I think this also can be helpful on upstream.
You're right. The main usage of this feature is reusing default theme but changing it a little. For example, if an app developer wants to use default "button", but the only concern is that background image doesn't fit his design. Currently, he should copy & paste raw edc source in his edc. Not all properties are changeable by classes (color/text/size), modifying edc cannot be avoidable.
Moreover, if that developer isn't used to writing edc, changing very small part or reading concise source (inherited groups only contain changed parts) will help managing his own edc.
By applying this feature, developers can inherit edje group from compiled one (Why edj not edc? Because edj already contains resources like images, so it can be obtained from edj directly. Or, developers will need to access image resource related to the group they wants to inherit every time)

I don't work for Tizen anymore, so I don't know their plan exactly. But the last time I heard about this, they wanted to provide the feature so as to allow developers to reuse Tizen default theme in their apps.

Edje files are already able to inherit images at runtime now as of rEFL3d9dcbd478e0e9e8a120b08344e08154cbc25d39. It seems to me like the 'import' proposal would result in memory duplication of images which edje is supposed to avoid in the first place?

I think it would be a better development policy to use this runtime inheritance feature along with rEFL07d0fb03db9dee3c8196cc13119ff11a81b7ead6 theme matching for overlays/extensions. Providing this import feature may be useful for quick hacks, but I think it seriously hurts the maintainability of code which uses it since developers/designers will then have to find the referenced group in the referenced edj source files to figure out why something is not working.

I think a better way to reproduce this feature would be to just use #include <file> to include the desired groups. The new group can then inherit from this included group, reusing the same image identifiers without including the image to produce the same effect with more maintainable code and less memory usage.

No, this 'import' doesn't work like #include syntax. By 'include', new EDJ will contain whole data from included one, but by 'import', only newly defined - inherited from imported EDJ but modified - group belong to generated EDJ. (EDJ will be compact and No duplication)
If user customized his theme based on specific version, of course, there can be someone who want his theme to be updated automatically based on the theme of new version (shared image id you mentioned can work here), but others might want to keep their original theme based on the theme of previous version.

I may be misunderstanding, but it seems like you are contradicting yourself.

By 'include', new EDJ will contain whole data from included one, but by 'import', only newly defined - inherited from imported EDJ but modified - group belong to generated EDJ. (EDJ will be compact and No duplication)

Based on how this patchset functions, these methods should be identical when used correctly. Consider these two cases:

  1. EDC file a.edc contains group group/1 which has the attribute inherit_only set.
    • group/1 is used with #include and inherit to create EDJ files app1.edj and app2.edj
    • result: a.edc exists, is easily readable, and can be directly referenced; group/1 does not exist in any EDJ file
  1. EDJ file app1.edj contains group app/group/1
      • EDJ file app2.edj uses your patchset to import app/group/1 from app1.edj
    • result: app2.edj contains app/group/1 code modified and reused in e.g., app2/group/1

The difference here is that with import, developers of app2.edj do not have the EDC for app/group/1 and must manually use edje_decc any time they want to check the code. This creates more work for app2.edj developers when the end result is the same. It may also be the case that app1.edj developers have used import to create their app/group/1 group, which means that they also must use edje_decc to check the code before they modify it. So with each use of import, you are creating more work for no benefit.

Perhaps I do not fully understand the reason for this patchset, but it seems to me that this can only result in worse development/maintenance practices and creates more work for everyone who uses it. If I am not mistaken in this, then I am opposed to including this feature upstream since it will only propagate bad development methodologies.

@zmike

Yes, you are right. If we only look into the usage you describe, 'import' and 'inherit_only' work similarly.
However, if multiple inheritance occurs, it becomes harder to manage all related resources like images etc. User should be able to access every image listed in all included EDC. I thought that this way is not portable.
The main benefit of this patch set is that it allows user to inherit some groups from single EDJ file and inherit from any group not marked as 'inherit_only'.
(By your way, parent EDJ should be written to have only inherit_only groups from the first not to include redundant groups in new EDJ)

What do you mean by 'multiple inheritance' ?

When #include is used, any group can be included, not only ones which set inherit_only. I was only specifying this as an example. More likely what it would look like for one of your use cases would be that the app which created a group would distribute the edc file somewhere into /usr, and then other apps can easily include this edc file. The apps which include it would then require the original app edj file in order to ensure access to the referenced images.

When using import, not only do you make the edc more challenging to read, you also have image data duplication for every app which imports a group.

I understand that the feature provided by these patches can be done inherit_only too, but in that case, parent EDJ should have only inherit_only groups. Otherwise, redundant groups would be included to new EDJ.
If we want provide an easier way to customize theme by modifying "default" theme, EDJ only with inherit_only groups should be written first. And both "default" and user theme could inherit from it.
However, managing image resources wouldn't be easy ... image files should be provided separately and users who want to use inherit_only EDJ also have to keep image resources in accessible path.

I had a plan to expose only require parts to generated EDC like eolian_gen, so it can be modified by users. For example, if elm.background and elm.text are only require parts in button theme, and user try inheriting group, generated EDC will show next raw code.

import "some_file.edj"

collections {
  group { "some_group";
    inherit: "parent_group";
    parts {
      image { "elm.background"; } // in real code, declared descriptions also provided in this stage.
      text { "elm.text"; }
    }
 }

In mobile or wearable side, the size of application is one of concerns. By import feature, user can make a small size EDJ for his custom button.

Moreover, import provide convenient way to replace image.
If parent group uses 'xxx.png', user would need to import group and put new 'xxx.png' in his image_dir. edje_cc will search in image_dir first (so as to replace image with user's) and bring parent group's image only when it fails to find image.

If you don't like this feature at all, I would talk to Tizen members about providing theme-devel package which contains ONLY inherit_only groups.

zmike added a comment.May 21 2018, 7:22 AM

The issue isn't that I dislike the idea of import--I think it is a great idea for introducing people to edje theming and for making "quick hacks". The two big issues that are raised for me are:

  1. Maintainability of code
  2. Increased memory usage due to image memory duplication

Combined, these seem like they will lead to measurably worse development experience and runtime performance the more that import is used. Given how relatively easy it is to use, it seems like it would quickly propagate to many projects and introduce these same issues.

I agree that distributing "public" edc files, which have group/part stubs to expand on, would be good. We do not currently do this with upstream, but I think this would be hugely beneficial to all app developers since a lot of apps copy edc from the default theme. I think the same is true in Tizen? I think that, going forward, it would be good to begin investigating some improvements that could be done here; @cedric do you have any thoughts?

zmike added a comment.Jun 15 2018, 2:53 PM

I've rejected the remainder of this series for now to get it out of the patch queue while we continue our discussion here. If you have any ideas for some edc "headers" that we could start shipping upstream then I'm interested, but I'm still opposed to adding the import feature for the reasons described above.

zmike added a project: Restricted Project.Jun 18 2018, 7:30 AM