[win32] Fix collapsed tab stops at fractional zoom levels

[vogella]

TextLayout.setTabs() takes tab stops in points, but computeRuns() compared them in pixels against a pen position summed from raw glyph advances. At zoom levels that are not a multiple of 100 the round trip can put a stop one pixel past a pen sitting exactly on it, so the tab advances by one pixel instead of reaching the next stop.

StyledText hits this because its single tab stop is the width of N spaces rounded to points. Stops beyond the first are multiples of that rounded value and drift further off the grid of spaces, so a tab after 8 or 12 spaces collapsed too. That is the "tabs are sometimes not indenting" report in eclipse-platform/eclipse.platform.ui#3052, exposed in 4.36 when monitor-specific scaling enabled zoom 125, 150 and 175.

Fix:

• decide whether a stop is still ahead in points, the unit of setTabs()
• implement setDefaultTabWidth() on win32 (StyledText already calls it; cocoa uses it since bug 519015): when the single stop matches the width of that many spaces, tabs go on exact multiples of their pixel width

The regression test now also checks a tab after 2N and 3N spaces. It fails on master at 125–200% and passes at 100–200% with the fix.

Verified on a monitor at 150% with StyledText, tab width 4, text after 4/8/12 spaces + tab compared to plain spaces: master is off for Consolas 9/12/15 and Courier New 8/11/13/16; with the fix all 18 font sizes (8–16pt) match exactly.

🤖 Generated with Claude Code

[github-actions]

Test Results (win32)

   35 files  ±0     35 suites  ±0   5m 34s ⏱️ +11s
4 948 tests +5  4 866 ✅ +5  82 💤 ±0  0 ❌ ±0 
1 471 runs  +5  1 444 ✅ +5  27 💤 ±0  0 ❌ ±0 

Results for commit 21be84b. ± Comparison against base commit a04b0b7.

♻️ This comment has been updated with latest results.

[vogella]

@HeikoKlare this is a scalling related bug fix, do you want to review?

[HeikoKlare]

Thank you work working on this.
The fix idea sounds reasonable to me. However, I am not able to easily understand both the original and the adapted logic in TextLayout.computeRun(). It may probably help for review to share a summary of the concept of that code (which should have been collected in the contributor's head / AI's context anyway).

With respect to the provided test: it's important to note this is not a parameterized test, even though its configuration pretends to be one. The zoom parameter is only used for retrieving a font at a specific zoom, but that does not have any effect on the TextLayout computations (or I do not understand it yet), as the TextLayout uses it's GC's zoom (or without a GC the zoom of the monitor a shell was last moved to). Please also note that the test even fails on the master code state on 100% zoom. Is that intended?
A proper test or a proper reproducer would be necessary (or at least very helpful) to better assess the proposal.

Just to be sure: this is something that is supposed to be merged for M1 and not for the upcoming release anyway, right? At least I would consider such a change too risky for M3.

[vogella]

Thank you work working on this. The fix idea sounds reasonable to me. However, I am not able to easily understand both the original and the adapted logic in TextLayout.computeRun(). It may probably help for review to share a summary of the concept of that code (which should have been collected in the contributor's head / AI's context anyway).

The real change is actually very small, I did first re-structure the code to make it easier to read and than changed the unit. Arguely this makes the diff harder to read. I will change it back to its original state and only apply the minimal change, the clean-up can be done later.

With respect to the provided test: it's important to note this is not a parameterized test, even though its configuration pretends to be one. The zoom parameter is only used for retrieving a font at a specific zoom, but that does not have any effect on the TextLayout computations (or I do not understand it yet), as the TextLayout uses it's GC's zoom (or without a GC the zoom of the monitor a shell was last moved to).

TextLayout.setFont propagates the font's zoom to the layout:

// TextLayout.java:3261
this.nativeZoom = this.font == null ? nativeZoom : this.font.zoom;

Please also note that the test even fails on the master code state on 100% zoom. Is that intended? A proper test or a proper reproducer would be necessary (or at least very helpful) to better assess the proposal.

Yes, try for example 125, that should fail. On CI Windows all zoom level expect 100% failed. At zoom 100 DPIUtil.pixelToPoint and pointToPixel both do the short path (if (zoom == 100 ...) return size),

Just to be sure: this is something that is supposed to be merged for M1 and not for the upcoming release anyway, right? At least I would consider such a change too risky for M3.

If you have objections this can wait. Maybe the shorter diff will feel less risky, would be nice to close eclipse-platform/eclipse.platform.ui#3052 IMHO.

[HeikoKlare]

Yes, try for example 125, that should fail. On CI Windows all zoom level expect 100% failed. At zoom 100 DPIUtil.pixelToPoint and pointToPixel both do the short path (if (zoom == 100 ...) return size),

I actually referred to a failure on 100%, but I cannot reproduce it anymore (same as for the zoom parameterization not working). Not sure what has changed in my setup as the test was not changed, and the explanation regarding setFont() is sound as well. So please ignore my previous concern.

If you have objections this can wait. Maybe the shorter diff will feel less risky, would be nice to close eclipse-platform/eclipse.platform.ui#3052 IMHO.

I agree that it would be nice to fix and with the latest simplification the change is also much easier to understand. We have just quite often experienced unexpected side effects when changing point/pixel conversions that were not obvious and visible immedately. That's why I usually prefer to have such changes in code with quite some impact (such as TextLayout) earlier in the release cycle (i.e., before M2) in case there is no conceptual exclusion of potentially introduced issues, so that we have some time for implicit testing. This in particular applies if it's not about regressions but about long-standing issues (such as this one, which has been reported a year ago).

That said, I would not block this from being merged now (in particular with the recent simplification that makes it quite easy to understand). So if you feel confident with the change, do not hesitate to merge it now.

[HeikoKlare]

What's the motivation for comparing the point instead of the pixel values here? Pixel values should be of higher precision.

[vogella]

Comparing in points is deliberately the coarser comparison. Then the cursor walks across the spaces and lands at 39 pixels, right where the stop was meant to be. SWT asks: have we reached it?

• In pixels: stop says 40, cursor is at 39, so no. The tab gets drawn one pixel wide. That's the bug.
• In points: stop says 20, cursor is at 20, so yes. Move on to the next stop. Correct.

[HeikoKlare]

Does that mean we solve the issue by "rounding away" an incorrect calculation, or is that really a conceptually correct response to the pixel/point conversion precision-loss?
I am just asking in a very general way because I have to admit that I do not understand the calculation done here yet (see my initial review comment on the explanation of the calculation concept), so I cannot assess if it makes sense or not.

[vogella]

We are using the point values which are correct not the converted values which due to their type sometimes get rounded to different values. IMHO this is the correct approach.

[HeikoKlare]

We are using the point values which are correct not the converted values

In the commented line, there is a conversion from a pixel-based tab value to a point value, which in general is a lossy conversion (note that pixel-to-point and point-to-pixel conversion are not mathematically inverse functions). So I am not sure how a converted value may ever be called (more) "correct" than or in comparison to a non-converted value.

[vogella]

You're right, that line should go. The related change went into #3495 sorry for that.

The clean version keeps the fallback in points and converts once at the end:

int tabX = tabs[tabsLength-1];
int lastTabWidth = tabsLength > 1 ? tabs[tabsLength-1] - tabs[tabsLength-2] : tabs[0];
if (lastTabWidth > 0) {
while (tabX <= lineWidthInPoints) tabX += lastTabWidth;
run.width = DPIUtil.pointToPixel(tabX, zoom) - lineWidth;
}

#3495 already does this for the whole block, including the merged-tab adjustment which has the same problem. I will update this PR.

[vogella]

PR updated 3 days ago.

@HeikoKlare should this wait for 4.22 or are you ok with it for 4.21?

[HeikoKlare]

should this wait for 4.22 or are you ok with it for 4.21?

I would prefer to hold such a change back for (4.42) M1. I did my assessment regarding regression risk vs. benefit taking into account how long this issue is known/accepted here #3493 (review), and we are already beyond M3 now.
But as I said, I won't block this if you or someone else to a different asessment and want to bring this into 4.41.

[Copilot]

Pull request overview

Fixes Win32 tab-stop selection at fractional zoom levels by consistently comparing positions in points.

Changes:

• Resolves tab-stop selection and repetition in point units.
• Adds regression coverage across 100–200% zoom levels and varied fonts/tab widths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
TextLayout.java	Corrects fractional-zoom tab-stop calculations.
TextLayoutWin32Tests.java	Adds parameterized regression tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[vogella]

Before:

After:

[vogella]

should this wait for 4.22 or are you ok with it for 4.21?

I would prefer to hold such a change back for (4.42) M1. I did my assessment regarding regression risk vs. benefit taking into account how long this issue is known/accepted here #3493 (review), and we are already beyond M3 now. But as I said, I won't block this if you or someone else to a different asessment and want to bring this into 4.41.

Let me know if you have additional feedback, otherwise I plan to merge it in two days.