Page MenuHomePhabricator

Bug: Evas text truncated for certain language
Closed, ResolvedPublic

Description

Please check the images attached:
This is happening after this commit rEFL20ef85e307818c7c92927688ee6fd5d9dec1ee8f

Before:

After:

Obviously advance of the item is bigger than its width.

subodh6129 updated the task description. (Show Details)
subodh6129 raised the priority of this task from to Normal.
subodh6129 added a project: efl.
subodh6129 added subscribers: subodh6129, herdsman, tasn and 2 others.
tasn added a subscriber: id213sin.Dec 31 2015, 6:19 AM

@herdsman, @id213sin: It seems that it broke in D3004. Comments?

herdsman claimed this task.Jan 12 2016, 7:29 AM

I can look at it.
But really, @subodh6129, please provide a simple test program including the input text.
Thanks.

herdsman lowered the priority of this task from Normal to Pending on user input.Jan 12 2016, 11:36 PM
subodh6129 raised the priority of this task from Pending on user input to Normal.Jan 13 2016, 2:38 AM

@herdsman
Please use the attached sample edc and check with edje_player.

Thank you for claiming this task

Thanks for the prompt reply :)

tasn added a comment.Jan 13 2016, 5:27 AM

@herdsman, as I said in the other patch, this is the problem with this approach. ;(

This is my result.
I'm not sure if this is good or bad, though.
The glyphs look different, too.
What font is being used for you? Also, do you have harfbuzz enabled?

tasn added a comment.Jan 13 2016, 6:33 AM

You need their font, and I'd assume he has harfbuzz enabled because:

  1. You can see the shaping, wouldn't have just happened.
  2. Who you are talking to...

:)

Was replying to @subodh6129 (of course!).
But my point was that I don't see it being truncated (at least I think I don't), so I'm going to need to know what font to test with.

Btw this is mine: DroidSansDevanagari-Regular.ttf

tasn added a comment.EditedJan 13 2016, 6:46 AM

I was referring to him too (that's why I said "he")... I think he's using the Tizen font.

herdsman lowered the priority of this task from Normal to Pending on user input.Jan 14 2016, 3:13 AM

Not ignoring the fact that this happens after D3004, but I just want to confirm why this doesn't happen to me.

Assuming that Evas Text now reports its width correctly as the actual width (compared to advance like in the past), then the output should not look truncated. Unless, there's a problem with the width values in the font you use.

I basically need to check with your font, or you try another font and check the results.

tasn added a comment.Jan 14 2016, 3:23 AM

Because you are using a different font... You just need his font.

subodh6129 raised the priority of this task from Pending on user input to Normal.Jan 14 2016, 3:43 AM

@herdsman
BreezeSansHindi-Regular.ttf Samsung font in Tizen mobile device
lohit_hi.ttf default font comes with Ubuntu.

Harfbuzz is enabled.

One question though, can width be less than the advance of the item in any case?

Not ignoring the fact that this happens after D3004, but I just want to confirm why this doesn't happen to me.

Assuming that Evas Text now reports its width correctly as the actual width (compared to advance like in the past), then the output should not look truncated. Unless, there's a problem with the width values in the font you use.

I basically need to check with your font, or you try another font and check the results.

I can see your result its good but what you want to say, is that I have to change the font for that commit, please correct me if I misunderstood something here.

tasn added a comment.Jan 14 2016, 4:29 AM

Width can be less or more than the advice, yes. This is common.

In T2992#42455, @tasn wrote:

Width can be less or more than the advice, yes. This is common.

Okay,
so in function _evas_object_text_layout

at line 803
 if (last_it)
    width += last_it->w - last_it->adv;

For single item as in my case and if

last_it->adv > last_it->w

Item width will be less than the actual one, no?, if so, then text will be truncated?
Please correct me if I am wrong.

tasn added a comment.Jan 14 2016, 4:45 AM

No.

Because the code before it is:
width += last_it->adv;

So it's actually:

if (last_it) width += w;

and for any other item:

if (last_it) width += adv;

@tasn

I am referring here only one item.

tasn added a comment.Jan 14 2016, 5:21 AM

If there's only one item, last_item is that. Check the code, the whole purpose of this line is to remove the advance and add width, essentially making it so the last item's width is taken into account instead of the advance.

@subodh6129, I don't know how this character is supposed to show when being a single character.
What I can say is that the font designer should make sure that the character's width value will still look correct.

If there's a problem with the font or not, I can't be sure until I inspect it.

In T2992#42466, @tasn wrote:

If there's only one item, last_item is that. Check the code, the whole purpose of this line is to remove the advance and add width, essentially making it so the last item's width is taken into account instead of the advance.

yes, I got that :)
but any reason for doing that or just to make it consistent with textblock, I saw the commit some truncation in Korean text but broke other text?
though one last question, Why can't we have a check there if width is greater than advance then only add width for the last item?

[snip]
though one last question, Why can't we have a check there if width is greater than advance then only add width for the last item?

And if it's "not greater" i.e. w < adv?

[snip]
though one last question, Why can't we have a check there if width is greater than advance then only add width for the last item?

And if it's "not greater" i.e. w < adv?

I am not sure, that's why I raised this ticket else I could have fixed it.

herdsman added a comment.EditedJan 14 2016, 6:35 AM

So, what I am wondering is: can you establish yourself that the appearance is wrong in the case of this sequence of characters? Is this sequence even valid on its own?

This is something that I can't determine by myself at the moment as I lack experience with this script.

@herdsman
Yes, sequence is correct and valid but not appearance, there is little truncation at the end of the text.

Meaning of this text is "Please do" (used for elders in respect). :)

Please feel free to ask if you need any help. :)

No problem. The thing is, that it still might be an issue with the font. I see it well, you don't.
I'll see if I can get this font.

May be font issue but the fact is it broke the existing things.
Also I provided two different fonts and its really hard to believe both are broken.
Anyways, please let me know if you need any more inputs from me.

Doh! Haven't noticed. Let me check those.

tasn added a subscriber: raster.Jan 14 2016, 7:50 AM

Actually, I think I know what the problem is! I just thought about it!

@subodh6129, I need your help, as I can't type in Devanagari (is it?), but I suspect it happens because of the shaping. I think the last character to be entered is the one that renders on top (check the x values of the glyph), and this one is indeed rendered in full, but the problem is, is that it's more offset to the left than the char it's set on, so it ends up cutting the previous char, that is more to the right. Is that correct?

@herdsman: I wonder if we can make the same happen with Nikkud, or maybe because it's RTL, we won't get it.

As for the solution: the only reason why we use "width" instead of "advance" is because @raster insisted on it (for legacy?), and wanted it to be the same as the text object. I wonder if it's more correct to change the text object and textblock to both have their width calculated with advance instead. What do you guys think?

@tasn, might be the reason. That's probably easy to solve then.

As for using advance, I'm still at "no". I guess I would need to provide an example but as I remember you can get really big unwanted extra space because of advance values.
I know it's causing us quite a few challenges, but I prefer the visual benefit of using width.

tasn added a comment.Jan 14 2016, 8:27 AM

I can't think of any broken visual example. Advance is how the text is intended to be, the proper spacing between the text and the "next element". I don't see why that is bad.

But this is about when there is no "next element". Yes, in typing (and placing the cursor) this has been somewhat of a problem.

Anyway, I will make time next week to do the fix and see if results are acceptable.

@tasn - the reason for width was that the width of a text object (or textblock) is .. the width of the TEXT. not the advance to the next char. you want the width for "Hello" to end at the right side of "o" (and of course begin at the left side to "H"). there were/are apis to query the advance beyond the object width - at least for text objects.

tasn added a comment.Jan 15 2016, 12:20 AM

@raster, I know, I was half trolling...

My point is though: maybe we should change object text and textblock to size themselves (each in his own way) with the last advance in mind, and not width...

Thoughts?

the reason width was used is so that when you take ANOTHEr object like a background (eg the beveled box behind a button or anything else) that you get this:

and not some extra space on the right like:

notice the extra pixel on the right. now the text and the backing it is on "looks wrong" because its not centered - off by one. GENERALLY this is what we want. an ENTRY is a special case where we are placing a cursor on the right within whitespace defined by the string (either extra spaces or advance of a char as we are expecting to be able to place a cursor there AND then type).

in fact if the cursor is a single pixel this all works. if the cursor were wider and fancier, even the advance may not cover enough space to show the cursor so it'll be clipped. the problem continues.

as @tasn said in the ticket T2583, this bug also seems to be related to last item width.
So we have actually last item width problem in both text object and textblock.
Just trying to gather the common bugs.

I can assure that there are many more bugs due to this last item width.This time in Tizen Z3 release we face many problem related to that.I will try to bring those problem to your notice guys.
I agree with @tasn here, advance is how the text intended to be.

what @raster is showing here is kind of acceptable "extra pixel" but truncation is definitely not.

So what I trying to say here instead of patching the symptoms, we need to fix the root problem.

i don't consider the etxra pixel acceptable. it leads to "badly aligned content" for the case of labels (buttons, checkboxes, labels, genlist items and everywhere else pretty much EXCEPT where you have to INPUT text and have an ENTRY).

entries are the minority use of text. breaking the majority use to fix this is not acceptable. find ANOTHER solution ... like being able to query the "advance" on the right edge (or left edge) or a textblock and then account for that in edje *IF* entry mode is enabled. ONLY then.

I think D3004 explains well why width is better than advance.
Just look at - that's a big extra space there.

Got a fix underway. Just need to check nothing is broken :)

Waiting eagerly :)

tasn added a comment.EditedJan 26 2016, 7:42 AM

OK, so @herdsman has been pestering me about this, so here I am giving my input.

@raster, I disagree with the "badly aligned content".
Who says it's badly aligned, you? It's not your job, it's the font designer's job... We traditionally have tried to "fix" (hack is a more correct term) alignment by checking the actual pixels and aligning according to that (so called "width").
Yes, maybe if we remove our hacks it won't be 100% symmetrical in a pixel sense, but heck, it's not symmetrical according to weight anyway. That is, a character like L (or j) would shift the alignment according to its right pixels although most of its pixels are on the left.
Alignment will always look odd to some people, but with the x bearing and the advance in mind, it should be OK even if we don't actively deal with it.

We have so much extra code to make it "better", but I think our definition of "better" is not actually better. It's definitely not better for code complexity (and as you can see, we've been chasing related issues for years now), but I think, it's not better in general. Especially when you consider the costs.

FWIW, in HTML (at least according to Firefox and Chrome), it's the same as what I'm describing, there's no concept of "width", it's aligned according to "advance". I assume it's the same everywhere else in the world...

Edit: made some corrections

tasn added a comment.Jan 26 2016, 7:54 AM

Just one more comment on the matter: our handling of width costs us a lot of CPU cycles and memory. Getting rid of it will reduce memory footprint and speed things up. This is just another benefit to the pile.

@raster, any thoughts regarding @tasn's reply?

my thoughts were as above already :)

tasn added a comment.Feb 15 2016, 2:51 AM

@raster, your example from above is not 100% correct, you made it sound as if it was:
(underscore indicates the spacing)
"Hello"
vs.
"Hello_"

While it's actually:
"Hello"
vs.
"_Hello_"

If we don't reduce/readd the left bearing already, we should, and that will essentially make them reasonably equal in all cases.

eh? we already account for left bearing. i did this day 0 for text objects because we needed the geometry to FIT. a lot of text goes LEFT of the "zero point" when you write it and that is taken care of by indenting the start of the text by the left bearing of the first char. the same is on last char - width is at right edge of last char... not last char PLUS advance or right edge of last char "(whichever is greater).

tasn added a comment.Feb 17 2016, 5:32 AM

I know we deal with bearing, and I know some have negative bearing. I was just saying your example was unfair.

Anyhow, this is the font's responsibility to decide such things, not yours, that's my point still. If they want negative left bearing, let them.

I don't think that will cause a negative effect at all, definitely not a drastic one, and it'll simplify and improve our code.

@raster
I faced one more problem caused by removing extra space from the last text item in Evas Textblock.

For example: "עצור Timeshift",

Logical item order: [עצור] [Timeshift]
Visual item order: [Timeshift] [עצור] => width 180.

  1. Resize Textblock object with width 180
  2. Enable ellipsis
  3. Text is ellipsized. => [Timesh... עצור]

Evas Textblock handle ellipsis with logically ordered text items.
But, the width is decided by visually ordered text items.
If Evas Textblock only use advance vaules of items for its width, there is no problem.
But, Evas Textblock will remove an extra space from visual last item.
If I resize Evas Textblcok object with text size which is retrieved from evas_object_textblock_size_native_get() or formatted_get(),
Evas Textblock makes an ellipsis!

So, we need to modify how to handle ellipsis in Evas Textblock.
I think we need to check visually ordered line for getting correct width whenever we need to check current x advance or width of line.

We are facing a lot of text width problems from global languages for each Tizen products. Everytime.
The last two Tizen mobile products were already released with modified code in their own way.
Because, the font was shared for multiple platforms and there was no problem like us. So, EFL was modified.

Why we need to keep this logic? I think text cut off problem is more serious than 1 pixel mis-aligned text.

subodh6129 closed this task as Resolved.Jun 1 2018, 5:18 AM