Page MenuHomePhabricator

Efl Canvas Text : Update style parsing code
ClosedPublic

Authored by AbdullehGhujeh on Thu, Nov 21, 6:06 AM.

Details

Summary

Based on comments in D10607

Update code responsible for parsing "style=" :

-Update string parsing code
-Make the old and new code more readable
-Add tests

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.
AbdullehGhujeh created this revision.Thu, Nov 21, 6:06 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.Thu, Nov 21, 6:06 AM
AbdullehGhujeh edited the summary of this revision. (Show Details)Thu, Nov 21, 6:17 AM
AbdullehGhujeh edited the summary of this revision. (Show Details)
AbdullehGhujeh retitled this revision from fix implicit casting int to a unsigned int added EINA_SAFETY_CHECK to Efl Canvas Text : Update style parsing code.Thu, Nov 21, 6:23 AM
bu5hm4n requested changes to this revision.Thu, Nov 21, 9:29 AM

This is an improvement, thank you :)

After the things in the inline comments, i am happy with this .

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

This is just a nitpick anyways. Can you write here that this function splits str by the comma, and fills the part before the comma into the buffer part1, and the rest into part2 ?

That makes it easier to understand.

1489

coding convention, its "void <linebreak> _style_string_split"

1512

coding convention, its "void <linebreak> _format_shadow_set"

1538

coding convention, its "void <linebreak> _format_shadow_direction_set"

src/tests/evas/evas_test_textblock.c
4597

One more block like this just with no comma, something like "style=foo" (This way we are testing the branches in the parsing function that are related to this)

This revision now requires changes to proceed.Thu, Nov 21, 9:29 AM

Update function description
Fix code convention
add more tests

AbdullehGhujeh marked 4 inline comments as done.Sun, Nov 24, 1:14 AM
AbdullehGhujeh added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
1487

The string sent to this function might contain multiple commas, the first part is the string before the first comma and the second part is the string after the last comma (this is the current support and we Ignore the rest of the string).
seems that I need to update the description to be more clear :).

@segfaultxavi @bu5hm4n @woohyun

Do you have any more comments ?

bu5hm4n added inline comments.Mon, Nov 25, 2:04 AM
src/lib/evas/canvas/evas_object_textblock.c
1487

Yeah i know, thats what i wrote. "Split the str by the comma, fill the part before the comma into the buffer part1, and the rest into part2" Which describes pretty much this ?

src/tests/evas/evas_test_textblock.c
4597

This was meant differently. Only something like
efl_canvas_text_style_set(txt, "style=glow");

So we ensure that we test, that there might be no part2 (in the code above).

AbdullehGhujeh marked an inline comment as done.Mon, Nov 25, 2:17 AM
AbdullehGhujeh added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
1487

If we have for example "style=soft_outline,top,bottom" then we will take "soft_outline" as part1 , "bottom" as part2 and "top" will be ignored

src/tests/evas/evas_test_textblock.c
4597

Yeah I know, already added it the first test case :)
I have added a two test, a test for no comma style and a test for more than one comma case.

zmike added a subscriber: zmike.Mon, Nov 25, 5:59 AM

This seems reasonable.

AbdullehGhujeh marked an inline comment as done.

Rebase

bu5hm4n requested changes to this revision.Tue, Nov 26, 5:53 AM
bu5hm4n added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
1487

Okay, take away: this still needs better docs, as its not correct what is there.

src/tests/evas/evas_test_textblock.c
4597

Ooooh sorry, I misread, thank you :)

This revision now requires changes to proceed.Tue, Nov 26, 5:53 AM
AbdullehGhujeh marked an inline comment as done.Tue, Nov 26, 6:17 AM
AbdullehGhujeh added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
1487

I have updated the docs, I think it is clear now.
anymore comment ?

segfaultxavi added inline comments.Wed, Nov 27, 3:47 AM
src/lib/evas/canvas/evas_object_textblock.c
1489

This is an internal function (not public API) so these docs are not as critical.
Still, the better we document things the safer we'll be in the future.
I propose this:

Split str using commas as separators. All characters from the beginning of the string up until
the first comma (excluded) are copied into part1.
All characters after the last comma (excluded) up until the end of str are copied into part2.
Any character in between part1 and part2 is ignored. For example, if str="str1,str2,str3,str4",
part1 will contain "str1" and part2 will contain "str4".
part1 and part2 must be already allocated and contain enough space for any possible outcome
of the parsing. The safest bet is that they should be as big as str.

The comment inside the function is then redundant and can be removed.

AbdullehGhujeh marked an inline comment as done.

Update Docs

AbdullehGhujeh marked an inline comment as done.Wed, Nov 27, 4:29 AM
AbdullehGhujeh added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
1489

updated

woohyun accepted this revision.Wed, Nov 27, 11:50 PM

I think this looks good now :)

Thank you for all people here !!

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Nov 27, 11:50 PM
Closed by commit rEFLfa1aa10d7921: Efl Canvas Text : Update style parsing code (authored by abdulleh Ghujeh <a.ghujeh@samsung.com>, committed by woohyun). ยท Explain Why
This revision was automatically updated to reflect the committed changes.