[Win32] [Hi-DPI] Non-deterministic Display.getBounds() / no way to create a consistent screenshot without internal API

[sratz]

Describe the bug
The behavior of Display.getBounds() is currently non-deterministic, as this internally uses DPIUtil.getDeviceZoom(), which means the autoscaling zoom for the shell whose zoom has last been changed (via moving the shell to another monitor or changing the zoom of a monitor).

Due to this it's pretty much impossible to deterministically create a screenshot of the whole Display (all monitors).

To Reproduce

• Set up a multi-monitor setup as follows:
  

• Run the snippet

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Objects;

import org.eclipse.swt.SWT;
import org.eclipse.swt.events.SelectionListener;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Image;
import org.eclipse.swt.graphics.ImageData;
import org.eclipse.swt.graphics.ImageLoader;
import org.eclipse.swt.graphics.Rectangle;
import org.eclipse.swt.internal.DPIUtil;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Monitor;
import org.eclipse.swt.widgets.Shell;

public final class ScreenshotProblemDisplayBounds {

	public static void main(String[] args) throws IOException {
		Display display = Display.getDefault();
		Shell shell = new Shell();
		shell.setSize(300, 200);
		shell.setLayout(new GridLayout(1, false));
		Button buttonPrintBounds = new Button(shell, SWT.NONE);
		buttonPrintBounds.setText("Print Bounds");
		buttonPrintBounds.addSelectionListener(SelectionListener.widgetSelectedAdapter(event -> {
			System.out.println("display bounds:  " + display.getBounds());
			System.out.println("shell bounds:    " + shell.getBounds());
			Monitor primaryMonitor = display.getPrimaryMonitor();
			for (int i = 0; i < display.getMonitors().length; i++) {
				Monitor monitor = display.getMonitors()[i];
				System.out.println("monitor" + i + " bounds: " + monitor.getBounds() + " zoom: " + monitor.getZoom()
						+ (Objects.equals(monitor, primaryMonitor) ? " [primary]" : ""));
			}
		}));
		Button buttonScreenshot1 = new Button(shell, SWT.NONE);
		buttonScreenshot1.setText("Screenshot (without workaround)");
		Button buttonScreenshot2 = new Button(shell, SWT.NONE);
		buttonScreenshot2.setText("Screenshot (with workaround)");
		SelectionListener widgetSelectedAdapter = SelectionListener.widgetSelectedAdapter(event -> {
			Path tempFile;
			try {
				tempFile = Files.createTempFile("screenshot", ".png");
			} catch (IOException e) {
				throw new IllegalStateException(e);
			}
			Rectangle rectangle = display.getBounds();
			int factor = 1;
			if ("win32".equals(SWT.getPlatform()) && event.getSource() == buttonScreenshot2) {
				// display.getBounds(); on windows currently is non-deterministic, but uses DPIUtil.getDeviceZoom(); internally
				// which itself depends refers to the "zoom of the last Shell that changed zoom"
				// but gc.copyArea() lateron works with the zoom of the primary monitor.
				// we have to workaround that
				factor = DPIUtil.getDeviceZoom() / display.getPrimaryMonitor().getZoom();
			}
			Image image = new Image(display, rectangle.width * factor, rectangle.height * factor);
			GC gc = new GC(display);
			gc.copyArea(image, rectangle.x * factor, rectangle.y * factor);
			ImageData imageData = image.getImageData();
			image.dispose();
			gc.dispose();
			ImageLoader imageLoader = new ImageLoader();
			imageLoader.data = new ImageData[] { imageData };
			imageLoader.save(tempFile.toAbsolutePath().toString(), SWT.IMAGE_PNG);
			System.out.println(tempFile + " (" + imageData.width + "x" + imageData.height + ")");
		});
		buttonScreenshot1.addSelectionListener(widgetSelectedAdapter);
		buttonScreenshot2.addSelectionListener(widgetSelectedAdapter);

		shell.open();
		while (!shell.isDisposed()) {
			if (!display.readAndDispatch()) {
				display.sleep();
			}
		}
		display.dispose();
	}
}

• Move the opened shell to the primary (100%) monitor.
• Press Print Bounds / Screenshot (without workaround) / Screenshot (with workaround) buttons once each
• Move the opened shell to the secondary (200%) monitor.
• Press Print Bounds / Screenshot (without workaround) / Screenshot (with workaround) buttons once each again

Check console:

display bounds:  Rectangle {0, 0, 5360, 1440}
shell bounds:    Rectangle {1559, 498, 300, 200}
monitor0 bounds: Rectangle {3440, 236, 960, 600} zoom: 200
monitor1 bounds: Rectangle {0, 0, 3440, 1440} zoom: 100 [primary]
C:\Users\user\AppData\Local\Temp\screenshot925734453198280273.png (5360x1440)
C:\Users\user\AppData\Local\Temp\screenshot287834727171878990.png (5360x1440)
display bounds:  Rectangle {0, 0, 2680, 720}
shell bounds:    Rectangle {3822, 403, 300, 200}
monitor0 bounds: Rectangle {3440, 236, 960, 600} zoom: 200
monitor1 bounds: Rectangle {0, 0, 3440, 1440} zoom: 100 [primary]
C:\Users\user\AppData\Local\Temp\screenshot64546334595754013261.png (2680x720)
C:\Users\user\AppData\Local\Temp\screenshot14362251863936518615.png (5360x1440)

When using the Screenshot (without workaround) button, while the little helper program's shell is currently on the 2nd monitor, the screenshot created is only half the expected size and therefore cut-off.

Expected behavior
Screenshots should always be created with size 5360x1440, covering the whole virtual screen area.
Display.getBounds() should return consistent values - no matter which Shell was last moved where, as the Display logically comprises all monitors and there is no notion of such a last-used Shell in that context.

Maybe

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

Line 1595 in 3db6f5d

return Win32DPIUtils.pixelToPoint(getBoundsInPixels(), DPIUtil.getDeviceZoom());

should be replaced with something like

return Win32DPIUtils.pixelToPoint(getBoundsInPixels(), getPrimaryMonitor().getZoom());

to be consistent and compatible with the coordinate system used by GC.copyArea()?

The workaround applied in the snippet uses internal API.

Environment:

1. Select the platform(s) on which the behavior is seen:

• • All OS
• • Windows
• • Linux
• • macOS

2. Additional OS info (e.g. OS version, Linux Desktop, etc)

3. JRE/JDK version

Version since
Tested against 4.41 I-builds.

[HeikoKlare]

Good catch. Everything explained sounds absolutely reasonable to me. The GC zoom initialization for a Display that the proposed change is based on is this:

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

Line 2806 in 3db6f5d

data.nativeZoom = getPrimaryMonitor().getZoom();

I wondered if we could somehow centralize this implicit display zoom information, as we would otherwise have the assumption that primary monitor zoom is used as display zoom spread across the code. There is already the Display.getDeviceZoom() method which seems to return exactly the desired value. So maybe just changing DPIUtil.getDeviceZoom() to getDeviceZoom() would properly resolve the issue without redundant implementations of assumptions?

Two additional notest thoughts/notes:

• Display.getClientArea() is affected in the same way: it also still uses DPIUtil.getDeviceZoom() which should probably be fixed in the same way.
• As a larger alternative we also could think about changing the semantics of DPIUtil.get[Native]DeviceZoom() to return the primary monitor zoom instead of the zoom of the monitor to which a shell was moved last. The current implementation is based on the idea that the returned zoom is the one that is most likely required (e.g., for resources) that upon creation do not know the zoom at which they will later be used. So we considered it a reasonable heuristics. But it also makes these values more difficult to understand and use, and in case of multi-shell scenarios the heuristics is limited anyway. And the usages of the method have also been significantly reduced. But I do not know about concrete impacts of such a change yet and it would definitely be more invasive than the proposal above. So if at all, it should be considered as a separate change/enhancement.

[sratz]

So maybe just changing DPIUtil.getDeviceZoom() to getDeviceZoom() would properly resolve the issue without redundant implementations of assumptions?

Ah, yes, I think that's the correct API.

As a larger alternative we also could think about changing the semantics of DPIUtil.get[Native]DeviceZoom() to return the primary monitor zoom instead of the zoom of the monitor to which a shell was moved last. The current implementation is based on the idea that the returned zoom is the one that is most likely required (e.g., for resources) that upon creation do not know the zoom at which they will later be used [...]

Could be an option, but I agree, better to not touch that in the correction for this issue.

Overall, I think much of the confusion arises from the overloaded method names with different semantics:

• DPIUtil.getDeviceZoom() and Display.getDeviceZoom() have very similar names but have completely different semantics
• DPIUtil is used for multiple operating systems, so the methods have slightly different meanings even across those.

So maybe one potential path forward could also be to

• rename some of the methods to make it clearer what they mean
• maybe introduce OS-specific methods for the Windows specialties, e.g. DPIUtil.getDeviceZoom() could be named DPIUtilWindows.getZoomOfLastScaledShell() or something to make it clearer.

[HeikoKlare]

The usage of getDeviceZoom() with different semantics in different contexts is a valid concern. This is long standing behavior, at least since 2018. Unfortunately, the method in Display is API and cannot be renamed. The method in DPIUtil may be renamed as it is not API, but to the best of my knowledge, quite some consumer access these internals and may break when renaming. But I think we could simply align the semantics of both methods here (in particular the behavior of DPIUtil.getDeviceZoom() on Windows) to resolve this, supported by adding an OS-specific method for the current behavior of that method, as you propose later on:

So maybe one potential path forward could also be to

• rename some of the methods to make it clearer what they mean
• maybe introduce OS-specific methods for the Windows specialties, e.g. DPIUtil.getDeviceZoom() could be named DPIUtilWindows.getZoomOfLastScaledShell() or something to make it clearer.

Fully agree. We already have Win32DPIUtils for exactly such OS- and DPI-specific behavior.