Skip to content

[Win32] Let GDI lay out text again unless GDI+ layout is required #3579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
HeikoKlare wants to merge 1 commit into
base: master
Choose a base branch
from
+133 −49
Draft

[Win32] Let GDI lay out text again unless GDI+ layout is required #3579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 57 additions & 7 deletions bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/GCWin32Tests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
* <p>
* 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);

Expand Down Expand Up @@ -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.
* <p>
* 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
Expand Down
45 changes: 32 additions & 13 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
73 changes: 44 additions & 29 deletions ...2/ManualTests/org/eclipse/swt/tests/win32/snippets/SWTIssue3091_GDIPlusTextRendering.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
* <p>
* 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.
* <p>
* 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:
* <ul>
* <li>"Use GDI+ (advanced) rendering" calls GC.setAdvanced() and re-renders
* every row, switching between plain GDI and GDI+.</li>
* <li>"Use legacy GDI text rendering" toggles the
* org.eclipse.swt.internal.win32.useGDITextRenderingWithGDIP system property,
* restoring the previous behavior of having GDI compute the glyph positions.
* It only matters while GDI+/advanced rendering is enabled and is disabled
* otherwise.</li>
* <li>"Use legacy GDI text rendering for decorated fonts" toggles the
* org.eclipse.swt.internal.win32.useGDITextRenderingForDecoratedFonts system
* property, restoring the previous behavior of having GDI compute the glyph
* positions for underlined and strikeout fonts as well, which makes them
* render blank. It only matters while GDI+/advanced rendering is enabled and
* is disabled otherwise.</li>
* </ul>
*
* The rows labelled "unsupported glyph (U+FFFE)" append U+FFFE, a Unicode
Expand All @@ -65,6 +74,10 @@
* combination.</li>
* <li> "Mnemonic" shows an underlined "F" in every combination (it does not
* depend on font-level decoration).</li>
* <li> 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.</li>
* <li> "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.</li>
Expand All @@ -73,7 +86,8 @@
* combination.</li>
* <li> "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.</li>
* for decorated fonts enabled, unless U+FFFE forces GDI+'s own text
* layout.</li>
* </ul>
*
* On platforms other than Windows, GC.setAdvanced() does not select a
Expand All @@ -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,
Expand All @@ -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<TextRow, Image> renderSamples(Display display, java.util.List<TextRow> rows,
Map<TextRow, Font> fonts, int sampleWidth, int sampleHeight, boolean advanced) {
Expand Down Expand Up @@ -133,23 +147,23 @@ public static void main(String[] args) {
String unsupportedGlyph = String.valueOf((char) 0xFFFE);

java.util.List<TextRow> 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"));
Expand All @@ -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.
Expand Down Expand Up @@ -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<TextRow, Image> 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();
Expand Down
Loading