Page MenuHomePhabricator

edje: implement text_class override at object level
Needs ReviewPublic

Authored by a.srour on Nov 5 2019, 1:59 AM.

Details

Summary

implement text_class override at object level, where you can change the text_class for single textblock object instead of override all objects that share same text class.
This imported from TIZEN

Note:
This also fix edje_textblock font/font_size parsing by adding condition

Test Plan
Evas_Object *win, *entry1, *entry2;

win = win_add(NULL, "entry", ELM_WIN_BASIC);
entry1 = elm_entry_add(win);
entry2 = elm_entry_add(win);

elm_object_text_set(entry1, "hello");
elm_object_text_set(entry2, "hello");

edje_object_text_class_set(elm_layout_edje_get(entry1), "entry_text", "Sans", 50); // only entry1 effected
edje_object_text_class_set(elm_layout_edje_get(entry2), "entry_text", "Serif", 20); // only entry2 effected

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D10598
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14678
Build 10108: arc lint + arc unit
a.srour created this revision.Nov 5 2019, 1:59 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

a.srour requested review of this revision.Nov 5 2019, 1:59 AM
ali.alzyod added inline comments.Nov 5 2019, 3:33 AM
src/lib/edje/edje_textblock_styles.c
299–300

If we do not need this function, I think it should be removed

446–447

If we do not need this function, I think it should be removed

ali.alzyod edited the test plan for this revision. (Show Details)Nov 5 2019, 3:35 AM
ali.alzyod added reviewers: smohanty, woohyun, bowonryu.
smohanty added inline comments.Nov 5 2019, 4:33 PM
src/lib/edje/edje_textblock_styles.c
299–300

yes can be removed .. tizen may need another patch as it calls these function during style change.

446–447

yes can be removed .. tizen may need another patch as it calls these function during style change.

cedric requested changes to this revision.Nov 6 2019, 9:56 AM
cedric added inline comments.
src/lib/edje/edje_textblock_styles.c
39

You can use eina_streq here.

100

I am not convinced this is the best data structure for this use. Did you consider using either an Eina_Array or an Eina_Hash?

102

You can use eina_streq here I would think.

106

I am surprised the compiler didn't trigger a warning for you, but obj_stl can be uninitialized here.

140

This seems like an unrelated change and it would be better with a macro for maintenance purpose. So please move it to a different patch proposal.

src/lib/edje/edje_util.c
5982

I am not a fan of this change as it make ed a necessity to be checked for every access. In the long term, it might get forgotten. Could you find a way to make it more obvious that all access to ed are to be checked. Even comment in the function would help.

This revision now requires changes to proceed.Nov 6 2019, 9:56 AM
a.srour updated this revision to Diff 26739.Nov 7 2019, 7:49 AM

Resolve changes request

a.srour marked 10 inline comments as done.Nov 7 2019, 7:50 AM
cedric requested changes to this revision.Nov 7 2019, 9:34 AM

This patch is a good idea, I think, but it is most likely breaking some existing application as before when you applied a style it would propagate everywhere the same file was loaded. If an application does rely on this it would be broken I would think. I am not expert on this part of the code, so I might be wrong, but if I am right, I think that we should most likely continue to update the edf->styles hash on every call to edje_object_text_class_set and handle the fallback case when a style is not found on the object to look on the file.

The example for tests would be amended like :

Evas_Object *win, *entry1, *entry2, *entry3;

win = win_add(NULL, "entry", ELM_WIN_BASIC);
entry1 = elm_entry_add(win);
entry2 = elm_entry_add(win);
entry3 = elm_entry_add(win);

elm_object_text_set(entry1, "hello");
elm_object_text_set(entry2, "hello");
elm_object_text_set(entry3, "hello");

ck_assert(edje_object_text_class_set(elm_layout_edje_get(entry1), "entry_text", "Sans", 50));
ck_assert(edje_object_text_class_set(elm_layout_edje_get(entry2), "entry_text", "Serif", 20));

int w1 = 0, h1 = 0, w2 = 0, h2 = 0, w3 = 0, h3 = 0;
evas_object_textblock_size_formatted_get(elm_entry_textblock_get(entry1), &w1, &h1);
evas_object_textblock_size_formatted_get(elm_entry_textblock_get(entry2), &w2, &h2);
evas_object_textblock_size_formatted_get(elm_entry_textblock_get(entry3), &w3, &h3);

ck_assert_int_ne(w1, w2);
ck_assert_int_ne(h1, h2);
ck_assert_int_eq(w2, w3);
ck_assert_int_eq(h2, h3);
src/lib/edje/edje_textblock_styles.c
40

I tend to prefer the use of Eina_Iterator which avoid callback design for a more linear code. Sorry to ask for this one more change.

75

From my understanding of the patch, I think you can use eina_hash_stringshared_new here. It will compute hashes slightly faster as it will look at pointer only and not the full string. This should work because the stl->name come from an eet files where they are mapped once and so have the same property as Stringshare.

If I misunderstood your patch, you can disregard this remark.

This revision now requires changes to proceed.Nov 7 2019, 9:34 AM

This patch is a good idea, I think, but it is most likely breaking some existing application as before when you applied a style it would propagate everywhere the same file was loaded. If an application does rely on this it would be broken I would think. I am not expert on this part of the code, so I might be wrong, but if I am right, I think that we should most likely continue to update the edf->styles hash on every call to edje_object_text_class_set and handle the fallback case when a style is not found on the object to look on the file.

From function description of edje_object_text_class_set() (Sorry can't find link due to broken links in documentation, but you will find it in code)

...
@brief Sets Edje text class.
This function sets the text class for the edje
...

So this function should change text_class at edje object level not file level.

Note that to change text_class for all edje objects created from edje file, edje_text_class_set() should be used.
From function description:

...
This function updates all Edje members at the process level which belong to this text class with the new font attributes.
...

So i think we can consider this as a bug fix not a new feature. And applications uses this function instead of edje_text_class_set() are relying on an undocumented feature.

The example for tests would be amended like :

Evas_Object *win, *entry1, *entry2, *entry3;
 
win = win_add(NULL, "entry", ELM_WIN_BASIC);
entry1 = elm_entry_add(win);
entry2 = elm_entry_add(win);
entry3 = elm_entry_add(win);
 
elm_object_text_set(entry1, "hello");
elm_object_text_set(entry2, "hello");
elm_object_text_set(entry3, "hello");
 
ck_assert(edje_object_text_class_set(elm_layout_edje_get(entry1), "entry_text", "Sans", 50));
ck_assert(edje_object_text_class_set(elm_layout_edje_get(entry2), "entry_text", "Serif", 20));

To make this test valid, we need to use edje_text_class_set() in the previous line, to be something like this:

ck_assert(edje_text_class_set("entry_text", "Serif", 20));
a.srour updated this revision to Diff 26780.Nov 10 2019, 5:27 AM

Fix naming & Resolve changes request

a.srour marked an inline comment as done.Nov 10 2019, 5:30 AM
a.srour added inline comments.
src/lib/edje/edje_textblock_styles.c
75

We need to cache Edje_Style object not only text_class name.

a.srour updated this revision to Diff 26781.Nov 10 2019, 6:15 AM

Resolve changes request

a.srour marked 2 inline comments as done.Nov 10 2019, 6:16 AM

This patch is a good idea, I think, but it is most likely breaking some existing application as before when you applied a style it would propagate everywhere the same file was loaded. If an application does rely on this it would be broken I would think. I am not expert on this part of the code, so I might be wrong, but if I am right, I think that we should most likely continue to update the edf->styles hash on every call to edje_object_text_class_set and handle the fallback case when a style is not found on the object to look on the file.

From function description of edje_object_text_class_set() (Sorry can't find link due to broken links in documentation, but you will find it in code)

...
@brief Sets Edje text class.
This function sets the text class for the edje
...

So this function should change text_class at edje object level not file level.

The doc is not obvious here I think. It could mean edje file or edje object. And since this function has been implemented it did mean edje file. Not that I agree with it, but that I think how it is (Would be nice to make this documentation more precise btw).

Note that to change text_class for all edje objects created from edje file, edje_text_class_set() should be used.
From function description:

...
This function updates all Edje members at the process level which belong to this text class with the new font attributes.
...

This function impact all edje members, not a specific files user. This is not equivalent to what edje_object_text_class_set has been doing for all its time.

So i think we can consider this as a bug fix not a new feature. And applications uses this function instead of edje_text_class_set() are relying on an undocumented feature.

I am really afraid that this will break quite a few applications as current documentation isn't great and past behavior was different. I still think we should keep past behavior working to some extent. Possible solution are :

  • Keep the implied logic for file level lookup as a fallback and always populate it on edje_object_text_class_set.
  • Add infrastructure to handle a set of function that would be edje_file_text_class_set and friends, with appropriate documentation on what is going on here.

I would be happy with both choice.

@cedric

The doc is not obvious here I think. It could mean edje file or edje object. And since this function has been implemented it did mean edje file. Not that I agree with it, but that I think how it is (Would be nice to make this documentation more precise btw).

By navigating other *_class_set functions in Edje_Legacy.h we can ensure what is meat is the Evas object

This function sets the min and max values for an object level size class.

EAPI Eina_Bool edje_object_size_class_set(Evas_Object *obj, const char * size_class, int minw, int minh, int maxw, int maxh);

This function sets the color values for an object level color class.

EAPI Eina_Bool edje_object_color_class_set(Evas_Object *obj, const char * color_class, int r, int g, int b, int a, int r2, int g2, int b2, int a2, int r3, int g3, int b3, int a3);

And the documentation for edje_object_text_class_set actully broken, parameters are different and description is different.
Looking at other developer's work we can see this function has been fixed in TIZEN, so if there are other developers use this function I think the do some workaround to fix it, and we can not leave the bugs because someone may use it as a feature :)

As I said the patch looks good and should go on, but not without addressing the past behavior in a way that application that might rely on it would be broken. I have proposed two possible path. If you have other idea to fix it, I will be fine. As long as the application can still have that feature in some way.

As I said the patch looks good and should go on, but not without addressing the past behavior in a way that application that might rely on it would be broken. I have proposed two possible path. If you have other idea to fix it, I will be fine. As long as the application can still have that feature in some way.

I think we will add new functions to change override classes at file level (get/set). I think classes are only (size, color, and text)

As I said the patch looks good and should go on, but not without addressing the past behavior in a way that application that might rely on it would be broken. I have proposed two possible path. If you have other idea to fix it, I will be fine. As long as the application can still have that feature in some way.

I think we will add new functions to change override classes at file level (get/set). I think classes are only (size, color, and text)

It should likely an step before this patch as this is removing code that such functionality would actually use.

@cedric while testing and using stuff, we found some strange behaviors, we will try to solve them first with their own patches then return to this one.

a.srour updated this revision to Diff 27088.Mon, Nov 25, 3:32 AM

Add edje file text_class functions
New API added:

  • edje_file_text_class_set(filepath, text_class, font, size)
    • Overrides text_class on file level, affect all Edje_Objects created from filepath
  • edje_file_text_class_del(filepath, text_class)
    • Remove text_class from file (at runtime only), affect all Edje_Objects created from filepath

These changes should resolve T8480 & T8481

a.srour updated this revision to Diff 27112.Tue, Nov 26, 5:41 AM

Add edje_file_text_class_get().
Update edje_file_text_class_del() to return bool.