-
Notifications
You must be signed in to change notification settings - Fork 135
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
Guard monitor.zoom with isRescalingAtRuntime flag #1508
Guard monitor.zoom with isRescalingAtRuntime flag #1508
Conversation
@@ -3193,7 +3193,7 @@ private Rectangle translateRectangleInPixelsInDisplayCoordinateSystem(int x, int | |||
|
|||
private Rectangle translateRectangleInPixelsInDisplayCoordinateSystem(int x, int y, int width, int height, Monitor monitorOfLocation, Monitor monitorOfArea) { | |||
Point topLeft = getPixelsFromPoint(monitorOfLocation, x, y); | |||
int zoom = DPIUtil.getZoomForAutoscaleProperty(monitorOfArea.zoom); | |||
int zoom = isRescalingAtRuntime() ? DPIUtil.getZoomForAutoscaleProperty(monitorOfArea.zoom) : getDeviceZoom(); |
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.
Two remarks from my side:
1.) getDeviceZoom() is the native zoom, so don't we need to do DPIUtil.getZoomForAutoscaleProperty(getDeviceZoom()) as well?
2.) As the zoom calculation got a bit more complex and it is used at five places, doesn't it make sense to extract that into a private method
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.
For 2, its a one-liner. Would you really want to move it to a method?
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 a one liner, because of the special notation we can use. here. But this logic must be applied the same on all places and it is some kind of logic, so I would vote for that.
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.
done
7746231
to
b492f42
Compare
@amartya4256 it's not clear to me what the error was (i.e. what does this PR fix?). I tested with this snippet: /*******************************************************************************
* Copyright (c) 2024 Yatta Solutions
*
* This program and the accompanying materials
* are made available under the terms of the Eclipse Public License 2.0
* which accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Yatta Solutions - initial API and implementation
*******************************************************************************/
package org.eclipse.swt.snippets;
import java.util.concurrent.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;
public class Snippet384 {
public static void main(String[] args) throws InterruptedException {
Display display = new Display();
// Open a shell where the mouse currently is
Shell shell = new Shell(display);
shell.setSize(100, 50);
shell.setLocation(180, 220);
shell.open();
// Move slowly to the left
for (int i = 0; i < 15; i++) {
TimeUnit.SECONDS.sleep(1);
Point location = shell.getLocation();
location.x -= 20; // to the left
System.out.println("Moving to position: (" + location.x + ", " + location.y + ")");
shell.setLocation(location);
}
display.dispose();
}
} Both without any VM-arguments and also with The result before and after this PR what the same:
Am I testing this wrong? Could you please add more details to the description of this PR? |
Did you use monitors with different zoom values? From my understanding the issue only occurs in such a setup. But an extended description of the actualy issue and how to reproduce it would be helpful. In addition, the issue sounds as if it could be covered with a proper regression test rather easily, can't it? Providing one would be the best way of documenting the issue and for preventing regressions. |
Yes: 100% on the left and 200% on the right. |
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 tested it and it works as expected. No regressions either.
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've not tested this, but code looks sound and should (based on code semantics) definitely improve/fix the regression w.r.t. existing behavior.
b492f42
to
d952d1f
Compare
This contribution protects the unintended usage of different zoom levels of different monitors in trasnlating the points and rectangles from control to display coordinate system when the isRescalingAtRuntime flag is disabled. contributes to eclipse-platform#62 and eclipse-platform#127
d952d1f
to
d491442
Compare
Reported API issues on Jenkins are incorrect / not related to this PR. An only reference build (759) is taken: The current master build (762) has the same issues as this PR: https://ci.eclipse.org/releng/job/eclipse.platform.swt/job/master/762/ |
This contribution protects the unintended usage of different zoom levels of different monitors in trasnlating the points and rectangles from control to display coordinate system when the isRescalingAtRuntime flag is disabled.
contributes to #62 and #127
How To Test
Note: The placement of the shell for x > -1000 is done wrt to the zoom of primary monitor in case it is outside the scaled down bounds. However, when the bounds of the monitor are set in setClientArea method, they are not scaled down according to their zoom level when autoScaleOnRuntime is disabled hence, the code considers -2000 to 0 to be under 200% monitor in th scaled down variant and scales it up by 200% when asked to translate the point. We cannot store the zoom differently to comply with the usage of monitor in other places since it is the part of common widgets. hence, we need to make sure we don't use the monitor zoom while mapping the coordinates when the flag is disabled but rather use the default zoom.