Fix inconsistencies between advanceWidth and bounding rects

Unify the logic in QTextEngine

Ensure tightBoundingRect, and the FreeType boundingRect, calculates from
the shaped x offset when calculating the max x coordinate for a glyph.

Fixes: QTBUG-7768
Task-number: QTBUG-70184
Task-number: QTBUG-85936
Task-number: QTBUG-94023
Change-Id: I6daafb25c79158dc7e777529abb5e8d3a284dac0
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

src/gui/text/freetype/qfontengine_ft.cpp

@@ -1705,9 +1705,8 @@ glyph_metrics_t QFontEngineFT::boundingBox(const QGlyphLayout &glyphs)
             QFixed y = overall.yoff + glyphs.offsets[i].y - g->y;
             overall.x = qMin(overall.x, x);
             overall.y = qMin(overall.y, y);
-            xmax = qMax(xmax, x + g->width);
+            xmax = qMax(xmax, x.ceil() + g->width);
-            ymax = qMax(ymax, y + g->height);
+            ymax = qMax(ymax, y.ceil() + g->height);
-            overall.xoff += g->advance;
             if (!cacheEnabled && g != &emptyGlyph)
                 delete g;
         } else {
@@ -1722,8 +1721,8 @@ glyph_metrics_t QFontEngineFT::boundingBox(const QGlyphLayout &glyphs)
             overall.y = qMin(overall.y, y);
             xmax = qMax(xmax, x + TRUNC(right - left));
             ymax = qMax(ymax, y + TRUNC(top - bottom));
-            overall.xoff += int(TRUNC(ROUND(face->glyph->advance.x)));
         }
+        overall.xoff += glyphs.effectiveAdvance(i);
     }
     overall.height = qMax(overall.height, ymax - overall.y);
     overall.width = xmax - overall.x;

src/gui/text/qfontengine.cpp

@@ -611,9 +611,9 @@ glyph_metrics_t QFontEngine::tightBoundingBox(const QGlyphLayout &glyphs)
         QFixed y = overall.yoff + glyphs.offsets[i].y + bb.y;
         overall.x = qMin(overall.x, x);
         overall.y = qMin(overall.y, y);
-        xmax = qMax(xmax, x + bb.width);
+        xmax = qMax(xmax, x.ceil() + bb.width);
-        ymax = qMax(ymax, y + bb.height);
+        ymax = qMax(ymax, y.ceil() + bb.height);
-        overall.xoff += bb.xoff;
+        overall.xoff += glyphs.effectiveAdvance(i);
         overall.yoff += bb.yoff;
     }
     overall.height = qMax(overall.height, ymax - overall.y);

src/gui/text/qtextengine.cpp

@@ -2098,35 +2098,30 @@ int QTextEngine::findItem(int strPos, int firstItem) const
     return right;
 }
 
-QFixed QTextEngine::width(int from, int len) const
+namespace {
+template<typename InnerFunc>
+void textIterator(const QTextEngine *textEngine, int from, int len, QFixed &width, InnerFunc &&innerFunc)
 {
-    itemize();
+    for (int i = 0; i < textEngine->layoutData->items.size(); i++) {
-
+        const QScriptItem *si = textEngine->layoutData->items.constData() + i;
-    QFixed w = 0;
-
-//     qDebug("QTextEngine::width(from = %d, len = %d), numItems=%d, strleng=%d", from,  len, items.size(), string.length());
-    for (int i = 0; i < layoutData->items.size(); i++) {
-        const QScriptItem *si = layoutData->items.constData() + i;
         int pos = si->position;
-        int ilen = length(i);
+        int ilen = textEngine->length(i);
 //          qDebug("item %d: from %d len %d", i, pos, ilen);
         if (pos >= from + len)
             break;
         if (pos + ilen > from) {
             if (!si->num_glyphs)
-                shape(i);
+                textEngine->shape(i);
 
             if (si->analysis.flags == QScriptAnalysis::Object) {
-                w += si->width;
+                width += si->width;
                 continue;
             } else if (si->analysis.flags == QScriptAnalysis::Tab) {
-                w += calculateTabWidth(i, w);
+                width += textEngine->calculateTabWidth(i, width);
                 continue;
             }
 
-
+            unsigned short *logClusters = textEngine->logClusters(si);
-            QGlyphLayout glyphs = shapedGlyphs(si);
-            unsigned short *logClusters = this->logClusters(si);
 
 //             fprintf(stderr, "  logclusters:");
 //             for (int k = 0; k < ilen; k++)
@@ -2151,11 +2146,24 @@ QFixed QTextEngine::width(int from, int len) const
                 glyphEnd = (charEnd == ilen) ? si->num_glyphs : logClusters[charEnd];
 
 //                 qDebug("char: start=%d end=%d / glyph: start = %d, end = %d", charFrom, charEnd, glyphStart, glyphEnd);
-                for (int i = glyphStart; i < glyphEnd; i++)
+                innerFunc(glyphStart, glyphEnd, si);
-                    w += glyphs.advances[i] * !glyphs.attributes[i].dontPrint;
             }
         }
     }
+}
+} // namespace
+
+QFixed QTextEngine::width(int from, int len) const
+{
+    itemize();
+
+    QFixed w = 0;
+//     qDebug("QTextEngine::width(from = %d, len = %d), numItems=%d, strleng=%d", from,  len, items.size(), string.length());
+    textIterator(this, from, len, w, [this, &w](int glyphStart, int glyphEnd, const QScriptItem *si) {
+        QGlyphLayout glyphs = this->shapedGlyphs(si);
+        for (int j = glyphStart; j < glyphEnd; j++)
+            w += glyphs.advances[j] * !glyphs.attributes[j].dontPrint;
+    });
 //     qDebug("   --> w= %d ", w);
     return w;
 }
@@ -2166,58 +2174,20 @@ glyph_metrics_t QTextEngine::boundingBox(int from,  int len) const
 
     glyph_metrics_t gm;
 
-    for (int i = 0; i < layoutData->items.size(); i++) {
+    textIterator(this, from, len, gm.width, [this, &gm](int glyphStart, int glyphEnd, const QScriptItem *si) {
-        const QScriptItem *si = layoutData->items.constData() + i;
+        if (glyphStart <= glyphEnd) {
-
+            QGlyphLayout glyphs = this->shapedGlyphs(si);
-        int pos = si->position;
+            QFontEngine *fe = this->fontEngine(*si);
-        int ilen = length(i);
-        if (pos > from + len)
-            break;
-        if (pos + ilen > from) {
-            if (!si->num_glyphs)
-                shape(i);
-
-            if (si->analysis.flags == QScriptAnalysis::Object) {
-                gm.width += si->width;
-                continue;
-            } else if (si->analysis.flags == QScriptAnalysis::Tab) {
-                gm.width += calculateTabWidth(i, gm.width);
-                continue;
-            }
-
-            unsigned short *logClusters = this->logClusters(si);
-            QGlyphLayout glyphs = shapedGlyphs(si);
-
-            // do the simple thing for now and give the first glyph in a cluster the full width, all other ones 0.
-            int charFrom = from - pos;
-            if (charFrom < 0)
-                charFrom = 0;
-            int glyphStart = logClusters[charFrom];
-            if (charFrom > 0 && logClusters[charFrom-1] == glyphStart)
-                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)
-                    charEnd = ilen-1;
-                int glyphEnd = logClusters[charEnd];
-                while (charEnd < ilen && logClusters[charEnd] == glyphEnd)
-                    charEnd++;
-                glyphEnd = (charEnd == ilen) ? si->num_glyphs : logClusters[charEnd];
-                if (glyphStart <= glyphEnd ) {
             glyph_metrics_t m = fe->boundingBox(glyphs.mid(glyphStart, glyphEnd - glyphStart));
             gm.x = qMin(gm.x, m.x + gm.xoff);
             gm.y = qMin(gm.y, m.y + gm.yoff);
-                    gm.width = qMax(gm.width, m.width+gm.xoff);
+            gm.width = qMax(gm.width, m.width + gm.xoff);
-                    gm.height = qMax(gm.height, m.height+gm.yoff);
+            gm.height = qMax(gm.height, m.height + gm.yoff);
             gm.xoff += m.xoff;
             gm.yoff += m.yoff;
         }
-            }
+    });
-        }
+
-    }
     return gm;
 }
 
@@ -2227,48 +2197,19 @@ glyph_metrics_t QTextEngine::tightBoundingBox(int from,  int len) const
 
     glyph_metrics_t gm;
 
-    for (int i = 0; i < layoutData->items.size(); i++) {
+    textIterator(this, from, len, gm.width, [this, &gm](int glyphStart, int glyphEnd, const QScriptItem *si) {
-        const QScriptItem *si = layoutData->items.constData() + i;
+        if (glyphStart <= glyphEnd) {
-        int pos = si->position;
+            QGlyphLayout glyphs = this->shapedGlyphs(si);
-        int ilen = length(i);
-        if (pos > from + len)
-            break;
-        if (pos + len > from) {
-            if (!si->num_glyphs)
-                shape(i);
-            unsigned short *logClusters = this->logClusters(si);
-            QGlyphLayout glyphs = shapedGlyphs(si);
-
-            // do the simple thing for now and give the first glyph in a cluster the full width, all other ones 0.
-            int charFrom = from - pos;
-            if (charFrom < 0)
-                charFrom = 0;
-            int glyphStart = logClusters[charFrom];
-            if (charFrom > 0 && logClusters[charFrom-1] == glyphStart)
-                while (charFrom < ilen && logClusters[charFrom] == glyphStart)
-                    charFrom++;
-            if (charFrom < ilen) {
-                glyphStart = logClusters[charFrom];
-                int charEnd = from + len - 1 - pos;
-                if (charEnd >= ilen)
-                    charEnd = ilen-1;
-                int glyphEnd = logClusters[charEnd];
-                while (charEnd < ilen && logClusters[charEnd] == glyphEnd)
-                    charEnd++;
-                glyphEnd = (charEnd == ilen) ? si->num_glyphs : logClusters[charEnd];
-                if (glyphStart <= glyphEnd ) {
             QFontEngine *fe = fontEngine(*si);
             glyph_metrics_t m = fe->tightBoundingBox(glyphs.mid(glyphStart, glyphEnd - glyphStart));
             gm.x = qMin(gm.x, m.x + gm.xoff);
             gm.y = qMin(gm.y, m.y + gm.yoff);
-                    gm.width = qMax(gm.width, m.width+gm.xoff);
+            gm.width = qMax(gm.width, m.width + gm.xoff);
-                    gm.height = qMax(gm.height, m.height+gm.yoff);
+            gm.height = qMax(gm.height, m.height + gm.yoff);
             gm.xoff += m.xoff;
             gm.yoff += m.yoff;
         }
-            }
+    });
-        }
-    }
     return gm;
 }
 

tests/auto/gui/text/qfontmetrics/tst_qfontmetrics.cpp

@@ -43,6 +43,7 @@ private slots:
     void same();
     void metrics();
     void boundingRect();
+    void boundingRect2();
     void elidedText_data();
     void elidedText();
     void veryNarrowElidedText();
@@ -149,6 +150,21 @@ void tst_QFontMetrics::boundingRect()
     QVERIFY(r.top() < 0);
 }
 
+void tst_QFontMetrics::boundingRect2()
+{
+    QFont f;
+    f.setPixelSize(16);
+    QFontMetricsF fm(f);
+    QString str("AVAVAVA vvvvvvvvvv fffffffff file");
+    QRectF br = fm.boundingRect(str);
+    QRectF tbr = fm.tightBoundingRect(str);
+    qreal advance = fm.horizontalAdvance(str);
+    // Bounding rect plus bearings should be similar to advance
+    qreal bearings = fm.leftBearing(QChar('A')) + fm.rightBearing(QChar('e'));
+    QVERIFY(qAbs(br.width() + bearings - advance) < fm.averageCharWidth()/2.0);
+    QVERIFY(qAbs(tbr.width() + bearings - advance) < fm.averageCharWidth()/2.0);
+}
+
 void tst_QFontMetrics::elidedText_data()
 {
     QTest::addColumn<QFont>("font");
@@ -374,6 +390,8 @@ void tst_QFontMetrics::zeroWidthMetrics()
     QCOMPARE(fm.horizontalAdvance(string3), fm.horizontalAdvance(string4));
     QCOMPARE(fm.boundingRect(string1).width(), fm.boundingRect(string2).width());
     QCOMPARE(fm.boundingRect(string3).width(), fm.boundingRect(string4).width());
+    QCOMPARE(fm.tightBoundingRect(string1).width(), fm.tightBoundingRect(string2).width());
+    QCOMPARE(fm.tightBoundingRect(string3).width(), fm.tightBoundingRect(string4).width());
 }
 
 QTEST_MAIN(tst_QFontMetrics)