From c78dab0afe8ad4c8455f60252805a0c18d392b6e Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 7 Sep 2026 20:28:22 +0200 Subject: [PATCH] [Win32] Let GDI lay out text again unless GDI+ layout is required Since GDI+ text layout became the default for advanced GCs, glyph advances are derived from the font's unhinted design metrics instead of the hinted, grid-fitted advances the platform uses everywhere else, i.e. in native controls, in TextLayout and in a non-advanced GC. The difference is a fraction of a pixel per glyph and mostly averages out across proportional text, which is why it went unnoticed. It does not average out for digits. A font's figures are typically tabular, so all ten share a single advance and therefore a single error that repeats with the same sign for every digit of a group. The excess accumulates and, since glyphs are still rasterized grid-fitted, is distributed unevenly across the gaps, which reads as broken tracking. Measured against a non-advanced GC across the test fonts and sizes, a 20-digit string came out up to 8% too wide; it is now within the rounding of the two extents. This change therefore restores GDI as the text layout engine and limits GDI+'s own layout to the cases where the glyph run cannot be drawn at all: text containing characters GDI has no glyph for, and fonts with an underline or strikeout style. The latter is the actual cause behind https://github.com/eclipse-platform/eclipse.platform.swt/issues/3091 : Graphics_DrawDriverString does not support font decoration and draws blank space instead of the glyphs, which is a documented GDI+ limitation whose recommended remedy is to use Graphics_DrawString for those fonts. Both cases still draw with GDI+, so this only selects which engine performs the layout, and decorated text keeps rendering exactly as it does today. Narrow the escape hatch introduced along with the GDI+ layout default accordingly, and rename it to reflect its remaining scope: it now only reverts the decorated-font case to the GDI glyph run, at the price of that text rendering blank again. It remains an internal safety net that may be removed at any point in time. Include SWT.DRAW_TAB in the condition deciding whether the glyph run path has to measure a segment. Placing the text after a tab consumes the bounds of the segment before it, so omitting the flag made tab-expanded text fail with a NullPointerException. The defect was latent while GDI+ performed the layout for every string. Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/3577 Assisted-by: Claude Opus 5 --- .../eclipse/swt/graphics/GCWin32Tests.java | 64 ++++++++++++++-- .../win32/org/eclipse/swt/graphics/GC.java | 45 ++++++++---- .../SWTIssue3091_GDIPlusTextRendering.java | 73 +++++++++++-------- 3 files changed, 133 insertions(+), 49 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/GCWin32Tests.java b/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/GCWin32Tests.java index f6a76fd3d09..23f3869450f 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/GCWin32Tests.java +++ b/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/GCWin32Tests.java @@ -154,13 +154,8 @@ private static int renderTextAndCountNonWhitePixels(Image target, Font font, Str /** * U+FFFE is a Unicode non-character that no standard font has a glyph for. * Appending it to a string makes an advanced GC lay that string out with - * GDI+ instead of letting GDI compute the glyph positions. - *

- * Since GDI+ text layout became the default for advanced GCs, this is no - * longer strictly required. It is kept deliberately so that the tab stop - * tests exercise the GDI+ layout path irrespective of the state of the - * {@code useGDITextRenderingWithGDIP} system property, which exists to - * switch back to GDI-computed glyph positions. + * GDI+ instead of letting GDI compute the glyph positions, which is how the + * tab stop tests reach the GDI+ layout path. */ private static final String UNSUPPORTED_GLYPH = String.valueOf((char) 0xFFFE); @@ -556,6 +551,61 @@ public void drawTextKerningSensitiveTextWidthIsComparableToGdi() { } } + /** + * Verifies that an advanced GC advances digits exactly like a plain, + * non-advanced GC does, which serves as the reference. + *

+ * Digits are the most sensitive probe for a layout engine's glyph advances: + * a font's figures usually all share a single advance, so a per-glyph error + * does not average out over a string but accumulates in one direction and + * shows up as visibly irregular gaps. Unlike the tolerant comparisons for + * proportional text, this is therefore asserted exactly, up to the rounding + * of the two extents. + */ + @ParameterizedTest + @MethodSource("tabStopTestFonts") + public void drawTextDigitAdvancesMatchGdi(String fontName) { + Display display = Display.getDefault(); + Image image = new Image(display, 600, 60); + String digits = "01234567890123456789"; + try { + for (int size : new int[] { 9, 12, 16 }) { + Font font = new Font(display, fontName, size, SWT.NORMAL); + try { + int advancedWidth = withGC(image, font, true, gc -> gc.textExtent(digits, SWT.NONE).x); + int gdiWidth = withGC(image, font, false, gc -> gc.textExtent(digits, SWT.NONE).x); + assertWithinRoundingTolerance(gdiWidth, advancedWidth, + "an advanced GC must advance digits like a non-advanced one for font " + fontName + + " at " + size + "pt"); + } finally { + font.dispose(); + } + } + } finally { + image.dispose(); + } + } + + /** + * Verifies that an advanced GC can draw tab-expanded text. Placing the + * segment after a tab requires the bounds of the segment before it, which + * the glyph-run based text rendering only computes on demand. + */ + @Test + public void drawTextWithTabsRendersVisibleInk() { + Display display = Display.getDefault(); + Font font = display.getSystemFont(); + Image image = new Image(display, 300, 60); + try { + int renderedPixels = renderTextAndCountNonWhitePixels(image, font, "A\tB\tC", + SWT.DRAW_TAB | SWT.DRAW_TRANSPARENT, SWT.NONE, true); + + assertTrue(renderedPixels > 0, "an advanced GC must draw visible ink for tab-expanded text"); + } finally { + image.dispose(); + } + } + /** * Asserts that {@code actual} is within {@code (1 +/- tolerance)} times * {@code expected}, i.e. flags gross deviations (roughly halved/doubled or diff --git a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java index 45414f64354..35818b25058 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java @@ -110,23 +110,26 @@ public final class GC extends Resource { static final float[] LINE_DASHDOT_ZERO = new float[]{9, 6, 3, 6}; static final float[] LINE_DASHDOTDOT_ZERO = new float[]{9, 3, 3, 3, 3, 3}; - private static final String USE_GDI_TEXT_RENDERING_WITH_GDIP = "org.eclipse.swt.internal.win32.useGDITextRenderingWithGDIP"; + private static final String USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS = "org.eclipse.swt.internal.win32.useGDITextRenderingForDecoratedFonts"; /** - * Whether text is laid out by GDI and only drawn by GDI+, instead of being - * laid out by GDI+ itself, which restores the behavior that was in place - * before GDI+ text layout became the default. + * Whether text in a font with an underline or strikeout style is laid out by + * GDI and only drawn by GDI+, instead of being laid out by GDI+ itself, + * which restores the behavior that was in place before GDI+ started to lay + * out such text. * * This is only a safety net for unexpected text rendering regressions, so * that consumers can fall back to the previous behavior instead of having - * to downgrade SWT. It may be removed at any point in time and must not be - * relied upon. + * to downgrade SWT. Note that the previous behavior draws no glyphs at all + * for decorated fonts, see + * https://github.com/eclipse-platform/eclipse.platform.swt/issues/3091 . + * It may be removed at any point in time and must not be relied upon. * * Evaluated once per GC rather than per drawing operation, so that reading * the system property does not add cost to text drawing, while a newly * created GC still picks up a value changed at runtime. */ - private final boolean useGdiTextLayoutWithGdip = Boolean.getBoolean(USE_GDI_TEXT_RENDERING_WITH_GDIP); + private final boolean useGdiTextLayoutForDecoratedFonts = Boolean.getBoolean(USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS); /** * Prevents uninitialized instances from being created outside the package. @@ -2878,13 +2881,26 @@ private void drawTextInPixels (String string, int x, int y, int flags) { * by GDI+ (Graphics_DrawDriverString). Note that both cases draw with GDI+, * so this only selects which engine performs the layout. * - * Unless the legacy GDI text layout is requested, GDI+ always lays out the - * text itself and the glyph inspection below is not reached. Both are to be - * removed together with the fallback. + * GDI is preferred, because it uses the hinted, grid-fitted glyph advances + * that the platform itself uses everywhere else (native controls, TextLayout + * and the non-advanced GC), whereas GDI+ lays out from unhinted font design + * metrics. The latter accumulates a sub-pixel error per glyph that is most + * apparent for tabular figures, where every digit shares the same advance and + * hence the same error, so that digit groups visibly spread apart. + * + * GDI+ layout is therefore only used where the glyph run cannot be drawn: + * when GDI cannot map all characters to glyphs, in which case GDI would draw + * missing-glyph boxes, and for fonts carrying an underline or strikeout style, + * which Graphics_DrawDriverString does not support and for which it draws + * blank space instead of the glyphs, see + * https://github.com/eclipse-platform/eclipse.platform.swt/issues/3091 . */ private boolean useGdipTextLayout(long hdc, char[] buffer) { - if (!useGdiTextLayoutWithGdip) { - return true; + if (!useGdiTextLayoutForDecoratedFonts) { + int fontStyle = Gdip.Font_GetStyle(data.gdipFont); + if ((fontStyle & (Gdip.FontStyleUnderline | Gdip.FontStyleStrikeout)) != 0) { + return true; + } } short[] glyphs = new short[buffer.length]; OS.GetGlyphIndices(hdc, buffer, buffer.length, glyphs, OS.GGI_MARK_NONEXISTING_GLYPHS); @@ -2975,7 +2991,10 @@ void drawText(long gdipGraphics, String string, int x, int y, int flags, Point s private RectF drawText(long gdipGraphics, char[] buffer, int start, int length, int x, int y, int flags, int mnemonicIndex, TEXTMETRIC lptm, boolean draw) { boolean drawMnemonic = draw && mnemonicIndex != -1 && (data.uiState & OS.UISF_HIDEACCEL) == 0; - boolean needsBounds = !draw || drawMnemonic || (flags & SWT.DRAW_TRANSPARENT) == 0 || (data.style & SWT.MIRRORED) != 0 || (flags & SWT.DRAW_DELIMITER) != 0; + // Tabs and delimiters need the bounds of the preceding segment to place the + // following one, so they require bounds just like the cases that consume + // them for drawing. + boolean needsBounds = !draw || drawMnemonic || (flags & SWT.DRAW_TRANSPARENT) == 0 || (data.style & SWT.MIRRORED) != 0 || (flags & (SWT.DRAW_DELIMITER | SWT.DRAW_TAB)) != 0; if (length <= 0) { RectF bounds = null; if (needsBounds) { diff --git a/tests/org.eclipse.swt.tests.win32/ManualTests/org/eclipse/swt/tests/win32/snippets/SWTIssue3091_GDIPlusTextRendering.java b/tests/org.eclipse.swt.tests.win32/ManualTests/org/eclipse/swt/tests/win32/snippets/SWTIssue3091_GDIPlusTextRendering.java index c772ebe56e6..a87c36c08bb 100644 --- a/tests/org.eclipse.swt.tests.win32/ManualTests/org/eclipse/swt/tests/win32/snippets/SWTIssue3091_GDIPlusTextRendering.java +++ b/tests/org.eclipse.swt.tests.win32/ManualTests/org/eclipse/swt/tests/win32/snippets/SWTIssue3091_GDIPlusTextRendering.java @@ -39,17 +39,26 @@ * independently, so kerning, tab stop width, mnemonic underlining and * bidi/mirroring can all come out differently depending on which one draws. *

+ * Even with GDI+, the glyph positions are normally still computed by GDI, + * because GDI uses hinted glyph advances that match what the platform does + * everywhere else, whereas GDI+'s own layout works from unhinted font design + * metrics and spreads text apart by a fraction of a pixel per glyph. GDI+ lays + * out the text itself only where its glyph run drawing cannot be used: for + * strings containing characters GDI has no glyph for, and for fonts with an + * underline or strikeout style, which GDI+ cannot draw as a glyph run. + *

* This snippet renders a series of text properties, one row per property, and * lets the rendering path be switched at runtime, so that the results can be * compared visually without restarting the process: *

* * The rows labelled "unsupported glyph (U+FFFE)" append U+FFFE, a Unicode @@ -65,6 +74,10 @@ * combination. *
  • "Mnemonic" shows an underlined "F" in every combination (it does not * depend on font-level decoration).
    • + *
  • Digits must keep an even, tight spacing in every row that contains them. + * Digits are the most sensitive to layout differences, because a font's + * figures usually all share one advance, so any per-glyph error repeats + * identically and becomes visible as irregular gaps.
    • *
  • "Kerning pair", the tab rows and "Mirrored / RTL" may differ slightly in * spacing/positioning between the engines, but should never render blank, * wildly stretched/compressed, or with overlapping glyphs.
    • @@ -73,7 +86,8 @@ * combination. *
  • "Underlined"/"Strikeout"/"Bold + underlined" render their decoration with * plain GDI and with GDI+, and go blank only with legacy GDI text rendering - * enabled, unless U+FFFE forces GDI+'s own text layout.
    • + * for decorated fonts enabled, unless U+FFFE forces GDI+'s own text + * layout. * * * On platforms other than Windows, GC.setAdvanced() does not select a @@ -84,8 +98,8 @@ */ public class SWTIssue3091_GDIPlusTextRendering { - static final String USE_GDI_TEXT_RENDERING_WITH_GDIP_PROPERTY = - "org.eclipse.swt.internal.win32.useGDITextRenderingWithGDIP"; + static final String USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS_PROPERTY = + "org.eclipse.swt.internal.win32.useGDITextRenderingForDecoratedFonts"; /** One row of the comparison: a label, the text properties to apply, and how to draw it. */ record TextRow(String label, int fontStyle, boolean underline, boolean strikeout, String text, int drawFlags, @@ -99,9 +113,9 @@ record TextRow(String label, int fontStyle, boolean underline, boolean strikeout * Renders one fresh sample {@link Image} per row, either with plain GDI * ({@code advanced == false}) or with GDI+ ({@code advanced == true}), in * the latter case reflecting whatever the - * {@link #USE_GDI_TEXT_RENDERING_WITH_GDIP_PROPERTY} system property is set - * to right now. Callers are responsible for disposing the previous set of - * images returned by an earlier call. + * {@link #USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS_PROPERTY} system + * property is set to right now. Callers are responsible for disposing the + * previous set of images returned by an earlier call. */ private static Map renderSamples(Display display, java.util.List rows, Map fonts, int sampleWidth, int sampleHeight, boolean advanced) { @@ -133,23 +147,23 @@ public static void main(String[] args) { String unsupportedGlyph = String.valueOf((char) 0xFFFE); java.util.List rows = new ArrayList<>(); - rows.add(new TextRow("Plain text", "Hello World")); - rows.add(new TextRow("Bold", SWT.BOLD, false, false, "Hello World", SWT.DRAW_TRANSPARENT, SWT.NONE)); - rows.add(new TextRow("Italic", SWT.ITALIC, false, false, "Hello World", SWT.DRAW_TRANSPARENT, SWT.NONE)); - rows.add(new TextRow("Underlined", SWT.NORMAL, true, false, "Hello World", SWT.DRAW_TRANSPARENT, SWT.NONE)); - rows.add(new TextRow("Strikeout", SWT.NORMAL, false, true, "Hello World", SWT.DRAW_TRANSPARENT, SWT.NONE)); - rows.add(new TextRow("Bold + underlined", SWT.BOLD, true, false, "Hello World", SWT.DRAW_TRANSPARENT, + rows.add(new TextRow("Plain text", "Hello World 12345")); + rows.add(new TextRow("Bold", SWT.BOLD, false, false, "Hello World 12345", SWT.DRAW_TRANSPARENT, SWT.NONE)); + rows.add(new TextRow("Italic", SWT.ITALIC, false, false, "Hello World 12345", SWT.DRAW_TRANSPARENT, SWT.NONE)); + rows.add(new TextRow("Underlined", SWT.NORMAL, true, false, "Hello World 12345", SWT.DRAW_TRANSPARENT, SWT.NONE)); + rows.add(new TextRow("Strikeout", SWT.NORMAL, false, true, "Hello World 12345", SWT.DRAW_TRANSPARENT, SWT.NONE)); + rows.add(new TextRow("Bold + underlined", SWT.BOLD, true, false, "Hello World 12345", SWT.DRAW_TRANSPARENT, SWT.NONE)); rows.add(new TextRow("Underlined + unsupported glyph (U+FFFE)", SWT.NORMAL, true, false, - "Hi" + unsupportedGlyph, SWT.DRAW_TRANSPARENT, SWT.NONE)); + "Hi 12345" + unsupportedGlyph, SWT.DRAW_TRANSPARENT, SWT.NONE)); rows.add(new TextRow("Mnemonic (accelerator underline)", SWT.NORMAL, false, false, "&File", SWT.DRAW_MNEMONIC | SWT.DRAW_TRANSPARENT, SWT.NONE)); - rows.add(new TextRow("Tab-separated columns", SWT.NORMAL, false, false, "A\tB\tC", SWT.DRAW_TAB, + rows.add(new TextRow("Tab-separated columns", SWT.NORMAL, false, false, "A1\tB2\tC3", SWT.DRAW_TAB, SWT.NONE)); rows.add(new TextRow("Tab-separated columns + unsupported glyph (U+FFFE)", SWT.NORMAL, false, false, - "A\tB\tC" + unsupportedGlyph, SWT.DRAW_TAB, SWT.NONE)); - rows.add(new TextRow("Kerning pair", "AVATAR WAVE To Yes")); - rows.add(new TextRow("Mirrored / RTL", SWT.NORMAL, false, false, "Hello World", SWT.DRAW_TRANSPARENT, + "A1\tB2\tC3" + unsupportedGlyph, SWT.DRAW_TAB, SWT.NONE)); + rows.add(new TextRow("Kerning pair", "AVATAR WAVE To Yes 12345")); + rows.add(new TextRow("Mirrored / RTL", SWT.NORMAL, false, false, "Hello World 12345", SWT.DRAW_TRANSPARENT, SWT.RIGHT_TO_LEFT)); rows.add(new TextRow("Arabic", "\u0645\u0631\u062d\u0628\u0627 \u0628\u0627\u0644\u0639\u0627\u0644\u0645")); rows.add(new TextRow("Hebrew", "\u05e9\u05dc\u05d5\u05dd \u05e2\u05d5\u05dc\u05dd")); @@ -173,11 +187,12 @@ public static void main(String[] args) { advancedCheckbox.setLayoutData(new GridData(SWT.FILL, SWT.CENTER, true, false)); Button legacyCheckbox = new Button(shell, SWT.CHECK | SWT.WRAP); - legacyCheckbox.setText("Use legacy GDI text rendering (org.eclipse.swt.internal.win32." - + "useGDITextRenderingWithGDIP = true) - historical, pre-#3091-fix behavior." + legacyCheckbox.setText("Use legacy GDI text rendering for decorated fonts (org.eclipse.swt.internal.win32." + + "useGDITextRenderingForDecoratedFonts = true) - historical, pre-#3091-fix behavior, which draws" + + " underlined and strikeout text blank." + "\nThe legacy fallback is only an escape hatch for the transition to GDI+ text rendering and is to" + " be removed in a future release, at which point this checkbox will have no effect anymore."); - legacyCheckbox.setSelection(Boolean.getBoolean(USE_GDI_TEXT_RENDERING_WITH_GDIP_PROPERTY)); + legacyCheckbox.setSelection(Boolean.getBoolean(USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS_PROPERTY)); legacyCheckbox.setLayoutData(new GridData(SWT.FILL, SWT.CENTER, true, false)); // The property only ever affects the advanced/GDI+ code path, so the // checkbox is meaningless (and disabled) while advanced rendering is off. @@ -217,18 +232,18 @@ public static void main(String[] args) { boolean advanced = advancedCheckbox.getSelection(); legacyCheckbox.setEnabled(advanced); boolean legacyFallback = advanced && legacyCheckbox.getSelection(); - System.setProperty(USE_GDI_TEXT_RENDERING_WITH_GDIP_PROPERTY, Boolean.toString(legacyFallback)); + System.setProperty(USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS_PROPERTY, Boolean.toString(legacyFallback)); Map old = samplesHolder.getAndSet( renderSamples(display, rows, fonts, sampleWidth, sampleHeight, advanced)); old.values().forEach(Image::dispose); shell.setText("Text rendering comparison (advanced = " + advanced - + ", legacy GDI text rendering = " + legacyFallback + ")"); + + ", legacy GDI text rendering for decorated fonts = " + legacyFallback + ")"); info.setText("GC.setAdvanced(" + advanced + "); system property " - + USE_GDI_TEXT_RENDERING_WITH_GDIP_PROPERTY + " = " + legacyFallback + + USE_GDI_TEXT_RENDERING_FOR_DECORATED_FONTS_PROPERTY + " = " + legacyFallback + "\nToggle the checkboxes below to compare plain GDI vs. GDI+ text rendering, and - while" - + " GDI+ is enabled - the default vs. the legacy text rendering path." + + " GDI+ is enabled - the default vs. the legacy text rendering path for decorated fonts." + " See the source comment for what to expect per row."); shell.layout(true, true); canvas.redraw();