Page MenuHomePhabricator

edje_textblock: remove duplicated textblock style properties
ClosedPublic

Authored by bowonryu on Jun 10 2020, 2:56 AM.

Details

Summary

When there is "font" and "font_size" in textblock style (already defined),
If "font" and "font_size" are set using edje_text_class_set(),
then "font" and "font_size" are defined duplicated in texblock style through eina_strbuf_append.

Duplicate properties use memory unnecessarily,
and also it is possible to cause confusion at debbuging.

This patch replaces duplicate properties "font", "font_size" using eina_strbuf_replace.

Test Plan
  • textblock style in edc

"font=Sans font_size=20 wrap=mixed text_class=TEXT_CLASS";

  • edje_text_class_set in c

edje_text_class_set("TEXT_CLASS", "font=DejavuSans", 40);

  • textblock style at runtime (BEFORE)

DEFAULT='font=Sans font_size=20 wrap=mixed font_size=40.0 font=DejavuSans'

  • textblock style at runtime (AFTER)

DEFAULT='font=DejavuSans font_size=40 wrap=mixed'

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.
bowonryu created this revision.Jun 10 2020, 2:56 AM
bowonryu requested review of this revision.Jun 10 2020, 2:56 AM

What if the font style contains more than one font or font_size with same value ?
I know it seems strange but it is valid (could be done with pushed user styles or user made more than one tag).

So I think we need to insure we replaced last tag for font and font_size

If there is more than one font or font_size in the textblock style, the last tag is replaced.
Because if there are multiple tags, then tag->font, tag->font_size in code is the last tag.
As you say, textblock style may have multiple font or font_size, however only the last tag is valid.

ali.alzyod added a comment.EditedJun 10 2020, 6:29 AM

If there is more than one font or font_size in the textblock style, the last tag is replaced.
Because if there are multiple tags, then tag->font, tag->font_size in code is the last tag.
As you say, textblock style may have multiple font or font_size, however only the last tag is valid.

maybe I was not clear, I hope this example clears my point (it is very simple and does not make a lot of sense, but it's mimic some style push functionality).

Let's suppose the style in edc file is: "font=Sans font_size=10 font_size=10 color=#00FF00 warp=word text_class=test_class"; as you can see font_size is defined twice
when user override text_class to set new font_size to 50. this patch will change the first occurance of font_size, the style will be:
"font=Sans font_size=50 font_size=10 color=#00FF00 warp=word"; no effect
but if the patch change last occurance it will be
"font=Sans font_size=10 font_size=50 color=#00FF00 warp=word";
take effect

bowonryu added a comment.EditedJun 10 2020, 9:05 PM

Finally I understood.
In general, if properties are duplicated, the last property in the textblock style is valid.
However, if the values ​​of duplicate properties are same, the properties located in front are valid. (The property in front is replaced by eina_strbuf_replace)
I will find a way to solve this problem.
Thanks.

I think changing this line to replace last occurrence instead of first will resolve the issue

eina_strbuf_replace(style, old_font_size, new_font_size, 1);

bowonryu updated this revision to Diff 30622.Jun 11 2020, 11:31 PM

ensure to replace the last tag

Can you take a look at D11990 ? I think it is good Idea to have this new function.

src/lib/edje/edje_textblock_styles.c
239

This can cause Issues where created string with double precision.

324

how about change the string in front of last "font_size="

334

how about change the string in front of last "font="

Good news : )
If use D11990, it will be simpler logic. I wiil try it. Thanks.

Example 1)

static void
_edje_textblock_font_size_tag_update(Eina_Strbuf *style, double old_value, double new_value)
{
   const char *font_size_key = "font_size=";
   char new_font_size[32] = {0,};
   char old_font_size[32] = {0,};
   //TODO: int VS double
   snprintf(old_font_size, sizeof(old_font_size), "%s%d", font_size_key, (int) old_value);
   snprintf(new_font_size, sizeof(new_font_size), "%s%d", font_size_key, (int) new_value);

   eina_strbuf_replace_last(style, old_font_size, new_font_size);
}

I have considered patch fixes. (example 1)
But there is a problem, the problem is that this patch cannot respond to the following exceptions.

textblock style in edc
"font=Sans font_size=20 wrap=mixed text_class=TEXT_CLASS font_size=205";

edje_text_class_set
edje_text_class_set("TEXT_CLASS", "font=DejavuSans", 40);

textblock style at runtime
"font=DejavuSans font_size=20 wrap=mixed text_class=TEXT_CLASS font_size=405";

Example 2)

static void
_edje_textblock_font_size_tag_update(Eina_Strbuf *style, double old_value, double new_value)
{
   const char *font_size_key = "font_size=";
   char new_font_size[32] = {0,};
   char old_font_size[32] = {0,};
   //TODO: int VS double
   snprintf(old_font_size, sizeof(old_font_size), "%s%d ", font_size_key, (int) old_value);
   snprintf(new_font_size, sizeof(new_font_size), "%s%d ", font_size_key, (int) new_value);

   eina_strbuf_replace_last(style, old_font_size, new_font_size);
}

So I need to pay more attention to text parsing.
As in Example 2, using the separator as space can solve most situations, ("%s%d ")

If the property is at the end and there is no space division behind it, it will not work as intended.

ex) "font=Sans wrap=mixed text_class=TEXT_CLASS font_size=20";

As a result, if font or font_size contains duplicate strings, it should be possible to distinguish.

I considered most exceptions. (maybe strange exceptions.) But this code is messy. But I couldn't find a better way.
what do you think?

ali.alzyod added a comment.EditedJun 22 2020, 11:43 PM

@bowonryu I think safest way is to search for the last
"font=" and "font_size="
Then replace the string in front of it (until you reach whitespace or null), with new value.
This way you do not care what was the value, it will be replaced anyway

bowonryu updated this revision to Diff 30804.Jun 30 2020, 4:22 AM

it is not very different from previous version of this patch.
but, I cut the string generation in half.

bowonryu updated this revision to Diff 30872.Jul 16 2020, 2:55 AM

this can handle all kinds of whitespace

ali.alzyod added inline comments.Jul 17 2020, 12:36 PM
src/lib/edje/edje_textblock_styles.c
228

I think this should be something like

eina_strbuf_append(style, key);
eina_strbuf_append(style, "=");
eina_strbuf_append(style, new_tag);
bowonryu added inline comments.Jul 19 2020, 10:45 PM
src/lib/edje/edje_textblock_styles.c
228

new_tag is already "key" + "=" + "new_tag" via _edje_textblock_font_tag_update.

Does your comment mean that the pre-action via _edje_textblock_font_tag_update should be removed?

The reason to create a tag in advance is to make sure to replace the string through eina_strbuf_replace_last.

ali.alzyod added inline comments.Jul 20 2020, 4:44 AM
src/lib/edje/edje_textblock_styles.c
228

Sorry I did not notice it

bowonryu updated this revision to Diff 30884.Jul 20 2020, 11:16 PM

fix build warning

bowonryu updated this revision to Diff 30885.Jul 21 2020, 12:49 AM

fix for ninja test

ali.alzyod accepted this revision.Jul 21 2020, 8:30 AM

Seems fine to me. thank you

This revision is now accepted and ready to land.Jul 21 2020, 8:30 AM
This revision was automatically updated to reflect the committed changes.