Fix issue #7253: port gl-js line breaking behavior

package.json

@@ -23,7 +23,7 @@
     "express": "^4.11.1",
     "lodash": "^4.16.4",
     "mapbox-gl-style-spec": "mapbox/mapbox-gl-style-spec#49e8b407bdbbe6f7c92dbcb56d3d51f425fc2653",
-    "mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#9c7063dc649821d03e6851dfa51fb9628c479f86",
+    "mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#e714350e53da55be6ebbbee9e5a5512ae2b1630d",
     "mkdirp": "^0.5.1",
     "node-cmake": "^1.2.1",
     "pixelmatch": "^4.0.2",

platform/default/bidi.cpp

@@ -8,20 +8,26 @@ namespace mbgl {
 
 class BiDiImpl {
 public:
-    BiDiImpl() : bidiText(ubidi_open()), bidiLine(ubidi_open()) {}
-    ~BiDiImpl() { ubidi_close(bidiText); ubidi_close(bidiLine); }
+    BiDiImpl() : bidiText(ubidi_open()), bidiLine(ubidi_open()) {
+    }
+    ~BiDiImpl() {
+        ubidi_close(bidiText);
+        ubidi_close(bidiLine);
+    }
 
     UBiDi* bidiText = nullptr;
     UBiDi* bidiLine = nullptr;
 };
+
+BiDi::BiDi() : impl(std::make_unique<BiDiImpl>()) {}
+BiDi::~BiDi() = default;
 
 // Takes UTF16 input in logical order and applies Arabic shaping to the input while maintaining
-// logical order
-//  Output won't be intelligible until the bidirectional algorithm is applied
+// logical order. Output won't be intelligible until the bidirectional algorithm is applied
 std::u16string applyArabicShaping(const std::u16string& input) {
     UErrorCode errorCode = U_ZERO_ERROR;
 
-    int32_t outputLength =
+    const int32_t outputLength =
         u_shapeArabic(input.c_str(), static_cast<int32_t>(input.size()), NULL, 0,
                       (U_SHAPE_LETTERS_SHAPE & U_SHAPE_LETTERS_MASK) |
                           (U_SHAPE_TEXT_DIRECTION_LOGICAL & U_SHAPE_TEXT_DIRECTION_MASK),
@@ -30,7 +36,7 @@ std::u16string applyArabicShaping(const std::u16string& input) {
     // Pre-flighting will always set U_BUFFER_OVERFLOW_ERROR
     errorCode = U_ZERO_ERROR;
 
-    std::unique_ptr<UChar[]> outputText = std::make_unique<UChar[]>(outputLength);
+    auto outputText = std::make_unique<UChar[]>(outputLength);
     u_shapeArabic(input.c_str(), static_cast<int32_t>(input.size()), outputText.get(), outputLength,
                   (U_SHAPE_LETTERS_SHAPE & U_SHAPE_LETTERS_MASK) |
                       (U_SHAPE_TEXT_DIRECTION_LOGICAL & U_SHAPE_TEXT_DIRECTION_MASK),
@@ -43,84 +49,77 @@ std::u16string applyArabicShaping(const std::u16string& input) {
     return std::u16string(outputText.get(), outputLength);
 }
 
-ProcessedBiDiText::ProcessedBiDiText(BiDi& p_bidi) : bidi(p_bidi) {
-}
-
-void ProcessedBiDiText::mergeParagraphLineBreaks(std::set<int32_t>& lineBreakPoints) {
-    int32_t paragraphCount = ubidi_countParagraphs(bidi.impl->bidiText);
+void BiDi::mergeParagraphLineBreaks(std::set<size_t>& lineBreakPoints) {
+    int32_t paragraphCount = ubidi_countParagraphs(impl->bidiText);
     for (int32_t i = 0; i < paragraphCount; i++) {
         UErrorCode errorCode = U_ZERO_ERROR;
         int32_t paragraphEndIndex;
-        ubidi_getParagraphByIndex(bidi.impl->bidiText, i, NULL, &paragraphEndIndex, NULL, &errorCode);
+        ubidi_getParagraphByIndex(impl->bidiText, i, NULL, &paragraphEndIndex, NULL, &errorCode);
 
-        if (U_FAILURE(errorCode))
+        if (U_FAILURE(errorCode)) {
             throw std::runtime_error(std::string("ProcessedBiDiText::mergeParagraphLineBreaks: ") +
                                      u_errorName(errorCode));
+        }
 
-        lineBreakPoints.insert(paragraphEndIndex);
+        lineBreakPoints.insert(static_cast<std::size_t>(paragraphEndIndex));
     }
 }
 
-std::vector<std::u16string>
-ProcessedBiDiText::applyLineBreaking(std::set<int32_t> lineBreakPoints) {
+std::vector<std::u16string> BiDi::applyLineBreaking(std::set<std::size_t> lineBreakPoints) {
     // BiDi::getLine will error if called across a paragraph boundary, so we need to ensure that all
-    // paragraph
-    //  boundaries are included in the set of line break points. The calling code might not include
-    //  the line break because it
-    //  didn't need to wrap at that point, or because the text was separated with a more exotic code
-    //  point such as (U+001C)
+    // paragraph boundaries are included in the set of line break points. The calling code might not
+    // include the line break because it didn't need to wrap at that point, or because the text was
+    // separated with a more exotic code point such as (U+001C)
     mergeParagraphLineBreaks(lineBreakPoints);
 
     std::vector<std::u16string> transformedLines;
-    int32_t start = 0;
-    for (int32_t lineBreakPoint : lineBreakPoints) {
-        transformedLines.push_back(bidi.getLine(start, lineBreakPoint));
+    std::size_t start = 0;
+    for (std::size_t lineBreakPoint : lineBreakPoints) {
+        transformedLines.push_back(getLine(start, lineBreakPoint));
         start = lineBreakPoint;
     }
 
     return transformedLines;
 }
 
-BiDi::BiDi() : impl(std::make_unique<BiDiImpl>())
-{
-}
-
-BiDi::~BiDi() {
-}
-
-ProcessedBiDiText BiDi::processText(const std::u16string& input) {
+std::vector<std::u16string> BiDi::processText(const std::u16string& input,
+                                              std::set<std::size_t> lineBreakPoints) {
     UErrorCode errorCode = U_ZERO_ERROR;
 
-    ubidi_setPara(impl->bidiText, input.c_str(), static_cast<int32_t>(input.size()), UBIDI_DEFAULT_LTR,
-                  NULL, &errorCode);
+    ubidi_setPara(impl->bidiText, input.c_str(), static_cast<int32_t>(input.size()),
+                  UBIDI_DEFAULT_LTR, NULL, &errorCode);
 
-    if (U_FAILURE(errorCode))
+    if (U_FAILURE(errorCode)) {
         throw std::runtime_error(std::string("BiDi::processText: ") + u_errorName(errorCode));
+    }
 
-    return ProcessedBiDiText(*this);
+    return applyLineBreaking(lineBreakPoints);
 }
 
-std::u16string BiDi::getLine(int32_t start, int32_t end) {
+std::u16string BiDi::getLine(std::size_t start, std::size_t end) {
     UErrorCode errorCode = U_ZERO_ERROR;
-    ubidi_setLine(impl->bidiText, start, end, impl->bidiLine, &errorCode);
+    ubidi_setLine(impl->bidiText, static_cast<int32_t>(start), static_cast<int32_t>(end), impl->bidiLine, &errorCode);
 
-    if (U_FAILURE(errorCode))
+    if (U_FAILURE(errorCode)) {
         throw std::runtime_error(std::string("BiDi::getLine (setLine): ") + u_errorName(errorCode));
+    }
 
     // Because we set UBIDI_REMOVE_BIDI_CONTROLS, the output may be smaller than what we reserve
     //  Setting UBIDI_INSERT_LRM_FOR_NUMERIC would require
     //  ubidi_getLength(pBiDi)+2*ubidi_countRuns(pBiDi)
-    int32_t outputLength = ubidi_getProcessedLength(impl->bidiLine);
-    std::unique_ptr<UChar[]> outputText = std::make_unique<UChar[]>(outputLength);
+    const int32_t outputLength = ubidi_getProcessedLength(impl->bidiLine);
+    auto outputText = std::make_unique<UChar[]>(outputLength);
 
     // UBIDI_DO_MIRRORING: Apply unicode mirroring of characters like parentheses
     // UBIDI_REMOVE_BIDI_CONTROLS: Now that all the lines are set, remove control characters so that
     // they don't show up on screen (some fonts have glyphs representing them)
     ubidi_writeReordered(impl->bidiLine, outputText.get(), outputLength,
                          UBIDI_DO_MIRRORING | UBIDI_REMOVE_BIDI_CONTROLS, &errorCode);
 
-    if (U_FAILURE(errorCode))
-        throw std::runtime_error(std::string("BiDi::getLine (writeReordered): ") + u_errorName(errorCode));
+    if (U_FAILURE(errorCode)) {
+        throw std::runtime_error(std::string("BiDi::getLine (writeReordered): ") +
+                                 u_errorName(errorCode));
+    }
 
     return std::u16string(outputText.get(), outputLength);
 }

platform/qt/src/bidi.cpp

@@ -16,20 +16,17 @@ std::u16string applyArabicShaping(const std::u16string& input) {
     return utf16string.toStdU16String();
 }
 
-ProcessedBiDiText::ProcessedBiDiText(BiDi& p_bidi) : bidi(p_bidi) {
-}
-
-void ProcessedBiDiText::mergeParagraphLineBreaks(std::set<int32_t>& lineBreakPoints) {
-    lineBreakPoints.insert(bidi.impl->string.length());
+void BiDi::mergeParagraphLineBreaks(std::set<std::size_t>& lineBreakPoints) {
+    lineBreakPoints.insert(static_cast<std::size_t>(bidi.impl->string.length()));
 }
 
 std::vector<std::u16string>
-ProcessedBiDiText::applyLineBreaking(std::set<int32_t> lineBreakPoints) {
+BiDi::applyLineBreaking(std::set<std::size_t> lineBreakPoints) {
     mergeParagraphLineBreaks(lineBreakPoints);
 
     std::vector<std::u16string> transformedLines;
-    int32_t start = 0;
-    for (int32_t lineBreakPoint : lineBreakPoints) {
+    std::size_t start = 0;
+    for (std::size_t lineBreakPoint : lineBreakPoints) {
         transformedLines.push_back(bidi.getLine(start, lineBreakPoint));
         start = lineBreakPoint;
     }
@@ -41,16 +38,15 @@ BiDi::BiDi() : impl(std::make_unique<BiDiImpl>())
 {
 }
 
-BiDi::~BiDi() {
-}
+BiDi::~BiDi() = default;
 
-ProcessedBiDiText BiDi::processText(const std::u16string& input) {
+std::vector<std::u16string> BiDi::processText(const std::u16string& input, std::set<std::size_t> lineBreakPoints) {
     impl->string = QString::fromStdU16String(input);
-    return ProcessedBiDiText(*this);
+    return applyLineBreaking(lineBreakPoints);
 }
 
-std::u16string BiDi::getLine(int32_t start, int32_t end) {
-    return impl->string.mid(start, end - start).toStdU16String();
+std::u16string BiDi::getLine(std::size_t start, std::size_t end) {
+    return impl->string.mid(static_cast<int32_t>(start), static_cast<int32_t>(end - start)).toStdU16String();
 }
 
 } // end namespace mbgl

src/mbgl/text/bidi.hpp

@@ -14,31 +14,17 @@ class BiDiImpl;
 
 std::u16string applyArabicShaping(const std::u16string&);
 
-class ProcessedBiDiText {
-public:
-    ProcessedBiDiText(BiDi&);
-
-    std::vector<std::u16string> applyLineBreaking(std::set<int32_t>);
-
-private:
-    void mergeParagraphLineBreaks(std::set<int32_t>&);
-
-    BiDi& bidi;
-};
-
 class BiDi : private util::noncopyable {
 public:
     BiDi();
     ~BiDi();
 
-    // Calling processText resets internal state, invalidating any existing ProcessedBiDiText
-    // objects
-    ProcessedBiDiText processText(const std::u16string&);
-
-    friend class ProcessedBiDiText;
+    std::vector<std::u16string> processText(const std::u16string&, std::set<std::size_t>);
 
 private:
-    std::u16string getLine(int32_t start, int32_t end);
+    void mergeParagraphLineBreaks(std::set<std::size_t>&);
+    std::vector<std::u16string> applyLineBreaking(std::set<std::size_t>);
+    std::u16string getLine(std::size_t start, std::size_t end);
 
     std::unique_ptr<BiDiImpl> impl;
 };