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

5 files changed, 61 insertions, 73 deletions

diff --git a/core/jni/android/graphics/TextLayoutCache.cpp b/core/jni/android/graphics/TextLayoutCache.cpp
index 6a13876..30fe298 100644
--- a/core/jni/android/graphics/TextLayoutCache.cpp
+++ b/core/jni/android/graphics/TextLayoutCache.cpp
@@ -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,
diff --git a/core/jni/android/graphics/TextLayoutCache.h b/core/jni/android/graphics/TextLayoutCache.h
index 690caac..10dee87 100644
--- a/core/jni/android/graphics/TextLayoutCache.h
+++ b/core/jni/android/graphics/TextLayoutCache.h
@@ -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
diff --git a/tests/BiDiTests/res/layout/canvas2.xml b/tests/BiDiTests/res/layout/canvas2.xml
index 2513c94..b3e038f 100644
--- a/tests/BiDiTests/res/layout/canvas2.xml
+++ b/tests/BiDiTests/res/layout/canvas2.xml
@@ -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>
 
diff --git a/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java b/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java
index 78b6495..0126dea 100644
--- a/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java
+++ b/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java
@@ -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);
diff --git a/tests/BiDiTests/src/com/android/bidi/DrawTextTestView.java b/tests/BiDiTests/src/com/android/bidi/BiDiTestViewDrawText.java
index 4a15100..dfdb807 100644
--- a/tests/BiDiTests/src/com/android/bidi/DrawTextTestView.java
+++ b/tests/BiDiTests/src/com/android/bidi/BiDiTestViewDrawText.java
@@ -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)";