Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c2063a5b authored by Fabrice Di Meglio's avatar Fabrice Di Meglio
Browse files

Fix bug #5037425 Canvas.drawText can't handle Right-to-Left text and text composing 

- optimization for single run case was broken
- pass isRTL boolean along the call stack instead of the dirFlags integer
  (which was only used as a "isRTL" in the shaper)
- update unit tests

Change-Id: I33110b76a433633a0b92fbd1db03785204e0c3e6
parent 8abef6b0

core/jni/android/graphics/TextLayoutCache.cpp

+23 −20
Original line number Diff line number Diff line
@@ -323,9 +323,7 @@ size_t TextLayoutCacheValue::getSize() { 


void TextLayoutCacheValue::setupShaperItem(HB_ShaperItem* shaperItem, HB_FontRec* font,

        FontData* fontData, SkPaint* paint, const UChar* chars, size_t start, size_t count,

        size_t contextCount, int dirFlags) {

    bool isRTL = dirFlags & 0x1;



        size_t contextCount, bool isRTL) {

    font->klass = &harfbuzzSkiaClass;

    font->userData = 0;

    // The values which harfbuzzSkiaClass returns are already scaled to
@@ -374,10 +372,10 @@ void TextLayoutCacheValue::setupShaperItem(HB_ShaperItem* shaperItem, HB_FontRec 


void TextLayoutCacheValue::shapeWithHarfbuzz(HB_ShaperItem* shaperItem, HB_FontRec* font,

        FontData* fontData, SkPaint* paint, const UChar* chars, size_t start, size_t count,

        size_t contextCount, int dirFlags) {

        size_t contextCount, bool isRTL) {

    // Setup Harfbuzz Shaper

    setupShaperItem(shaperItem, font, fontData, paint, chars, start, count,

            contextCount, dirFlags);

            contextCount, isRTL);



    // Shape

    resetGlyphArrays(shaperItem);
@@ -430,7 +428,7 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar 
                    LOGD("computeValuesWithHarfbuzz -- forcing run with LTR=%d RTL=%d",

                            forceLTR, forceRTL);

#endif

            computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount, dirFlags,

            computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount, forceRTL,

                    outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);



            if (forceRTL && *outGlyphsCount > 1) {
@@ -451,10 +449,15 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar 
                    LOGD("computeValuesWithHarfbuzz -- dirFlags=%d run-count=%d paraDir=%d", dirFlags, rc, paraDir);

#endif

                    if (rc == 1 || !U_SUCCESS(status)) {

                        bool isRTL = (paraDir == 1);

#if DEBUG_GLYPHS

                        LOGD("computeValuesWithHarfbuzz -- processing SINGLE run "

                                "-- run-start=%d run-len=%d isRTL=%d", start, count, isRTL);

#endif

                        computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount,

                                dirFlags, outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);

                                isRTL, outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);



                        if (dirFlags == 1 && *outGlyphsCount > 1) {

                        if (isRTL && *outGlyphsCount > 1) {

                            reverseGlyphArray(*outGlyphs, *outGlyphsCount);

                        }

                    } else {
@@ -485,14 +488,14 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar 


                            lengthRun = endRun - startRun;



                            int newFlags = (runDir == UBIDI_RTL) ? kDirection_RTL : kDirection_LTR;

                            bool isRTL = (runDir == UBIDI_RTL);

                            jfloat runTotalAdvance = 0;

#if DEBUG_GLYPHS

                            LOGD("computeValuesWithHarfbuzz -- run-start=%d run-len=%d newFlags=%d",

                                    startRun, lengthRun, newFlags);

                            LOGD("computeValuesWithHarfbuzz -- run-start=%d run-len=%d isRTL=%d",

                                    startRun, lengthRun, isRTL);

#endif

                            computeRunValuesWithHarfbuzz(paint, chars, startRun,

                                    lengthRun, contextCount, newFlags,

                                    lengthRun, contextCount, isRTL,

                                    outAdvances, &runTotalAdvance,

                                    &runGlyphs, &runGlyphsCount);
@@ -506,7 +509,7 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar 
                                LOGD("                          -- glyphs[%d]=%d", j, runGlyphs[j]);

                            }

#endif

                            glyphRuns.push(GlyphRun(runGlyphs, runGlyphsCount, newFlags));

                            glyphRuns.push(GlyphRun(runGlyphs, runGlyphsCount, isRTL));

                        }

                        *outGlyphs = new jchar[*outGlyphsCount];
@@ -528,13 +531,15 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar 
                ubidi_close(bidi);

            } else {

                // Cannot run BiDi, just consider one Run

                bool isRTL = (bidiReq = 1) || (bidiReq = UBIDI_DEFAULT_RTL);

#if DEBUG_GLYPHS

                LOGD("computeValuesWithHarfbuzz -- cannot run BiDi, considering only one Run");

                LOGD("computeValuesWithHarfbuzz -- cannot run BiDi, considering a SINGLE Run "

                        "-- run-start=%d run-len=%d isRTL=%d", start, count, isRTL);

#endif

                computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount, dirFlags,

                computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount, isRTL,

                        outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);



                if (dirFlags == 1 && *outGlyphsCount > 1) {

                if (isRTL && *outGlyphsCount > 1) {

                    reverseGlyphArray(*outGlyphs, *outGlyphsCount);

                }

            }
@@ -545,17 +550,15 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar 
}



void TextLayoutCacheValue::computeRunValuesWithHarfbuzz(SkPaint* paint, const UChar* chars,

        size_t start, size_t count, size_t contextCount, int dirFlags,

        size_t start, size_t count, size_t contextCount, bool isRTL,

        jfloat* outAdvances, jfloat* outTotalAdvance,

        jchar** outGlyphs, size_t* outGlyphsCount) {



    bool isRTL = dirFlags & 0x1;



    HB_ShaperItem shaperItem;

    HB_FontRec font;

    FontData fontData;

    shapeWithHarfbuzz(&shaperItem, &font, &fontData, paint, chars, start, count,

            contextCount, dirFlags);

            contextCount, isRTL);



#if DEBUG_GLYPHS

    LOGD("HARFBUZZ -- num_glypth=%d - kerning_applied=%d", shaperItem.num_glyphs,

core/jni/android/graphics/TextLayoutCache.h

+3 −3
Original line number Diff line number Diff line
@@ -128,11 +128,11 @@ public: 


    static void setupShaperItem(HB_ShaperItem* shaperItem, HB_FontRec* font, FontData* fontData,

            SkPaint* paint, const UChar* chars, size_t start, size_t count, size_t contextCount,

            int dirFlags);

            bool isRTL);



    static void shapeWithHarfbuzz(HB_ShaperItem* shaperItem, HB_FontRec* font, FontData* fontData,

            SkPaint* paint, const UChar* chars, size_t start, size_t count, size_t contextCount,

            int dirFlags);

            bool isRTL);



    static void computeValuesWithHarfbuzz(SkPaint* paint, const UChar* chars, size_t start,

            size_t count, size_t contextCount, int dirFlags,
@@ -179,7 +179,7 @@ private: 
    static void resetGlyphArrays(HB_ShaperItem* shaperItem);



    static void computeRunValuesWithHarfbuzz(SkPaint* paint, const UChar* chars, size_t start,

            size_t count, size_t contextCount, int dirFlags,

            size_t count, size_t contextCount, bool isRTL,

            jfloat* outAdvances, jfloat* outTotalAdvance,

            jchar** outGlyphs, size_t* outGlyphsCount);

}; // TextLayoutCacheValue

tests/BiDiTests/res/layout/canvas2.xml

+9 −6
Original line number Diff line number Diff line
@@ -27,36 +27,39 @@ 


        <TextView

            android:text="@string/ltr"

            android:textSize="40dip"

            android:gravity="center"

            android:layout_width="fill_parent"

            android:layout_height="wrap_content" />



        <com.android.bidi.DrawTextTestView

        <com.android.bidi.BiDiTestViewDrawText

            local:text="@string/ltr"

            android:layout_width="fill_parent"

            android:layout_height="40dp" />

            android:layout_height="64dp" />



        <TextView

            android:text="@string/rtl"

            android:textSize="40dip"

            android:gravity="center"

            android:layout_width="fill_parent"

            android:layout_height="wrap_content"/>



        <com.android.bidi.DrawTextTestView

        <com.android.bidi.BiDiTestViewDrawText

            local:text="@string/rtl"

            android:layout_width="fill_parent"

            android:layout_height="40dp" />

            android:layout_height="64dp" />



        <TextView

            android:text="@string/composing"

            android:textSize="40dip"

            android:gravity="center"

            android:layout_width="fill_parent"

            android:layout_height="wrap_content"/>



        <com.android.bidi.DrawTextTestView

        <com.android.bidi.BiDiTestViewDrawText

            local:text="@string/composing"

            android:layout_width="fill_parent"

            android:layout_height="40dp" />

            android:layout_height="64dp" />



    </LinearLayout>

tests/BiDiTests/src/com/android/bidi/BiDiTestView.java

+21 −39
Original line number Diff line number Diff line
@@ -21,7 +21,6 @@ import android.graphics.Canvas; 
import android.graphics.Color;

import android.graphics.Paint;

import android.graphics.Rect;

import android.graphics.Typeface;

import android.text.TextPaint;

import android.util.AttributeSet;

import android.util.Log;
@@ -38,7 +37,6 @@ public class BiDiTestView extends View { 


    private static final float DEFAULT_ITALIC_SKEW_X = -0.25f;



    private TextPaint paint = new TextPaint();

    private Rect rect = new Rect();



    private String NORMAL_TEXT;
@@ -54,8 +52,6 @@ public class BiDiTestView extends View { 
    private String HEBREW_TEXT;

    private String RTL_TEXT;



    private Typeface typeface;



    private int currentTextSize;



    public BiDiTestView(Context context) {
@@ -86,9 +82,6 @@ public class BiDiTestView extends View { 
        MIXED_TEXT_1 = context.getString(R.string.mixed_text_1);

        HEBREW_TEXT = context.getString(R.string.hebrew_text);

        RTL_TEXT = context.getString(R.string.rtl);



        typeface = paint.getTypeface();

//        paint.setAntiAlias(true);

    }



    public void setCurrentTextSize(int size) {
@@ -98,54 +91,56 @@ public class BiDiTestView extends View { 


    @Override

    public void onDraw(Canvas canvas) {

        drawInsideRect(canvas, Color.BLACK);

        drawInsideRect(canvas, new Paint(), Color.BLACK);



        int deltaX = 0;



        deltaX  = testString(canvas, NORMAL_TEXT, ORIGIN, ORIGIN,

                paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);

                false, false,  Paint.DIRECTION_LTR, currentTextSize);



        deltaX += testString(canvas, ITALIC_TEXT, ORIGIN + deltaX, ORIGIN,

                paint, typeface, true, false,  Paint.DIRECTION_LTR, currentTextSize);

                true, false,  Paint.DIRECTION_LTR, currentTextSize);



        deltaX += testString(canvas, BOLD_TEXT, ORIGIN + deltaX, ORIGIN,

                paint, typeface, false, true,  Paint.DIRECTION_LTR, currentTextSize);

                false, true,  Paint.DIRECTION_LTR, currentTextSize);



        deltaX += testString(canvas, BOLD_ITALIC_TEXT, ORIGIN + deltaX, ORIGIN,

                paint, typeface, true, true,  Paint.DIRECTION_LTR, currentTextSize);

                true, true,  Paint.DIRECTION_LTR, currentTextSize);



        // Test with a long string

        deltaX = testString(canvas, NORMAL_LONG_TEXT, ORIGIN, ORIGIN + 2 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);

                false, false,  Paint.DIRECTION_LTR, currentTextSize);



        // Test with a long string

        deltaX = testString(canvas, NORMAL_LONG_TEXT_2, ORIGIN, ORIGIN + 4 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);

                false, false,  Paint.DIRECTION_LTR, currentTextSize);



        // Test with a long string

        deltaX = testString(canvas, NORMAL_LONG_TEXT_3, ORIGIN, ORIGIN + 6 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);

                false, false,  Paint.DIRECTION_LTR, currentTextSize);



        // Test Arabic ligature

        deltaX = testString(canvas, ARABIC_TEXT, ORIGIN, ORIGIN + 8 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_RTL, currentTextSize);

                false, false,  Paint.DIRECTION_RTL, currentTextSize);



        // Test Chinese

        deltaX = testString(canvas, CHINESE_TEXT, ORIGIN, ORIGIN + 10 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);

                false, false,  Paint.DIRECTION_LTR, currentTextSize);



        // Test Mixed (English and Arabic)

        deltaX = testString(canvas, MIXED_TEXT_1, ORIGIN, ORIGIN + 12 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);

                false, false,  Paint.DIRECTION_LTR, currentTextSize);



        // Test Hebrew

        deltaX = testString(canvas, RTL_TEXT, ORIGIN, ORIGIN + 14 * currentTextSize,

                paint, typeface, false, false,  Paint.DIRECTION_RTL, currentTextSize);

                false, false,  Paint.DIRECTION_RTL, currentTextSize);

    }



    private int testString(Canvas canvas, String text, int x, int y, Paint paint, Typeface typeface,

    private int testString(Canvas canvas, String text, int x, int y,

            boolean isItalic, boolean isBold, int dir, int textSize) {

        paint.setTypeface(typeface);



        TextPaint paint = new TextPaint();

        paint.setAntiAlias(true);



        // Set paint properties

        boolean oldFakeBold = paint.isFakeBoldText();
@@ -156,9 +151,9 @@ public class BiDiTestView extends View { 
            paint.setTextSkewX(DEFAULT_ITALIC_SKEW_X);

        }



        Log.v(TAG, "START -- drawTextWithCanvasDrawText");

        drawTextWithCanvasDrawText(text, canvas, x, y, textSize, Color.WHITE, dir);

        Log.v(TAG, "END   -- drawTextWithCanvasDrawText");

        paint.setTextSize(textSize);

        paint.setColor(Color.WHITE);

        canvas.drawText(text, x, y, paint);



        int length = text.length();

        float[] advances = new float[length];
@@ -170,12 +165,6 @@ public class BiDiTestView extends View { 
        logAdvances(text, textWidthHB, textWidthICU, advances);

        drawMetricsAroundText(canvas, x, y, textWidthHB, textWidthICU, textSize, Color.RED, Color.GREEN);



        paint.setColor(Color.WHITE);



        Log.v(TAG, "START -- drawText");

        canvas.drawText(text, x, y + currentTextSize, this.paint);

        Log.v(TAG, "END   -- drawText");



        // Restore old paint properties

        paint.setFakeBoldText(oldFakeBold);

        paint.setTextSkewX(oldTextSkewX);
@@ -188,7 +177,7 @@ public class BiDiTestView extends View { 
        paint.setBidiFlags(dir);

    }



    private void drawInsideRect(Canvas canvas, int color) {

    private void drawInsideRect(Canvas canvas, Paint paint, int color) {

        paint.setColor(color);

        int width = getWidth();

        int height = getHeight();
@@ -196,16 +185,9 @@ public class BiDiTestView extends View { 
        canvas.drawRect(rect, paint);

    }



    private void drawTextWithCanvasDrawText(String text, Canvas canvas,

            float x, float y, float textSize, int color, int dir) {

        setPaintDir(paint, dir);

        paint.setColor(color);

        paint.setTextSize(textSize);

        canvas.drawText(text, x, y, paint);

    }



    private void drawMetricsAroundText(Canvas canvas, int x, int y, float textWidthHB,

            float textWidthICU, int textSize, int color, int colorICU) {

        Paint paint = new Paint();

        paint.setColor(color);

        canvas.drawLine(x, y - textSize, x, y + 8, paint);

        canvas.drawLine(x, y + 8, x + textWidthHB, y + 8, paint);

tests/BiDiTests/src/com/android/bidi/DrawTextTestView.java→tests/BiDiTests/src/com/android/bidi/BiDiTestViewDrawText.java

+5 −5
Original line number Diff line number Diff line
@@ -25,25 +25,25 @@ import android.text.TextPaint; 
import android.util.AttributeSet;

import android.view.View;



public class DrawTextTestView extends View {

public class BiDiTestViewDrawText extends View {

    private float mSize;

    private int mColor;

    private String mText;



    public DrawTextTestView(Context context) {

    public BiDiTestViewDrawText(Context context) {

        this(context, null);

    }



    public DrawTextTestView(Context context, AttributeSet attrs) {

    public BiDiTestViewDrawText(Context context, AttributeSet attrs) {

        this(context, attrs, 0);

    }



    public DrawTextTestView(Context context, AttributeSet attrs, int defStyle) {

    public BiDiTestViewDrawText(Context context, AttributeSet attrs, int defStyle) {

        super(context, attrs, defStyle);



        final TypedArray a = context.obtainStyledAttributes(attrs,

                R.styleable.DrawTextTestView, defStyle, 0);

        mSize = a.getDimension(R.styleable.DrawTextTestView_size, 21.0f);

        mSize = a.getDimension(R.styleable.DrawTextTestView_size, 40.0f);

        mColor = a.getColor(R.styleable.DrawTextTestView_color, Color.YELLOW);

        final CharSequence text = a.getText(R.styleable.DrawTextTestView_text);

        mText = (text != null) ? text.toString() : "(empty)";