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();