Skip to content
This repository was archived by the owner on Mar 7, 2025. It is now read-only.

Pango fixes #384

Merged
merged 16 commits into from
Aug 8, 2018
Merged

Pango fixes #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
64742e3
Pango font description needs the size in points.
PreferLinux Jun 18, 2018
18f7fb0
Pango MeasureString measuring whitespace
PreferLinux Jun 18, 2018
b8fc10e
Fix Pango LineLimit cliping.
PreferLinux Jun 18, 2018
78435ca
Fix font family collection segfault.
PreferLinux Jun 18, 2018
802b06d
LineLimit should not be affected by clip
PreferLinux Jun 18, 2018
47a0216
Test MeasureString on whitespace-only lines.
PreferLinux Jul 17, 2018
55012d3
Test MeasureString LineLimit behaviour.
PreferLinux Jul 17, 2018
3d7d3c8
Correct text-pango counting of multi-byte characters removed.
PreferLinux Jul 17, 2018
fa81009
Fix pango_MeasureString chars fitted with multi-byte chars.
PreferLinux Jul 17, 2018
bcddd2a
Fix mistake in couning characters removed.
PreferLinux Jul 19, 2018
7eac989
Only limit Pango layout to the bounding box.
PreferLinux Jul 19, 2018
cbf8a50
Fix MeasureSring expected line count when no lines fitted.
PreferLinux Jul 25, 2018
e2e5e86
In MeasureString, don't count lines eliminated by LineLimit.
PreferLinux Jul 25, 2018
dc4ca4d
Fix out-of-bounds read in MeasureString getting chars fitted.
PreferLinux Jul 25, 2018
393311c
Fix another out-of-bounds read in pango_MeasureString.
PreferLinux Jul 26, 2018
d4320b5
Tweak test parameters to suit a range of fonts.
PreferLinux Jul 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/font.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ static int ref_familyMonospace = 0;
static void
gdip_fontfamily_init (GpFontFamily *fontFamily)
{
fontFamily->collection = NULL;
fontFamily->height = -1;
fontFamily->linespacing = -1;
fontFamily->celldescent = -1;
Expand Down Expand Up @@ -374,6 +375,7 @@ GdipGetFontCollectionFamilyList (GpFontCollection *font_collection, INT num_soug
return OutOfMemory;
}

gpfamilies[i]->collection = font_collection;
gpfamilies[i]->pattern = font_collection->fontset->fonts[i];
gpfamilies[i]->allocated = FALSE;
}
Expand Down Expand Up @@ -780,7 +782,10 @@ gdip_get_pango_font_description (GpFont *font)
if (!font->pango) {
font->pango = pango_font_description_new ();
pango_font_description_set_family (font->pango, (char *)font->face);
pango_font_description_set_size (font->pango, font->emSize * PANGO_SCALE);

float sizeInPoints = gdip_unit_conversion (font->unit, UnitPoint, gdip_get_display_dpi(), gtMemoryBitmap, font->emSize);

pango_font_description_set_size (font->pango, sizeInPoints * PANGO_SCALE);
Copy link
Contributor

@filipnavara filipnavara Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this won't work on the Pango CoreText backend. There's a bug in Pango where the point size is treated as CoreText point size, which is actually not the typographic point size, but something that HTML calls "px" (device independent pixel). One of them 1/72 inch, the other one is 1/96 inch.

Then again, it was broken before, the bug is in Cairo and not here, and the FreeType backend is currently enforced on all platforms anyway... so just a random note :-)

Copy link
Contributor Author

@PreferLinux PreferLinux Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would pango_font_description_set_absolute_size() (meant to be the size in device units) solve that? I was in two minds about whether it was better to calculate the point size (which I did in the end), or to use that and the pixel size which is already computed on creating the font...

Copy link
Contributor

@filipnavara filipnavara Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally it would because the CoreText backend doesn't differentiate them properly, so it would cancel out the bug. I am not sure what is a better option, but I am fine with either one.

Copy link
Contributor Author

@PreferLinux PreferLinux Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I settled on the way I did because of the possibility that the output dpi was different to the display dpi (e.g. for a printer). I realise this does not solve that completely, but it does mean the only unit affected is Pixel. I guess the correct thing to do would be to pass in the dpi to use. That'd require a change in #383 (or moving those changes) to set the resolution on the Graphics object used when drawing to a GraphicsPath.


if (font->style & FontStyleBold)
pango_font_description_set_weight (font->pango, PANGO_WEIGHT_BOLD);
Expand Down
93 changes: 49 additions & 44 deletions src/text-pango.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ gdip_set_array_values (int *array, int value, int num)
static GString *
gdip_process_string (gchar *text, int length, int removeAccelerators, int trimSpace, PangoAttrList *list, int **charsRemoved)
{
int i, j;
int c, j, r;
int nonws = 0;
gchar *iter;
gchar *iter2;
Expand Down Expand Up @@ -105,15 +105,16 @@ gdip_process_string (gchar *text, int length, int removeAccelerators, int trimSp
}

iter = text;
i = 0;
j = 0;
// c: Characters handled in the inner loop, that might be removed.
j = 0; // Index in output
r = 0; // Characters removed
while (iter - text < length) {
ch = g_utf8_get_char (iter);
if (ch == GDIP_WINDOWS_ACCELERATOR && removeAccelerators && (iter - text < length - 1)) {
nonws = 1;
iter2 = g_utf8_next_char (iter);
i += iter2 - iter;
iter = iter2;
r++;
ch = g_utf8_get_char (iter);
/* add an attribute on the next character */
if (list && (iter - text < length) && (ch != GDIP_WINDOWS_ACCELERATOR)) {
Expand All @@ -126,43 +127,49 @@ gdip_process_string (gchar *text, int length, int removeAccelerators, int trimSp
nonws = 1;
} else if (trimSpace && ch != '\r' && ch != '\n') {
/* unless specified we don't consider the trailing spaces, unless there is just one space (#80680) */
c = 1;
for (iter2 = g_utf8_next_char (iter); iter2 - text < length; iter2 = g_utf8_next_char (iter2)) {
ch = g_utf8_get_char (iter2);
if (ch == '\r' || ch == '\n')
break;
if (!g_unichar_isspace (ch)) {
c = 0;
g_string_append_len (res, iter, iter2 - iter);
if (charsRemoved && *charsRemoved)
gdip_set_array_values ((*charsRemoved)+j, i - j, iter2 - iter);
gdip_set_array_values ((*charsRemoved)+j, r, iter2 - iter);
j += iter2 - iter;
break;
}
c++;
}
i += iter2 - iter;
r += c;
iter = iter2;
continue;
} else if ((ch == '\r' && (iter - text == length - 2) && (*g_utf8_next_char (iter) == '\n')) || (ch == '\n' && iter - text == length - 1)) {
/* in any case, ignore a final newline as pango will add an extra line to the measurement while gdi+ does not */
i = length;
r += length - (iter - text);
break;
}
iter2 = g_utf8_next_char (iter);
g_string_append_len (res, iter, iter2 - iter);
/* save these for string lengths later */
if (charsRemoved && *charsRemoved)
gdip_set_array_values ((*charsRemoved)+j, i - j, iter2 - iter);
gdip_set_array_values ((*charsRemoved)+j, r, iter2 - iter);
j += iter2 - iter;
i += iter2 - iter;
iter = iter2;
}

/* always ensure that at least one space is measured */
if (!nonws && trimSpace) {
g_string_append_c (res, ' ');
j++;
if (!nonws && trimSpace && length > 0) {
iter = text;
iter2 = g_utf8_next_char (iter);
g_string_append_len (res, iter, iter2 - iter);
j += iter2 - iter;
Copy link
Contributor

@filipnavara filipnavara Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to make simple unit test for this? Just so we can compare the behavior on Windows and Linux.

Copy link
Contributor Author

@PreferLinux PreferLinux Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be, measure a string with, for example, an em-space (the issue I had). I'll have a go tomorrow.

Copy link
Contributor Author

@PreferLinux PreferLinux Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, doing that was easy enough, but it uncovered something else – the reported number of characters fitted is incorrect when removing multi-byte code points, or removing immediately after them. This change does not make it wrong, it just alters the (incorrect) behaviour slightly.

Copy link
Contributor Author

@PreferLinux PreferLinux Jul 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I hope I've sorted out counting the characters.

But I did a few checks on Windows (via .Net) as to the behaviour there, and it is weird. To me, it looks like unless there is a line ending, the only whitespace removed from the end are actual spaces. If there is a line ending, all trailing whitespace (considering tab as non-whitespace!) is removed. Furthermore, there is no leaving one space / character behind (gives a width of 0). It'd be good to do some testing without .Net in between, although I doubt it'd change. And I'm sure using Cairo text rendering doesn't follow Windows any more than it does here.

Copy link
Contributor Author

@PreferLinux PreferLinux Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just added a test case for this (aded2ce). I have also added fixes for the reported number of characters fitted (in two parts, one issue was counting the characters removed (109fe41), the other was in counting the characters in the layout (5b6db10)).

r--;
}
if (charsRemoved && *charsRemoved && j > 0) {
int prevj = (g_utf8_prev_char (res->str + j) - res->str);
gdip_set_array_values (*charsRemoved + prevj, i - j, j - prevj);
gdip_set_array_values (*charsRemoved + prevj, r, j - prevj);
}
return res;
}
Expand All @@ -187,10 +194,6 @@ gdip_pango_setup_layout (cairo_t *cr, GDIPCONST WCHAR *stringUnicode, int length
int FrameY; /* rc->Y (or rc->X if vertical) */
int y0; /* y0,y1,clipNN used for checking line positions vs. clip rectangle */
int y1;
double clipx1;
double clipx2;
double clipy1;
double clipy2;
int trimSpace; /* whether or not to trim the space */
BOOL use_horizontal_layout;

Expand Down Expand Up @@ -427,23 +430,14 @@ gdip_pango_setup_layout (cairo_t *cr, GDIPCONST WCHAR *stringUnicode, int length
/* Also prevents drawing whole lines outside the boundaries if NoClip was specified */
/* In case of pre-existing clipping, use smaller of clip rectangle or our specified height */
if (FrameHeight > 0) {
clipy2 = FrameHeight;
if (!(fmt->formatFlags & StringFormatFlagsNoClip)) {
cairo_clip_extents (cr, &clipx1, &clipy1, &clipx2, &clipy2);
if (fmt->formatFlags & StringFormatFlagsDirectionVertical) {
clipy2 = min (clipx2 - rc->X, FrameWidth);
} else {
clipy2 = min (clipy2 - rc->Y, FrameHeight);
}
}
iter = pango_layout_get_iter (layout);
do {
if (iter == NULL)
break;
pango_layout_iter_get_line_yrange (iter, &y0, &y1);
//g_warning("yrange: %d %d clipy2: %f", y0 / PANGO_SCALE, y1 / PANGO_SCALE, clipy2);
//g_warning("yrange: %d %d FrameHeight: %f", y0 / PANGO_SCALE, y1 / PANGO_SCALE, FrameHeight);
/* StringFormatFlagsLineLimit */
if (((fmt->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE >= clipy2) || (y0 / PANGO_SCALE >= clipy2)) {
if (((fmt->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE > FrameHeight) || (y0 / PANGO_SCALE >= FrameHeight)) {
Copy link
Contributor Author

@PreferLinux PreferLinux Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed the second contition to use FrameHeight too, and eliminated clipy2 and co.

Looking further, clipy2 and co were of very limited reliability for MeasureString because when used from managed code, the layout rectangle will have either the location or the size, not both. This code only does anything when the layout rectangle has size information, therefore there is a good chance that basing anything on the clip rectangle is meaningless. Drawing would probably not be affected.

PangoLayoutLine *line = pango_layout_iter_get_line_readonly (iter);
pango_layout_set_text (layout, pango_layout_get_text (layout), line->start_index);

Expand Down Expand Up @@ -590,6 +584,7 @@ pango_MeasureString (GpGraphics *graphics, GDIPCONST WCHAR *stringUnicode, int l
int lastIndex;
int y0;
int y1;
int len;
double min_x;
double max_x;
double max_y;
Expand All @@ -609,7 +604,7 @@ pango_MeasureString (GpGraphics *graphics, GDIPCONST WCHAR *stringUnicode, int l
if (iter == NULL)
break;
pango_layout_iter_get_line_yrange (iter, &y0, &y1);
if (y0 / PANGO_SCALE >= max_y)
if ((format && (format->formatFlags & StringFormatFlagsLineLimit) && y1 / PANGO_SCALE > max_y) || y0 / PANGO_SCALE >= max_y)
break;
lines++;
if (pango_layout_iter_at_last_line (iter)) {
Expand All @@ -630,22 +625,32 @@ pango_MeasureString (GpGraphics *graphics, GDIPCONST WCHAR *stringUnicode, int l

if (codepointsFitted) {
layoutText = pango_layout_get_text (layout);
/* this can happen when the string ends in a newline */
if (lastIndex >= strlen (layoutText))
lastIndex = strlen (layoutText) - 1;
/* Add back in any & characters removed and the final newline characters (if any) */
charsFitted = g_utf8_strlen (layoutText, lastIndex + 1) + charsRemoved [lastIndex];
//g_warning("lastIndex: %d\t\tcharsRemoved: %d", lastIndex, charsRemoved[lastIndex]);
/* safe because of null termination */
switch (layoutText [lastIndex + 1]) {
case '\r':
charsFitted++;
if (layoutText [lastIndex + 2] == '\n')

if (lines > 0)
len = strlen (layoutText);

if (lines > 0 && len > 0) {
/* this can happen when the string ends in a newline */
if (lastIndex >= len) {
lastIndex = g_utf8_prev_char(layoutText + len) - layoutText;
}
/* Add back in any & characters removed and the final newline characters (if any) */
charsFitted = g_utf8_strlen (layoutText, g_utf8_next_char (layoutText + lastIndex) - layoutText) + charsRemoved [lastIndex];
//g_warning("lastIndex: %d\t\tcharsRemoved: %d", lastIndex, charsRemoved[lastIndex]);
/* safe because of null termination */
switch (layoutText [lastIndex + 1]) {
case '\r':
charsFitted++;
break;
case '\n':
charsFitted++;
break;
if (layoutText [lastIndex + 2] == '\n')
charsFitted++;
break;
case '\n':
charsFitted++;
break;
}
} else {
// Nothing was fitted. Most likely either the input length was zero or LineLimit prevented fitting any lines (the height of the first line is greater than the height of the bounding box).
charsFitted = 0;
}
*codepointsFitted = charsFitted;
}
Expand Down
79 changes: 78 additions & 1 deletion tests/testtext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,17 @@ static void test_measure_string(void)
GpStatus status;
GpRectF rect, bounds, saved_bounds;
const WCHAR teststring1[] = { 'M', '\n', '\n', 'M', 0 };
const WCHAR teststring2[] = { ' ', L'\u2003', ' ', 0 }; // Space, em space, space
const WCHAR teststring3[] = { L'\u2003', L'\u2003', L'\u2003', 0 }; // em spaces
int glyphs;
int lines;
const SHORT fontSize = 10;

status = GdipCreateStringFormat (0, 0, &format);
expect (Ok, status);
status = GdipGetGenericFontFamilySansSerif (&family);
expect (Ok, status);
status = GdipCreateFont (family, 10, FontStyleRegular, UnitPixel, &font);
status = GdipCreateFont (family, fontSize, FontStyleRegular, UnitPixel, &font);
expect (Ok, status);
status = GdipCreateBitmapFromScan0 (400, 400, 0, PixelFormat32bppRGB, NULL, (GpBitmap **) &image);
expect (Ok, status);
Expand Down Expand Up @@ -93,6 +96,55 @@ static void test_measure_string(void)
expect (4, glyphs);
expect (3, lines);

// Attempt to fit 2 glyphs / 1 line into a bounding box from 2 glyphs / 1 line
rect = saved_bounds;
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring1, 2, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (2, glyphs);
expect (1, lines);

// Try again with StringFormatFlagsLineLimit set. Should still fit.
status = GdipSetStringFormatFlags(format, StringFormatFlagsLineLimit);
expect (Ok, status);
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring1, 2, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (2, glyphs);
expect (1, lines);

// Try again, but use clipping to only "show" the top half of a line. Should fit even with LineLimit.
status = GdipSetClipRect (graphics, rect.X, rect.Y, rect.Width, rect.Height / 2, CombineModeReplace);
expect (Ok, status);
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring1, 2, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (2, glyphs);
expect (1, lines);

// Reset the clip region
status = GdipResetClip (graphics);
expect (Ok, status);

// Now shorten the box slightly. Should not fit anything due to LineLimit.
rect.Height *= 0.9;
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring1, 2, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (0, glyphs);
expect (0, lines);

// Clear LineLimit flag
status = GdipSetStringFormatFlags(format, 0);
expect (Ok, status);

// Without LineLimit, it should fit the line successfully.
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring1, 2, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (2, glyphs);
expect (1, lines);

// Attempt to fit 3 glyphs / 2 lines into a bounding box from 2 glyphs / 1 line
rect = saved_bounds;
set_rect_empty (&bounds);
Expand All @@ -101,6 +153,31 @@ static void test_measure_string(void)
expect (2, glyphs);
expect (1, lines);

// Set rect large again.
rect.Width = 200.0;
rect.Height = 200.0;

// Use the Generic Typographic string format for below, to prevent Windows adding extra space.
GdipDeleteStringFormat (format);
status = GdipStringFormatGetGenericTypographic (&format);
expect (Ok, status);

// Check measuring a string with only whitespace
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring2, 3, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (3, glyphs); // Should be reported despite being trimmed
expect (1, lines);
expectf_(fontSize * 0.4, bounds.Width, fontSize * 0.3); // neither the expected value nor the precision is particularly accurate, but should be OK.

// Check measuring a string with only whitespace, that starts with non-space whitespace
set_rect_empty (&bounds);
status = GdipMeasureString (graphics, teststring3, 3, font, &rect, format, &bounds, &glyphs, &lines);
expect (Ok, status);
expect (3, glyphs); // Should be reported despite being trimmed
expect (1, lines);
expectf ((double)fontSize, bounds.Width); // An em-space should be the same width as the font size.

GdipDeleteGraphics (graphics);
GdipDeleteFont (font);
GdipDeleteFontFamily (family);
Expand Down