Page MenuHomePhabricator

evas_common_format_color_parse: support color names
ClosedPublic

Authored by ali.alzyod on Fri, Jun 21, 5:48 PM.

Details

Summary

efl user can now specify colors by there names ( not only Hex RGB/RGBA values), which is very convenient specially for first time users (where user may think it is supported by default)

<color=#FF0000> == <color=red> == <color==RED>

there are two main types of color names and values ( X11, web colors), here we are using X11 color.

Update for documentation pages should be added like:
https://www.enlightenment.org/_legacy_embed/evas_textblock_style_page.html#evas_textblock_style_color

I do not know how to update it :(

Test Plan
#define EFL_EO_API_SUPPORT 1
#define EFL_BETA_API_SUPPORT 1

#include <Eina.h>
#include <Elementary.h>
#include <Efl_Ui.h>

static void
_gui_quit_cb(void *data EINA_UNUSED, const Efl_Event *event EINA_UNUSED)
{
   efl_exit(0);
}

static void
_gui_setup()
{
   Eo *win, *box;

   win = efl_add(EFL_UI_WIN_CLASS, efl_main_loop_get(),
                 efl_ui_win_type_set(efl_added, EFL_UI_WIN_TYPE_BASIC),
                 efl_text_set(efl_added, "Hello World"),
                 efl_ui_win_autodel_set(efl_added, EINA_TRUE));

   // when the user clicks "close" on a window there is a request to delete
   efl_event_callback_add(win, EFL_UI_WIN_EVENT_DELETE_REQUEST, _gui_quit_cb, NULL);

   box = efl_add(EFL_UI_BOX_CLASS, win,
                efl_content_set(win, efl_added),
                efl_gfx_hint_size_min_set(efl_added, EINA_SIZE2D(360, 240)));

   efl_add(EFL_UI_TEXT_CLASS, box,
           efl_text_markup_set(efl_added, 
           "<color=red>this is red color line(color = red)<color><br>"
           "<color=#0000FF>this is blue color line (color = #0000FF)<color><br>"
           "<color=gray>this is gray color line (color = gray)<color><br>"),
           efl_gfx_hint_weight_set(efl_added, 1.0, 0.9),
           efl_gfx_hint_align_set(efl_added, 0.5, 0.5),
           efl_text_multiline_set(efl_added,EINA_TRUE),
           efl_pack(box, efl_added));

   efl_add(EFL_UI_BUTTON_CLASS, box,
           efl_text_set(efl_added, "Quit"),
           efl_gfx_hint_weight_set(efl_added, 1.0, 0.1),
           efl_pack(box, efl_added),
           efl_event_callback_add(efl_added, EFL_UI_EVENT_CLICKED,
                                  _gui_quit_cb, efl_added));
}

EAPI_MAIN void
efl_main(void *data EINA_UNUSED, const Efl_Event *ev EINA_UNUSED)
{
   _gui_setup();
}
EFL_MAIN()

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.Fri, Jun 21, 5:48 PM

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.Fri, Jun 21, 5:48 PM
ali.alzyod edited the summary of this revision. (Show Details)Fri, Jun 21, 5:53 PM
ali.alzyod edited the test plan for this revision. (Show Details)
ali.alzyod edited the summary of this revision. (Show Details)Fri, Jun 21, 5:55 PM
ali.alzyod edited the summary of this revision. (Show Details)
ali.alzyod edited the summary of this revision. (Show Details)
vtorri added a subscriber: vtorri.Fri, Jun 21, 8:22 PM
vtorri added inline comments.
src/lib/evas/common/evas_text_utils.c
15

i think that an unsigned char could be used here, i doubt that a color name will have more than 256 characters. And it will save memory.

709

same here and in this function, size_t is a bit too much

709

also, isn't a hash table interesting in that case ?

1306

size_t

1313

instead of tolower, maybe

((*source >= 'A') && (*source <= 'Z')) ? *source + 32 : ;

vtorri requested changes to this revision.Fri, Jun 21, 8:23 PM
This revision now requires changes to proceed.Fri, Jun 21, 8:23 PM
ali.alzyod added inline comments.Fri, Jun 21, 11:32 PM
src/lib/evas/common/evas_text_utils.c
15

This structure used in global array initialized at compile time (no run time allocation will happen)
But I think converting to byte can save some memory too

1313

Are there any reason not to use to lower?

ali.alzyod added inline comments.Fri, Jun 21, 11:40 PM
src/lib/evas/common/evas_text_utils.c
709

I think creating hash table for variation of strings will consume more memory, and binary search more convenient (maybe I am wrong can you please suggest how to implement)

update some data types

ali.alzyod marked 3 inline comments as done.Sat, Jun 22, 12:08 AM
vtorri added inline comments.Sat, Jun 22, 3:25 AM
src/lib/evas/common/evas_text_utils.c
709

about hash, it was just an idea i had, that's all, i have no problem with binary search

1313

not calling a function (so faster), also tolower also depends on the locale (so could possibly take accent char into account, so slower, except if "C" locale is used).

segfaultxavi requested changes to this revision.Sat, Jun 22, 3:49 AM

Hi @ali.alzyod, nice work!

Some comments:

Documentation at https://www.enlightenment.org/_legacy_embed/evas_textblock_style_page.html#evas_textblock_style_color is generated from the doxygen comments in ./src/lib/evas/canvas/evas_object_textblock.c.
This documentation has to be manually generated and the produced HTML pages uploaded to the site. Unfortunately the site server is currently unreachable so reference docs cannot be updated.
The above link, though, is for the legacy API. The Unified API has no description of the markup language anywhere that I know of, and we need to fix that.

Regarding the implementation in this patch, I see you added the code to perform a binary search, just like in D8610 for text escapes. Having the same algorithm twice is definitely not a good thing. Please consider using existing Eina methods (maybe eina_list_search_sorted_list) or adding this algorithm to Eina.

Regarding the huge list of colors, I've found at least another similar list in src/lib/elementary/elm_colorselector.c:46. Could you study the possibility of merging these two lists? Maybe having the lists in a common place accessible by Evas Text and the Elm Colorselector widget.

This revision now requires changes to proceed.Sat, Jun 22, 3:49 AM
ali.alzyod added a comment.EditedSat, Jun 22, 5:56 AM

@segfaultxavi thanks a lot for the information, The Idea of this patch is adding important feature to EFL color style using color name

Regarding the implementation in this patch, I see you added the code to perform a binary search, just like in D8610 for text escapes. Having the same algorithm twice is definitely not a good thing. Please consider using existing Eina methods (maybe eina_list_search_sorted_list) or adding this algorithm to Eina.

You are right, common source code is much better, now we have common function in evas_text_util to use in both textblock and this commit (I will update textblock escape search later)

Regarding the huge list of colors, I've found at least another similar list in src/lib/elementary/elm_colorselector.c:46. Could you study the possibility of merging these two lists? Maybe having the lists in a common place accessible by Evas Text and the Elm Colorselector widget.

You are right, we should consider using common functionality, for color I think we should have Eina_Color for both evas and elm,
And for the list, this on is much bigger than elm list, where here we have around 680 color name, and in elm we have around 150 color name
maybe afterwards we can implement color parser in Eina and make other models us it.

use common function for search array

add comment for search function

add search array function to header

ali.alzyod added inline comments.Sat, Jun 22, 11:22 AM
src/lib/evas/common/evas_text_utils.c
1313

actually tolower is faster than (((*source >= 'A') && (*source <= 'Z')) ? *source + 32 : ;)
Maybe it uses hashing.

ali.alzyod added inline comments.Sat, Jun 22, 11:46 AM
src/lib/evas/common/evas_text_utils.c
1313

actually tolower is slower than (((*source >= 'A') && (*source <= 'Z')) ? *source + 32 : ;)
but not to much (difference around 10%)

ali.alzyod marked an inline comment as done.

remove tolower

ali.alzyod marked an inline comment as done.Sat, Jun 22, 11:53 AM
  • use c standard functions

@segfaultxavi @vtorri Do you have any issues left ?

vtorri accepted this revision.Mon, Jun 24, 11:59 PM

I like that you used an already-existing function (bsearch), let's see if it is available in all platforms.

Just some minor formatting nitpicks.

src/lib/evas/common/evas_text_utils.c
1337

format nitpick: space after commas

1471

Why do you return here? There was no return in the #RGB or #RRGGBB cases above, they just set a = 0xff.

src/lib/evas/common/evas_text_utils.h
28 ↗(On Diff #22950)

Do not add useless whitespace... this file is actually not modified at all :)

193 ↗(On Diff #22950)

Do not add useless whitespace... this file is actually not modified at all :)

ali.alzyod added inline comments.Tue, Jun 25, 9:19 AM
src/lib/evas/common/evas_text_utils.c
1471

I think there was additional not important processing.
because it multiply number with 255(0xFF alpha value) then divide it to 255.

ali.alzyod updated this revision to Diff 22967.Tue, Jun 25, 9:20 AM
  • remove empty line

Sure, sure, I'm not questioning the theory... I just said I want to see what our CI thinks about it :D

src/lib/evas/common/evas_text_utils.c
1471

Yeah, I also think multiplying and dividing by 255 is useless, but then add the early return to the other cases, for consistency.

ali.alzyod updated this revision to Diff 22970.Tue, Jun 25, 9:48 AM
  • remove empty line
ali.alzyod marked an inline comment as done.Tue, Jun 25, 9:48 AM

@segfaultxavi to be honest I do not know what is the meaning of this calculation, why it multiple each color channel with alpha.

anyway I updated the code to ignore this case if a = 0xFF

segfaultxavi accepted this revision.Tue, Jun 25, 10:27 AM

@segfaultxavi to be honest I do not know what is the meaning of this calculation, why it multiple each color channel with alpha.

It's probably because it works with premultiplied alpha.

anyway I updated the code to ignore this case if a = 0xFF

Cool! I have no further comments :)

This revision is now accepted and ready to land.Tue, Jun 25, 10:27 AM
This revision was automatically updated to reflect the committed changes.