[Win32] Remove obsolete fallback for drawing text without GDI+ - #3100
[Win32] Remove obsolete fallback for drawing text without GDI+#3100HeikoKlare wants to merge 2 commits into
Conversation
|
Interestingly, this change would also have an impact on some issues reported to GEF (@ptziegler fyi). It seems like the following fix would be come obsolete as I see the behavior also fixed with this PR: Without this change on 150% monitor: With this change on 150% monitor: For the underlying issue and the snippet posted there, I rather see a change that I would consider a regression caused by this PR: This is how it looks without this change on 100%: And this is how it looks with this change on 100%: But comparing the position of the text on 100% monitor with its position on 150% monitor, the behavior after this change seems to be even more "correct". Anyway, it seems like the Win32 |
4ccca1a to
6ff4768
Compare
20e6c08 to
fbd9f98
Compare
5835e66 to
7269ed0
Compare
|
Testing approach and additional findings To validate this change more thoroughly and guard against regressions, I (with assistance from GitHub Copilot) added:
Finding: tab stop size regression While building out this coverage, we found that the tab stop width computed by the GDI+ We fixed this by having Note: This tab-stop discrepancy appears to be a pre-existing bug in the GDI+ text rendering path (unrelated to the underline/strikeout issue this PR addresses) that this PR's change to always use that path simply made visible/impactful by default. It's probably worth splitting the tab-stop fix out into its own separate PR, since it fixes an independent, pre-existing defect rather than something introduced by this change - happy to do that if preferred. 
|
b4d797e to
8882328
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates SWT’s Win32 GC.drawText() behavior in advanced mode to rely on GDI+ for text layout by default (fixing cases where decorated fonts could render no text at all), while keeping a temporary internal system-property escape hatch for reverting to the legacy “GDI computes glyph positions, GDI+ draws” path. It also adds a Windows-focused snippet and expands Win32 GC tests to pin down the intended rendering behavior, including tab-stop consistency and underline/strikeout regression coverage.
Changes:
- Win32
GC.drawText()now prefers GDI+ text layout in advanced mode, with a per-GCcached internal property to opt back into legacy layout. - GDI+ tab-stop computation is aligned with Win32/GDI behavior by using
TEXTMETRIC.tmAveCharWidth * 8(removing the previous “space glyph width * 8” metric). - Adds/updates tooling: a Win32 comparison snippet (Snippet395), documentation entry, platform classpath exclusions, and additional
GCWin32Tests.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet395.java | Adds a Win32-focused UI snippet to compare GDI vs GDI+ text rendering paths and the legacy fallback. |
| examples/org.eclipse.swt.snippets/Snippets.md | Registers Snippet395 in the snippet index with a preview link. |
| examples/org.eclipse.swt.snippets/.classpath_gtk | Excludes the Win32-specific snippet from GTK snippet builds. |
| examples/org.eclipse.swt.snippets/.classpath_cocoa | Excludes the Win32-specific snippet from Cocoa snippet builds. |
| bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java | Switches advanced text layout to GDI+ by default, adds internal legacy fallback property, and fixes GDI+ tab-stop width to match GDI. |
| bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/GCWin32Tests.java | Adds regression and behavior tests for advanced text rendering (underline/strikeout, mnemonic, mirroring, scripts, kerning, and tab stops). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The GDI+ based text rendering path in GC.drawText() derived the DRAW_TAB
tab stop width from the width of a single space glyph, while the GDI
based path and plain (non-advanced) GDI both use eight times the font's
average character width, which is the convention Win32's own DrawText()
and TabbedTextOut() follow.
Those are two different metrics, not two ways of computing the same one:
in proportional fonts a space is roughly half the average character
width, so tab stops came out about half as wide whenever the GDI+ path
was taken. In monospace fonts the two nearly coincide, which is why the
discrepancy was easy to miss.
The GDI+ path is changed to reuse the GDI text metric already computed
by its caller, so that tab stops are sized consistently no matter which
path renders the text. This restores the behaviour established for the
GDI path in bug 289244 ("GDI+ drawText is not consistent with GDI"),
which the GDI+ fallback path reintroduced in bug 305815 had missed.
Add tests that pin down the tab stop width itself (eight average
character widths), its consistency across both rendering paths, and two
properties that must hold for either engine: consecutive tabs advance by
whole tab stops, and text following a tab starts at the same tab stop
regardless of what precedes the tab. They are measured from a leading
tab, which expands to exactly one tab stop and so is not diluted by the
slightly different glyph advances of the two engines, and cover both
proportional and monospace fonts.
Add Snippet395 as a manual/visual companion to the automated tests: it
renders a series of text properties, one row per property, and offers a
checkbox to switch between plain GDI and GDI+ (advanced) rendering at
runtime, without restarting, to visually compare the resulting output.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
A check in GC.drawText() makes the implementation fall back to the text rendering based on glyphs and their positions calculated by GDI even when GDI+ is used in all cases except when it contains specific characters that GDI cannot handle. This was necessary more than a decade ago because specific Chinese characters were not properly rendered by GDI+. But this is quite non-intuitive as in case GDI+/advance mode is enabled the consumer will usually expected GDI+ to be used for rendering. In addition, the problematic scenario from back then works fine now. This change thus adapts the GC.drawText() implementation to use plain GDI+ whenever it is enabled. The fallback code is preserved but only executed if a newly introduced system property is enabled. This allows to switch back to previous behavior in case unexpected regressions are found. The property is to be removed in a future release in case no regressions are found. It is evaluated once per GC rather than per drawing operation, so that reading it does not add cost to text drawing, while a newly created GC still picks up a value changed at runtime. Rename useGDIP() to useGdipTextLayout(), since both of its outcomes draw with GDI+ and it only decides which engine performs the layout. Extend GCWin32Tests with additional regression tests covering not only the previously fixed underline/strikeout invisibility issue, but also style-decoration combinations, mnemonic underlining, mirrored/RTL text, complex scripts and charsets (Arabic, Hebrew, CJK, Cyrillic, Greek, combining diacritics) and kerning-sensitive text width. The tests assert the correctness of the GDI+ rendering itself and, where a reference value is required, use plain (non-advanced) GDI rendering of the same text as baseline, so that they remain meaningful once the system property is removed again. The fallback itself is deliberately left untested, as it is meant to be removed once the new behavior has proven itself. Extend Snippet395 with a second checkbox toggling that system property at runtime, so that the default and the legacy text rendering path can be visually compared while GDI+ is active. Fixes eclipse-platform#3091 Assisted-by: Claude Opus 5 <noreply@anthropic.com>
|
This PR started as an experiment to find out whether the GDI fallback in What changed since the experimental state:
As suggested in my earlier comment, the tab stop fix is now extracted into its own PR, since it fixes a pre-existing defect in the GDI+ path rather than anything introduced here: #3518. This PR is based on it, so until #3518 is merged its commit is part of the diff here. |
Important
This PR is based on and should be merged after:
I propose to merge this PR early for 2026-12 M1
On Windows,
GC.drawText()behaves counter-intuitively onceGC.setAdvanced(true)is active: instead of letting GDI+ lay out the text, it has GDI compute the glyphs and their positions and uses GDI+ only to draw them. GDI+ performs the layout itself only for strings containing characters GDI has no glyph for.That fallback has visible consequences. Most severely, text drawn with a font carrying an underline or strikeout decoration is not rendered at all, not merely undecorated, but entirely absent, which is the defect reported in #3091.
The fallback was introduced more than a decade ago because specific Chinese characters were not rendered correctly by GDI+ (bug 305815, bug 289244). Running the snippets from those bugs against current Windows shows none of those problems anymore.
How this change fixes it
GC.drawText()now lets GDI+ lay out the text whenever advanced mode is enabled, instead of only for strings GDI cannot provide glyphs for.useGDIP()is renamed touseGdipTextLayout(), since both of its outcomes draw with GDI+ and it only decides which engine performs the layout.The previous behavior is preserved behind the
org.eclipse.swt.internal.win32.useGDITextRenderingWithGDIPsystem property. It is purely a safety net: should anyone hit an unexpected text rendering regression, it lets them fall back instead of downgrading SWT. It is not API, may be removed at any point, and is intended to be removed once the new behavior has proven itself, together with the fallback code it guards. The property is evaluated once perGCrather than perdrawing operation, so it costs nothing on the text drawing path while a newly created
GCstill picks up a value changed at runtime.GCWin32Testsgains six tests (14 executions) that pin down the resulting rendering: the underline/strikeout case from #3091, decoration across weight and slant combinations, mnemonic underlining, mirrored/RTL text, complex scripts and charsets (Arabic, Hebrew, CJK, Cyrillic, Greek, combining diacritics), and kerning-sensitive text width. Where a reference value is needed, plain non-advanced GDI rendering of the same text serves as the baseline, so the tests stay meaningful once the property and the fallback are gone. The fallback itself is deliberately left untested, as it is meant to be removed.Consumers drawing text through an advanced
GCwill see rendering differences beyond the fixed defect, since layout is now performed by a different engine: kerning, tab stop width, mnemonic underlining and bidi/mirroring may all come out slightly differently.Related
This PR is based on #3518, which fixes a pre-existing tab stop defect in the GDI+ text rendering path. That defect is independent of this change but would become visible to everyone once this path is used by default, so it was extracted for separate review. Until it is merged, its commit is part of this PR's diff and should be reviewed there.
How to test
Snippet395, added by #3518 and extended here, renders a set of text properties one row per property and switches between plain GDI, GDI+ and the legacy path at runtime, so the engines can be compared visually.It also makes sense to test with GEF diagram with
SWTGraphics, as those use GC's advanced mode when zooming.Fixes #3091
🤖 Generated with Claude Code