Page MenuHomePhabricator

Efl Canvas Text : Modify Style Property
ClosedPublic

Authored by AbdullehGhujeh on Nov 6 2019, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
segfaultxavi added inline comments.Nov 18 2019, 1:45 AM
src/lib/evas/canvas/efl_canvas_text.eo
77–78

I think separating the style property into two separate methods is a good idea, given that the behavior is slightly different.
I also like that you used different names (style_set vs. all_styles_get).

Write-only properties are extremely weird. I much prefer the method.

93

The format of this string needs to be fully explained. Something like It's a whitespace-separated list of $[property=value] pairs, for example, $[font=sans size=30].

Also, the list of available properties needs to be documented. This is critical.

Also, the relationship between this method and the individual APIs like font_set has to be explained, as you did in the commit message.

95

It is worth mentioning the relationship with style_set (as you did in the commit message).

AbdullehGhujeh marked 2 inline comments as done.Nov 19 2019, 1:57 AM

@bu5hm4n

can you check that all branches of the parsing function is checked and tested (also with asan enabled)?

I have tested only new code.

The tests for example do not cover "style=" which has quite a bit of string parsing code

The parsing code for "style=" is an old 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 ?

if there is a problem in the value set in style, it will be ignored (no error message).

For the inline comments
Most comments are about old code, I prefer making separate patch for old code fixes (I will specify this for each comment).

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

This is an old code, if this is really needed then I can do it in a separate patch and update all style code (new and old) to print an error.

2793

changed the function to return unsigned int

2843

added EINA_SAFETY_CHECK

2863

This is an old code, I can do it in a separate patch

3222

for me it was more convenient to maintain, if others also see benefit from changing it to be hard-coded in every call I don't mind to change it.

AbdullehGhujeh marked 2 inline comments as done.
  • fix implicit casting int to a unsigned int
ali.alzyod added inline comments.Nov 19 2019, 2:11 AM
src/lib/evas/canvas/efl_canvas_text.eo
93

This documentation should be handled in other task, since it will be huge (additinal to this change) and was not available from beginning.

So we will handle this in other task/patch

@bu5hm4n

can you check that all branches of the parsing function is checked and tested (also with asan enabled)?

I have tested only new code.

Wait, one step back: There are 2 sorts of errors in ASAN; leaks, and memory read / write errors. leaks are not good but they are not critical to the runtime of the project (as long as they are not giant). But there is no way to accept memory read / write errors. If something like that happens, the normal process in EFL is that someone reverts the commit. (Which has happened to the last 2 revisions). Please ensure by 100% that there are NO memory read / write errors, they are *not* acceptable.

The tests for example do not cover "style=" which has quite a bit of string parsing code

The parsing code for "style=" is an old code.

That does not really matter, there should be tests covering this, independent from new or old.

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 ?

if there is a problem in the value set in style, it will be ignored (no error message).

We have right now the possibility of changing that (as this is a new API), and maybe it makes sense to *print* an error when there is something wrong with the added text. (@woohyun I think we should speak about that).

For the inline comments
Most comments are about old code, I prefer making separate patch for old code fixes (I will specify this for each comment).

No problem for me, as long as they are resolved.

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

As i wrote, we probably want to speak about if we want to tell the user, if there is a error.

  • fix implicit casting int to a unsigned int
  • added EINA_SAFETY_CHECK
  • updated docs
ali.alzyod added inline comments.Nov 19 2019, 9:05 AM
src/lib/evas/canvas/evas_object_textblock.c
2919

@bu5hm4n writing style this way used in other part of text.
this request enhancement can be done later since code at current state does not break anything and does not violate any rule (so this is just style of writting).

Okay, i am starting to be annoyed by replies like this. I am called here to do review, i do review, and all that is coming back is "we will do it later" "its used somewhere else like this" "we should not document concepts like this".
There is no golden rule about what we declare as easy to read code, however, this block is the perfect example of what it is NOT, it takes ONE single macro to get this cleaned up, where you have something like MY_MACRO("off", EVAS_TEXT_STYLE_PLAIN, EFL_TEXT_STYLE_EFFECT_TYPE_NONE) one call for each line up there. Which is WAY more readable than what is there right now. We also do EXACTLY this macro stuff ALL over the place, so the argument of "used in other part of text" is completely irrelevant and does not mean anything.
Other people have to READ that code again, have to MAINTAIN that code in some way, and someone on a bug-hunt will likely come across it, why should we make the life of these people harder ?
Have you ever heard of the "Broken windows theory" ? Whenever there is something broken or ugly, someone else, who does not want to make it any better will exactly refer to this code "eh, it was done like this before", could you please try to get off that attitude a bit ? This code here will last a long time, and why would you want to start with a hard-to-read version if you can invest 5 min. and make it nice ? (Same applies to other comments and revisions regarding tests, or string parsing, or or or )

@bu5hm4n first of all Thank you for your help

Okay, i am starting to be annoyed by replies like this. I am called here to do review, i do review, and all that is coming back is "we will do it later" "its used somewhere else like this" "we should not document concepts like this".

All our comments are we will do it later !!, this is not true, check the comments
These changes and patches talk about specific features, we will fix any new change we cause, but fixing old stuff should be done in separate patches, not these ones

could you please try to get off that attitude a bit ?

I do not see where this comes from, we should not discuss any suggestion ?! (arguing in not attitude, I am sorry if you feel it this way)
It is normal to discuss requests, then we can find the best solution :)

if there are any bugs or violations in our code we will do it immediately, but if there is an argument about a specific coding style or choosing another way of doing stuff it is ok to discuss it.

There is no need to discuss something like "please make use of a macro here" or "this should be tested", nor are these *suggesstions*, they changes that i am requesting.
I am also not saying that discussions are not allowed, but coming to a discussion with a argument "its also like this somewhere else" is not going to be usefull at any rate.
Replying to a inline comment where we would like to have a change with "we will do it later" is also not really a discussion, why would you start later on again on old code file fixing something that was said ahead of time, even if it could have been fixed before this piece of code would have even landed ? That makes no sense to me. Fix it now, no need to discuss things, no need to revisit things, and more testing, review can happen.

And last but not least: I would really appreciate if comments like "get off that attitude a bit" are not taken out of context. I said that in relation to the "Broken window theory" never to the wish preventing any discussion.

ali.alzyod added a comment.EditedNov 19 2019, 11:37 PM

There is no need to discuss something like "please make use of a macro here" or "this should be tested", nor are these *suggesstions*, they changes that i am requesting.

Why do you think there are no need ? because they are changes you request, this is no argument !!!

I am also not saying that discussions are not allowed, but coming to a discussion with a argument "its also like this somewhere else" is not going to be usefull at any rate.

this is an argument about used coding style in EFL, what you request is your personal preferences not a real argument

Replying to a inline comment where we would like to have a change with "we will do it later" is also not really a discussion, why would you start later on again on old code file fixing something that was said ahead of time, even if it could have been fixed before this piece of code would have even landed ? That makes no sense to me. Fix it now, no need to discuss things, no need to revisit things, and more testing, review can happen.

If there are some part of code have issues we did not cause in this patch, we will not fix it now, if you like you can fix or anyone can fix it, but at the same time if we broke something here then you are right we need to fix it because it is our responsibility

And last but not least: I would really appreciate if comments like "get off that attitude a bit" are not taken out of context. I said that in relation to the "Broken window theory" never to the wish preventing any discussion.

your story are out of the context, please give feed back related to code, if you like to tell stories I am ok with it, I can read them any time but not in patches reviews

And for this line in particular :

That makes no sense to me. Fix it now, no need to discuss things, no need to revisit things, and more testing, review can happen.

This is not good way for discussing with others,

bu5hm4n requested changes to this revision.Nov 20 2019, 2:03 AM

Okay, please stop twisting my words. You are claiming that i have said things that i never said. I also do not see any value "continuing" this discussion:

Again: Add tests for the "style=" parsing, and use a macro in the block that i have commented on. Otherwise i will not greenlight this. (To ensure a basic quality on this code)

This revision now requires changes to proceed.Nov 20 2019, 2:03 AM
AbdullehGhujeh updated this revision to Diff 27024.EditedNov 20 2019, 10:30 PM
AbdullehGhujeh marked an inline comment as done.
  • update docs
segfaultxavi added inline comments.Nov 21 2019, 2:05 AM
src/lib/evas/canvas/efl_canvas_text.eo
93

OK, please create a task for this and give it showstopper priority. We cannot release this API without these docs.

ali.alzyod added inline comments.Nov 21 2019, 2:19 AM
src/lib/evas/canvas/efl_canvas_text.eo
93
bu5hm4n added inline comments.Nov 21 2019, 3:19 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

Sorry, i forgot to answer here, please move them into the printf line. Otherwise you cannot be easily sure which parameters to pass etc. And you need to jump accross the source file in order to read and understand the code.

AbdullehGhujeh added inline comments.Nov 21 2019, 4:08 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

np :)
I defined it once and used it multiple places (same as using macro), also every string format indicate the type needed to be used.
What If I moved it in every call and someone needs to update a format? this will be hard for him.

segfaultxavi added inline comments.Nov 21 2019, 4:20 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

I agree with both views and I propose a third one :)
Why don't we create a series of macros like

#define APPEND_INT(name, val) eina_strbuf_append_printf(format_buffer, "%s=%d ", name, val);

I think this is clear enough to use:

APPEND_INT(font_sizestr, _FMT_INFO(size));

And all the formatting strings are grouped in the same place.

bu5hm4n added inline comments.Nov 21 2019, 4:21 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

If you update the fomat of %s=%s then you probebly also want to change the name, which also opens the question of "if one case needs a new format, do the others need that new format as well" ?
A macro however is also not really suitable here, this is just a simple string, the reason i reqested a macro in the other place is that this makes the whole block way more readable.

bu5hm4n added inline comments.Nov 21 2019, 4:31 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

Okay, a macro like xavi proposed does make sense indeed.

AbdullehGhujeh added inline comments.Nov 21 2019, 4:53 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

So we have three proposes that are right :) ,
as we agree on the third one I will change it to become a macro.

bu5hm4n added inline comments.Nov 21 2019, 5:10 AM
src/lib/evas/canvas/evas_object_textblock.c
3222

Good with me :)

move strbuf to macros
AbdullehGhujeh marked 4 inline comments as done.Nov 21 2019, 6:27 AM
AbdullehGhujeh added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
2863

Created D10715 for this

2919

Created D10715 for this

3222

Updated

segfaultxavi resigned from this revision.Nov 21 2019, 6:49 AM

I have no further concerns. I'll let you review the code.

AbdullehGhujeh added a comment.EditedNov 24 2019, 1:16 AM

@woohyun
@bu5hm4n
We have closed all the requests.

@segfaultxavi @bu5hm4n @woohyun

Do you have any more comments ?

zmike requested changes to this revision.EditedNov 25 2019, 6:06 AM
zmike added a subscriber: zmike.

style_set should be changed to style_apply. set and get should be reserved for properties.

all_styles_get should become style_string_create or similar since this is not actually a "get" function and it serializes a string (additionally it is not a property, and it thus should not use get).

This revision now requires changes to proceed.Nov 25 2019, 6:06 AM
ali.alzyod added a comment.EditedNov 25 2019, 6:15 AM

@zmike
style_string_create ?, this could make the user think this method will return a string he needs to free

how about make all_styles property readonly

Rename style_set to style_apply
Make all_styles_get read only property

@zmike
any other comments?

zmike added a comment.Nov 25 2019, 8:47 AM

I still don't really like using get here since this is a function which performs string creation operations and get typically implies an immediate retrieval.

zmike added a comment.Nov 25 2019, 8:59 AM

Also this patch does not apply to current master.

zmike requested changes to this revision.Nov 25 2019, 9:00 AM
This revision now requires changes to proceed.Nov 25 2019, 9:00 AM

I still don't really like using get here since this is a function that performs string creation operations and get typically implies an immediate retrieval.

This string is managed internally (user should not free), and In future, we will do style string creating if needed only, but it will be enhancement,

Also this patch does not apply to current master.

Fixed

I still don't really like using get here since this is a function which performs string creation operations and get typically implies an immediate retrieval.

Hmm. I don't think that many developers will consider that the style string is created when calling this API.
For me, it just seems like "giving an internal style string from the text object".

@segfaultxavi
Do you have better idea for "all_styles_get" ?

bu5hm4n resigned from this revision.Nov 26 2019, 6:07 AM

My comments have been addressed in D10715

This string is managed internally (user should not free)

If this is the case then we need to indicate it somehow. Maybe make the method return a const(string) instead of string, otherwise it's misleading.
I agree _create suggests that the user has to free the string.
Maybe a readonly all_styles const string property is the cleanest solution. I would then explain in the docs its relationship with the style_apply method.

This string is managed internally (user should not free)

If this is the case then we need to indicate it somehow. Maybe make the method return a const(string) instead of string, otherwise it's misleading.
I agree _create suggests that the user has to free the string.
Maybe a readonly all_styles const string property is the cleanest solution. I would then explain in the docs its relationship with the style_apply method.

I also like this idea.

@ali.alzyod @AbdullehGhujeh
Could you add @const for @segfaultxavi 's approach ?

@segfaultxavi @woohyun
what is the usage of const(string) ?
I used "string" type because it is evaluated to "const char *" (see old property in canvas text called bidi_delimiters also use "string" type for "const char*" , for "char *" usually "mstring" used, maybe I'm wrong ? )

Sorry, I was confused. Some time ago const() was valid in Eolian but not anymore. Now, string is a constant string (which cannot be modified or freed) and mstring is a mutable string (which can be modified). Additionally, if the caller should free the string, then we need to add @move.

So, we agree on a readonly all_styles: string property?

I think the patch almost includes everything for feedback now.
If anyone does not give feedback in a day~ I'll close this :)

Thanks for good reviews here !

woohyun accepted this revision.Nov 27 2019, 1:23 AM

Once this lands I'll update the documentation. It will probably be faster than adding even more comments to this already-full review :)

@segfaultxavi you mean update D10729 ?

No, I meant the docs in this same diff. I'll review D10729 too.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 27 2019, 8:23 PM
Closed by commit rEFLf6caca1d70b7: Efl Canvas Text : Modify Style Property (authored by abdulleh Ghujeh <a.ghujeh@samsung.com>, committed by woohyun). · Explain Why
This revision was automatically updated to reflect the committed changes.

@segfaultxavi

I just closed this :) It's your turn (thank you very much )