Page MenuHomePhabricator

efl.canvas.textblock: allow all white spaces in style string not just space
ClosedPublic

Authored by ali.alzyod on Feb 9 2020, 9:12 AM.

Details

Summary

style string can contain any kind of white spaces and it will be fine

For example

"font=sans font_size=30 color=red "

Is the same as

"font=sans\tfont_size=30\n  color=red "

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.
ali.alzyod created this revision.Feb 9 2020, 9:12 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/

ali.alzyod requested review of this revision.Feb 9 2020, 9:12 AM
ali.alzyod updated this revision to Diff 28918.Feb 9 2020, 9:54 AM

update + add test case

ali.alzyod updated this revision to Diff 28920.Feb 9 2020, 10:12 AM

reduce if statments

segfaultxavi requested changes to this revision.Feb 10 2020, 12:29 AM
segfaultxavi added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
3310

I hate that this list of whitespace chars is duplicated from the "ifs" in line 1333.
This is a maintainability accident waiting to happen.
Can you make this method call _is_char_white to avoid duplicated code?
Use inline if you're worried about performance.

This revision now requires changes to proceed.Feb 10 2020, 12:29 AM
ali.alzyod added inline comments.Feb 10 2020, 1:05 AM
src/lib/evas/canvas/evas_object_textblock.c
3310

This method can not call _is_char_white, because that method does not return white space characters

I think you misunderstood a bit what @segfaultxavi is pointing out. There should not be the "same" list of characters twice. It would be way better from a quality POV that both functions either use the same list of characters that are considered a whitespace, or simply iterate over the str and call _is_char_white on every character in the string. So we deduplicate this. And only have a single point of "we consider this whitespace".

ali.alzyod updated this revision to Diff 28926.Feb 10 2020, 5:17 AM

used single array for whitespace characters functions

ali.alzyod marked 2 inline comments as done.Feb 10 2020, 5:17 AM
segfaultxavi requested changes to this revision.Feb 10 2020, 5:56 AM
segfaultxavi added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
1329

Now that you've decided to use an array, isn't this method functionally equivalent to strchr?
Moreover, what's the difference with isspace (already used in other parts of EFL)?

This revision now requires changes to proceed.Feb 10 2020, 5:56 AM
ali.alzyod added inline comments.Feb 10 2020, 8:01 AM
src/lib/evas/canvas/evas_object_textblock.c
1329

Now that you've decided to use an array, isn't this method functionally equivalent to strchr?

Of course, it is not !!

Moreover, what's the difference with isspace (already used in other parts of EFL)?

No difference

ali.alzyod updated this revision to Diff 28927.Feb 10 2020, 8:16 AM
  • replace _is_char_white with isspace
ali.alzyod planned changes to this revision.Feb 10 2020, 8:33 AM

Nice! The patch is getting smaller and smaller.

Now, regarding, _strchr_whitespace, it is still unclear to me why do you need such a complex method. Could something like this work?

// Returns first occurrence of whitespace character or NULL
static char*
_strchr_whitespace(const char* str)
{
   while (*str && !isspace(*str)) str++;
   if (*str) return str;
   return NULL;
}

Nice! The patch is getting smaller and smaller.

Now, regarding, _strchr_whitespace, it is still unclear to me why do you need such a complex method. Could something like this work?

// Returns first occurrence of whitespace character or NULL
static char*
_strchr_whitespace(const char* str)
{
   while (*str && !isspace(*str)) str++;
   if (*str) return str;
   return NULL;
}

true %100

segfaultxavi requested changes to this revision.Feb 11 2020, 12:38 AM
segfaultxavi added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
3292

This code will not return NULL if a whitespace is not found, it will return a pointer to the \0 at the end of the string.
If this is what you want, please change the function's documentation.
Please note that my initial suggestion did return NULL.

This revision now requires changes to proceed.Feb 11 2020, 12:38 AM
ali.alzyod marked an inline comment as done.Feb 11 2020, 12:51 AM
ali.alzyod added inline comments.
src/lib/evas/canvas/evas_object_textblock.c
3292

I am not aging well!!!

segfaultxavi accepted this revision.Feb 11 2020, 3:09 AM

Good!

I have no more complains :)
Builds, passes tests, and the initial issue from T8532 is fixed.

This revision is now accepted and ready to land.Feb 11 2020, 3:09 AM
This revision was automatically updated to reflect the committed changes.