On Sun, 4 Oct 2020 06:22:45 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 six commits: > > - Merge branch 'master' into JDK-8211999 > - Update FullscreenWindowProps.java > - Merge branch 'master' into JDK-8211999 > - Fix fullscreen in HiDPI mode > - self review > - Initial fix version src/java.desktop/share/classes/sun/java2d/SunGraphicsEnvironment.java line 445: > 443: * @return the point which uses device space(pixels) > 444: */ > 445: public static Point toDeviceSpace(int x, int y) { Does this function converts _absolute_ coordinates? I see it uses a different formula to convert: the coordinates are scaled as opposed to `-Abs` set of functions. src/java.desktop/windows/classes/sun/awt/windows/WComponentPeer.java line 539: > 537: winGraphicsConfig = (Win32GraphicsConfig)gc; > 538: if (gc != null && !old.equals(gc.getDefaultTransform())) { > 539: syncBounds(); // the bound of the peer depends on the DPI Suggestion: syncBounds(); // the bounds of the peer depend on the DPI src/java.desktop/windows/classes/sun/awt/windows/WWindowPeer.java line 311: > 309: > 310: final void syncBounds(){ > 311: // The Windows will take care of the top-level > window/frame/dialog, Suggestion: final void syncBounds() { // Windows will take care of the top-level window/frame/dialog, Does it override another implementation of `syncBounds()`? Or does it implement a method in an interface? Shall it have `@Override` annotation? src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line 692: > 690: } > 691: > 692: void AwtWin32GraphicsDevice::InitDesktopScales() { Suggestion: void AwtWin32GraphicsDevice::InitDesktopScales() { Shall the opening brace remain on the next line as it was for consistency with other functions? src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 1211: > 1209: int screen = AwtWin32GraphicsDevice::GetScreenFromHMONITOR(monitor); > 1210: AwtWin32GraphicsDevice *device = devices->GetDevice(screen); > 1211: // Try set the correct size and jump to the correct location, even > if it is Suggestion: // Try to set the correct size and jump to the correct location, even if it is src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2197: > 2195: // The shared code is not ready to the top-level window which crosses a > few > 2196: // monitors with different DPI. Popup windows will start to use wrong > screen, > 2197: // will be placed in the wrong place and will be use wrong size, see > 8249164 Suggestion: // will be placed in the wrong place and will use the wrong size, see 8249164 src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 2217: > 2215: y = y < bounds.top ? bounds.top : y; > 2216: x = (x + w > bounds.right) ? bounds.right - w : x; > 2217: y = (y + h > bounds.bottom) ? bounds.bottom - h : y; Can't this adjustment cause `x`, `y` to become less than `bounds.left` and `bounds.top` correspondingly? Shall it adjust the width and height? src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3987: > 3985: } > 3986: > 3987: } /* extern "C" */ Probably, it's better to preserve the line end at the end of the file. test/jdk/java/awt/Component/SetComponentsBounds/SetComponentsBounds.java line 102: > 100: } > 101: if (bounds.height > HEIGHT) { > 102: // different check for HEIGHT, it depends from the > font Suggestion: // different check for HEIGHT, it depends on the font test/jdk/java/awt/List/ListMultipleSelectTest/ListMultipleSelectTest.java line 90: > 88: for (int i = 0; i < aList.getItemCount(); i++) { > 89: //select all items in the List > 90: mousePress(p.x + listSize.height / 2, p.y + stepY / 2 + > stepY * i); x be based on width? mousePress(p.x + listSize.width / 2, p.y + stepY / 2 + stepY * i); test/jdk/java/awt/Window/WindowSizeDifferentScreens/WindowSizeDifferentScreens.java line 110: > 108: case "dialog" -> new Dialog((Dialog) null); > 109: case "frame" -> new Frame(); > 110: default -> throw new IllegalStateException("Unexpected: " + > top); `IllegalArgumentException` more appropriate? default -> throw new IllegalArgumentException("Unexpected: " + top); test/jdk/javax/swing/JTextArea/8149849/DNDTextToScaledArea.java line 81: > 79: robot.mousePress(InputEvent.BUTTON1_MASK); > 80: robot.mouseRelease(InputEvent.BUTTON1_MASK); > 81: robot.waitForIdle(); Why is clicking the `dstTextArea` required? ------------- PR: https://git.openjdk.java.net/jdk/pull/375
