[PATCH v4 0/1] MR3407: gdiplus: Disable StringFormatFlagsLineLimit

[David Kahurani (＠kahurani)]

Builtin gdiplus behaves as documented but tests are indicating that Windows behaviour sometimes differs from the documentation.

I believe this issue is at least one of the contributing issues to bug https://bugs.winehq.org/show_bug.cgi?id=46947, if not the sole issue. I propose that this flag be disabled until it is well understood. I noticed as issue with DrawString seeming to ignore the flag sometimes(sometimes the text fits without any noticeable defects which I believe is the case with the program in the bug) and sometimes even drawing clipped text with the flag on. Other times it works as expected. I can't seem to find a pattern to identify that could be used to predict its behaviour.

Signed-off-by: David Kahurani [e-mail]

--
  v4: gdiplus: Fix StringFormatFlagsLineLimit handling

https://gitlab.winehq.org/wine/wine/-/merge_requests/3407

[PATCH v4 1/1] gdiplus: Fix StringFormatFlagsLineLimit handling

[David Kahurani]

From: David Kahurani

Always write the first line regardless of the whether it fits but
don't write any more if there's not enough room.

Signed-off-by: David Kahurani 
---
 dlls/gdiplus/graphics.c           | 18 +++++++++--
 dlls/gdiplus/tests/stringformat.c | 51 +++++++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c
index 6dc34707bbf..647e2b03df4 100644
--- a/dlls/gdiplus/graphics.c
+++ b/dlls/gdiplus/graphics.c
@@ -5309,8 +5309,9 @@ GpStatus gdip_format_string(HDC hdc,
         if(height + size.cy > nheight)
         {
             if (format->attr & StringFormatFlagsLineLimit)
-                break;
-            bounds.Height = nheight - (height + size.cy);
+                bounds.Height = nheight;
+            else
+                bounds.Height = nheight - (height + size.cy);
         }
         else
             bounds.Height = size.cy;
@@ -5343,6 +5344,8 @@ GpStatus gdip_format_string(HDC hdc,
         if (stat != Ok)
             break;

+        if ((format->attr & StringFormatFlagsLineLimit) && height + size.cy > nheight)
+            break;

if (unixstyle_newline)
         {
@@ -5537,7 +5540,16 @@ static GpStatus measure_string_callback(HDC hdc,
         *args->codepointsfitted = index + length;

if (args->linesfilled)
-        (*args->linesfilled)++;
+    {
+        SIZE size;
+        int fit;
+
+        GetTextExtentExPointW(hdc, string, length,
+                              bounds->Width, &fit, NULL, &size);
+
+        if (bounds->Height >= size.cy)
+            (*args->linesfilled)++;
+    }

return Ok;
 }
diff --git a/dlls/gdiplus/tests/stringformat.c b/dlls/gdiplus/tests/stringformat.c
index 71fb860e474..938fbf817c6 100644
--- a/dlls/gdiplus/tests/stringformat.c
+++ b/dlls/gdiplus/tests/stringformat.c
@@ -373,12 +373,26 @@ static void test_getgenericdefault(void)
     expect(Ok, stat);
 }

+void set_rect_empty(RectF *rect)
+{
+     rect->X = 0;
+     rect->Y = 0;
+     rect->Width = 0;
+     rect->Height = 0;
+}
+
 static void test_stringformatflags(void)
 {
     GpStringFormat *format;
     GpStatus stat;
-
-    INT flags;
+    HDC hdc;
+    GpGraphics *graphics;
+    LOGFONTA lf;
+    GpFont *font;
+    RectF rect, bounds;
+    REAL height;
+    const WCHAR *string = L"Hello";
+    INT flags, linesfilled;

stat = GdipCreateStringFormat(0, LANG_NEUTRAL, &format);
     expect(Ok, stat);
@@ -430,6 +444,39 @@ static void test_stringformatflags(void)
     expect(Ok, stat);
     expect(0xdeadbeef, flags);

+    hdc = CreateCompatibleDC(0);
+    GdipCreateFromHDC(hdc, &graphics);
+    memset(&lf, 0, sizeof(lf));
+
+    lstrcpyA(lf.lfFaceName, "Tahoma");
+    lf.lfHeight = -100;
+    stat = GdipCreateFontFromLogfontA(hdc, &lf, &font);
+    expect(Ok, stat);
+    stat = GdipGetFontHeight(font, graphics, &height);
+    stat = GdipSetStringFormatFlags(format, StringFormatFlagsLineLimit);
+    expect(Ok, stat);
+    stat = GdipGetStringFormatFlags(format, &flags);
+    expect(Ok, stat);
+    expect(StringFormatFlagsLineLimit, flags);
+    /* Get an estimate of line height */
+    set_rect_empty(&rect);
+    set_rect_empty(&bounds);
+    stat = GdipMeasureString(graphics, string, 5, font, &rect, format, &bounds, NULL, NULL);
+    expect(Ok, stat);
+
+    /* request to draw in a shallow rectangle with half the line height */
+    rect.Height = (int)bounds.Height / 2;
+    rect.Width = 600;
+    set_rect_empty(&bounds);
+    stat = GdipMeasureString(graphics, string, 5, font, &rect, format, &bounds, NULL, &linesfilled);
+    expect(Ok, stat);
+    expect(0, linesfilled);
+    expectf(rect.Height, bounds.Height);
+
+    DeleteObject(hdc);
+    GdipDeleteGraphics(graphics);
+    GdipDeleteFont(font);
+
     stat = GdipDeleteStringFormat(format);
     expect(Ok, stat);
 }

[PATCH v4 1/1] gdiplus: Fix StringFormatFlagsLineLimit handling

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=135284

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w7u_adm (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w7u_el (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w8 (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w8adm (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w864 (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064v1507 (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064v1809 (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064_tsign (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w10pro64 (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w11pro64 (32 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w7pro64 (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w864 (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064v1507 (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064v1809 (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064_2qxl (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064_adm (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w1064_tsign (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w10pro64 (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w10pro64_en_AE_u8 (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w10pro64_ar (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w10pro64_ja (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w10pro64_zh_CN (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== w11pro64_amd (64 bit report) ===

gdiplus:
stringformat.c:473: Test failed: Expected 00000000, got 00000001

=== debian11 (32 bit report) ===

gdiplus:
graphics.c:4504: Test failed: Expected 1, got 0
graphics.c:4507: Test failed: Expected 40.666668, got 34.000000
graphics.c:4623: Test failed: Expected 1, got 0
graphics.c:4626: Test failed: Expected 40.666668, got 34.000000

=== debian11b (64 bit WoW report) ===

gdiplus:
graphics.c:4504: Test failed: Expected 1, got 0
graphics.c:4507: Test failed: Expected 40.666668, got 34.000000
graphics.c:4623: Test failed: Expected 1, got 0
graphics.c:4626: Test failed: Expected 40.666668, got 34.000000

[David Kahurani (＠kahurani)]

Alright, I updated this.

[Esme Povirk (＠madewokherd)]

I'm still not convinced by this, so I took an old test program and modified it a little bit: [measuretext.c](/uploads/65d666c29788a2f9451b0cc7bc79dfbe/measuretext.c) [measuretext.exe](/uploads/6e98b643b1b984264fc00b201301c57c/measuretext.exe)

It seems that on Windows, LineLimit does what msdn says, but there's a little bit of a gap between the height that MeasureCharacterRanges reports and the height that MeasureString reports. If the size is inside that gap, MeasureString will report the height it's given, and LineLimit will not prevent the last line from being drawn.

It seems like fixing this will require gdip_format_string to have a little bit more awareness of the font metrics.

[David Kahurani (＠kahurani)]

I am assuming this is in addition to always drawing the first line.(??)

[David Kahurani (＠kahurani)]

The "unpredictable" behaviour I was talking about was observed when using different font sizes but now that you mentioned font metrics, we might be talking about the same behaviour.

[PATCH v4 0/1] MR3407: gdiplus: Fix StringFormatFlagsLineLimit handling

[Esme Povirk (＠madewokherd)]

I was wrong about it always drawing the first line.

[PATCH v4 0/1] MR3407: gdiplus: Fix StringFormatFlagsLineLimit handling

[David Kahurani (＠kahurani)]

Hi @madewokherd

It doesn't look like there's a concrete definition of a line. It seems like the general consensus is the font height so it doesn't look like we're going to get any further in determining a more precise line height going by conventional methods in Wine. It also doesn't look like Windows relies on font height either because, `TEXTMETRICS.tmHeight` is a long value but GdipMeasure*() methods always return real floating point values(with fractional parts). On top of that `tmHeight`  and `GetTextExtentExPointW` return the same height which is the height of the font.

To me it looks like the only way around this would be to analyze glyph outlines(GetGlyphOutlineW) which is probably expensive. I don't know whether there could be another approach, though.

What do you think?

[PATCH v4 0/1] MR3407: gdiplus: Fix StringFormatFlagsLineLimit handling

[Esme Povirk (＠madewokherd)]

Native GDI+ goes to great pains to make it look like fonts can be scaled without any distortion, so what you are seeing may be font metrics scaled from the font's em size to the size of the font you're drawing. I doubt it's based on the font geometry.

We don't do this because it's a lot of work to make the fonts look worse, as native GDI+ has to mess with character spacing a lot to achieve this.

It could be that the "gap" I'm seeing between MeasureString and MeasureCharacterRanges is "wiggle room" added by GDI+ to make it more likely that the text can fit in a scaled rectangle.