[win32] Fix collapsed tab stops at fractional zoom levels

[vogella]

TextLayout.setTabs() takes the tab stops in points, but computeRuns() converted them to pixels and compared them against a pen position accumulated from raw glyph advances. At zoom levels that are not a multiple of 100 that point/pixel round trip can place a stop one pixel past a pen position that actually sits exactly on that stop, so the tab advanced by a single pixel instead of moving on to the next stop.

StyledText runs straight into this because it measures its tab width as the width of N spaces in points and hands that back as the only tab stop, so a tab following N spaces lands precisely on the stop. That is the "tabs are sometimes not indenting" report in eclipse-platform/eclipse.platform.ui#3052. It surfaced in 4.36 when monitor-specific scaling turned the effective auto-scale from "integer" into "quarter", exposing zoom 125, 150 and 175.

Only the selection of the stop was ever wrong, so the change is limited to it: the comparison that asks whether a stop is still ahead of the pen, and the walk past the last stop, both happen in points now. No stop is converted to pixels and back on the way. The block adjusting merged consecutive tabs is untouched; #3495 is stacked on top and moves it to points as well.

The regression test was pushed on its own first: the Windows job flagged 45 combinations of zoom, font height and tab length, at 125, 150, 175 and also 200 percent for odd tab lengths. 100 percent passed without the fix, as expected, since both conversions are identities at that zoom.

[github-actions]

Test Results (win32)

   35 files  ±0     35 suites  ±0   5m 23s ⏱️ - 2m 21s
4 873 tests +5  4 797 ✅ +5  76 💤 ±0  0 ❌ ±0 
1 400 runs  +5  1 376 ✅ +5  24 💤 ±0  0 ❌ ±0 

Results for commit 5c1642a. ± Comparison against base commit b228e3f.

♻️ 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.