Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI) #375

Closed
wants to merge 14 commits into from

Conversation

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 27, 2020

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:

========================================================
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.

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI)

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/375/head:pull/375
$ git checkout pull/375

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 27, 2020

👋 Welcome back serb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 27, 2020

@mrserb The following labels will be automatically applied to this pull request: 2d awt swing.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@mrserb mrserb marked this pull request as ready for review Sep 27, 2020
@openjdk openjdk bot added the rfr label Sep 27, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 27, 2020

Webrevs

@victordyakov
Copy link

@victordyakov victordyakov commented Oct 2, 2020

@prsadhuk can you review this?

@victordyakov
Copy link

@victordyakov victordyakov commented Oct 2, 2020

@prrace Can you review this?

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Oct 29, 2020

I'm trying to test these changes but i have weird problems with mouse event coordinate translation. For example - i have two displays side by side with some vertical shift and with different scale factor. After this fix if the window spans across multiple monitors when i move cursor over the part on the secondary monitor related to the main window position mouse events got sent to the wrong component. Look at the attached image - by the position of the tooltip you can see that the mouse is hovering over the menu bar area - namely over "Options" menu, yet the tooltip is from the JProgressBar demo and if you look at the toolbar you will see JProgressBar demo button highlighted as if mouse is on top of it. And if i click instead of selecting the menu the button got pressed. Not sure why it happens but seems like it was not happened before the fix.
events

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 29, 2020

You found this bug https://bugs.openjdk.java.net/browse/JDK-8249164 the case when the window is split across two screens with different DPI are not properly supported before and after the fix, in both cases, there are values which break it. But I need to confirm that, please provide exact display resolution/java pipeline/and scales.
BTW this is the case why thr JUMP TO machinery was added to the "awt_Window.cpp"

@openjdk
Copy link

@openjdk openjdk bot commented Nov 10, 2020

@mrserb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8211999: Window positioning bugs due to overlapping GraphicsDevice bounds (Windows/HiDPI)

Reviewed-by: kizune, aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • d6f1463: 8233332: Need to create exploded tests covering all forms of modules
  • f2a0bf3: 8256017: Remove unused elapsedTimer constructor
  • 7d4e86b: 8138588: VerifyMergedCPBytecodes option cleanup needed

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 10, 2020
*
* @param config the graphics configuration which bounds are requested
* @return the bounds of the area covered by this
* {@code GraphicsConfiguration} in device space(pixels).
Copy link
Member

@aivanov-jdk aivanov-jdk Nov 10, 2020

Suggested change
* {@code GraphicsConfiguration} in device space(pixels).
* {@code GraphicsConfiguration} in device space (pixels).

If changed here, all other similar places should be updated too to keep doc comments consistent.

Copy link
Member Author

@mrserb mrserb Nov 10, 2020

I'll fix it and also merge the master, then update the PR.

Copy link
Member

@aivanov-jdk aivanov-jdk Nov 10, 2020

In this case, also consider adding a space between the word and the opening parenthesis in these coordinates (x, y) and size (w, h) and, probably, a space after comma.

Copy link
Member Author

@mrserb mrserb Nov 10, 2020

Spaces are added.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Nov 11, 2020

/integrate

@openjdk openjdk bot closed this Nov 11, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 11, 2020

@mrserb Since your change was applied there have been 4 commits pushed to the master branch:

  • 0a41ca6: 8254354: Add a withInvokeExactBehavior() VarHandle combinator
  • d6f1463: 8233332: Need to create exploded tests covering all forms of modules
  • f2a0bf3: 8256017: Remove unused elapsedTimer constructor
  • 7d4e86b: 8138588: VerifyMergedCPBytecodes option cleanup needed

Your commit was automatically rebased without conflicts.

Pushed as commit be63525.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk-notifier bot referenced this issue Nov 11, 2020
…unds (Windows/HiDPI)

Reviewed-by: kizune, aivanov
@mrserb mrserb deleted the JDK-8211999 branch Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants