Fix bug #4338103 (Android Arabic and Hebrew input is broken)

- fix glyph order when there is only a single run
- update tests

Change-Id: I113f28a8c76cab622fb75ce84bc50d1d38fa254e

4 files changed, 79 insertions, 49 deletions

diff --git a/core/jni/android/graphics/TextLayoutCache.cpp b/core/jni/android/graphics/TextLayoutCache.cpp
index df017f5..26aa7d9 100644
--- a/core/jni/android/graphics/TextLayoutCache.cpp
+++ b/core/jni/android/graphics/TextLayoutCache.cpp
@@ -399,6 +399,14 @@ struct GlyphRun {
     int isRTL;
 };
 
+void static reverseGlyphArray(jchar* glyphs, size_t count) {
+    for (size_t i = 0; i < count / 2; i++) {
+        jchar temp = glyphs[i];
+        glyphs[i] = glyphs[count - 1 - i];
+        glyphs[count - 1 - i] = temp;
+    }
+}
+
 void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar* chars,
         size_t start, size_t count, size_t contextCount, int dirFlags,
         jfloat* outAdvances, jfloat* outTotalAdvance,
@@ -424,6 +432,10 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar
 #endif
             computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount, dirFlags,
                     outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);
+
+            if (forceRTL && *outGlyphsCount > 1) {
+                reverseGlyphArray(*outGlyphs, *outGlyphsCount);
+            }
         } else {
             UBiDi* bidi = ubidi_open();
             if (bidi) {
@@ -438,71 +450,81 @@ void TextLayoutCacheValue::computeValuesWithHarfbuzz(SkPaint* paint, const UChar
 #if DEBUG_GLYPHS
                     LOGD("computeValuesWithHarfbuzz -- dirFlags=%d run-count=%d paraDir=%d", dirFlags, rc, paraDir);
 #endif
-
                     if (rc == 1 || !U_SUCCESS(status)) {
                         computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount,
                                 dirFlags, outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);
-                        ubidi_close(bidi);
-                        return;
-                    }
 
-                    size_t runIndex = 0;
-                    Vector<GlyphRun> glyphRuns;
-                    for (size_t i = 0; i < rc; ++i) {
-                        int32_t startRun;
-                        int32_t lengthRun;
-                        UBiDiDirection runDir = ubidi_getVisualRun(bidi, i, &startRun, &lengthRun);
-
-                        int newFlags = (runDir == UBIDI_RTL) ? kDirection_RTL : kDirection_LTR;
-                        jfloat runTotalAdvance = 0;
+                        if (dirFlags == 1 && *outGlyphsCount > 1) {
+                            reverseGlyphArray(*outGlyphs, *outGlyphsCount);
+                        }
+                    } else {
+                        Vector<GlyphRun> glyphRuns;
                         jchar* runGlyphs;
                         size_t runGlyphsCount = 0;
-
+                        size_t runIndex = 0;
+                        for (size_t i = 0; i < rc; ++i) {
+                            int32_t startRun;
+                            int32_t lengthRun;
+                            UBiDiDirection runDir = ubidi_getVisualRun(bidi, i, &startRun, &lengthRun);
+
+                            int newFlags = (runDir == UBIDI_RTL) ? kDirection_RTL : kDirection_LTR;
+                            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 newFlags=%d",
+                                    startRun, lengthRun, newFlags);
 #endif
-                        computeRunValuesWithHarfbuzz(paint, chars, startRun,
-                                lengthRun, contextCount, newFlags,
-                                outAdvances + runIndex, &runTotalAdvance,
-                                &runGlyphs, &runGlyphsCount);
+                            computeRunValuesWithHarfbuzz(paint, chars, startRun,
+                                    lengthRun, contextCount, newFlags,
+                                    outAdvances + runIndex, &runTotalAdvance,
+                                    &runGlyphs, &runGlyphsCount);
 
-                        runIndex += lengthRun;
-
-                        *outTotalAdvance += runTotalAdvance;
-                        *outGlyphsCount += runGlyphsCount;
+                            runIndex += lengthRun;
 
+                            *outTotalAdvance += runTotalAdvance;
+                            *outGlyphsCount += runGlyphsCount;
 #if DEBUG_GLYPHS
-                        LOGD("computeValuesWithHarfbuzz -- run=%d run-glyphs-count=%d",
-                                i, runGlyphsCount);
-                        for (size_t j = 0; j < runGlyphsCount; j++) {
-                            LOGD("                          -- glyphs[%d]=%d", j, runGlyphs[j]);
-                        }
-#endif
-                        glyphRuns.push(GlyphRun(runGlyphs, runGlyphsCount, newFlags));
-                    }
-
-#if DEBUG_GLYPHS
-                    LOGD("computeValuesWithHarfbuzz -- total-glyphs-count=%d", *outGlyphsCount);
+                            LOGD("computeValuesWithHarfbuzz -- run=%d run-glyphs-count=%d",
+                                    i, runGlyphsCount);
+                            for (size_t j = 0; j < runGlyphsCount; j++) {
+                                LOGD("                          -- glyphs[%d]=%d", j, runGlyphs[j]);
+                            }
 #endif
-                    *outGlyphs = new jchar[*outGlyphsCount];
-                    jchar* glyphs = *outGlyphs;
-                    for (size_t i = 0; i < glyphRuns.size(); i++) {
-                        const GlyphRun& glyphRun = glyphRuns.itemAt(i);
-                        if (glyphRun.isRTL) {
-                            for (size_t n = 0; n < glyphRun.glyphsCount; n++) {
-                                glyphs[glyphRun.glyphsCount - n - 1] = glyphRun.glyphs[n];
+                            glyphRuns.push(GlyphRun(runGlyphs, runGlyphsCount, newFlags));
+                        }
+                        *outGlyphs = new jchar[*outGlyphsCount];
+
+                        jchar* glyphs = *outGlyphs;
+                        for (size_t i = 0; i < glyphRuns.size(); i++) {
+                            const GlyphRun& glyphRun = glyphRuns.itemAt(i);
+                            if (glyphRun.isRTL) {
+                                for (size_t n = 0; n < glyphRun.glyphsCount; n++) {
+                                    glyphs[glyphRun.glyphsCount - n - 1] = glyphRun.glyphs[n];
+                                }
+                            } else {
+                                memcpy(glyphs, glyphRun.glyphs, glyphRun.glyphsCount * sizeof(jchar));
                             }
-                        } else {
-                            memcpy(glyphs, glyphRun.glyphs, glyphRun.glyphsCount * sizeof(jchar));
+                            glyphs += glyphRun.glyphsCount;
+                            delete[] glyphRun.glyphs;
                         }
-                        glyphs += glyphRun.glyphsCount;
-                        delete[] glyphRun.glyphs;
                     }
                 }
                 ubidi_close(bidi);
+            } else {
+                // Cannot run BiDi, just consider one Run
+#if DEBUG_GLYPHS
+                LOGD("computeValuesWithHarfbuzz -- cannot run BiDi, considering only one Run");
+#endif
+                computeRunValuesWithHarfbuzz(paint, chars, start, count, contextCount, dirFlags,
+                        outAdvances, outTotalAdvance, outGlyphs, outGlyphsCount);
+
+                if (dirFlags == 1 && *outGlyphsCount > 1) {
+                    reverseGlyphArray(*outGlyphs, *outGlyphsCount);
+                }
             }
         }
+#if DEBUG_GLYPHS
+        LOGD("computeValuesWithHarfbuzz -- total-glyphs-count=%d", *outGlyphsCount);
+#endif
 }
 
 void TextLayoutCacheValue::computeRunValuesWithHarfbuzz(SkPaint* paint, const UChar* chars,
diff --git a/tests/BiDiTests/res/layout/biditest_main.xml b/tests/BiDiTests/res/layout/biditest_main.xml
index 087c9a3..044a355 100644
--- a/tests/BiDiTests/res/layout/biditest_main.xml
+++ b/tests/BiDiTests/res/layout/biditest_main.xml
@@ -43,7 +43,6 @@
                   android:layout_height="wrap_content"
                   android:layout_width="match_parent"
                   android:textSize="32dip"
-                  android:text="@string/edittext_text"
         />
 
     </LinearLayout>
diff --git a/tests/BiDiTests/res/values/strings.xml b/tests/BiDiTests/res/values/strings.xml
index d20600e..c272df1 100644
--- a/tests/BiDiTests/res/values/strings.xml
+++ b/tests/BiDiTests/res/values/strings.xml
@@ -20,10 +20,11 @@
     <string name="normal_long_text">mmmmmmmmmmmmmmmmmmmmmmmm</string>
     <string name="normal_long_text_2">nnnnnnnnnnnnnnnnnnnnnnnn</string>
     <string name="normal_long_text_3">Notify me when an open network is available</string>
-    <string name="arabic_text">&#x0644;&#x0627; &#x0627;&#x0646;&#x0627; hello world</string>
+    <string name="arabic_text">&#x0644;&#x0627; &#x0627;&#x0646;&#x0627; hello Arabic</string>
     <string name="chinese_text">利比亚局势或影响美俄关系发展</string>
     <string name="italic_text">Italic String</string>
     <string name="bold_text">Bold String - other text</string>
     <string name="bold_italic_text">Bold Italic String</string>
     <string name="mixed_text_1">he said in Arabic: &#x0644;&#x0627;. Wow this is cool</string>
+    <string name="hebrew_text">&#x05DD;&#x05DE;</string>
 </resources>
\ No newline at end of file
diff --git a/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java b/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java
index 76031a8..4f17e52 100644
--- a/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java
+++ b/tests/BiDiTests/src/com/android/bidi/BiDiTestView.java
@@ -50,6 +50,7 @@ public class BiDiTestView extends View {
     private String ARABIC_TEXT;
     private String CHINESE_TEXT;
     private String MIXED_TEXT_1;
+    private String HEBREW_TEXT;
 
     private Typeface typeface;
 
@@ -81,6 +82,7 @@ public class BiDiTestView extends View {
         ARABIC_TEXT = context.getString(R.string.arabic_text);
         CHINESE_TEXT = context.getString(R.string.chinese_text);
         MIXED_TEXT_1 = context.getString(R.string.mixed_text_1);
+        HEBREW_TEXT = context.getString(R.string.hebrew_text);
 
         typeface = paint.getTypeface();
         paint.setAntiAlias(true);
@@ -95,7 +97,9 @@ public class BiDiTestView extends View {
     public void onDraw(Canvas canvas) {
         drawInsideRect(canvas, Color.BLACK);
 
-        int deltaX = testString(canvas, NORMAL_TEXT, ORIGIN, ORIGIN,
+        int deltaX = 0;
+
+        deltaX  = testString(canvas, NORMAL_TEXT, ORIGIN, ORIGIN,
                 paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);
 
         deltaX += testString(canvas, ITALIC_TEXT, ORIGIN + deltaX, ORIGIN,
@@ -130,6 +134,10 @@ public class BiDiTestView extends View {
         // Test Mixed (English and Arabic)
         deltaX = testString(canvas, MIXED_TEXT_1, ORIGIN, ORIGIN + 12 * currentTextSize,
                 paint, typeface, false, false,  Paint.DIRECTION_LTR, currentTextSize);
+
+        // Test Hebrew
+        deltaX = testString(canvas, HEBREW_TEXT, ORIGIN, ORIGIN + 14 * currentTextSize,
+                paint, typeface, false, false,  Paint.DIRECTION_RTL, currentTextSize);
     }
 
     private int testString(Canvas canvas, String text, int x, int y, Paint paint, Typeface typeface,