> 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 ------------- Changes: https://git.openjdk.java.net/jdk/pull/375/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=375&range=01 Stats: 1397 lines in 35 files changed: 965 ins; 280 del; 152 mod Patch: https://git.openjdk.java.net/jdk/pull/375.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/375/head:pull/375 PR: https://git.openjdk.java.net/jdk/pull/375
