Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

akoeplinger · 2017-11-24T13:53:34Z

MeasureCharacterRanges now counts LF, CR, tab as characters for measuring a CharacterRanges. Also utilises layoutRect.
MeasureString now deals with all alignment, rather than DrawString. Doing so has meant that StringAlignment can now work correctly. Also handles boundingBox better.
DrawString has been changed to work with the changes to MeasureString.

Note: these are the extracted cairo changes from #28 + cleanup, let's see what CI says 😄

…easureString MeasureCharacterRanges now counts LF, CR, tab as characters for measuring a CharacterRanges. Also utilises layoutRect. MeasureString now deals with all alignment, rather than DrawString. Doing so has meant that StringAlignment can now work correctly. Also handles boundingBox better. DrawString has been changed to work with the changes to MeasureString.

akoeplinger · 2017-11-24T13:54:19Z

@hughbe extracted the left over pieces from #28, maybe it makes more sense in this cleaned up form :)

hughbe · 2017-11-24T14:29:03Z

Doesn't compile with MSVC:

13:54:07 d:\j\workspace\test-libgdiplus-pull-request-windows\src\text-cairo.c(1124): error C2057: expected constant expression [d:\j\workspace\test-libgdiplus-pull-request-windows\src\libgdiplus.vcxproj]
13:54:07 d:\j\workspace\test-libgdiplus-pull-request-windows\src\text-cairo.c(1124): error C2466: cannot allocate an array of constant size 0 [d:\j\workspace\test-libgdiplus-pull-request-windows\src\libgdiplus.vcxproj]
13:54:07 d:\j\workspace\test-libgdiplus-pull-request-windows\src\text-cairo.c(1124): error C2133: 'index_matching': unknown size [d:\j\workspace\test-libgdiplus-pull-request-windows\src\libgdiplus.vcxproj]

hughbe · 2017-11-24T14:31:34Z

src/text-cairo.c

@@ -1099,6 +1121,7 @@ cairo_MeasureCharacterRanges (GpGraphics *graphics, GDIPCONST WCHAR *stringUnico
 	RectF			charRect;
 	RectF 			rc_coords, *layoutRect = &rc_coords;
 	BOOL			optimize_convert;
+	int			index_matching[length];


This is a gnu extension - I think you need to change this to

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

Also should be freed in cleanup:

Actually it's C99, but agreed with the allocation.

hughbe · 2017-11-24T14:32:45Z

I don't really understand the code well enough. Also would be nice to get #5 merged or closed :D

Using array initializer with a variable is a gnu extensions and doesn't work on MSVC.

akoeplinger · 2017-11-24T14:38:18Z

Yeah merging #5 would be nice but we'd need to find out if it's correct first since I don't understand this code well enough either 😄

hughbe · 2017-12-15T09:40:07Z

Is anything holding you back from merging this?

akoeplinger · 2017-12-17T02:09:54Z

@hughbe I'd like to give it a test run with some WinForms apps to make sure it doesn't horribly break something but didn't get to it yet. Hopefully during Christmas break :)

PreferLinux · 2017-12-21T23:31:13Z

I think I still understand most of it (the changes in particular), so if there's anything you'd like me to clarify I'll try to help.

filipnavara · 2018-03-29T18:24:31Z

src/text-cairo.c

+					boundingBox->Y += (rc->Width - boundingBox->Width) / 2;
+				} else {
+					boundingBox->X += (rc->Width - boundingBox->Width) / 2;
+				}


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

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.

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.

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.

filipnavara · 2018-03-29T18:24:52Z

src/text-cairo.c

+					boundingBox->X += (rc->Height - boundingBox->Height) / 2;
+				} else {
+					boundingBox->Y += (rc->Height - boundingBox->Height) / 2;
+				}


filipnavara · 2018-03-30T14:44:50Z

I went through the text-cairo code and it's almost universally broken whenever there are TABs, CRs, LFs in the string. It does some string preprocessing to strip these character and accounts for that in the layout, but it utterly fails to account for it when any measurements are reported back. That involves mainly the codepointsFitted metric and MeasureCharacterRanges. This patch tries hard to compensate for it in the context of MeasureCharacterRanges, but other places remain broken.

What would make much more sense though is NOT to strip the characters in the input and instead handle them in the layout pass. That way StringDetails indices will match the actual string that is passed in.

PreferLinux · 2018-03-30T20:15:22Z

Yes, definitely. I was trying to make it work for me, which meant I went for the obvious route rather than changing a whole lot.

filipnavara · 2018-03-30T22:18:54Z

I'm not questioning your choices, just trying to find out how to proceed further. There's couple of separate changes in this patch:

Moving horizontal/vertical alignment adjustments from DrawString to MeasureString. This is reasonable change. The alignment affects the overall positioning of letters (as used by cairo_MeasureCharacterRanges and DrawString) and the final bounding rectangle (as used by cairo_MeasureString).
A possible optimization is to move this into separate function that is used by cairo_DrawString and cairo_MeasureCharacterRanges since cairo_MeasureString doesn't need the recomputed coordinates.
The weird bounding box centring I commented on before. This intention is correct, but the implementation is not. The bounding box has to be re-positioned according to both horizontal and vertical alignment. This is currently not done and the X, Y coordinates are wrong. However, they are not used in System.Drawing, so the impact is limited.
In a similar fashion to the above optimization we can apply the alignment to the bounding box only in cairo_MeasureString since cairo_DrawString and cairo_MeasureCharacterRanges don't use it.
Fixing of the indexes in cairo_MeasureCharacterRanges to account for the string clean up in MeasureString function.

The first two changes can be separated quite easily into individual patches. First one is trivial refactoring by moving code. Second one is merely copying code in PR #245 from Pango backend to the Cairo one. The last one I would propose to implement differently, but that will definitely require digging deeper into the code and refactoring it. A first step would be to have unit tests covering the problematic cases. I wanted to save some time on that by PR #262, but that will likely go nowhere. So in the interest of sharing my thoughts, here are the cases that need to be tested:

codepointsFitted should count LFs in cairo_MeasureString, most likely the same applies to CR and TAB
check how codepointsFitted accounts for '&' character in different modes of HotkeyPrefix.
cairo_MeasureString should set X, Y of bounding box accordingly to StringFormat->Alignment/LineAlignment and combinations of StringFormatFlagsDirectionVertical and StringFormatFlagsDirectionRightToLeft (not properly implement, btw)
cairo_MeasureCharacterRanges should correctly account for the CR, LF and TAB characters.
horizontal alignment in combination with StringFormatFlagsMeasureTrailingSpaces
how should invisible character be reported by cairo_MeasureCharacterRanges? (eg. LFs, trailing spaces)
should linesFilled count only full lines or also partial lines? does it depend on StringFormatFlagsLineLimit?

akoeplinger · 2018-03-31T01:12:00Z

@filipnavara I'd definitely be in favor of splitting the no-brainer changes into separate PRs so we get some of it finally merged :)

filipnavara · 2018-03-31T01:14:31Z

I will submit the PRs in next few days.

filipnavara · 2018-04-01T11:32:25Z

I fixed up the coordinate part of this patch in PR #275. Tested with "notepad" in Mono repository and unit tests. That leaves the index_matching part.

hughbe · 2018-07-26T22:54:29Z

Where are with here? Would be awesome to get this merged in :D

akoeplinger · 2018-07-27T12:04:06Z

I won't have time to work on this so whoever wants to take over and drive this forward please do :)

filipnavara · 2018-07-27T12:17:43Z

@hughbe Basically everything but the cairo_MeasureCharacterRanges function was already merged in one way or another. I would prefer if that part of patch was rewritten to use the charsRemoved approach from gdip_process_string in text-pango.c. Now it's doing twice the same processing, which is non-transparent and bound to break with any change. The maintenance cost of this code is already high, so anything that could converge towards the Pango code would be nice.

migueldeicaza · 2018-09-26T19:38:31Z

This needs to be rebased.

filipnavara · 2018-09-26T19:41:46Z

@migueldeicaza Thanks for taking care of the pull requests. This one needs more than rebasing, but currently no one is working on that. 2/3 of the patch are already rewritten and merged.

akoeplinger · 2019-10-03T20:05:39Z

Talked with @filipnavara and at this point there's no value in the actual code change anymore. We'll open an issue instead.

akoeplinger mentioned this pull request Nov 24, 2017

Various fixes #28

Closed

hughbe approved these changes Nov 24, 2017

View reviewed changes

Allocate index_matching array with malloc

5ad77c2

Using array initializer with a variable is a gnu extensions and doesn't work on MSVC.

Free index_matching

b0881d5

filipnavara reviewed Mar 29, 2018

View reviewed changes

This was referenced Apr 1, 2018

RFC: Support Pango text rendering by default #269

Merged

Initial test and fixes for GdipMeasureCharacterRanges #275

Merged

Merge branch 'master' into pr-28-cleanup-cairo

4ceae55

akoeplinger added the help-wanted label Aug 6, 2019

akoeplinger closed this Oct 3, 2019

akoeplinger deleted the pr-28-cleanup-cairo branch October 3, 2019 20:05

filipnavara mentioned this pull request Jul 29, 2020

Bug with MeasureCharacterRanges on Linux, if text contains LF/CR #663

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

akoeplinger commented Nov 24, 2017

akoeplinger commented Nov 24, 2017

hughbe commented Nov 24, 2017

hughbe Nov 24, 2017

filipnavara Feb 26, 2018

hughbe commented Nov 24, 2017

akoeplinger commented Nov 24, 2017

hughbe commented Dec 15, 2017

akoeplinger commented Dec 17, 2017

PreferLinux commented Dec 21, 2017

filipnavara Mar 29, 2018

PreferLinux Mar 29, 2018

filipnavara Mar 29, 2018

PreferLinux Mar 29, 2018

filipnavara Mar 29, 2018

filipnavara commented Mar 30, 2018 •

edited

Loading

PreferLinux commented Mar 30, 2018

filipnavara commented Mar 30, 2018 •

edited

Loading

akoeplinger commented Mar 31, 2018

filipnavara commented Mar 31, 2018

filipnavara commented Apr 1, 2018

hughbe commented Jul 26, 2018

akoeplinger commented Jul 27, 2018

filipnavara commented Jul 27, 2018

migueldeicaza commented Sep 26, 2018

filipnavara commented Sep 26, 2018

akoeplinger commented Oct 3, 2019

Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

Corrections to MeasureCharacterRanges, improvements to DrawString / MeasureString #137

Conversation

akoeplinger commented Nov 24, 2017

akoeplinger commented Nov 24, 2017

hughbe commented Nov 24, 2017

hughbe Nov 24, 2017

Choose a reason for hiding this comment

filipnavara Feb 26, 2018

Choose a reason for hiding this comment

hughbe commented Nov 24, 2017

akoeplinger commented Nov 24, 2017

hughbe commented Dec 15, 2017

akoeplinger commented Dec 17, 2017

PreferLinux commented Dec 21, 2017

filipnavara Mar 29, 2018

Choose a reason for hiding this comment

PreferLinux Mar 29, 2018

Choose a reason for hiding this comment

filipnavara Mar 29, 2018

Choose a reason for hiding this comment

PreferLinux Mar 29, 2018

Choose a reason for hiding this comment

filipnavara Mar 29, 2018

Choose a reason for hiding this comment

filipnavara commented Mar 30, 2018 • edited Loading

PreferLinux commented Mar 30, 2018

filipnavara commented Mar 30, 2018 • edited Loading

akoeplinger commented Mar 31, 2018

filipnavara commented Mar 31, 2018

filipnavara commented Apr 1, 2018

hughbe commented Jul 26, 2018

akoeplinger commented Jul 27, 2018

filipnavara commented Jul 27, 2018

migueldeicaza commented Sep 26, 2018

filipnavara commented Sep 26, 2018

akoeplinger commented Oct 3, 2019

filipnavara commented Mar 30, 2018 •

edited

Loading

filipnavara commented Mar 30, 2018 •

edited

Loading