[Win] StyledText with paint artifacts

[tmssngr]

Win32DPIUtils.pixelToPointWithSufficientlyLargeSize: If we need the rectangle that is large enough, the left top corner must be rounded down and the right bottom corner rounded up.

Scrollable.getClientArea and the bounds of the Paint event also need to be large enough to contain any pixel. In the worst case, it contains "half" points, but it does not matter if it draws one pixel here and there into the invisible area.

However, it might be better if Scrollable.getClientArea might return a special rectangle (similar to Rectangle.OfFloat) that can be asked for the "inner" and "outer" rectangle, because only the user of knows what rectangle is important for him: for painting the larger rect is important. But, for example, for a table that resizes a single column depending on the size, the smaller rect might be of interest to prevent horizontal scrollbars.

[github-actions]

Test Results (win32)

   30 files  ±0     30 suites  ±0   4m 1s ⏱️ -12s
4 697 tests ±0  4 622 ✅ ±0  75 💤 ±0  0 ❌ ±0 
1 227 runs  ±0  1 203 ✅ ±0  24 💤 ±0  0 ❌ ±0 

Results for commit 91677aa. ± Comparison against base commit 823aeba.

[HeikoKlare]

The finding regarding the calculation results of pixelToPointWithSufficientlyLargeSize are interesting. But the current proposal leads to incorrect results. Note that this kind of change needs to be tested with different monitor setups/zooms and different contexts (layouts etc.) to see that nothing breaks. We quite often ran into the situation that just one specific combination of monitor zoom and SWT layout (and maybe others) broke. If possible, the scenarios currently not working correctly should be reflected in test cases to Win32DPIUtilTests or ControlWin32Tests.

In this case, one scenario that breaks is at 175%. In the following screenshots you see that at least the border at the bottom is not rendered anymore.

With this change:

Without this change:

[HeikoKlare]

This calculation can lead to an unintended overcalculation. In addition, it does not consider input rectangles already being at floating-point precision, for which an exact conversion was done before this change.
Can't we solve the issue at the root (inside the pixelToPoint method), which already seems to yield unexpected results when using rounding mode "up". E.g., the scaledBottomRight calculation may incorporate the rounding mode with something like Point.OfFloat.from(pixelToPoint(floatRect.getBottomRight(), zoom, sizeRounding)).
Also note that the pixelToPoint calculation does some internal rounding (calculating based on rounded values instead of the float-based values and rounding afterwards) because everything else broke one case or another.

It would also be good to isolate this change from further changes, such as consumers of this method.

[tmssngr]

Please treat this PR as discussion base. What about the suggestion regarding the Rectangle? I think, we need to deprecate the access to the x, y, width, height fields (or to Rectangle at all), because on Windows we either need float values or better ints rounded up/down - according to the user's wish. Something like rect.getXCeil(), rect.getXFloor(), ... BTW, I do not see any use-case for the rounding mode "round", only for "up" or "down".

[HeikoKlare]

I think, we need to deprecate the access to the x, y, width, height fields (or to Rectangle at all), because on Windows we either need float values or better ints rounded up/down - according to the user's wish.

Two thoughts on this:

1. We can deprecate the fields of Rectangle (and then also Point), such as x, y, width and height. But you will never be able to deprecate them for removal, as they have been there forever and they are commonly used. So you would break (stale) consumers (like framework extensions) when removing direct access. That's the reason why we chose to provide new types of Rectangle and Point and use them at those places that require them, including the "dynamic conversion" with methods like Rectangle.OfFloat.from() and the like to avoid breaking changes of API that accepts or returns Point or Rectangle.
2. There should not be something like a "user's wish". Once we start shifting the effort of dealing with autoscaling to the user, the whole idea of SWT autoscaling dilutes. On the other hand, we know that it's impossible to deal with every limitation of autoscaling concept when it comes to fractional scaling behind the APIs. For that reason, we chose to track the problematic points and find solutions for them. This particularly includes two points: (1) the global coordinate system which became non-continuous and (2) pixel-perfect rendering scenarios (like diagrams). Primarily for (1), we provided new API to Rectangle and Point to allow for better preservation of precision outside of the API without thinking about it and without explicitly dealing with floating-point precision. For (2), we provided the concept of autoscale disablement at control level, so that specific control can deal with scaling on their own. Until now, these solutions were mostly sufficient. So I would only be in favor of further breaking encapsulation of autoscaling if we do it in a very focussed way based on specific problem scenarios that we have identified rather than doing it in a quite generic way.

BTW, I do not see any use-case for the rounding mode "round", only for "up" or "down".

I guess you mean there are only use cases for "round" and "up" but not for "down", right? We have once used rounding mode "down" for specific use cases of rectangle conversions but found that to be inaccurate in specific situation, so that we revised the code again and now ended up not using "down" anywhere. It's rather there for the sake of completeness and for easy testing when trying to further improve calculations related to conversions and rounding.

That said, I think you have a good catch with the unexpected results of pixelToPointWithSufficientlyLargeSize. A good step could be to identify where those unexpected calculation results lead to unexpected user-facing behavior and fix that. Maybe it is even related to the StyledText painting issue.

[tmssngr]

Regarding 1. "Deprecating Rectangle fields": How does overriding solve this problem? Any code which uses a rectangle (or a subclass) can modify x, y, width, height and expect other code to respect these changes. The problem: subclasses don't get notified if code changes x, y, width or height.

Regarding 2. "User's wish": only the user of a rectangle knows which values they need - rounded up or down. This depends on the use case. I don't think, SWT magically can fix everything itself, but it needs to provide API that the user can solve their problem.

I guess you mean there are only use cases for "round" and "up" but not for "down", right?
No, I mean that I do not see any use-case for "round", except of getting just a rough estimation. Until now I only can imagine use-cases for rounding up or down:

• rounding up: clipping region, drawing the full client area, drawing an upper/left border
• rounding down: drawing a lower/right border, make columns as wide as the client area without showing horizontal scrollbars

The bad thing about pixelToPointWithSufficientlyLargeSize is, that I could not find any code unit-testing it.

What do we need (from a user's perspective of SWT): getSize(), getBounds(), getClientArea() need a way to provide the rounded up or rounded down coordinate (IMHO it makes sense to think about the Rectangle as two coordinates, left-up and right-bottom). For MacOS and Linux the reported coordinates (rounded up or down) would be always the same, because they are precise. But for Windows, these coordinates rounded up or down could be off by one point. In the worst case, the different of height or width could be 2 points.

Going further: if code, like gc.drawRect() gets a rectangle, the user might be able to specify whether it means the rounded up or rounded down scaled pixel (for each coordinate).

[HeikoKlare]

I think there is a fundamental difference in our understanding of how SWT should behave here. You expect SWT to expose internal about autoscaling so that the user can (and has to) deal with things related to autoscaling on their own. As I explained in my previous post that will completely change how autoscaling in SWT is designed. Autoscaling was designed to be fully encapsulated in SWT so that consumers of SWT only operate on logical point coordinates. In a perfect world, they should not need to know that those logical point coordinates are internally converted into pixels based on monitor zooms, they should just deal with a logical coordinate space. Once you pass out any information about necessary rounding methods and require consumers to decide if they want to round up or round down, you completely break the whole design of autoscaling. I don't say that's not possible, but it will put a burden on consumers that was never intended.
Everything we did so far in terms of extensions to make monitor-specific scaling possible was made with this design in mind. That did not lead to the cleanest solution one would like to have (because that's impossible with integer-based logical points and fractional scaling) but it worked sufficiently well to not throw away the overall design idea.

No, I mean that I do not see any use-case for "round", except of getting just a rough estimation. Until now I only can imagine use-cases for rounding up or down:

You can see a bunch of consumers of the "Round" rounding mode:

[tmssngr]

I've shown you above in several examples that there is no one solution-fits-all-use-cases, and you also confirmed that (maybe unintentionally). I think, we can't hide the details of the auto-scaling because it introduces (rounding) errors; and to produce good looking results the SWT user somehow need to deal with them. Hence the API needs to provide this information.

You can see a bunch of consumers of the "Round" rounding mode:

Is rounding really correct in all of these uses? Or was it just used because the API did not had better means?

Maybe the auto-scaling with non-integer scaling factors does not work out on Windows (at least with the restrictions of the SWT API), if one cares about good looking results. There might be a good reason why Apple offered only integer scaling.

I hope we agree, that

• we want SWT to produce good looking, pixel-precise results,
• the current state of SWT has some problems with auto-scaling on Windows (especially with fractional scaling) which causes artifacts, asymmetric painting results, scrollbars were there shouldn't be some,
• we want to solve all of them,
• we only disagree in how to solve them.

Have a nice weekend! 😊