Skip to content

Commit

Permalink
Include right bearing in width of layouts wrapping on spaces
Browse files Browse the repository at this point in the history
When we're calculating the width of the layout, we include the
right bearing of the last character in the text line if it is
negative (i.e. it exceeds the advance width). We do this by
storing the last glyph that has been verified to fit in the
line, so that we can retrieve its right bearing when we find
a break.

However, when we were wrapping on spaces this previous glyph
would always be a space, and the right bearing would
subsequently be 0. But then the trailing spaces would be
trimmed and the right bearing of the actual last glyph
would not be recorded and never added to the text width.

This caused a failure in tst_qquicktext on Windows with both
DirectWrite and Freetype: This was purely unlucky, because
the metrics of the Tahoma font happened to be such that the
right bearing on the 'k' was enough to cause a line to
overflow. Since we didn't account for it when setting the
width, we ended up with unexpected line breaks, causing the
test to fail.

This did not happen with GDI, since it rounded the right
bearing of the character down to 0 (which was actually
visible in the layout, in that the k was painted a fraction
of a pixel outside the text layout's width).

In addition, QTBUG-130313 was causing us to pick a different
font when resolving the non-existent font requested by the test,
so therefore the bug was not found immediately when moving to
DirectWrite as the default but only when QTBUG-130313 was fixed.

We fix this by
  1. When adding a chunk of whitespace, we record the previous
     non-whitespace glyph that has been verified to fit.
  2. When adding a chunk of non-whitespace, we only record the
     previous glyph verified to fit *if* it is not whitespace.
	 Otherwise we keep whatever we recorded before adding the
	 spaces.

Pick-to: 6.8 6.9
Fixes: QTBUG-132075
Change-Id: I8d9a2f3197068f5f93520d217a6bb89633644e95
Reviewed-by: Lars Knoll <[email protected]>
  • Loading branch information
eskilblomfeldt committed Dec 11, 2024
1 parent 8784ea1 commit 2501170
Showing 1 changed file with 16 additions and 4 deletions.
20 changes: 16 additions & 4 deletions src/gui/text/qtextlayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1862,6 +1862,8 @@ void QTextLine::layout_helper(int maxGlyphs)
bool hasInlineObject = false;
QFixed maxInlineObjectHeight = 0;

const bool includeTrailingSpaces = eng->option.flags() & QTextOption::IncludeTrailingSpaces;

while (newItem < eng->layoutData->items.size()) {
lbh.resetRightBearing();
if (newItem != item) {
Expand Down Expand Up @@ -1963,6 +1965,10 @@ void QTextLine::layout_helper(int maxGlyphs)

} else if (attributes[lbh.currentPosition].whiteSpace
&& eng->layoutData->string.at(lbh.currentPosition).decompositionTag() != QChar::NoBreak) {
// If we are adding a space block, we save the last non-whitespace glyph for calculating
// the right bearing later
if (lbh.currentPosition > 0 && !attributes[lbh.currentPosition - 1].whiteSpace)
lbh.saveCurrentGlyph();
lbh.whiteSpaceOrObject = true;
while (lbh.currentPosition < end
&& attributes[lbh.currentPosition].whiteSpace
Expand All @@ -1976,7 +1982,15 @@ void QTextLine::layout_helper(int maxGlyphs)

lbh.whiteSpaceOrObject = false;
bool sb_or_ws = false;
lbh.saveCurrentGlyph();
// We save the previous glyph so we can use it for calculating the right bearing
// later. If we are trimming trailing spaces, the previous glyph is whitespace
// and we have already recorded a non-whitespace glyph, we keep that one instead.
if (lbh.currentPosition == 0
|| lbh.previousGlyph == 0
|| includeTrailingSpaces
|| !attributes[lbh.currentPosition - 1].whiteSpace) {
lbh.saveCurrentGlyph();
}
QFixed accumulatedTextWidth;
do {
addNextCluster(lbh.currentPosition, end, lbh.tmpData, lbh.glyphCount,
Expand Down Expand Up @@ -2137,9 +2151,7 @@ void QTextLine::layout_helper(int maxGlyphs)
line.descent.toReal(), line.textWidth.toReal(), lbh.spaceData.width.toReal());
LB_DEBUG(" : '%s'", eng->layoutData->string.mid(line.from, line.length).toUtf8().data());

const QFixed trailingSpace = (eng->option.flags() & QTextOption::IncludeTrailingSpaces
? lbh.spaceData.textWidth
: QFixed(0));
const QFixed trailingSpace = (includeTrailingSpaces ? lbh.spaceData.textWidth : QFixed(0));
if (eng->option.wrapMode() == QTextOption::WrapAtWordBoundaryOrAnywhere) {
if ((lbh.maxGlyphs != INT_MAX && lbh.glyphCount > lbh.maxGlyphs)
|| (lbh.maxGlyphs == INT_MAX && line.textWidth > (line.width - trailingSpace))) {
Expand Down

0 comments on commit 2501170

Please sign in to comment.