edje_text: Mirror support for RTL direction.
Needs RevisionPublic

Authored by tanwar.umesh07 on Jan 19 2016, 9:59 PM.

Details

Summary

In case of EVAS_BIDI_DIRECTION_RTL the alignment
should be mirrored as of EVAS_BIDI_DIRECTION_LTR. example:
if align_x = 0.2 for English (LTR) script, then for Hebrew (RTL)
script align_x should be 0.8.

@fix

Signed-off-by: Umesh Tanwar <umesh.tanwar@samsung.com>

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1115
Build 1180: arc lint + arc unit
tanwar.umesh07 retitled this revision from to edje_text: Mirror support for RTL direction..
tanwar.umesh07 updated this object.
tanwar.umesh07 added a subscriber: sachin.dev.
tanwar.umesh07 updated this object.
tanwar.umesh07 added a reviewer: id213sin.
herdsman edited edge metadata.Jan 23 2016, 11:56 PM

I'm not sure this is actually a @fix. Was the behavior at some point different than what we have now?

id213sin edited edge metadata.Jan 24 2016, 5:34 PM

@tanwar.umesh07
After applying this patch, If I want to show always left aligned text, what should I do?

@Mr. Herdsman
Previously -1 provides mirroring between align 0 and align 1 state. If LTR script is set to align 0 it is implicitly equivalent to align 1 for RTL script. The logical align for LTR and RTL scripts are different in visual alignment. For logical 0 align the visual align for LTR is align 0 and for RTL is align 1, because LTR starts from left side and RTL starts from right side. text.align= -1 takes care of this thing.
The same concept for other align values (0, 0.1 .....0.5....1) should be provided. This is the intention of this patch. Hope I convey my opinion.

@Mr. id213sin
to set visual left align
For LTR text.align:0
for RTL text.align:1

Thanx for your consideration.

tasn requested changes to this revision.Jan 26 2016, 2:40 AM
tasn edited edge metadata.

We decided not to adjust text alignment back then. I don't remember the exact reason. I guess the main reason was that if you have auto text alignment, it'll adjust itself according to the text and that would be correct anyway. I guess I could be swayed otherwise, but you'd need a good case.

As for the patch itself, I believe it's incorrect even if we agree on the idea:

  1. Why do you change it based on the part's bidi direction?
  2. Why only when the text is RTL?

I don't understand this patch and what it's trying to solve.

This revision now requires changes to proceed.Jan 26 2016, 2:40 AM

Ans 1. In case text.align = -1, the handling is done on the basis of bidi-direction.

Ans2. Only RTL scripts follow opposite alignment than usual alignment.

src/lib/edje/edje_text.c
526

Here the comparison is done on the basis of bididirection already.

tasn added a comment.Feb 3 2016, 2:20 AM

Ans 1. In case text.align = -1, the handling is done on the basis of bidi-direction.
Ans2. Only RTL scripts follow opposite alignment than usual alignment.

Extremely stupid questions on my part, don't know what I was thinking.

Anyhow, I think I remember part of why we didn't do it: we didn't have the time to implement it in textblock, and didn't want text and textblock to be inconsistent in that regard.

@tasn, do you have more concerns about this patch?

tasn added a comment.Feb 10 2016, 2:28 AM

The big question and reason why we didn't do it still remains. This isn't done on textblock, should it be done only here?

I guess it's fine, but it can cause some weird looking behaviours. Thoughts?

raster edited edge metadata.Jul 11 2016, 2:03 AM

pingzors

herdsman requested changes to this revision.Jul 11 2016, 7:00 AM
herdsman edited edge metadata.

I am willing to accept this patch, as this behavior hasn't been defined in the docs.
Also, see the inline comments for further changes.

src/lib/edje/edje_text.c
526

So this "if, else" is actually not needed, right? Just give it FROM_INT(1), and the auto-alignment code in the next few lines will mirror it. (see my next inline comment to drop the else).

536

No longer need this "else", right?

543

Isn't there FROM_INT(1) missing here?

Fixed incorrect value there.

src/lib/edje/edje_text.c
526

Oops, I meant FROM_INT(0), of course :)

Also, the docs should also be updated to reflect this new behavior.

Suggestions will be followed.

tanwar.umesh07 added a comment.EditedJul 25 2016, 2:51 AM

@herdsman: I thought it into two steps, since there is support of negative text alignment.

  1. Provide Mirror support for RTL direction.
  2. Removal of negative text alignment. At this steps comments at 526 and 536 come into effect.

In this issue our focus is on step 1. Is this okay?

src/lib/edje/edje_text.c
543

Implicit type conversion, Is explicit type conversion required here?

JackDanielZ resigned from this revision.Jul 30 2016, 4:27 AM
JackDanielZ removed a reviewer: JackDanielZ.