Guard monitor.zoom with isRescalingAtRuntime flag #1508

amartya4256 · 2024-10-07T13:33:05Z

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

Set your monitor setup like this:

Left Monitor: 200% Zoom, Right Monitor : 100% Zoom (Primary Monitor)

Create this Snippet:

public class TestSnippet {

	public static void main(String[] args) {
		Display display = new Display();
		Shell shell = new Shell(display);
		shell.setText("Snippet Lol");
		Text text = new Text(shell, SWT.BORDER);
		text.setBounds(20, 20, 500, 30);
		Button button = new Button(shell, SWT.PUSH);
		button.setBounds(100, 80, 100, 30);
		button.setText("Relocate");
		button.addSelectionListener(new SelectionAdapter() {
			@Override
			public void widgetSelected(SelectionEvent e) {
				Point location = shell.getLocation();
				location.x = Integer.parseInt(text.getText());
				shell.setLocation(location);
			}
		});
		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch())
				display.sleep();
		}
		display.dispose();
	}
}

Checkout the master branch
Disable rescaling
Let's say your 200% monitor has pixels from -2000 to 0. Enter the value of x more than the half, i.e. > -1000 and < 0.
You will see the shell placed in the other monitor (100% monitor in the right) and not in the one expected, because 200 is used as the scaling factor and not the primary monitor zoom.
Now Enable rescaling.
Try it again, you'll see the placement to be in the 200% monitor.
Now, switch to the branch guard_monitor_zoom_usage and try the whole scenario with rescaling disabled. It should use the primary monitor zoom (100%) and the shell should be positioned in the 200% monitor when you set the x >-1000 and < 0.
If you enable the flag, the behavior should be identical to the behavior on master.

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.

github-actions · 2024-10-07T13:41:25Z

Test Results

486 files ±0 486 suites ±0 7m 24s ⏱️ -14s
4 154 tests ±0 4 146 ✅ ±0 8 💤 ±0 0 ❌ ±0
16 370 runs ±0 16 278 ✅ ±0 92 💤 ±0 0 ❌ ±0

Results for commit d491442. ± Comparison against base commit 8fe0fb0.

♻️ This comment has been updated with latest results.

akoch-yatta · 2024-10-08T09:13:35Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java

@@ -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();


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

For 2, its a one-liner. Would you really want to move it to a method?

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.

fedejeanne · 2024-10-11T13:05:13Z

@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 -Dswt.autoScale.updateOnRuntime=false

The result before and after this PR what the same:

No visual differences
Console output was:

Moving to position: (160, 220)
Moving to position: (140, 220)
Moving to position: (120, 220)
Moving to position: (100, 220)
Moving to position: (80, 220)
Moving to position: (-1753, 220)
Moving to position: (-1773, 220)
Moving to position: (-1793, 220)
Moving to position: (-1813, 220)
Moving to position: (-1833, 220)
Moving to position: (-1853, 220)
Moving to position: (-1873, 220)
Moving to position: (-1893, 220)
Moving to position: (-1913, 220)
Moving to position: (-1933, 220)

Am I testing this wrong?

Could you please add more details to the description of this PR?

HeikoKlare · 2024-10-11T14:14:32Z

I tested with this snippet:

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.

fedejeanne · 2024-10-14T05:10:12Z

Did you use monitors with different zoom values?

Yes: 100% on the left and 200% on the right.

fedejeanne

I tested it and it works as expected. No regressions either.

HeikoKlare

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.

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

HeikoKlare · 2024-10-14T19:32:26Z

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/

amartya4256 requested a review from niraj-modi as a code owner October 7, 2024 13:33

akoch-yatta reviewed Oct 8, 2024

View reviewed changes

amartya4256 force-pushed the guard_monitor_zoom_usage branch 2 times, most recently from 7746231 to b492f42 Compare October 10, 2024 11:39

fedejeanne mentioned this pull request Oct 14, 2024

Add automatic test for issue #1508 vi-eclipse/Eclipse-Platform#131

Open

fedejeanne approved these changes Oct 14, 2024

View reviewed changes

HeikoKlare approved these changes Oct 14, 2024

View reviewed changes

HeikoKlare force-pushed the guard_monitor_zoom_usage branch from b492f42 to d952d1f Compare October 14, 2024 18:38

HeikoKlare force-pushed the guard_monitor_zoom_usage branch from d952d1f to d491442 Compare October 14, 2024 19:13

HeikoKlare merged commit 7a04f7a into eclipse-platform:master Oct 14, 2024
11 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guard monitor.zoom with isRescalingAtRuntime flag #1508

Guard monitor.zoom with isRescalingAtRuntime flag #1508

amartya4256 commented Oct 7, 2024 •

edited by fedejeanne

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

akoch-yatta Oct 8, 2024

amartya4256 Oct 10, 2024

akoch-yatta Oct 10, 2024

amartya4256 Oct 10, 2024

fedejeanne commented Oct 11, 2024

HeikoKlare commented Oct 11, 2024

fedejeanne commented Oct 14, 2024

fedejeanne left a comment

HeikoKlare left a comment

HeikoKlare commented Oct 14, 2024

Guard monitor.zoom with isRescalingAtRuntime flag #1508

Guard monitor.zoom with isRescalingAtRuntime flag #1508

Conversation

amartya4256 commented Oct 7, 2024 • edited by fedejeanne Loading

How To Test

github-actions bot commented Oct 7, 2024 • edited Loading

Test Results

akoch-yatta Oct 8, 2024

Choose a reason for hiding this comment

amartya4256 Oct 10, 2024

Choose a reason for hiding this comment

akoch-yatta Oct 10, 2024

Choose a reason for hiding this comment

amartya4256 Oct 10, 2024

Choose a reason for hiding this comment

fedejeanne commented Oct 11, 2024

HeikoKlare commented Oct 11, 2024

fedejeanne commented Oct 14, 2024

fedejeanne left a comment

Choose a reason for hiding this comment

HeikoKlare left a comment

Choose a reason for hiding this comment

HeikoKlare commented Oct 14, 2024

amartya4256 commented Oct 7, 2024 •

edited by fedejeanne

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading