Improve readability of QTextLine's handling of (negative) right bearing

  - Helper functions in LineBreakHelper have been renamed from
    adjustFoo to calculateFoo, to signal that they do work, and
    are not adjusting something relative to something else.

  - The use of QFixed(1) and >= 0 has been replaced with an explicit
    constant for the case of the bearing not being calculated yet.

  - A helper function negativeRightBearing() has been added that
    returns the absolute value of any negative right bearing, making
    the width computations simpler.

  - Comments have been added and rewritten to make the logic clearer.

Change-Id: I1d3a0214cfa8b4fed4551f3444b43a37d55bd69b
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@theqtcompany.com>
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@theqtcompany.com>

1 files changed, 55 insertions, 26 deletions

diff --git a/src/gui/text/qtextlayout.cpp b/src/gui/text/qtextlayout.cpp
index 7da3e84041..013cd8ae0f 100644
--- a/src/gui/text/qtextlayout.cpp
+++ b/src/gui/text/qtextlayout.cpp
@@ -1605,8 +1605,8 @@ namespace {
         bool checkFullOtherwiseExtend(QScriptLine &line);
 
         QFixed calculateNewWidth(const QScriptLine &line) const {
-            return line.textWidth + tmpData.textWidth + spaceData.textWidth + softHyphenWidth
-                    - qMin(rightBearing, QFixed());
+            return line.textWidth + tmpData.textWidth + spaceData.textWidth
+                    + softHyphenWidth + negativeRightBearing();
         }
 
         inline glyph_t currentGlyph() const
@@ -1626,33 +1626,51 @@ namespace {
             }
         }
 
-        inline void adjustRightBearing(glyph_t glyph)
+        inline void calculateRightBearing(glyph_t glyph)
         {
             qreal rb;
             fontEngine->getGlyphBearings(glyph, 0, &rb);
-            rightBearing = qMin(QFixed(), QFixed::fromReal(rb));
+
+            // We only care about negative right bearings, so we limit the range
+            // of the bearing here so that we can assume it's negative in the rest
+            // of the code, as well ase use QFixed(1) as a sentinel to represent
+            // the state where we have yet to compute the right bearing.
+            rightBearing = qMin(QFixed::fromReal(rb), QFixed(0));
         }
 
-        inline void adjustRightBearing()
+        inline void calculateRightBearing()
         {
             if (currentPosition <= 0)
                 return;
-            adjustRightBearing(currentGlyph());
+            calculateRightBearing(currentGlyph());
         }
 
-        inline void adjustPreviousRightBearing()
+        inline void calculateRightBearingForPreviousGlyph()
         {
             if (previousGlyph > 0)
-                adjustRightBearing(previousGlyph);
+                calculateRightBearing(previousGlyph);
         }
 
+        static const QFixed RightBearingNotCalculated;
+
         inline void resetRightBearing()
         {
-            rightBearing = QFixed(1); // Any positive number is defined as invalid since only
-                                      // negative right bearings are interesting to us.
+            rightBearing = RightBearingNotCalculated;
+        }
+
+        // We express the negative right bearing as an absolute number
+        // so that it can be applied to the width using addition.
+        inline QFixed negativeRightBearing() const
+        {
+            if (rightBearing == RightBearingNotCalculated)
+                return QFixed(0);
+
+            return qAbs(rightBearing);
         }
     };
 
+const QFixed LineBreakHelper::RightBearingNotCalculated = QFixed(1);
+
 inline bool LineBreakHelper::checkFullOtherwiseExtend(QScriptLine &line)
 {
     LB_DEBUG("possible break width %f, spacew=%f", tmpData.textWidth.toReal(), spaceData.textWidth.toReal());
@@ -1805,7 +1823,7 @@ void QTextLine::layout_helper(int maxGlyphs)
                                current, lbh.logClusters, lbh.glyphs);
             } else {
                 lbh.tmpData.length++;
-                lbh.adjustPreviousRightBearing();
+                lbh.calculateRightBearingForPreviousGlyph();
             }
             line += lbh.tmpData;
             goto found;
@@ -1887,22 +1905,29 @@ void QTextLine::layout_helper(int maxGlyphs)
                     lbh.tmpData.textWidth += lbh.glyphs.advances[lbh.logClusters[lbh.currentPosition - 1]];
             }
 
-            // The actual width of the text needs to take the right bearing into account. The
-            // right bearing is left-ward, which means that if the rightmost pixel is to the right
-            // of the advance of the glyph, the bearing will be negative. We flip the sign
-            // for the code to be more readable. Logic borrowed from qfontmetrics.cpp.
-            // We ignore the right bearing if the minimum negative bearing is too little to
-            // expand the text beyond the edge.
             if (sb_or_ws|breakany) {
-                QFixed rightBearing = lbh.rightBearing; // store previous right bearing
+                // To compute the final width of the text we need to take negative right bearing
+                // into account (negative right bearing means the glyph has pixel data past the
+                // advance length). Note that the negative right bearing is an absolute number,
+                // so that we can apply it to the width using straight forward addition.
+
+                // Store previous right bearing (for the already accepted glyph) in case we
+                // end up breaking due to the current glyph being too wide.
+                QFixed previousRightBearing = lbh.rightBearing;
+
+                // We ignore the right bearing if the minimum negative bearing is too little to
+                // expand the text beyond the edge.
                 if (lbh.calculateNewWidth(line) - lbh.minimumRightBearing > line.width)
-                    lbh.adjustRightBearing();
+                    lbh.calculateRightBearing();
+
                 if (lbh.checkFullOtherwiseExtend(line)) {
-                    // we are too wide, fix right bearing
-                    if (rightBearing <= 0)
-                        lbh.rightBearing = rightBearing; // take from cache
+                    // We are too wide to accept the next glyph with its bearing, so we restore the
+                    // right bearing to that of the previous glyph (the one that was already accepted),
+                    // so that the bearing can be be applied to the final width of the text below.
+                    if (previousRightBearing != LineBreakHelper::RightBearingNotCalculated)
+                        lbh.rightBearing = previousRightBearing;
                     else
-                        lbh.adjustPreviousRightBearing();
+                        lbh.calculateRightBearingForPreviousGlyph();
 
                     if (!breakany) {
                         line.textWidth += lbh.softHyphenWidth;
@@ -1919,10 +1944,14 @@ void QTextLine::layout_helper(int maxGlyphs)
     LB_DEBUG("reached end of line");
     lbh.checkFullOtherwiseExtend(line);
 found:
-    if (lbh.rightBearing > 0 && !lbh.whiteSpaceOrObject) // If right bearing has not yet been adjusted
-        lbh.adjustRightBearing();
     line.textAdvance = line.textWidth;
-    line.textWidth -= qMin(QFixed(), lbh.rightBearing);
+
+    // If right bearing has not been calculated yet, do that now
+    if (lbh.rightBearing == LineBreakHelper::RightBearingNotCalculated && !lbh.whiteSpaceOrObject)
+        lbh.calculateRightBearing();
+
+    // Then apply any negative right bearing
+    line.textWidth += lbh.negativeRightBearing();
 
     if (line.length == 0) {
         LB_DEBUG("no break available in line, adding temp: length %d, width %f, space: length %d, width %f",