-
Notifications
You must be signed in to change notification settings - Fork 179
Fix label cutoff #2381
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.
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
base: master
Are you sure you want to change the base?
Fix label cutoff #2381
Conversation
Test Results 320 files - 226 320 suites - 226 17m 54s ⏱️ - 11m 39s Results for commit d7a4725. ± Comparison against base commit fdbb769. This pull request removes 287 tests.
This pull request skips 17 tests.
♻️ This comment has been updated with latest results. |
4e2fcda
to
fc7dedf
Compare
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/GridData.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/GridData.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One general question on this: wouldn't it possible or even reasonable to stick to width/height values inside GridData
but just make them floats and only use Point.OfFloat
for the returns of API methods?
Two reasons for this:
- SWT is generally lacking some
Size
object and, unfortunately, at many placesPoint
is used to represent a size (even though that semantically does not make that much sense). Thus, we usually try to stick towidth
/height
when considering sizes. - It would be less error prone as now if you accidentally use, e.g.,
size.x
instead ofsize.getX()
, you may calculate with a wrong value.
I agree, I'll make the changes. |
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/GridLayout.java
Outdated
Show resolved
Hide resolved
Hi! Is this fix included in the M3 build released last Friday? |
Given the PR isn't merged, no. |
The necessary change was too risky to do that late (M3) in the development cycle, so it will not go in the upcoming 2025-09 relesae. We plan to process it as early as possible for 2025-12 M1. |
fc7dedf
to
b0d972e
Compare
@HeikoKlare converted the fields back to float instead of Point.OfFloat and refactored as needed. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken, this change breaks the calculations inside GridLayout
, as previous integer divisions and remainder calculations are simply changed to float arithmetics even though the considerations of int division remainders seems to be wrong there.
bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/layout/GridData.java
Outdated
Show resolved
Hide resolved
float equalWidth = (w + spanWidth) / hSpan; | ||
float remainder = (w + spanWidth) % hSpan; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this calculation break given that this is no integer division anymore (applies to subsequent places as well)? The equalWidth
wil now contain an exact division value but remainder
still contains the remainder as if doing an int division.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the logic and I realized, it was only done to regularize the size of grid cells. But now they we have size in float, we do not need to calculate remainder and we can get rid of it. I tested the code and it seems fine to me. I'll update the code here so that you can review that.
public static Point.OfFloat from(Point point) { | ||
if (point instanceof Point.OfFloat pointOfFloat) { | ||
return new Point.OfFloat(pointOfFloat.getX(), pointOfFloat.getY()); | ||
} | ||
return new Point.OfFloat(point.x, point.y); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a duplication of the method in FloatAwareGeometryFactory
, isn't it? Shouldn't that be streamlined in an OS-independent way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With vi-eclipse/Eclipse-Platform#320 we wanted to provide such utility methods directly inside rectangle and point since, they are platform independent. FloatAwareGeometryFactory is a private class inside Win32DPIUtils. Do you think it makes sense to keep using that or maybe we should move towards Point.from and Rectangle.from.
For now I can use FloatAwareGeometryFactory and create a PR following the refactoring as suggested. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that we should not introduce duplicated code unnecessarily. If we introduce this method as proposed, we can remove the according method from the FloatAwareGeometryFactory
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that separately. I would also need to adapt other places. For now I'll remove this from here and use the Factory method. And I'll refactor everything in the next PR. Sounds good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work? The factory is win32-specific while this is OS-independent code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get what you mean.
I mean why is it important where GridLayout
is located for the part about FloatAwareGeometryFactory
and Win32DPIUtils
? A Layout Manger shold never need to know anything about DPI scaling and alike...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read my comment above. Everything should be in there: it does not make sense (and technically you cannot even) reference OS-specific code from an OS-agnostic class (like the layout-related classes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeikoKlare I read the comments but they do not explain why GridLayout
now needs to use Point.OfFloat and be rewritten in using internal API now, so for me it means EVERY LayoutManger
now needs to be potentially be rewritten as well (what could be code outside of SWT as well).
As LayoutMangers should already work on the "points" and not the "pixels" so something seems fundamentally wrong here!
And while I can understand that there might be some slight rounding errors, the example above shows a noticeable cut-of that can not be explained by a sub-pixel rounding error to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HeikoKlare I read the comments but they do not explain why
GridLayout
now needs to use Point.OfFloat and be rewritten in using internal API now, so for me it means EVERYLayoutManger
now needs to be potentially be rewritten as well (what could be code outside of SWT as well).
Yes, that might be the case.
As LayoutMangers should already work on the "points" and not the "pixels" so something seems fundamentally wrong here!
As we know, points are imprecise because of their limitation to integers and according rounding errors.
And while I can understand that there might be some slight rounding errors, the example above shows a noticeable cut-of that can not be explained by a sub-pixel rounding error to me!
It is such an off-by-one rounding error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is such an off-by-one rounding error.
It does not really look like being of by a few pixels. For me it looks like more the reported size of the label is smaller than it actually should, so the Label
class needs to be fixed (or the Point
/Rectangle
) to not round down but round up instead so in such case we get a slightly larger size (and maybe show a tiny little extra whitespace) than one that is too small. This would then fix the issue for many other cases as well instead of trying to "fix" all callers.
@@ -179,7 +179,7 @@ static int checkStyle (int style) { | |||
if (hHint != SWT.DEFAULT) height = hHint; | |||
width += border * 2; | |||
height += border * 2; | |||
return new Point (width, height); | |||
return new Point.OfFloat (width, height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? The calling method seems to the wrap the point into an OfFloat
(via Win32DPIUtil#pixelToPoint()
) anyway, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't if the passed object is not OfFloat. Since we want to have precision scaling here, we need to return it as OfFloat so that the scaling method uses the OfFloat scaling. We have such behavior in Win32DPIUtil#pixelToPoint() because we did not want to enable OfFloat scaling SWT wide, because we had problems in some widgets if you remember. Hence, we decided to enable is specifically for those where it is needed. Which means those methods need to return an OfFloat variant to enable themselves for precision scaling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would enable it for SWT wide once we have figured out the issues with problematic implementations. We have separate issue for that. One of them being vi-eclipse/Eclipse-Platform#303
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't get it. Control#computeSize()
calls Win32DPIUtils.pixelToPoint()
:
eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Control.java
Lines 618 to 624 in 1ae0e18
public Point computeSize (int wHint, int hHint, boolean changed){ | |
checkWidget (); | |
int zoom = getZoom(); | |
wHint = (wHint != SWT.DEFAULT ? Win32DPIUtils.pointToPixel(wHint, zoom) : wHint); | |
hHint = (hHint != SWT.DEFAULT ? Win32DPIUtils.pointToPixel(hHint, zoom) : hHint); | |
return Win32DPIUtils.pixelToPoint(computeSizeInPixels(wHint, hHint, changed), zoom); | |
} |
And that method returns a
Point.OfFloat
if zoom is not 100:Lines 116 to 123 in 1ae0e18
public static Point pixelToPoint(Point point, int zoom) { | |
if (zoom == 100 || point == null) return point; | |
Point.OfFloat fPoint = FloatAwareGeometryFactory.createFrom(point); | |
float scaleFactor = DPIUtil.getScalingFactor(zoom); | |
float scaledX = fPoint.getX() / scaleFactor; | |
float scaledY = fPoint.getY() / scaleFactor; | |
return new Point.OfFloat(scaledX, scaledY); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you are right. The behaviour I explained earlier is only valid for Rectangle
, not for Point
. My bad. Yes we can just return Point
.
e536441
to
2c7ad44
Compare
This commit adds Rectangle.OfFloat.from and Point.OfFloat.from methods and removes the Win32DPIUtils.FloatAwareGeometryFactory class to make these methods OS-independent.
2c7ad44
to
81790ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layouts have always operated on whole "points" and requiring one specific layout manger for correct function seems totally wrong to me.
So I miss the following:
- A general description why the issue happens at all and fractions of a point make a difference now
- What are the plans to potentially test (and fix) other layout mangers as well
- How we expect custom
LayoutMangers
out in the wild to adapt here if the float based API is still not public. - I miss test-cases that testing the new GridLayout behaviour
From the referenced issue it more seems that a relay-outing is missing here and wrapping should instead happen.
I agree that the description needs to be extended to make it easier to understand for everyone (and document for ourselves) what's going on. @amartya4256 please do so.
We plan to do that. Sorry that we did not create a public issue for it so far. We have it in our backlog:
Let's do one step after another. We will check the other layouts with the above mentioned ticket and see what problems we can expect in general. We have not experienced any issues with any layout but the
Yes, adding test cases for problematic cases definitely makes sense. @amartya4256 please do so.
That's not the case. If it was that easy, we would already have solved it like that. |
In general a call to the size should report the size the component would need for the given constrains. This might not always be sufficient to show the component if constraints are to small, but given we pass in |
Taking a quick look at the current implementation. I think using For |
I have updated the description. When it comes to tests, testing this whole issue in GridLayout as a whole is difficult as nothing is exposed as an API and the problem happens when the pixels values are wrong and not in point values. It all depends on if we have Point object or Point.OfFloat object. And testing the inversibility of pixel to point to pixel is already there in org.eclipse.swt.tests.win32.Win32DPIUtilTests.scaleDownscaleUpPointInvertible() and org.eclipse.swt.tests.win32.Win32DPIUtilTests.scaleDownscaleUpRectangleInvertible(). And on top, I can use the snippet on top as an example that shows how the label cuts off on master but this PR fixes it. Is that enough? @HeikoKlare |
@amartya4256 Your example is not really intuitive to me. You argue that the value without using floats is actually larger than it should (what is somehow expected) but why should setting it to a larger width should trigger wrapping here what should happen when the label is actually smaller so I suspect it is the other way round. This is even reproducible under Linux GTK by modify the example with
then the label wraps because I set a bound 1 pixel to small, but if I change it to +1 pixel (or even +10 pixels) it does not wrap. |
This commit contributes to enable GridLayout:layout to utilize float values in GridData to calculate precise size on scaling using Point.OfFloat APIs. contributes to eclipse-platform#2166
81790ee
to
d7a4725
Compare
@laeubi I had a look at the snippet itself and found the following difference in the behavior after comparing this branch with master:
On this branch:
|
@amartya4256 that makes much more sense here, and that's why I would suggest to round up any values that represent a width/height. In general I think this can only solved consistently with something like this: |
This PR contributes to precise scaling of computation of size inside gridLayout by using float precision. This fixes the issue reported in #2166
Explanation
The issue happens because of the loss of precision while calculating the size of the grid in the GridLayout. The code at first calculates the size of widget in GridData#computeSize. Since the width and height in GridData were stored as integer, the returned Point.OfFloat from Control#computeSize is not fully utilized and the residualX and residualY (the floating pointer values) are lost and only integer is used.
Later this less precision integer value is used to set the bounds of the grid, which leads to wrapping the widget with the wrong size. In case of #2166, that leads to wrapping of the text in the widget while the height of the grid is set thinking that the text doesn't need wrapping.
A simple example for loss of precision:
Let's say a pixel value 223 is set at 150%.
when it is obtained in points by GridData#computeSize, it is 223/1.5 = 148.6667 i.e. 149
When scaled up on GridLayout#layout on child.setBounds, it becomes 149 * 1.5 = 223.5 i.e. 224
Hence, this difference of 1 pixel can have the wrong wrapping effect.
While using float in the layout, the inversibility is maintained. Hence, 223/1.5 = 148.66667 and 148.6666667 * 1.5 = 223
How to test
Before:

After:

depends on #2486