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>

3 files changed, 57 insertions, 117 deletions

diff --git a/src/gui/text/freetype/qfontengine_ft.cpp b/src/gui/text/freetype/qfontengine_ft.cpp
index ec870e2e76..863e81b8a2 100644
--- a/src/gui/text/freetype/qfontengine_ft.cpp
+++ b/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);
-            ymax = qMax(ymax, y + g->height);
-            overall.xoff += g->advance;
+            xmax = qMax(xmax, x.ceil() + g->width);
+            ymax = qMax(ymax, y.ceil() + g->height);
             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;
diff --git a/src/gui/text/qfontengine.cpp b/src/gui/text/qfontengine.cpp
index 72f90ae53b..10d3b5d2cb 100644
--- a/src/gui/text/qfontengine.cpp
+++ b/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);
-        ymax = qMax(ymax, y + bb.height);
-        overall.xoff += bb.xoff;
+        xmax = qMax(xmax, x.ceil() + bb.width);
+        ymax = qMax(ymax, y.ceil() + bb.height);
+        overall.xoff += glyphs.effectiveAdvance(i);
         overall.yoff += bb.yoff;
     }
     overall.height = qMax(overall.height, ymax - overall.y);
diff --git a/src/gui/text/qtextengine.cpp b/src/gui/text/qtextengine.cpp
index a30ca258ac..14e44f56d9 100644
--- a/src/gui/text/qtextengine.cpp
+++ b/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();
-
-    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;
+    for (int i = 0; i < textEngine->layoutData->items.size(); i++) {
+        const QScriptItem *si = textEngine->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;
             }
 
-
-            QGlyphLayout glyphs = shapedGlyphs(si);
-            unsigned short *logClusters = this->logClusters(si);
+            unsigned short *logClusters = textEngine->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++)
-                    w += glyphs.advances[i] * !glyphs.attributes[i].dontPrint;
+                innerFunc(glyphStart, glyphEnd, si);
             }
         }
     }
+}
+} // 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++) {
-        const QScriptItem *si = layoutData->items.constData() + i;
-
-        int pos = si->position;
-        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.height = qMax(gm.height, m.height+gm.yoff);
-                    gm.xoff += m.xoff;
-                    gm.yoff += m.yoff;
-                }
-            }
+    textIterator(this, from, len, gm.width, [this, &gm](int glyphStart, int glyphEnd, const QScriptItem *si) {
+        if (glyphStart <= glyphEnd) {
+            QGlyphLayout glyphs = this->shapedGlyphs(si);
+            QFontEngine *fe = this->fontEngine(*si);
+            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.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++) {
-        const QScriptItem *si = layoutData->items.constData() + i;
-        int pos = si->position;
-        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.height = qMax(gm.height, m.height+gm.yoff);
-                    gm.xoff += m.xoff;
-                    gm.yoff += m.yoff;
-                }
-            }
-        }
-    }
+    textIterator(this, from, len, gm.width, [this, &gm](int glyphStart, int glyphEnd, const QScriptItem *si) {
+        if (glyphStart <= glyphEnd) {
+            QGlyphLayout glyphs = this->shapedGlyphs(si);
+            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.height = qMax(gm.height, m.height + gm.yoff);
+            gm.xoff += m.xoff;
+            gm.yoff += m.yoff;
+        }
+    });
     return gm;
 }