From 4748369eb2e458e12f5228a113b9a6dda440b24b Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Tue, 12 Oct 2021 12:52:39 +0200 Subject: [PATCH] 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 Reviewed-by: Lars Knoll Reviewed-by: Shawn Rutledge --- src/gui/text/freetype/qfontengine_ft.cpp | 7 +- src/gui/text/qfontengine.cpp | 6 +- src/gui/text/qtextengine.cpp | 159 ++++++------------ .../text/qfontmetrics/tst_qfontmetrics.cpp | 18 ++ 4 files changed, 74 insertions(+), 116 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 +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; } diff --git a/tests/auto/gui/text/qfontmetrics/tst_qfontmetrics.cpp b/tests/auto/gui/text/qfontmetrics/tst_qfontmetrics.cpp index c78475a76f..9ed88fe8ed 100644 --- a/tests/auto/gui/text/qfontmetrics/tst_qfontmetrics.cpp +++ b/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("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)