Reland "Attach whitespaces to the neighbor unresolved blocks"

This reverts commit 1436462. Reason for revert: Have to fix it and reland again Original change's description: > Revert "Attach whitespaces to the neighbor unresolved blocks" > > This reverts commit 131c5ad. > > Reason for revert: Blocking the G3 roll > > Original change's description: > > Attach whitespaces to the neighbor unresolved blocks > > > > Fix the situation when an unresolved {arabic} text is broken into > > many small runs by resolved english spaces. > > > > Bug: skia:10487 > > Change-Id: I3a739501c0fb7e0fc845e68392e1d214df9302db > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/304000 > > Commit-Queue: Julia Lavrova <[email protected]> > > Reviewed-by: Ben Wagner <[email protected]> > > [email protected],[email protected] > > Change-Id: Iaa338dd5fb5c9962df2ee32bafbc089da0e2b8a1 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: skia:10487 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/305797 > Reviewed-by: Robert Phillips <[email protected]> > Commit-Queue: Robert Phillips <[email protected]> [email protected],[email protected],[email protected] Bug: skia:10487 Change-Id: If8a85254fd536f465d80766de0406053d90c96a3 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/306062 Reviewed-by: Ben Wagner <[email protected]> Reviewed-by: Julia Lavrova <[email protected]> Commit-Queue: Julia Lavrova <[email protected]>
rive-app · Jul 28, 2020 · 3d404be · 3d404be
1 parent ba15fe1
commit 3d404be
Show file tree

Hide file tree

Showing 4 changed files with 142 additions and 14 deletions.
diff --git a/modules/skparagraph/src/OneLineShaper.cpp b/modules/skparagraph/src/OneLineShaper.cpp
@@ -284,6 +284,7 @@ void OneLineShaper::addUnresolvedWithRun(GlyphRange glyphRange) {
     fUnresolvedBlocks.emplace_back(unresolved);
 }
 
+#ifdef SK_PARAGRAPH_OLD_SPACE_RESOLUTION
 void OneLineShaper::sortOutGlyphs(std::function<void(GlyphRange)>&& sortOutUnresolvedBLock) {
 
     auto text = fCurrentRun->fMaster->text();
@@ -336,6 +337,67 @@ void OneLineShaper::sortOutGlyphs(std::function<void(GlyphRange)>&& sortOutUnres
         sortOutUnresolvedBLock(block);
     }
 }
+#else
+// Glue whitespaces to the next/prev unresolved blocks
+// (so we don't have chinese text with english whitespaces broken into millions of tiny runs)
+void OneLineShaper::sortOutGlyphs(std::function<void(GlyphRange)>&& sortOutUnresolvedBLock) {
+
+    auto text = fCurrentRun->fMaster->text();
+    size_t unresolvedGlyphs = 0;
+
+    TextIndex whitespacesStart = EMPTY_INDEX;
+    GlyphRange block = EMPTY_RANGE;
+    for (size_t i = 0; i < fCurrentRun->size(); ++i) {
+
+        const char* cluster = text.begin() + clusterIndex(i);
+        SkUnichar codepoint = nextUtf8Unit(&cluster, text.end());
+        bool isControl8 = isControl(codepoint);
+        bool isWhitespace8 = isWhitespace(codepoint);
+
+        // Inspect the glyph
+        auto glyph = fCurrentRun->fGlyphs[i];
+        if (glyph == 0 && !isControl8) { // Unresolved glyph and not control codepoint
+            ++unresolvedGlyphs;
+            if (block.start == EMPTY_INDEX) {
+                // Start new unresolved block
+                // (all leading whitespaces glued to the resolved part if it's not empty)
+                block.start = whitespacesStart == 0 ? 0 : i;
+                block.end = EMPTY_INDEX;
+            } else {
+                // Keep skipping unresolved block
+            }
+        } else { // Resolved glyph or control codepoint
+            if (block.start == EMPTY_INDEX) {
+                // Keep skipping resolved code points
+            } else if (isWhitespace8) {
+                // Glue whitespaces after to the unresolved block
+                ++unresolvedGlyphs;
+            } else {
+                // This is the end of unresolved block (all trailing whitespaces glued to the resolved part)
+                block.end = whitespacesStart == EMPTY_INDEX ? i : whitespacesStart;
+                sortOutUnresolvedBLock(block);
+                block = EMPTY_RANGE;
+                whitespacesStart = EMPTY_INDEX;
+            }
+        }
+
+        // Keep updated the start of the latest whitespaces patch
+        if (isWhitespace8) {
+            if (whitespacesStart == EMPTY_INDEX) {
+                whitespacesStart = i;
+            }
+        } else {
+            whitespacesStart = EMPTY_INDEX;
+        }
+    }
+
+    // One last block could have been left
+    if (block.start != EMPTY_INDEX) {
+        block.end = fCurrentRun->size();
+        sortOutUnresolvedBLock(block);
+    }
+}
+#endif
 
 void OneLineShaper::iterateThroughFontStyles(TextRange textRange,
                                              SkSpan<Block> styleSpan,

diff --git a/modules/skparagraph/src/ParagraphUtil.cpp b/modules/skparagraph/src/ParagraphUtil.cpp
@@ -41,5 +41,9 @@ bool isControl(SkUnichar utf8) {
     return u_iscntrl(utf8);
 }
 
+bool isWhitespace(SkUnichar utf8) {
+    return u_isWhitespace(utf8);
+}
+
 }
 }
diff --git a/modules/skparagraph/src/ParagraphUtil.h b/modules/skparagraph/src/ParagraphUtil.h
@@ -10,6 +10,7 @@ namespace textlayout {
 SkString SkStringFromU16String(const std::u16string& utf16text);
 SkUnichar nextUtf8Unit(const char** ptr, const char* end);
 bool isControl(SkUnichar utf8);
+bool isWhitespace(SkUnichar utf8);
 }
 }
 

diff --git a/modules/skparagraph/tests/SkParagraphTest.cpp b/modules/skparagraph/tests/SkParagraphTest.cpp
@@ -1239,7 +1239,7 @@ DEF_TEST(SkParagraph_HeightOverrideParagraph, reporter) {
     paragraph->layout(550);
 
     auto impl = static_cast<ParagraphImpl*>(paragraph.get());
-    REPORTER_ASSERT(reporter, impl->runs().size() == 5);
+    REPORTER_ASSERT(reporter, impl->runs().size() == 4);
     REPORTER_ASSERT(reporter, impl->styles().size() == 1);  // paragraph style does not count
     REPORTER_ASSERT(reporter, impl->styles()[0].fStyle.equals(text_style));
 
@@ -3989,27 +3989,28 @@ DEF_TEST(SkParagraph_FontFallbackParagraph, reporter) {
     paragraph->layout(TestCanvasWidth);
     paragraph->paint(canvas.get(), 10.0, 15.0);
 
-    REPORTER_ASSERT(reporter, paragraph->unresolvedGlyphs() == 2); // From the text1
+    REPORTER_ASSERT(reporter, paragraph->unresolvedGlyphs() == 3); // From the text1 ("字典 " - including the last space)
 
     auto impl = static_cast<ParagraphImpl*>(paragraph.get());
 
-    // Font resolution in Skia produces 6 runs because 2 parts of "Roboto 字典 " have different
-    // script (Minikin merges the first 2 into one because of unresolved) [Apple + Unresolved ]
-    // [Apple + Noto] [Apple + Han]
-    REPORTER_ASSERT(reporter, impl->runs().size() == 7);
+    REPORTER_ASSERT(reporter, impl->runs().size() == 6);
 
+    // Font resolution in Skia produces 6 runs because 2 parts of "Roboto 字典 " have different
+    // script (Minikin merges the first 2 into one because of unresolved)
+    // [Apple + Unresolved ] 0, 1
+    // [Apple + Noto] 2, 3
+    // [Apple + Han] 4, 5
     auto robotoAdvance = impl->runs()[0].advance().fX +
-                         impl->runs()[1].advance().fX +
-                         impl->runs()[2].advance().fX;
+                         impl->runs()[1].advance().fX;
     REPORTER_ASSERT(reporter, SkScalarNearlyEqual(robotoAdvance, 64.199f, EPSILON50));
-    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[3].advance().fX, 139.125f, EPSILON100));
-    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[4].advance().fX, 27.999f, EPSILON100));
-    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[5].advance().fX, 62.248f, EPSILON100));
-    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[6].advance().fX, 27.999f, EPSILON100));
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[2].advance().fX, 139.125f, EPSILON100));
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[3].advance().fX, 27.999f, EPSILON100));
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[4].advance().fX, 62.248f, EPSILON100));
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(impl->runs()[5].advance().fX, 27.999f, EPSILON100));
 
     // When a different font is resolved, then the metrics are different.
-    REPORTER_ASSERT(reporter, impl->runs()[4].correctAscent() != impl->runs()[6].correctAscent());
-    REPORTER_ASSERT(reporter, impl->runs()[4].correctDescent() != impl->runs()[6].correctDescent());
+    REPORTER_ASSERT(reporter, impl->runs()[3].correctAscent() != impl->runs()[5].correctAscent());
+    REPORTER_ASSERT(reporter, impl->runs()[3].correctDescent() != impl->runs()[5].correctDescent());
 }
 
 // Checked: NO DIFF
@@ -5457,3 +5458,63 @@ DEF_TEST(SkParagraph_LineMetricsTextAlign, reporter) {
     REPORTER_ASSERT(reporter, width[2] == width[0]);
     REPORTER_ASSERT(reporter, width[3] > width[0]); // delta == 0
 }
+
+DEF_TEST(SkParagraph_FontResolutionInRTL, reporter) {
+    sk_sp<ResourceFontCollection> fontCollection = sk_make_sp<ResourceFontCollection>(true);
+    if (!fontCollection->fontsFound()) return;
+    TestCanvas canvas("SkParagraph_FontResolutionInRTL.png");
+    const char* text = " אאא בּבּבּבּ אאאא בּבּ אאא בּבּבּ אאאאא בּבּבּבּ אאאא בּבּבּבּבּ ";
+    const size_t len = strlen(text);
+
+    ParagraphStyle paragraph_style;
+    paragraph_style.setMaxLines(14);
+    paragraph_style.setTextAlign(TextAlign::kRight);
+    paragraph_style.setTextDirection(TextDirection::kRtl);
+    paragraph_style.turnHintingOff();
+    ParagraphBuilderImpl builder(paragraph_style, fontCollection);
+
+    TextStyle text_style;
+    text_style.setFontFamilies({SkString("Ahem")});
+    text_style.setFontSize(26);
+    text_style.setColor(SK_ColorBLACK);
+    builder.pushStyle(text_style);
+    builder.addText(text, len);
+    builder.pop();
+
+    auto paragraph = builder.Build();
+    paragraph->layout(TestCanvasWidth);
+    paragraph->paint(canvas.get(), 0, 0);
+
+    auto impl = static_cast<ParagraphImpl*>(paragraph.get());
+    REPORTER_ASSERT(reporter, impl->runs().size() == 1);
+}
+
+DEF_TEST(SkParagraph_FontResolutionInLTR, reporter) {
+    sk_sp<ResourceFontCollection> fontCollection = sk_make_sp<ResourceFontCollection>(true);
+    if (!fontCollection->fontsFound()) return;
+    TestCanvas canvas("SkParagraph_FontResolutionInLTR.png");
+    auto text = u"abc \u01A2 \u01A2 def";
+
+    ParagraphStyle paragraph_style;
+    paragraph_style.setMaxLines(14);
+    paragraph_style.turnHintingOff();
+    ParagraphBuilderImpl builder(paragraph_style, fontCollection);
+
+    TextStyle text_style;
+    text_style.setFontFamilies({SkString("Roboto")});
+    text_style.setFontSize(26);
+    text_style.setColor(SK_ColorBLACK);
+    builder.pushStyle(text_style);
+    builder.addText(text);
+    builder.pop();
+
+    auto paragraph = builder.Build();
+    paragraph->layout(TestCanvasWidth);
+    paragraph->paint(canvas.get(), 0, 0);
+
+    auto impl = static_cast<ParagraphImpl*>(paragraph.get());
+    REPORTER_ASSERT(reporter, impl->runs().size() == 3);
+    REPORTER_ASSERT(reporter, impl->runs()[0].textRange().width() == 4); // "abc "
+    REPORTER_ASSERT(reporter, impl->runs()[1].textRange().width() == 5); // "{unresolved} {unresolved}"
+    REPORTER_ASSERT(reporter, impl->runs()[2].textRange().width() == 4); // " def"
+}