[OpenJDK 2D-Dev] RFR: 8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI) [v4]
Alexey Ivanov
aivanov at openjdk.java.net
Tue Nov 10 09:54:59 UTC 2020
On Wed, 28 Oct 2020 23:46:55 GMT, Sergey Bylokhov <serb at 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.
>
> Sergey Bylokhov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits:
>
> - Update WindowSizeDifferentScreens.java
> - Update ListMultipleSelectTest.java
> - syncBounds code reformat
> - javadoc fix for SunGraphicsEnvironment#toDeviceSpace
> - Merge branch 'master' into JDK-8211999
> - Apply suggestions from code review
>
> Co-authored-by: Aleksei Ivanov <70774172+aivanov-jdk at users.noreply.github.com>
> - Merge branch 'master' into JDK-8211999
> - Update FullscreenWindowProps.java
> - Merge branch 'master' into JDK-8211999
> - Fix fullscreen in HiDPI mode
> - ... and 2 more: https://git.openjdk.java.net/jdk/compare/1a5e6c98...636b7cc4
Marked as reviewed by aivanov (Reviewer).
src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java line 357:
> 355: * @param config the graphics configuration which bounds are requested
> 356: * @return the bounds of the area covered by this
> 357: * {@code GraphicsConfiguration} in device space(pixels).
Suggestion:
* {@code GraphicsConfiguration} in device space (pixels).
If changed here, all other similar places should be updated too to keep doc comments consistent.
-------------
PR: https://git.openjdk.java.net/jdk/pull/375
More information about the 2d-dev
mailing list