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

Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

Closed
akoeplinger wants to merge 4 commits into from
Closed

Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e6c490
Corrections to MeasureCharacterRanges, improvements to DrawString / M…
PreferLinux Nov 24, 2017
5ad77c2
Allocate index_matching array with malloc
akoeplinger Nov 24, 2017
b0881d5
Free index_matching
akoeplinger Nov 24, 2017
4ceae55
Merge branch 'master' into pr-28-cleanup-cairo
akoeplinger Aug 6, 2019
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
147 changes: 127 additions & 20 deletions src/text-cairo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,11 +707,27 @@ MeasureString (GpGraphics *graphics, GDIPCONST WCHAR *stringUnicode, int *length
boundingBox->Width = MaxX;
boundingBox->Height = MaxY;
}
if ((rc->Width > 0) && (boundingBox->Width > rc->Width)) {
boundingBox->Width = rc->Width;
if (rc->Width > 0) {
if (boundingBox->Width > rc->Width) {
boundingBox->Width = rc->Width;
} else {
if (format->formatFlags & StringFormatFlagsDirectionVertical) {
boundingBox->Y += (rc->Width - boundingBox->Width) / 2;
} else {
boundingBox->X += (rc->Width - boundingBox->Width) / 2;
}
Copy link
Contributor

@filipnavara filipnavara Mar 29, 2018

Choose a reason for hiding this comment

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

I don't see how this makes any sense. Why would you center the bounding box irrespective of the alignment?

Copy link
Contributor

@PreferLinux PreferLinux Mar 29, 2018

Choose a reason for hiding this comment

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

I wouldn't have noticed any impact on the bounding box directly, because only cairo_MeasureString (and therefore GdipMeasureString) even supply boundingBox to populate, and System.Drawing.Graphics.MeasureString() only uses the size component of that. I was probably trying to fix something but had the wrong place, and the code was never removed – what is here as the final result actually took several iterations to achieve.

The only other thing I can think of is something relating to counting how many characters were fitted. That doesn't make any sense though, because it'll instead mess that up rather badly due to what seems to me to be a bug in that code – it uses boundingBox / rc X + Width, not just Width. Before this change, that would have worked because the only System.Drawing.Graphics.MeasureString overload that returns that information provides a layoutRect (and therefore rc) with X and Y of 0.

So, I would suggest either changing it to take alignment into account and position the bounding box correctly and correcting the character-counting code (technically correct, although probably pointless), or reverting this.

Copy link
Contributor

@filipnavara filipnavara Mar 29, 2018

Choose a reason for hiding this comment

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

I was solving similar issue in the Pango text rendering backend. Ideally we should write some tests for the correct behavior. I ended up returning the actual bounding box of the text with all the alignment taken into account.

The thing is that in the Pango case the line alignment (ie. vertical alignment for most cases) is calculated at the very end once the bounding box height is already known. Unless there is some major flaw in that approach I would prefer to structure the Cairo code in the same way since it makes the code simpler to reason about.

Copy link
Contributor

@PreferLinux PreferLinux Mar 29, 2018

Choose a reason for hiding this comment

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

Yeah, that's probably the best way. MeasureString is complicated enough (and long enough) that anything that makes it easier to figure out is a good idea.

}
}
if ((rc->Height > 0) && (boundingBox->Height > rc->Height)) {
boundingBox->Height = rc->Height;
if (rc->Height > 0) {
if (boundingBox->Height > rc->Height) {
boundingBox->Height = rc->Height;
} else {
if (format->formatFlags & StringFormatFlagsDirectionVertical) {
boundingBox->X += (rc->Height - boundingBox->Height) / 2;
} else {
boundingBox->Y += (rc->Height - boundingBox->Height) / 2;
}
Copy link
Contributor

@filipnavara filipnavara Mar 29, 2018

Choose a reason for hiding this comment

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

Ditto.

}
}

/* avoid conversion computations if possible */
Expand Down Expand Up @@ -776,6 +792,72 @@ MeasureString (GpGraphics *graphics, GDIPCONST WCHAR *stringUnicode, int *length
double height = (boundingBox) ? boundingBox->Height : min (MaxY, rc->Height);
*linesFilled = floor (height / LineHeight);
}

if (AlignHorz != StringAlignmentNear || AlignVert != StringAlignmentNear) {
// Update alignment
int length = 0;
int current_line_length = 0;
for (i = 0; i < StringLen; i++) {
if (i == current_line_length) {
length = StringDetails[i].LineLen;
current_line_length = min(length + i, StringLen);
}

if ((format->formatFlags & StringFormatFlagsDirectionVertical) == 0) {
switch (AlignHorz) {
case StringAlignmentNear:
break;
case StringAlignmentCenter:
if ((current_line_length == 1) || (StringDetails [current_line_length - 1].PosX > 0)) {
StringDetails[i].PosX += (rc->Width - StringDetails [current_line_length - 1].PosX -
StringDetails [current_line_length - 1].Width) / 2;
}
break;
case StringAlignmentFar:
StringDetails[i].PosX += rc->Width - StringDetails [current_line_length - 1].PosX -
StringDetails [current_line_length - 1].Width;
break;
}

switch (AlignVert) {
case StringAlignmentNear:
break;
case StringAlignmentCenter:
StringDetails[i].PosY += (rc->Height - MaxY) / 2;
break;
case StringAlignmentFar:
StringDetails[i].PosY += rc->Height - MaxY;
break;
}
} else {
switch (AlignVert) {
case StringAlignmentNear:
break;
case StringAlignmentCenter:
if ((current_line_length == 1) || (StringDetails [current_line_length - 1].PosX > 0)) {
StringDetails[i].PosX += (rc->Height - StringDetails [current_line_length - 1].PosX -
StringDetails [current_line_length - 1].Width) / 2;
}
break;
case StringAlignmentFar:
StringDetails[i].PosX += rc->Height - StringDetails [current_line_length - 1].PosX -
StringDetails [current_line_length - 1].Width;
break;
}

switch (AlignHorz) {
case StringAlignmentNear:
break;
case StringAlignmentCenter:
StringDetails[i].PosY += (rc->Width - MaxY) / 2;
break;
case StringAlignmentFar:
StringDetails[i].PosY += rc->Width - MaxY;
break;
}
}
}
}

if (AlignHorz != StringAlignmentNear || AlignVert != StringAlignmentNear) {
// Update alignment
Expand Down Expand Up @@ -1100,10 +1182,15 @@ cairo_MeasureCharacterRanges (GpGraphics *graphics, GDIPCONST WCHAR *stringUnico
RectF charRect;
RectF rc_coords, *layoutRect = &rc_coords;
BOOL optimize_convert;
int *index_matching;
WCHAR *CleanString;
GpDrawTextData data; /* avoid recomputation of stuff done while measuring */
int StringLen = length;

index_matching = (int *) malloc (sizeof (int) * length);
if (!index_matching)
return OutOfMemory;

GpStatus status = AllocStringData (&CleanString, &StringDetails, length);
if (status != Ok)
return status;
Expand Down Expand Up @@ -1143,7 +1230,34 @@ cairo_MeasureCharacterRanges (GpGraphics *graphics, GDIPCONST WCHAR *stringUnico
if (status != Ok)
goto cleanup;

lineHeight = data.line_height + data.descent;
lineHeight = data.line_height; //+ data.descent;

j = 0; // index in StringDetails
for (i = 0; i < length; i++) {
switch (stringUnicode[i]) {
case '\r': /* CR */
case '\t': /* Tab */
case '\n': /* LF */
{
index_matching[i] = -1; // not present in StringDetails
break;
}

case '&':
{
if (format->hotkeyPrefix != HotkeyPrefixNone && (StringDetails[j].Flags & STRING_DETAIL_HOTKEY)) {
// Not present, because it is a hotkey
index_matching[i] = -1;
break;
} // else fall through to default
}

default:
{
index_matching[i] = j++;
}
}
}

/* Create a region for every char range */
for (i = 0; i < format->charRangeCount; i++) {
Expand All @@ -1170,26 +1284,18 @@ cairo_MeasureCharacterRanges (GpGraphics *graphics, GDIPCONST WCHAR *stringUnico

/* calculate the regions */
for (j = start; j < end; j++) {

/* the prefix char (&) always count - even if we are not actually print it as a char */
if ((StringDetails[j].Flags & STRING_DETAIL_HOTKEY) && (format->hotkeyPrefix != HotkeyPrefixNone)) {
end--; /* '&' count as invisible char */
continue;
}

/* workaround the fact that current implementation thinks LF is on the next line */
if ((j == end - 1) && (StringDetails[j].Flags & STRING_DETAIL_LF))
if (index_matching[j] == -1)
continue;

if (format->formatFlags & StringFormatFlagsDirectionVertical) {
charRect.X = layoutRect->X + StringDetails [j].PosY;
charRect.Y = layoutRect->Y + StringDetails [j].PosX;
charRect.X = layoutRect->X + StringDetails [index_matching[j]].PosY;
charRect.Y = layoutRect->Y + StringDetails [index_matching[j]].PosX + data.descent;
charRect.Width = lineHeight;
charRect.Height = StringDetails [j].Width;
charRect.Height = StringDetails [index_matching[j]].Width;
} else {
charRect.X = layoutRect->X + StringDetails [j].PosX;
charRect.Y = layoutRect->Y + StringDetails [j].PosY;
charRect.Width = StringDetails [j].Width;
charRect.X = layoutRect->X + StringDetails [index_matching[j]].PosX;
charRect.Y = layoutRect->Y + StringDetails [index_matching[j]].PosY + data.descent;
charRect.Width = StringDetails [index_matching[j]].Width;
charRect.Height = lineHeight;
}

Expand All @@ -1210,6 +1316,7 @@ cairo_MeasureCharacterRanges (GpGraphics *graphics, GDIPCONST WCHAR *stringUnico

cleanup:
/* Cleanup */
free (index_matching);
GdipFree (CleanString);
GdipFree (StringDetails);

Expand Down