Page MenuHomePhabricator

Efl Canvas Text : Modify Style Property
Needs RevisionPublic

Authored by AbdullehGhujeh on Wed, Nov 6, 4:36 AM.

Details

Summary

This patch defines the way style property will work at canvas_text object

1- Changing canvas_text style property using Font/Format/Style interfaces or with efl_canvas_text style property are the same.

Example:
efl_text_font_set(tb, "Arial", 30);
//is same as
efl_canvas_text_style_set(tb, "font=Arial font_size=30");

//which means calling 
char * font;
int size;
int font_size;
efl_text_font_get(tb, &font, &size);
// calling this after any of the top two functions will return same result

2- style_get_property

Will return string that contains full details about all the current applied style at canvas_text  level.

3- style_set_property

Will only override passed styles and leave everything else as it is
efl_canvas_text_style_set(tb, "font=Arial");  // overrider font name to Arial and leave everthing else
efl_canvas_text_style_set(tb, "font_size=30");  // overrider font size to 30 and leave everthing else (font name will stay arial)

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D10607
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14542
Build 10001: arc lint + arc unit
AbdullehGhujeh created this revision.Wed, Nov 6, 4:36 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/

AbdullehGhujeh requested review of this revision.Wed, Nov 6, 4:36 AM
ali.alzyod edited the summary of this revision. (Show Details)Wed, Nov 6, 5:09 AM
ali.alzyod updated this revision to Diff 26730.Thu, Nov 7, 12:28 AM
  • fix compilation
ali.alzyod edited the summary of this revision. (Show Details)Thu, Nov 7, 12:51 AM
ali.alzyod updated this revision to Diff 26731.Thu, Nov 7, 1:11 AM
  • fix test
ali.alzyod updated this revision to Diff 26732.Thu, Nov 7, 1:38 AM
  • style_get update return value
a.srour added a subscriber: a.srour.Thu, Nov 7, 3:04 AM

I think this feature change looks good :)

src/lib/evas/canvas/efl_canvas_text.eo
65–66

The description of this property should be changed completely.
For "setter", we need to introduce what would be happened when users set some parts for whole properties. (That is, only the style properties in the string would be changed, and the other properties would be just the same.)
For "getter", we need to explain whole properties would be given as a string.

@woohyun
The documentation needs update, but we think also split this property into two methods because currently working with it as a property may create some confuse
for example:
current_style_get != previous_style_set

ali.alzyod updated this revision to Diff 26797.Mon, Nov 11, 1:01 AM
  • fix compilation
  • split style property to set/get functions
AbdullehGhujeh marked an inline comment as done.Mon, Nov 11, 3:37 AM
AbdullehGhujeh added inline comments.
src/lib/evas/canvas/efl_canvas_text.eo
65–66

updated docs & splitted the property

Looks good for me. Just check my inline comment for the small modification in documentation.

src/lib/evas/canvas/efl_canvas_text.eo
75

Let's use "G" (capital letter) for the first character of the documentation.

AbdullehGhujeh marked 2 inline comments as done.
  • split style property to set/get functions + Fix docs
  • split style property to set/get functions + fix docs
woohyun added inline comments.Thu, Nov 14, 6:16 PM
src/lib/evas/canvas/efl_canvas_text.eo
73

just "all_styles_get" would be better. Nope ?

  • update name
ali.alzyod added inline comments.Thu, Nov 14, 10:30 PM
src/lib/evas/canvas/efl_canvas_text.eo
73

@woohyun Fixed

@ali.alzyod @AbdullehGhujeh

I'm wondering why this patch includes modifications which are not related to this text_style.
(such as diffs in src/lib/evas/canvas/efl_canvas_animation_scale.eo)

ali.alzyod updated this revision to Diff 26930.EditedFri, Nov 15, 2:40 AM
  • cleanup, remove unnecessary changes

@woohyun I mixed patches with each other :), sorry, now it is fixed

@ali.alzyod
Thanks for cleaning up.

@segfaultxavi
Could you give any advice on method names and their documentations?

@bu5hm4n
Could you give any advice on implementations for this feature ?

bu5hm4n requested changes to this revision.Sun, Nov 17, 2:33 AM

Hi, additionally to my comments, can you check that all branches of the parsing function is checked and tested (also with asan enabled)? The tests for example do not cover "style=" which has quite a bit of string parsing code. I am also wondering if there is some guideline on what is happening when attributes of the style are not within the normal behavior, is there normally a error ?

src/lib/evas/canvas/efl_canvas_text.eo
65–66

Maybe we need to ask here the C# people if this acceptable, or if a set-only property would be better.

73

Same as above, maybe this should be a @property with only a getter. But as above, lets ask the C# people.

src/lib/evas/canvas/evas_object_textblock.c
2979

You probebly want to error out here when the size is < 0 ?

2996

This implicitly casts a int to a unsigned int, which might cause ugly issues ?

I know that the method actually does not return something < 0, however, it is not declared like that ... so it might happen at some point.

Same applies to the next calling points of these functions.

3046

This here assumes that param != NULL, maybe this should be a EINA_SAFETY_CHECK at the top of this function.

3066

I am not entirely sure about this whole blog here tbh. The string parsing seems to be written to be fast, however. the strstr call already walks parts of the string, which is then walked again, which is kind of wasted time. Additionally: you could just have two char pointers, p1 = param, and p2 to the pointer which is the first character after a potetial "," or NULL. After that you can just use strncmp and compare for the exact size of the params you would expect there (without the ending \0). Then this is 0 copy, and just potentially walking a few times for the checking. However, the method is less branchy, and IMO easier to read.

If I am wrong with my assumation that it should be fast. Then you can just use eina_str_split or eina_str_split_full, which would make this whole thing way easier.

3122

I am wondering if this could be refactored into some macro, which makes the lines shorter, and the whole thing easier to read.

Additionally, further down the mapping is the other way arround, a macro that specifies the mapping from A to be B could maybe also be reused to map from B to A, so the code below could be a lot smaller.

3426

I am not a big fan of having them up here as const char. This makes it just harder to read.

This revision now requires changes to proceed.Sun, Nov 17, 2:33 AM