Skip to content

Win32DPIUtils:pointToPixel shouldn't have residual #2364

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Win32DPIUtils:pointToPixel shouldn't have residual #2364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/internal/Win32DPIUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,8 @@ public static Point pointToPixel(Point point, int zoom) {
float scaleFactor = DPIUtil.getScalingFactor(zoom);
float scaledX = fPoint.getX() * scaleFactor;
float scaledY = fPoint.getY() * scaleFactor;
return new Point.OfFloat(scaledX, scaledY);
Point scaledPoint = new Point.OfFloat(scaledX, scaledY);
return new Point.OfFloat(scaledPoint.x, scaledPoint.y);
Comment on lines +228 to +229
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to instantiate twice, do either:

// Cast to int so that there are no residuals
return new Point.OfFloat((int) scaledX, (int) scaledY);

or even better, simply declare scaledX and scaledY as ints

// Use ints so that there are no residuals
int scaledX = ...;
int scaledY = ...;
return new Point.OfFloat(scaledX, scaledY);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(int) will not round it properly. We will need to use Math.round instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/eclipse-platform/eclipse.platform.swt/pull/2364/files/ca6ef3a35062a41e9e47b39d9e3b071fabbc3811#r2379074441 since you are probably trying to do the same here and you have to preserve the other information (monitor) here too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the point of this code to ensure that we have a consistent way of rounding (by delegating the responsibility to Point.OfFloat), which would break with replicating the actual rounding implementation (via Math.round) here?

}

public static Point pointToPixel(Drawable drawable, Point point, int zoom) {
Expand All @@ -248,7 +249,8 @@ public static Rectangle pointToPixel(Rectangle rect, int zoom) {
}

private static Rectangle pointToPixel(Rectangle.OfFloat rect, int zoom) {
return scaleBounds(rect, zoom, 100);
Rectangle scaledRectangle = scaleBounds(rect, zoom, 100);
return new Rectangle.OfFloat(scaledRectangle.x, scaledRectangle.y, scaledRectangle.width, scaledRectangle.height);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to create yet another copy? scaleBounds(...) already returns a shallow copy. In fact, in doing so, you might loose information about the monitor since you're not using clone, you're using a constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking if we could somehow reset the residual values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Tricky to do without exposing too much about the internal workings of OfFloat.

I would be OK with doing this:

Rectangle scaledRectangle = scaleBounds(rect, zoom, 100);
scaledRectangle.setX(Math.round(scaledRectangle.getX());
scaledRectangle.setY(Math.round(scaledRectangle.getY());

It's not perfectly encapsulated (exposes the fact that you use Math.round(...) instead of just casting to int) but at least it preserves the monitor (if it's there).

If you don't want to expose that detail then you should add yet another method to Rectangle.OfFloat that does the rounding and looses the residuals. Something like:

void round() {
   setX(Math.round(getX());
   setY(Math.round(getY());
}

Your call.

}

public static Rectangle pointToPixel(Drawable drawable, Rectangle rect, int zoom) {
Expand Down
40 changes: 24 additions & 16 deletions ...rg.eclipse.swt.tests.win32/JUnit Tests/org/eclipse/swt/tests/win32/Win32DPIUtilTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,27 @@ public void scaleUpRectangle() {

@Test
public void scaleDownscaleUpRectangleInvertible() {
int[] zooms = new int[] {25, 50, 75, 100, 125, 150, 175, 200, 225, 250, 275, 300, 325, 350, 375, 400};
for (int zoom : zooms) {
for (int i = 1; i <= 10000; i++) {
Rectangle rect = new Rectangle.OfFloat(0, 0, i, i);
Rectangle scaleDown = Win32DPIUtils.pixelToPoint(rect, zoom);
Rectangle scaleUp = Win32DPIUtils.pointToPixel(scaleDown, zoom);
assertEquals(rect.width, scaleUp.width);
assertEquals(rect.height, scaleUp.height);
}
}
}

@Test
public void scaleBoundsRectangleInvertible() {
int[] zooms = new int[] {25, 50, 75, 100, 125, 150, 175, 200, 225, 250, 275, 300, 325, 350, 375, 400};
for (int zoom1 : zooms) {
for (int zoom2 : zooms) {
for (int i = 1; i <= 10000; i++) {
Rectangle rect = new Rectangle.OfFloat(0, 0, i, i);
Rectangle scaleDown = Win32DPIUtils.pixelToPoint(rect, zoom1);
Rectangle scaleUp = Win32DPIUtils.pointToPixel(scaleDown, zoom2);
scaleDown = Win32DPIUtils.pixelToPoint(scaleUp, zoom2);
scaleUp = Win32DPIUtils.pointToPixel(scaleDown, zoom1);
Rectangle scaleDown = Win32DPIUtils.scaleBounds(rect, zoom1, zoom2);
Rectangle scaleUp = Win32DPIUtils.scaleBounds(scaleDown, zoom2, zoom1);
assertEquals(rect.width, scaleUp.width);
assertEquals(rect.height, scaleUp.height);
}
Expand All @@ -240,18 +252,14 @@ public void scaleDownscaleUpRectangleInvertible() {

@Test
public void scaleDownscaleUpPointInvertible() {
int[] zooms = new int[] {25, 50, 75, 100, 125, 150, 175, 200, 225, 250, 275, 300, 325, 350, 375, 400};
for (int zoom1 : zooms) {
for (int zoom2 : zooms) {
for (int i = 1; i <= 10000; i++) {
Point pt = new Point(i, i);
Point scaleDown = Win32DPIUtils.pixelToPoint(pt, zoom1);
Point scaleUp = Win32DPIUtils.pointToPixel(scaleDown, zoom2);
scaleDown = Win32DPIUtils.pixelToPoint(scaleUp, zoom2);
scaleUp = Win32DPIUtils.pointToPixel(scaleDown, zoom1);
assertEquals(pt.x, scaleUp.x);
assertEquals(pt.y, scaleUp.y);
}
int[] zooms = new int[] {100, 125, 150, 175, 200, 225, 250, 275, 300, 325, 350, 375, 400};
for (int zoom : zooms) {
for (int i = 1; i <= 10000; i++) {
Point pt = new Point(i, i);
Point scaleDown = Win32DPIUtils.pixelToPoint(pt, zoom);
Point scaleUp = Win32DPIUtils.pointToPixel(scaleDown, zoom);
assertEquals(pt.x, scaleUp.x);
assertEquals(pt.y, scaleUp.y);
}
}
}
Expand Down
Loading