8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors #924

johanvos · 2022-10-20T14:37:04Z

The root problem is actually broader than stated in the JBS issue. This PR now translates screencoordinates from absolute coordinates into coordinates that take the platformScale into account.
The whole process is complicated by the fact that throughout our code, we use e.g. x and y without clearly stating if those are absolute, logical, screen or rendering coordinates.
I believe the most consistent approach is to have the different entry points (e.g. a Glass Window or a JFXPanel) to deal with platformScale before passing screen coordinates. This is already done in the Glass approach, and this PR does the same in JFXPanel. That means some code is duplicated, but since this is only about 12 lines, and said code lives in 2 different modules, I think it's not worth the hassle of moving that into e.g. the base module.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Author)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/924/head:pull/924
$ git checkout pull/924

Update a local copy of the PR:
$ git checkout pull/924
$ git pull https://git.openjdk.org/jfx pull/924/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 924

View PR using the GUI difftool:
$ git pr show -t 924

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/924.diff

Swing to JavaFX Fix for JDK-8222210

bridgekeeper · 2022-10-20T14:38:07Z

👋 Welcome back jvos! 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.

mlbridge · 2022-10-20T14:44:39Z

Webrevs

andy-goryachev-oracle · 2022-10-20T16:02:46Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

+        Rectangle awtBounds = graphicsConfiguration.getBounds();
+        AffineTransform awtScales = graphicsConfiguration.getDefaultTransform();
+        for (Screen screen : Screen.getScreens()) {
+            if ((Math.abs(screen.getPlatformX() - awtBounds.getX() * awtScales.getScaleX()) < 0.001) &&


minor: would it be better to create a standard method?

isNearZero(double)?

although the value of the constant might depend on a situation.

it's a good question, and I don't know the "best" answer. What we want to achieve here is to get the FX Screen that matches the AWT graphicsConfiguration. I assume that the screen.getPlatformX() being equal to the bounds.getX() etc, but I have no evidence for it. I could not find clear definitions of either the JavaFX Screen parameters nor the AWT concepts. In practice, the approach above (which I tweaked because at least on my Ubuntu, there are differences between the JavaFX platformScale and the AWT scales) works correctly. However, as I don't see any clear definition, I think it is safe to account for rounding errors (given the mixture of int, float, doubles that are used in the different areas).

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

andy-goryachev-oracle · 2022-10-20T16:06:36Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

@@ -407,14 +446,17 @@ private void sendMouseEventToFX(MouseEvent e) {
        if (e.getID() == MouseEvent.MOUSE_PRESSED || e.getID() == MouseEvent.MOUSE_RELEASED) {
            popupTrigger = e.isPopupTrigger();
        }
+        Dimension2D onScreen = getSwingToFxPixel(getGraphicsConfiguration(), e.getXOnScreen(), e.getYOnScreen());
+        int fxXOnScreen = (int) onScreen.getWidth();
+        int fxYOnScreen = (int) onScreen.getHeight();


question: should it be (int) or Math.round()?
also, coordinates can be negative - will it work then?

I believe these values should be int in the first place. Otherwise, we have issues in JavaFX where we process them as ints.
About negative coordinates: yes, that works. Is the question about using Dimension2D (width/height) for something that is actually a Point2D? We can use a Point2D here as well, but that class seems to have a bit more overhead than Dimension2D for this goal -- but I'm open to change it to Point2D.

No, what I meant is this:

System.out.println("(int)-1.9999=" + (int)-1.9999 + " (int)Math.round(-1.9999)=" + (int)(Math.round(-1.9999))); prints (int)-1.9999=-1 (int)Math.round(-1.9999)=-2

i.e. typecast to int is equivalent to Math.ceil for negative values and Math.floor for positive. We probably should use Math.round().

This code should be better tested on Windows with
a) second monitor positioned to the left of the main one, making its coordinates negative
b) using fractional HiDPI scaling which is only available on Windows it seems

And yes, I did not see Dimension2D - we probably should remove confusion and use Point2D (why does it have more overhead?)

Depending on the desired result, we use Math.floor, Math.ceil, or Math.round -- it's important to pick the right one.

But yes, using one of those is (almost always) better than just casting to (int) if the value might be negative.

I think in this case, Math.floor() is what you want. I wonder, though, whether it needs to be converted to an int at all? It looks like everywhere it is used, it can be kept as a double.

Btw, I almost wrote a comment asking a question as to whether you were using the values of x, y, width, and height correctly, because I got confused that you were using a Dimension to hold an x,y point. If you decide to keep it as a Dimension, I might suggest adding a clarifying comment.

We need the int values, as they are used to invoke mouseEvent, scrollEvent and menuEvent on com.sun.javafx.embed.EmbeddedSceneInterface .
For some reason, scrollEvent takes doubles, but mouseEvent and menuEvent take ints.
I'll use Math.floor() instead of the int cast. The key thing here is that the strategy for the mouse/scroll/menu events should be the same as the strategy for the window location changes.

And yes, I did not see Dimension2D - we probably should remove confusion and use Point2D (why does it have more overhead?)

Point2D provides much more functionality -- but has 2 fields only. So it should not be an overhead at all, hence I'll switch to Point2D. Thanks for Pointing that out :)

I suspect Math.floor() would be incorrect - we should use Math.round() for coordinates and Math.floor() for sizes (sizes will always be positive, I hope, and the rounded size will be slightly smaller than the original value).

But for coordinates we should use (int)Math.round()

I'll change it to Math.round, although I doubt it doesn't improve anything (but it won't make it worse either).
The reason we need the transformation on those coordinates is to position new windows, relative against the location of the root window that we are given by Swing. Hence, if the operation applied to the window location coordinates (in sendMoveEventToFX) is the same as the operation applied to the mouse point (in sendMouseEventToFX), we are good.
A bigger problem imho is that mouseEvents, scrollEvents and menuEvents are dealing with different precisions in the EmbeddedSceneInterface -- but that is a different problem.

I also expect that it won't make much of a difference here, but Andy is right that, for snapping positions, Math.round is preferred. That's what layout panes do in Region::snapPositionX/Y

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

kevinrushforth · 2022-10-22T12:15:32Z

/reviewers 2

openjdk · 2022-10-22T12:16:30Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Fix detection on screen, based on awtScale factors

kevinrushforth

I think this looks basically correct, although I did leave a couple questions / comments inline. I'll test it next week.

kevinrushforth · 2022-10-28T23:29:36Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

@@ -407,14 +446,17 @@ private void sendMouseEventToFX(MouseEvent e) {
        if (e.getID() == MouseEvent.MOUSE_PRESSED || e.getID() == MouseEvent.MOUSE_RELEASED) {
            popupTrigger = e.isPopupTrigger();
        }
+        Dimension2D onScreen = getSwingToFxPixel(getGraphicsConfiguration(), e.getXOnScreen(), e.getYOnScreen());
+        int fxXOnScreen = (int) onScreen.getWidth();
+        int fxYOnScreen = (int) onScreen.getHeight();


I think in this case, Math.floor() is what you want. I wonder, though, whether it needs to be converted to an int at all? It looks like everywhere it is used, it can be kept as a double.

Btw, I almost wrote a comment asking a question as to whether you were using the values of x, y, width, and height correctly, because I got confused that you were using a Dimension to hold an x,y point. If you decide to keep it as a Dimension, I might suggest adding a clarifying comment.

kevinrushforth · 2022-10-28T23:33:26Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

+
+    private Dimension2D convertSwingToFxPixel(GraphicsConfiguration g, float wx, float wy) {
+        float newx, newy;
+        Screen screen = findScreen(getGraphicsConfiguration());


Did you mean to use the passed in graphics config, g? If not, you might remove the g parameter, since it is otherwise unused.

kevinrushforth · 2022-10-28T23:42:30Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

+            newx = sx + (wx - px) * awtScaleX / pScaleX;
+            newy = sy + (wy - py) * awtScaleY / pScaleY;


px and py are already (scaled) "FX" coordinates, so I'm not sure AWT scale should be applied to that. I might be missing something here.

Without this correction, there is an offset on Linux (when awtScaleX = 1 and pScaleX =2 for example, versus 1.5 for both on Windows).
TBH, I think the relations between the different scale factors should be written out clearly somewhere, as I think there are a number of places where we face platform-specific differences.

Yeah, I agree this should be more clearly documented. I've tested it on macOS and Windows and it works fine, so I expect that this is correct as you have it.

kevinrushforth

Looks good. If you want to switch to using Math.round I'll reapprove.

kevinrushforth · 2022-11-04T17:15:37Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

+            newx = sx + (wx - px) * awtScaleX / pScaleX;
+            newy = sy + (wy - py) * awtScaleY / pScaleY;


Yeah, I agree this should be more clearly documented. I've tested it on macOS and Windows and it works fine, so I expect that this is correct as you have it.

kevinrushforth · 2022-11-04T17:37:32Z

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

@@ -407,14 +446,17 @@ private void sendMouseEventToFX(MouseEvent e) {
        if (e.getID() == MouseEvent.MOUSE_PRESSED || e.getID() == MouseEvent.MOUSE_RELEASED) {
            popupTrigger = e.isPopupTrigger();
        }
+        Dimension2D onScreen = getSwingToFxPixel(getGraphicsConfiguration(), e.getXOnScreen(), e.getYOnScreen());
+        int fxXOnScreen = (int) onScreen.getWidth();
+        int fxYOnScreen = (int) onScreen.getHeight();


I also expect that it won't make much of a difference here, but Andy is right that, for snapping positions, Math.round is preferred. That's what layout panes do in Region::snapPositionX/Y

…or coordinates

openjdk · 2022-11-08T18:27:28Z

@johanvos 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:

8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors

Reviewed-by: kcr, angorya

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 13 new commits pushed to the master branch:

b08f135: 8296283: JUnit5 tests using Params API fails to compile
7f17447: 8290844: Add Skin.install() method
01735b3: 8265835: Exception in Quantum due to null platformWindow
748107a: 8295327: JavaFX - IllegalArgumentException when printing with margins equal to 0
ac8382b: 8294400: Provide media support for libavcodec version 59
4a2afb4: 8295962: Reference to State in Task.java is ambiguous when building with JDK 19
91e1a27: 8278426: ImagePool uses terminally deprecated System.runFinalization method
60a7915: 8295236: Update JavaDoc in javafx.geometry.Point3D
... and 3 more: https://git.openjdk.org/jfx/compare/35675c8d27d54a26059b182614e18152794dbcec...master

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.

johanvos · 2022-11-10T12:03:28Z

/integrate

openjdk · 2022-11-10T12:06:03Z

Going to push as commit f4e27e9.
Since your change was applied there have been 14 commits pushed to the master branch:

706261b: 8296592: Skip failing test StraightLineTest on Linux
b08f135: 8296283: JUnit5 tests using Params API fails to compile
7f17447: 8290844: Add Skin.install() method
01735b3: 8265835: Exception in Quantum due to null platformWindow
748107a: 8295327: JavaFX - IllegalArgumentException when printing with margins equal to 0
ac8382b: 8294400: Provide media support for libavcodec version 59
4a2afb4: 8295962: Reference to State in Task.java is ambiguous when building with JDK 19
91e1a27: 8278426: ImagePool uses terminally deprecated System.runFinalization method
60a7915: 8295236: Update JavaDoc in javafx.geometry.Point3D
... and 4 more: https://git.openjdk.org/jfx/compare/35675c8d27d54a26059b182614e18152794dbcec...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-10T12:06:28Z

@johanvos Pushed as commit f4e27e9.

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

Transform coordinates using platform scale if needed when bridging from

0458c74

Swing to JavaFX Fix for JDK-8222210

openjdk bot added the rfr Ready for review label Oct 20, 2022

andy-goryachev-oracle reviewed Oct 20, 2022

View reviewed changes

kevinrushforth requested review from kevinrushforth and arapte October 22, 2022 12:15

address reviewer comments

af931be

Fix detection on screen, based on awtScale factors

kevinrushforth reviewed Oct 28, 2022

View reviewed changes

Process reviewer comments

783c45a

kevinrushforth self-requested a review November 2, 2022 21:26

kevinrushforth approved these changes Nov 4, 2022

View reviewed changes

arapte mentioned this pull request Nov 7, 2022

8296283: JUnit5 tests using Params API fails to compile #939

Closed

3 tasks

Process reviewer comments. Use Math.round() instead of Math.floor() f…

4dd021c

…or coordinates

kevinrushforth approved these changes Nov 8, 2022

View reviewed changes

andy-goryachev-oracle approved these changes Nov 8, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Nov 8, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 10, 2022

openjdk bot closed this Nov 10, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Nov 10, 2022

johanvos deleted the 8222210-jfxpanel branch November 14, 2022 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors #924

8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors #924

johanvos commented Oct 20, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 20, 2022

mlbridge bot commented Oct 20, 2022 •

edited

Loading

andy-goryachev-oracle Oct 20, 2022

johanvos Oct 28, 2022

andy-goryachev-oracle Oct 20, 2022

johanvos Oct 28, 2022

andy-goryachev-oracle Oct 28, 2022

kevinrushforth Oct 28, 2022

kevinrushforth Oct 28, 2022

johanvos Nov 2, 2022

johanvos Nov 2, 2022

andy-goryachev-oracle Nov 2, 2022

johanvos Nov 2, 2022

kevinrushforth Nov 4, 2022

kevinrushforth commented Oct 22, 2022

openjdk bot commented Oct 22, 2022

kevinrushforth left a comment

kevinrushforth Oct 28, 2022

kevinrushforth Oct 28, 2022

kevinrushforth Oct 28, 2022

johanvos Nov 2, 2022 •

edited

Loading

kevinrushforth Nov 4, 2022

kevinrushforth left a comment

kevinrushforth Nov 4, 2022

kevinrushforth Nov 4, 2022

openjdk bot commented Nov 8, 2022

johanvos commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

		newx = sx + (wx - px) * awtScaleX / pScaleX;
		newy = sy + (wy - py) * awtScaleY / pScaleY;

8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors #924

8222210: JFXPanel popups open at wrong coordinates when using multiple hidpi monitors #924

Conversation

johanvos commented Oct 20, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 20, 2022

mlbridge bot commented Oct 20, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth commented Oct 22, 2022

openjdk bot commented Oct 22, 2022

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanvos Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Nov 8, 2022

johanvos commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

johanvos commented Oct 20, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 20, 2022 •

edited

Loading

johanvos Nov 2, 2022 •

edited

Loading