Page MenuHomePhabricator

edje_textblock: remove duplicated textblock style properties
Needs ReviewPublic

Authored by bowonryu on Wed, Jun 10, 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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17084
Build 11362: arc lint + arc unit
bowonryu created this revision.Wed, Jun 10, 2:56 AM
bowonryu requested review of this revision.Wed, Jun 10, 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.EditedWed, Jun 10, 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.EditedWed, Jun 10, 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.Thu, Jun 11, 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
238

This can cause Issues where created string with double precision.

317

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

327

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.EditedMon, Jun 22, 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.Tue, Jun 30, 4:22 AM

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