[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: [OpenJDK 2D-Dev] Integrated: 8211999: Window positioning bugs due to overlapping GraphicsDevice boun
From: Sergey Bylokhov <serb () openjdk ! java ! net>
Date: 2020-11-11 1:34:57
Message-ID: 6xdyT0w04J4Q21Cy5LLc2gRnOjyjCHgvDzV6tn9g1KI=.c549d830-c144-4d8e-b4be-464ff836ced0 () github ! com
[Download RAW message or body]
On Sun, 27 Sep 2020 22:16:22 GMT, Sergey Bylokhov <serb@openjdk.org> wrote:
> Hello.
> Please review the fix for jdk.
>
> Old review request:
> https://mail.openjdk.java.net/pipermail/awt-dev/2020-July/015991.html
>
>
> (Note: the fix use API available since Windows 8.1: WM_DPICHANGED, but it should be \
> fine for Windows 7, because it does not support different DPI for different \
> monitors)
> ========================================================
> Short description:
> In the multi-screen configurations when each screen have different DPI
> we calculate the bounds of each monitor by these formulas:
>
> userSpaceBounds = deviceX / scaleX, deviceY / scaleY, deviceW / scaleX, deviceH / \
> scaleY devSpaceBounds = userX * scaleX, userY * scaleY, userW * scaleX, userH * \
> scaleY
> This formula makes the next configuration completely broken:
> - The main screen on the left and 100% DPI
> - The second screen on the right and 200% DPI
> When we translate the bounds of the config from the device space to the user's \
> space, the bounds of both screen overlap in the user's space, because we use bounds \
> of the main screen as-is, and the X/Y of the second screen are divided by the \
> scaleX/Y.
> Since the screens are overlapped we cannot be sure how to translate the user's \
> space coordinates to device space in the overlapped zone.
> As a result => we use the wrong screen
> => got wrong coordinates in the device space
> => show top-level windows/popups/tooltips/menus/etc on the wrong screen
>
> The proposed solution for this bug is to change the formulas to these:
>
> userSpaceBounds = deviceX, deviceY, deviceW / scaleX, deviceH / scaleY
> devSpaceBounds = userX, userY, userW * scaleX, userH * scaleY
>
> In other words, we should not transform the X and Y coordinates of the screen(the \
> top/left corner). This will complicate the way of how we transform coordinates on \
> the screen: user's <--> device spaces:
> Before the fix:
>
> userX = deviceX * scaleX;
> deviceX = userX / scaleX;
>
> After the fix(only the part appeared on this screen should be scaled)
>
> userX = screenX + (deviceX - screenX) * scaleX
> deviceX = screenX + (userX - screenX) / scaleX
>
> Note that these new formulas are applicable only for the coordinates on the screen \
> such as X and Y. If we will need to calculate the "size" such as W and H then the \
> old formula should be used.
> The main changes for the problem above are:
> - Skip transformation of X and Y of the screen bounds:
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsConfig.cpp.sdiff.html
>
> - A number of utility methods in java and native:
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp.sdiff.html
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java.sdiff.html
>
>
> ========================================================
> Long description:
> Unfortunately, the changes above are not enough to fix the problem when different \
> monitors have different DPI, even if the bounds are *NOT* overlapped.
>
> - Currently, when we try to set the bounds of the window, we manually convert it in \
> "java" to the expected device position based on the current GraphicsConfiguration \
> of the peer, and then additionally, tweak it in native using the device scale \
> stored in native code. Unfortunately this two scale might not be in sync:(after we \
> use the GraphicsConfiguration scale in peer, the config might be changed and the \
> tweak in native will use a different screen).
> As a fix I have moved all transformation from the peer to the native code, it will \
> be executed on the toolkit thread:
> See the change in WWindowPeer.setBounds() and awt_Window.cpp AwtWindow::Reshape
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/classes/sun/awt/windows/WWindowPeer.java.sdiff.html
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp.sdiff.html
> I think at some point we should delete all transformation in java, and apply it in \
> the native code only.
>
> - We had a special code that tracked the dragging of the window by the user from \
> one screen to another, and change the size of the window only when the user stops \
> the drag operation. I've proposed to use standard Windows machinery for that via \
> WM_DPICHANGED: https://docs.microsoft.com/en-us/windows/win32/hidpi/wm-dpichanged \
> As a result, Windows will provide the "best" windows bounds on the new screen. Note \
> that the code has a workaround for https://bugs.openjdk.java.net/browse/JDK-8249164
>
> - I've also fix a variation of the bug previously fixed on macOS \
> https://hg.openjdk.java.net/jdk/jdk/rev/b5cdba232fca If we try to change the \
> position of the window, and Windows ignores this request then we need to call all \
> "callbacks" manually, otherwise, the shared code will use the bounds ignored by the \
> Windows. See the end of void AwtWindow::Reshape():
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp.sdiff.html
>
> - Currently the HW componets such as Canvas scales everything based on such code:
>
> 770 int AwtComponent::ScaleUpX(int x) {
> 4771 int screen = AwtWin32GraphicsDevice::DeviceIndexForWindow(GetHWnd());
> 4772 Devices::InstanceAccess devices;
> 4773 AwtWin32GraphicsDevice* device = devices->GetDevice(screen);
> 4774 return device == NULL ? x : device->ScaleUpX(x);
> 4775 }
>
> But it does not work well if the smaller part of the top-level window is located on \
> one screen1(DPI=100) but the biggest part is located on the screen2(DPI=200). If \
> the Canvas is located on the "smaller" part of the window, then the canvas will use \
> DPI=100 for scale, but the whole window will use DPI=200
> As a fix, all HW components will try to use the scale factor of the top-level \
> window, see AwtComponent::GetScreenImOn: \
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/native/libawt/windows/awt_Component.cpp.sdiff.html
>
> - Our current implementation does not take care of the case when the HW component \
> bounds are updated when the top-level the window is moved to another screen. For \
> example, if the window does not use LayoutManager and the user sets some specific \
> bounds for the Canvas. It is expected that the size of the Canvas will be updated \
> when the window will be moved to another screen, but only HW top-level window and \
> Swing components will update its size.
> As a fix I suggest to resync the bounds of the HW components if the \
> GraphicsCOnfiguration is changed, see WComponentPeer.syncBounds(): \
> http://cr.openjdk.java.net/~serb/8211999/webrev.04/src/java.desktop/windows/classes/sun/awt/windows/WComponentPeer.java.sdiff.html
>
> - Note that the current fix is for Windows only, but the bug exists on Linux as \
> well, so I have to create a kind of "ifdef" in the Robot class, on windows we will \
> use the new logic, on other platforms the old logic will be used.
> - The win32GraphicsDevice incorrectly sets the size of the full-screen window. It \
> uses the display mode width/height(which are stored in pixels), but the bounds of \
> the graphics config(which are stored in user's space) should be used.
> ========================================================
> Some other bugs which are not fixed.
>
> - We have a lot of places where we mix(unintentionally) the coordinate in user's \
> space and device space. For example when we create the component we read \
> x/y/width/height by the JNI(of course in a user's space) but pass this coordinates \
> as-is to the ::CreateWindowEx() which is incorrect. It works currently because \
> later we reshape the component to the correct bounds. Will fix it later.
> - When the frame is iconized and we try to save a new bounds for the future use, we \
> store the coordinates in the user's space to the field which use device space:
> https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#L664
>
> Probably there are some other bugs and possibility to cleanups, but I would like to \
> do that in separate issues.
>
> ========================================================
> The tests
> - I have updated a couple of the tests to check multiple screens if possible, it \
> helps to found some bugs in my initial implementation
> - Four new tests were added: SetComponentsBounds, SlowMotion, \
> WindowSizeDifferentScreens, FullscreenWindowProps
> - One test is moved from the closed repo(I made a small cleanup of it): \
> ListMultipleSelectTest
> ========================================================
> I have run jck, regression tests in different configurations, when the main screen \
> is on the left, up, down, right, and have different DPI such as 100, 125, 150, and \
> 200. No new issues were found so far.
> The mach5 is also green.
>
> PS: hope I did not forget some important information, will send it later if any.
This pull request has now been integrated.
Changeset: be635258
Author: Sergey Bylokhov <serb@openjdk.org>
URL: https://git.openjdk.java.net/jdk/commit/be635258
Stats: 1393 lines in 35 files changed: 962 ins; 276 del; 155 mod
8211999: Window positioning bugs due to overlapping GraphicsDevice bounds \
(Windows/HiDPI)
Reviewed-by: kizune, aivanov
-------------
PR: https://git.openjdk.java.net/jdk/pull/375
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic