Correct last right bearing in boundingBox(glyphs)

Commit 07880542ecc479807c23c5646d263135240822ff introduced regression to
QFontMetrics::boundingRect and QTextEngine::boundingBox on FT, XLFD and
QPF font engine. Because on these platforms, rightBearing of the last
glyphs is already removed from the width returned in glyph_metrics_t.
Subtracting that rightBearing twice will cause the resulting boundingBox
smaller than expected.

This patch fix this by removing last right bearing accounting code from
generic QTextEngine::boundingBox, instead, we put it into font engines
that need them: QFontEngineWin, QCoreTextFontEngine, QFontEngineMac,
QFontEngineS60 and QFontEngineQWS. So that the resulting width should be
correct on all platforms without introducing any performance penalties.

Task-number: QTBUG-6854, QTBUG-12950
Reviewed-by: Eskil

7 files changed, 26 insertions, 17 deletions

diff --git a/src/gui/text/qfontengine.cpp b/src/gui/text/qfontengine.cpp
index 194c5f3..3f758b1 100644
--- a/src/gui/text/qfontengine.cpp
+++ b/src/gui/text/qfontengine.cpp
@@ -1093,6 +1093,18 @@ const QVector<QRgb> &QFontEngine::grayPalette()
     return *qt_grayPalette();
 }
 
+QFixed QFontEngine::lastRightBearing(const QGlyphLayout &glyphs, bool round)
+{
+    if (glyphs.numGlyphs >= 1) {
+        glyph_t glyph = glyphs.glyphs[glyphs.numGlyphs - 1];
+        glyph_metrics_t gi = boundingBox(glyph);
+        if (gi.isValid())
+            return round ? QFixed(qRound(gi.xoff - gi.x - gi.width))
+                         : QFixed(gi.xoff - gi.x - gi.width);
+    }
+    return 0;
+}
+
 // ------------------------------------------------------------------
 // The box font engine
 // ------------------------------------------------------------------
diff --git a/src/gui/text/qfontengine_mac.mm b/src/gui/text/qfontengine_mac.mm
index 91b6082..bdf3848 100644
--- a/src/gui/text/qfontengine_mac.mm
+++ b/src/gui/text/qfontengine_mac.mm
@@ -455,12 +455,13 @@ bool QCoreTextFontEngine::stringToCMap(const QChar *, int, QGlyphLayout *, int *
 glyph_metrics_t QCoreTextFontEngine::boundingBox(const QGlyphLayout &glyphs)
 {
     QFixed w;
+    bool round = fontDef.styleStrategy & QFont::ForceIntegerMetrics;
+
     for (int i = 0; i < glyphs.numGlyphs; ++i) {
-        w += (fontDef.styleStrategy & QFont::ForceIntegerMetrics)
-             ? glyphs.effectiveAdvance(i).round()
-             : glyphs.effectiveAdvance(i);
+        w += round ? glyphs.effectiveAdvance(i).round()
+                   : glyphs.effectiveAdvance(i);
     }
-    return glyph_metrics_t(0, -(ascent()), w, ascent()+descent(), w, 0);
+    return glyph_metrics_t(0, -(ascent()), w - lastRightBearing(glyphs, round), ascent()+descent(), w, 0);
 }
 glyph_metrics_t QCoreTextFontEngine::boundingBox(glyph_t glyph)
 {
@@ -1480,12 +1481,12 @@ void QFontEngineMac::recalcAdvances(QGlyphLayout *glyphs, QTextEngine::ShaperFla
 glyph_metrics_t QFontEngineMac::boundingBox(const QGlyphLayout &glyphs)
 {
     QFixed w;
+    bool round = fontDef.styleStrategy & QFont::ForceIntegerMetrics;
     for (int i = 0; i < glyphs.numGlyphs; ++i) {
-        w += (fontDef.styleStrategy & QFont::ForceIntegerMetrics)
-             ? glyphs.effectiveAdvance(i).round()
-             : glyphs.effectiveAdvance(i);
+        w += round ? glyphs.effectiveAdvance(i).round()
+                   : glyphs.effectiveAdvance(i);
     }
-    return glyph_metrics_t(0, -(ascent()), w, ascent()+descent(), w, 0);
+    return glyph_metrics_t(0, -(ascent()), w - lastRightBearing(glyphs, round), ascent()+descent(), w, 0);
 }
 
 glyph_metrics_t QFontEngineMac::boundingBox(glyph_t glyph)
diff --git a/src/gui/text/qfontengine_p.h b/src/gui/text/qfontengine_p.h
index 922acfb..3b91cd8 100644
--- a/src/gui/text/qfontengine_p.h
+++ b/src/gui/text/qfontengine_p.h
@@ -253,6 +253,7 @@ public:
 
 protected:
     static const QVector<QRgb> &grayPalette();
+    QFixed lastRightBearing(const QGlyphLayout &glyphs, bool round = false);
 
 private:
     struct GlyphCacheEntry {
diff --git a/src/gui/text/qfontengine_qws.cpp b/src/gui/text/qfontengine_qws.cpp
index a7a95d0..decc89c 100644
--- a/src/gui/text/qfontengine_qws.cpp
+++ b/src/gui/text/qfontengine_qws.cpp
@@ -557,7 +557,7 @@ glyph_metrics_t QFontEngineQPF1::boundingBox(const QGlyphLayout &glyphs)
     QFixed w = 0;
     for (int i = 0; i < glyphs.numGlyphs; ++i)
         w += glyphs.effectiveAdvance(i);
-    return glyph_metrics_t(0, -ascent(), w, ascent()+descent()+1, w, 0);
+    return glyph_metrics_t(0, -ascent(), w - lastRightBearing(glyphs), ascent()+descent()+1, w, 0);
 }
 
 glyph_metrics_t QFontEngineQPF1::boundingBox(glyph_t glyph)
diff --git a/src/gui/text/qfontengine_s60.cpp b/src/gui/text/qfontengine_s60.cpp
index 52a1fe7..2cc3f50 100644
--- a/src/gui/text/qfontengine_s60.cpp
+++ b/src/gui/text/qfontengine_s60.cpp
@@ -345,7 +345,7 @@ glyph_metrics_t QFontEngineS60::boundingBox(const QGlyphLayout &glyphs)
     for (int i = 0; i < glyphs.numGlyphs; ++i)
         w += glyphs.effectiveAdvance(i);
 
-    return glyph_metrics_t(0, -ascent(), w, ascent()+descent()+1, w, 0);
+    return glyph_metrics_t(0, -ascent(), w - lastRightBearing(glyphs), ascent()+descent()+1, w, 0);
 }
 
 glyph_metrics_t QFontEngineS60::boundingBox_const(glyph_t glyph) const
diff --git a/src/gui/text/qfontengine_win.cpp b/src/gui/text/qfontengine_win.cpp
index a805612..4bed2b5 100644
--- a/src/gui/text/qfontengine_win.cpp
+++ b/src/gui/text/qfontengine_win.cpp
@@ -487,7 +487,7 @@ glyph_metrics_t QFontEngineWin::boundingBox(const QGlyphLayout &glyphs)
     for (int i = 0; i < glyphs.numGlyphs; ++i)
         w += glyphs.effectiveAdvance(i);
 
-    return glyph_metrics_t(0, -tm.tmAscent, w, tm.tmHeight, w, 0);
+    return glyph_metrics_t(0, -tm.tmAscent, w - lastRightBearing(glyphs), tm.tmHeight, w, 0);
 }
 
 #ifndef Q_WS_WINCE
diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp
index 8d6dd6c..c30091e 100644
--- a/src/gui/text/qtextengine.cpp
+++ b/src/gui/text/qtextengine.cpp
@@ -1646,7 +1646,6 @@ glyph_metrics_t QTextEngine::boundingBox(int from,  int len) const
 
     for (int i = 0; i < layoutData->items.size(); i++) {
         const QScriptItem *si = layoutData->items.constData() + i;
-        QFontEngine *fe = fontEngine(*si);
 
         int pos = si->position;
         int ilen = length(i);
@@ -1676,6 +1675,7 @@ glyph_metrics_t QTextEngine::boundingBox(int from,  int len) const
                 while (charFrom < ilen && logClusters[charFrom] == glyphStart)
                     charFrom++;
             if (charFrom < ilen) {
+                QFontEngine *fe = fontEngine(*si);
                 glyphStart = logClusters[charFrom];
                 int charEnd = from + len - 1 - pos;
                 if (charEnd >= ilen)
@@ -1694,11 +1694,6 @@ glyph_metrics_t QTextEngine::boundingBox(int from,  int len) const
                     gm.yoff += m.yoff;
                 }
             }
-
-            glyph_t glyph = glyphs.glyphs[logClusters[ilen - 1]];
-            glyph_metrics_t gi = fe->boundingBox(glyph);
-            if (gi.isValid())
-                gm.width -= qRound(gi.xoff - gi.x - gi.width);
         }
     }
     return gm;