On Wed, 28 Oct 2020 23:46:55 GMT, Sergey Bylokhov <s...@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-...@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 kizune (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/375
