Page MenuHomePhabricator

evas_object_textblock: add support for variation sequences
Needs ReviewPublic

Authored by ali.alzyod on May 29 2019, 11:42 PM.

Details

Summary

update font processing to handle variation sequences unicodes to select proper glypg in respect to variation seqences

Test Plan
#define EFL_EO_API_SUPPORT 1
#define EFL_BETA_API_SUPPORT 1
#include <Eina.h>
#include <Efl.h>
#include <Elementary.h>

EAPI_MAIN int
elm_main(int argc, char **argv)
{
   Evas_Object *win, *textblock;

   elm_policy_set(ELM_POLICY_QUIT, ELM_POLICY_QUIT_LAST_WINDOW_CLOSED);

   win = elm_win_util_standard_add("Main", "");
   elm_win_autodel_set(win, EINA_TRUE);
   textblock = evas_object_textblock_add(win);
   efl_canvas_text_style_set(textblock,NULL,"DEFAULT='font=DejaVuSans font_fallbacks=SamsungColorEmoji color=#000 font_size=20'");
   evas_object_textblock_text_markup_set(textblock, "8&#xfe0f;&#x20E3;&#x262a;&#xfe0f;AAA&#x262a;&#xfe0E;1234567&#xfe0f;&#x20E3;");



   evas_object_size_hint_weight_set(textblock, EVAS_HINT_EXPAND, EVAS_HINT_EXPAND);
   evas_object_size_hint_align_set(textblock, EVAS_HINT_FILL, EVAS_HINT_FILL);
   evas_object_show(textblock);
   evas_object_move(textblock, 0, 0);
   evas_object_resize(textblock, 320, 320);
   evas_object_resize(win, 320, 320);
   evas_object_show(win);
   elm_run();
   return 0;

}

ELM_MAIN()

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D9053
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14867
Build 10233: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • fix compilation error if harfbuzz was not enabled
ali.alzyod updated this revision to Diff 22628.EditedMay 31 2019, 6:21 AM

update test suite with variation sequences
@cedric

This seems good for me. If there would be no more comments in 2 days ~ I'll accept this :)

ali.alzyod updated this revision to Diff 22641.Jun 2 2019, 5:31 AM
  • Add function comment decleration,Fix caching Issue, Fix searching fonts issue
ali.alzyod updated this revision to Diff 22708.Jun 12 2019, 6:59 AM

evas_textblock: add support for caching variation sequence

fix binary insertion

@herdsman are you there?

I don't think @herdsman can check this comment in a short time. He is not working here these days :)

@ali.alzyod
Small query here,
why do you need separate caching logic for variation sequence? Can't it be accommodated in the current logic?

@ali.alzyod
Small query here,
why do you need separate caching logic for variation sequence? Can't it be accommodated in the current logic?

-because variation sequences can vary from ( 0xFE00-0xFE0F , 0xE0100-0xE01EF, 0x180B-0x180D), So I think create hashing tables for these values for each character (or characters that could have variation sequences) will consume more memory, and logic will become more complicated where one Unicode value could have many variation sequences.

-So I thought of binary search as good solution where we will keep old logic as it is, and if character has variation sequence it will be added to variation sequence sorted list, and accessing will be fast specially list will be small for each unicode value.

@ali.alzyod
Small query here,
why do you need separate caching logic for variation sequence? Can't it be accommodated in the current logic?

-because variation sequences can vary from ( 0xFE00-0xFE0F , 0xE0100-0xE01EF, 0x180B-0x180D), So I think create hashing tables for these values for each character (or characters that could have variation sequences) will consume more memory, and logic will become more complicated where one Unicode value could have many variation sequences.

-So I thought of binary search as good solution where we will keep old logic as it is, and if character has variation sequence it will be added to variation sequence sorted list, and accessing will be fast specially list will be small for each unicode value.

Making myself clear,
Memory will not be issue here, only issue I can see is that single Unicode can have multiple variation sequence. So if same unicode with different variation sequence belonging to different font file will fetch you the incorrect font in some cases, if current logic is used.
Am I correct?
If so, then it looks good to me.

@ali.alzyod
Small query here,
why do you need separate caching logic for variation sequence? Can't it be accommodated in the current logic?

-because variation sequences can vary from ( 0xFE00-0xFE0F , 0xE0100-0xE01EF, 0x180B-0x180D), So I think create hashing tables for these values for each character (or characters that could have variation sequences) will consume more memory, and logic will become more complicated where one Unicode value could have many variation sequences.

-So I thought of binary search as good solution where we will keep old logic as it is, and if character has variation sequence it will be added to variation sequence sorted list, and accessing will be fast specially list will be small for each unicode value.

Making myself clear,
Memory will not be issue here, only issue I can see is that single Unicode can have multiple variation sequence. So if same unicode with different variation sequence belonging to different font file will fetch you the incorrect font in some cases, if current logic is used.
Am I correct?
If so, then it looks good to me.

"if same unicode with different variation sequence belonging to different font file will fetch you the incorrect font in some cases, if current logic is used.
Am I correct?
If so, then it looks good to me."

Yes you are

"only issue I can see is that single Unicode can have multiple variation sequence" this is the reason why "Memory will be an issue"
We will need to allocate more memory if we want to create hash-tables (we do not need to allocate all of them at once for each glyph), and it will be more than what we need to allocate for searching small subset of variations

Sorry, I wasn't precise enough. I mean evas unit test in src/tests/evas/evas_test_textblock.c, so that it get integrated inside ninja test properly.

Sorry, I wasn't precise enough. I mean evas unit test in src/tests/evas/evas_test_textblock.c, so that it get integrated inside ninja test properly.

For variation sequences test I could not think of unit testing for this feature, you can detect this feature visually only.

On the one side you *could* check it also visually in a test. On the other side, a test that just uses the feature, checks that the getter returns the same as the setter, checks that there are no errors, checks that there are no crashes, would also be a good start.

On the one side you *could* check it also visually in a test. On the other side, a test that just uses the feature, checks that the getter returns the same as the setter, checks that there are no errors, checks that there are no crashes, would also be a good start.

there are no getter and setter for this feature, it uses same as markup get/set.
It is about how to render text

there are no getter and setter for this feature, it uses same as markup get/set.
It is about how to render text

I am talking about the getter and setter of markup. If the rendering would fail or some parser stuff permits this feature, then there should likely either be a error, or a different returned string. So this can be tested in this way.

there are no getter and setter for this feature, it uses same as markup get/set.
It is about how to render text

I am talking about the getter and setter of markup. If the rendering would fail or some parser stuff permits this feature, then there should likely either be a error, or a different returned string. So this can be tested in this way.

First : I think getter and setter of markup must have error code I am 100% with this option, and it should be used before rendering (check parsed string).

Second: returning error for failed rendering is not convenient (My opinion ), for example when you do not have glyph and render '?' this is not real error, it is that your system does not know how to render it.

For unit test visual option automated way (just an Idea)
render Textblock on Image, save it. with each new build XOR new image with saved one to check if there are change

I am not talking about a return code.
I am talking about a *printed* error, you can detect those errors, examples can be found in the elementary (efl_ui_suite) testsuite.
(For the visual option, talk to @cedric, there are a few things that are getting difficult here)

@bu5hm4n I will check (efl_ui_suite) testsuite.
Thank you

ali.alzyod updated this revision to Diff 22991.Jun 26 2019, 8:15 AM

change way of finding substitute glyphs for Text Emojis

Exactly what @bu5hm4n said. As for the visual test, the easy way with text is just to ask for the object size once the markup is set. If all the characters are properly processed, you will always have the same size. There are already test like that in evas_test_textblock. It isn't perfect, but it ensure that we do not break to much the parsing and sizing of this characters that might not otherwise be seen by anyone during a release cycle.

ali.alzyod updated this revision to Diff 23046.Jun 28 2019, 8:42 AM
  • skip caching saved values
ali.alzyod updated this revision to Diff 23494.Jul 18 2019, 1:51 AM

Add test cases, update substitute color or text presentation

cedric accepted this revision.Jul 29 2019, 11:02 AM
This revision is now accepted and ready to land.Jul 29 2019, 11:02 AM
This revision was automatically updated to reflect the committed changes.
bu5hm4n reopened this revision.Jul 30 2019, 10:12 AM

I reverted this for now, as it fails on travis.

This revision is now accepted and ready to land.Jul 30 2019, 10:12 AM
ali.alzyod updated this revision to Diff 23774.Jul 30 2019, 2:24 PM

add font file for test

@bu5hm4n @zmike
Font files for test have been added

@zmike can you please close this one

bu5hm4n requested changes to this revision.Sep 5 2019, 2:17 AM

Still does not pass tests.

This revision now requires changes to proceed.Sep 5 2019, 2:17 AM

Still does not pass tests.

I uploaded 6MB file (NotoColorEmoji.ttf), but when I apply the patch its size is 0, do you know what may cause this issue ?

ali.alzyod updated this revision to Diff 24833.Sep 9 2019, 3:15 AM
  • upload new font file

@ali.alzyod

Could you check whether this patch still passes all the tests now ?
If there is no problem, I'll close soon.

Yes it passes

woohyun accepted this revision.Oct 18 2019, 6:39 PM

If there is no feedback anymore ~ I'll close this patch soon :)

This revision was not accepted when it landed; it landed in state Needs Review.Oct 18 2019, 10:03 PM
This revision was automatically updated to reflect the committed changes.
ali.alzyod reopened this revision.Nov 3 2019, 9:48 AM
ali.alzyod updated this revision to Diff 26645.Nov 3 2019, 9:48 AM
  • protect memory access
ali.alzyod updated this revision to Diff 26646.Nov 3 2019, 9:53 AM
  • fix spelling in comment

96%: Checks: 28, Failures: 1, Errors: 0

../src/tests/evas/evas_test_textblock.c:4143:F:Object Textblock:evas_textblock_variation_sequence:0: Failure 'fw_new == fw && fh_new == fh' occur   red

I got an error while running tests.
Do I need to do something else or are we missing something ?

ali.alzyod added a comment.EditedNov 3 2019, 11:08 PM
96%: Checks: 28, Failures: 1, Errors: 0                                         
  ../src/tests/evas/evas_test_textblock.c:4143:F:Object Textblock:evas_textblock_variation_sequence:0: Failure 'fw_new == fw && fh_new == fh' occur   red

I got an error while running tests.
Do I need to do something else or are we missing something ?

can you please check the size of these files, please? I do not know for some reason it maybe copied when applying the patch as 0 byte

  • src/tests/evas/fonts/NotoColorEmoji.ttf
  • src/tests/evas/fonts/NotoEmoji-Regular.ttf
raster added a subscriber: raster.Nov 8 2019, 3:14 AM

well terminology doesn't barf this time :) evas tests fail for me (along with a bunch of other tests too all barfing on aarch64). :( but this is already the case without this patch. so this certainly improved things vs the last version.

ali.alzyod updated this revision to Diff 26933.Fri, Nov 15, 5:41 AM
  • rebase + test_Check
ali.alzyod added a comment.EditedFri, Nov 15, 5:42 AM

@woohyun can you please check if this patch test fails at your side?

ali.alzyod updated this revision to Diff 26958.Mon, Nov 18, 3:53 AM
  • update test

@raster @woohyun
This now should not have any issues in terms of memory and test cases.