Cleaning up line breaking code.

* Don't include trailing spacing (as opposed to whitespace) in lineLength
* Modify BiDi interface to require initial bidi layout and linebreaking to happen in one call.
* Code style changes suggested by @kkaefer

4 files changed, 72 insertions, 79 deletions

diff --git a/platform/default/bidi.cpp b/platform/default/bidi.cpp
index 5b0c2f8da7..9de6328a05 100644
--- a/platform/default/bidi.cpp
+++ b/platform/default/bidi.cpp
@@ -8,20 +8,30 @@ 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() {
+}
 
 // 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 +40,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,75 +53,66 @@ 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<int32_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);
     }
 }
 
-std::vector<std::u16string>
-ProcessedBiDiText::applyLineBreaking(std::set<int32_t> lineBreakPoints) {
+std::vector<std::u16string> BiDi::applyLineBreaking(std::set<int32_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));
+        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<int32_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) {
     UErrorCode errorCode = U_ZERO_ERROR;
     ubidi_setLine(impl->bidiText, start, 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
@@ -119,8 +120,10 @@ std::u16string BiDi::getLine(int32_t start, int32_t end) {
     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);
 }
diff --git a/platform/qt/src/bidi.cpp b/platform/qt/src/bidi.cpp
index 184d1daf9a..efb6932252 100644
--- a/platform/qt/src/bidi.cpp
+++ b/platform/qt/src/bidi.cpp
@@ -16,15 +16,12 @@ 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) {
+void BiDi::mergeParagraphLineBreaks(std::set<int32_t>& lineBreakPoints) {
     lineBreakPoints.insert(bidi.impl->string.length());
 }
 
 std::vector<std::u16string>
-ProcessedBiDiText::applyLineBreaking(std::set<int32_t> lineBreakPoints) {
+BiDi::applyLineBreaking(std::set<int32_t> lineBreakPoints) {
     mergeParagraphLineBreaks(lineBreakPoints);
 
     std::vector<std::u16string> transformedLines;
@@ -44,9 +41,9 @@ 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<int32_t> lineBreakPoints) {
     impl->string = QString::fromStdU16String(input);
-    return ProcessedBiDiText(*this);
+    return applyLineBreaking(lineBreakPoints);
 }
 
 std::u16string BiDi::getLine(int32_t start, int32_t end) {
diff --git a/src/mbgl/text/bidi.hpp b/src/mbgl/text/bidi.hpp
index 6c209c427c..73bb556a00 100644
--- a/src/mbgl/text/bidi.hpp
+++ b/src/mbgl/text/bidi.hpp
@@ -14,30 +14,16 @@ 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<int32_t>);
 
 private:
+    void mergeParagraphLineBreaks(std::set<int32_t>&);
+    std::vector<std::u16string> applyLineBreaking(std::set<int32_t>);
     std::u16string getLine(int32_t start, int32_t end);
 
     std::unique_ptr<BiDiImpl> impl;
diff --git a/src/mbgl/text/glyph_set.cpp b/src/mbgl/text/glyph_set.cpp
index f1cb85a03a..3b2687fbac 100644
--- a/src/mbgl/text/glyph_set.cpp
+++ b/src/mbgl/text/glyph_set.cpp
@@ -48,10 +48,9 @@ const Shaping GlyphSet::getShaping(const std::u16string& logicalInput,
     // different from the glyphs that get shown
     Shaping shaping(translate.x * 24, translate.y * 24, logicalInput);
 
-    ProcessedBiDiText processedText = bidi.processText(logicalInput);
-
     std::vector<std::u16string> reorderedLines =
-        processedText.applyLineBreaking(determineLineBreaks(logicalInput, spacing, maxWidth));
+        bidi.processText(logicalInput,
+                         determineLineBreaks(logicalInput, spacing, maxWidth));
 
     shapeLines(shaping, reorderedLines, spacing, lineHeight, horizontalAlign, verticalAlign,
                justify, translate);
@@ -84,8 +83,9 @@ void justifyLine(std::vector<PositionedGlyph>& positionedGlyphs,
                  uint32_t start,
                  uint32_t end,
                  float justify) {
-    if (!justify)
+    if (!justify) {
         return;
+    }
 
     PositionedGlyph& glyph = positionedGlyphs[end];
     auto it = sdfs.find(glyph.glyph);
@@ -105,14 +105,13 @@ float GlyphSet::determineIdeographicLineWidth(const std::u16string& logicalInput
     float totalWidth = 0;
 
     // totalWidth doesn't include the last character for magical tuning reasons. This makes the
-    // algorithm a little
-    //  more agressive about trying to fit the text into fewer lines, taking advantage of the
-    //  tolerance for going a little
-    //  over maxWidth
+    // algorithm a little more agressive about trying to fit the text into fewer lines, taking
+    // advantage of the tolerance for going a little over maxWidth
     for (uint32_t i = 0; i < logicalInput.size() - 1; i++) {
         auto it = sdfs.find(logicalInput[i]);
-        if (it != sdfs.end())
+        if (it != sdfs.end()) {
             totalWidth += it->second.metrics.advance + spacing;
+        }
     }
 
     int32_t lineCount = std::fmax(1, std::ceil(totalWidth / maxWidth));
@@ -124,14 +123,17 @@ float GlyphSet::determineIdeographicLineWidth(const std::u16string& logicalInput
 std::set<int32_t> GlyphSet::determineLineBreaks(const std::u16string& logicalInput,
                                                 const float spacing,
                                                 float maxWidth) const {
-    if (!maxWidth)
+    if (!maxWidth) {
         return {};
+    }
 
-    if (logicalInput.empty())
+    if (logicalInput.empty()) {
         return {};
+    }
 
-    if (util::i18n::allowsIdeographicBreaking(logicalInput))
+    if (util::i18n::allowsIdeographicBreaking(logicalInput)) {
         maxWidth = determineIdeographicLineWidth(logicalInput, spacing, maxWidth);
+    }
 
     std::set<int32_t> lineBreakPoints;
     float currentX = 0;
@@ -140,8 +142,9 @@ std::set<int32_t> GlyphSet::determineLineBreaks(const std::u16string& logicalInp
 
     for (uint32_t i = 0; i < logicalInput.size(); i++) {
         auto it = sdfs.find(logicalInput[i]);
-        if (it == sdfs.end())
+        if (it == sdfs.end()) {
             continue;
+        }
 
         const SDFGlyph& glyph = it->second;
 
@@ -186,24 +189,28 @@ void GlyphSet::shapeLines(Shaping& shaping,
         // Collapse whitespace so it doesn't throw off justification
         boost::algorithm::trim_if(line, boost::algorithm::is_any_of(u" \t\n\v\f\r"));
 
-        if (line.empty())
+        if (line.empty()) { 
             continue;
+        }
 
         uint32_t lineStartIndex = static_cast<uint32_t>(shaping.positionedGlyphs.size());
         for (char16_t chr : line) {
             auto it = sdfs.find(chr);
-            if (it == sdfs.end())
+            if (it == sdfs.end()) {
                 continue;
+            }
 
             const SDFGlyph& glyph = it->second;
             shaping.positionedGlyphs.emplace_back(chr, x, y);
             x += glyph.metrics.advance + spacing;
         }
 
-        if (static_cast<uint32_t>(shaping.positionedGlyphs.size()) == lineStartIndex)
+        if (static_cast<uint32_t>(shaping.positionedGlyphs.size()) == lineStartIndex) {
             continue;
+        }
 
-        maxLineLength = util::max(x, maxLineLength);
+        float lineLength = x - spacing; // Don't count trailing spacing
+        maxLineLength = util::max(lineLength, maxLineLength);
 
         justifyLine(shaping.positionedGlyphs, sdfs, lineStartIndex,
                     static_cast<uint32_t>(shaping.positionedGlyphs.size()) - 1, justify);