Bug ID: JDK-8162551 SwingNode scaling doesn't work

JDK 9
9Fixed

Changeset: 140a56440fa4 Author: ssadetsky Date: 2016-09-26 14:55 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/140a56440fa4 8162551: SwingNode scaling doesn't work Reviewed-by: flar, kcr
26-09-2016
[~ssadetsky] Please file any needed follow-up issues (e.g., the gap issue that you and Jim discussed above)
26-09-2016
Looks good. +1
26-09-2016
The popup shift may require allowing floating point layouts in order to fix it. Right now there is no way to determine exactly where the end of a component is (which would require floating point bounds) and to exactly place a popup window on a pixel (which would require floating point setLocation), so the best we could do is to make sure that all of the rounding errors in the system cooperate to get the popups to within a pixel. To really fix it we'd need things like methods to get/set locations/sizes/bounds that either take float coordinates in the logical pixels or take (int) physical pixel coordinates and require the caller to deal with the scaling. We'd probably want those on Event coordinates, Component/Window size/location/bounds, and a few other places. The synchronization of the uiScale properties is more of a wish list item since the property is only used for testing and/or debugging and it only matters when testing or debugging an embedded situation.
23-09-2016
Thank you for review, Jim. When 8153522 is fixed, I will decide where to address the popup shift and will file the bug then. Also, the synchronizition 2d/FX debug scales system properties. At the moment it is not fully clear to me if it is really necessary. Will be grateful for any input on this topic. I found that JDK supports separated sun.java2d.win.uiScaleX/Y properties but I did not find their FX analogs. Kevin, if you approve the fix couldn't you push it to the repo? I'm not a FX commiter yet.
23-09-2016
Confirmed that it works well on b137 as per Kevin's comment above. The popups can still separate from their control by a small amount due to roundoff error, but that is a separate issue. +1
23-09-2016
But what is the sense to use b109? It is more than half a year old build and since HiDPI is a new feature in 9 it may be not properly working in old builds. I always use the latest available JDK build for testing purposes. At the moment it is b137.
23-09-2016
@Semyon: I think you are onto something with your question about JDK version. I applied your patch, and I see the same thing Jim sees if I use JDK 9+109 and run like this: $ java -version java version "9-ea" Java(TM) SE Runtime Environment (build 9-ea+109-2016-03-09-175345.javare.4620) Java HotSpot(TM) Client VM (build 9-ea+109-2016-03-09-175345.javare.4620, mixedmode) $ java -Xbootclasspath/a:$JFX/rt/build/sdk/lib/jfxrt.jar ... However, if I run a more recent build of JDK 9, I don't see the size problem: $ java -version java version "9-ea" Java(TM) SE Runtime Environment (build 9-ea+134) Java HotSpot(TM) Server VM (build 9-ea+134, mixed mode) $ java @$JFX/rt/build/patchmodule.args ...
22-09-2016
I cannot confirm these 3 pictures. I have attached the 150% native scale screenshot on Windows. I would not expect that the fix works with fractional scale well because JDK-8153522 is not fixed yet (because it is blocked by the current issue). Yes, I confirm that there is a small gap between compo and its popup. This can be caused by precision lost when coordinates are upscaled or different ways to round coordinates. But this is not the problem about which this bug is. The actual problem is that FX uses JLightweightFrame API incorrectly. If the fix works with the debug scale then it should work with the natural scale. Because it is simply another source to obtain the same scale number. In one case it is read from native platform, in another case it is get from system properties. If results for the same scale number are different then there is an error in some other place but not in the code related to this fix. Which JDK version did you use for testing?
22-09-2016
I can confirm that the popups behavior if you use the command line matches the behavior if you change the system setting as long as you set both the glass.win.uiScale and sun.java2d.uiScale to the same value. So, that one issue with the command line overrides is even less pressing than it was, though it would be nice if you only had to override the setting that matched the toolkit you were using (i.e. if it is an FX app with Swing embedded, then you only have to set the glass setting and if it is a Swing app with embedded FX scene then you only have to set the java2d setting). This is independent of the issue that I showed in the 3 screen shots that I just posted, though - those problems are seen without any command line overrides.
22-09-2016
I've attached 3 screen captures of the look of the combobox popups at 100%, 175% and 200% scaling on my Windows 10 machine. All 3 runs were done with the latest patch from webrev.03. All 3 tests were done by changing the actual scaling size in the control panel, no command line overrides were used. The 100% shows the popup to be the same width as the combobox and it has a scrollbar. The other 2 show that the rendering of the text in the popup appears to want to be the right size for the what the popup should be, but the popup is too small by an appropriate ratio as if it were sized with a 100% scale, but drawn with the required 175% or 200% scale. Also, there are gaps between the control and the popup, but the popup should be flush with the bottom of the combobox control as it is in the 100% rendering.
22-09-2016
Hi Jim, Thank you for your review. May I ask you, how did you determine that popups are wrong? Do you have any special test for that? Could you be more precise and explain what specifically is wrong with popups? I would appreciate it. By the way, when you run your test, did you set the glass.win.uiScale Java VM system property along with sun.java2d.uiScale Java VM system property? Did you set the same scale value for both properties? Did the value had integer or floating point precision? On which platform I can reproduce the mentioned wrong behavior?
22-09-2016
Also, if the scale is overridden with -Dglass.win.uiScale=... then the popups and tooltips are in the wrong place, though that is less critical since it involves an override that is only intended for testing applications.
21-09-2016
I'm still seeing popups and tooltips looking wrong. Do we at least have a handle on why those are wrong so that we can know if this fix is a step in the right direction. They can be fixed as a follow-on bug, but I'd like to be at least confident that it isn't because this is the wrong fix for the basic scaling problem.
21-09-2016
Kevin, Jim I have updated the fix: the test is compatible with OS X now. Also I took into account the code reorganization added after the last revision. http://cr.openjdk.java.net/~ssadetsky/8162551/webrev.03/
19-09-2016
Yeah, gradle's filtering for running single tests leaves a lot to be desired. The unit test looks good now and runs fine on Windows. However, I tried it on Mac and it deadlocks in AWT toolkit initialization, probably because of the SwingUtilities.invokeAndWait() in the FX application thread. You may need to either exclude this on the Mac, or else change it to an invokeLater with the rest of the code that is currently after the invokeAndWait being moved to a Platform.runLater inside the body of the invokeLater. As for the dependent issue, I see that you added a defer request for JDK-8153522. The defer request needs to be removed, since without that, this fix is incomplete. I do not think we can defer that bug.
29-07-2016
+1 on the current fix.
29-07-2016
Testing on WIndows 10 it looks like this fix actually improves popup locations under the default (i.e. running the app on a retina MBP without an override on the UI scale), though their sizes are off - and the scaling is better. Some more work will need to be done to get the popup sizes right (they appear to be drawn at 200% and positioned right, but at the size they would be for 100%), but that can be done as follow-on work.
29-07-2016
Thanks, Kevin. This information was very helpful. The corrected test: http://cr.openjdk.java.net/~ssadetsky/8162551/webrev.02/ by the way, gradle --info -PFULL_TEST=true :systemTests:test --tests test.javafx.embed.swing.SwingNodeScaleTest has started 62 executors anyway.
29-07-2016
1. The unit test fails with a timeout, which is caused by an NPE in the start method. The static "request" field is initialized in the @Before method, which is too late since it is used in the static @BeforeClass method. test.swing.SwingNodeScaleTest STANDARD_ERROR Exception in Application start method Exception in thread "Thread-4" java.lang.RuntimeException: Exception in Application start method at com.sun.javafx.application.LauncherImpl.launchApplication1(LauncherImpl.java:897) at com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(LauncherImpl.java:188) at java.lang.Thread.run(Thread.java:804) Caused by: java.lang.NullPointerException at test.swing.SwingNodeScaleTest$MyApp.start(SwingNodeScaleTest.java:102) at com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$9(LauncherImpl.java:843) at com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(PlatformImpl.java:452) at com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:421) at java.security.AccessController.doPrivileged(Native Method) at com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:420) at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96) at com.sun.glass.ui.win.WinApplication._runLoop(Native Method) at com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:189) ... 1 more Timeout waiting for Application to launch Note that the system tests are not run unless you specify the "-PFULL_TEST=true" flag. gradle --continue --info -PFULL_TEST=true test If you want to run just the one test (sadly, it still spends a lot of time skipping the remaining tests), you can run: gradle --info -PFULL_TEST=true :systemTests:test --tests test.swing.SwingNodeScaleTest 2. The following logic in build.gradle is intended to exclude Swing / AWT tests: if (!IS_AWT_TEST) { // Disable all AWT-based tests exclude("*/javafx/embed/swing/."); exclude("/com/sun/javafx/application/Swing."); } Can you either change the first exclude line to "/swing/.*" or else move the new unit test to test.javafx.embed.swing so the new test will be excluded when IS_AWT_TEST is set to false?
29-07-2016
Kevin, but to make SwingNode support 1.25 scale I need to fix JDK-8153522 which is blocked by this bug fix review.
29-07-2016
OK, I will finish the review keeping in mind that the fix for blurriness will be a follow-up.
29-07-2016
I can also see the blurriness on Windows 7. On both systems, I had set 125% scaling natively (I was not using any system properties).
28-07-2016
@Semyon: I tested on Windows 10.
28-07-2016
test is corrected: http://cr.openjdk.java.net/~ssadetsky/8162551/webrev.01/
28-07-2016
@Kevin I cannot confirm 125% scale. Which OS did you run the test?
28-07-2016
Jim, the popup positioning was not an issue. One just need to add sun.java2d.uiScale property with the same scale to reach swing and fx coherence. If scale is read from the OS then it will be done automatically. I see 1px shift in the popups location. It is caused by rounding coordinates because fx uses float but swing uses integer. This may be addressed to JDK-8153522.
28-07-2016
I checked with a test case that used a Swing component with popups. Before the fix the popups appeared in the right location on a HiDPI screen, though the components were small and hard to read. After the fix, the components are the right size, but popups appear in random locations on the screen. This was one of the reasons that we decided to go with the way the code is currently written - something deeper inside the lightweight mechanism needs to be addressed to fix the blurriness without breaking popups.
27-07-2016
@Kevin Could you attach the simple SwingNode app you have 125% scale? @Jim I didn't try popups. I will try. Not sure that the native scale should work differently I will check. I don't have purpose to fix all related issues but to allow change the JLightFrame API for JDK-8153522. The test I used to get the picture: public void start(Stage stage) throws Exception { StackPane pane = new StackPane(); Scene scene = new Scene(pane, 200, 100); stage.setScene(scene); SwingUtilities.invokeAndWait(() -> { node = new SwingNode(); panel = new JPanel(); b = new JButton("Swing Button", UIManager.getDefaults().getIcon("FileView.computerIcon")); panel.add(b, BorderLayout.EAST); node.setContent(panel); }); pane.getChildren().add(node); Button b1 = new Button("FX Button"); pane.getChildren().add(b1); stage.show(); }
27-07-2016
Also, which test case was run for the screen shots attached above?
26-07-2016
Two questions on testing the fix: - Did you try this on a display with scaling set to 200% natively? - Did you try this with a swing component that has a popup window? Did the popup window appear in the right place?
26-07-2016
Jim can comment on the code change itself. The unit test has two failures: 1. test.javafx.embed.swing.SwingNodeScaleTest$NodeTest > initializationError FAILED java.lang.Exception: No runnable methods This is because the application subclass ends with Test (NodeTest). You will need to choose a different name so it doesn't get picked up by the build system as a valid test class that contains annotated Test methods. 2. test.javafx.embed.swing.SwingNodeScaleTest > testScale FAILED java.lang.IllegalStateException: Application launch must not be called more than once at com.sun.javafx.application.LauncherImpl.launchApplication(LauncherImpl.java:168) at com.sun.javafx.application.LauncherImpl.launchApplication(LauncherImpl.java:149) at javafx.application.Application.launch(Application.java:191) at test.javafx.embed.swing.SwingNodeScaleTest.testScale(SwingNodeScaleTest.java:57) 3 tests completed, 2 failed :swing:test FAILED Take a look at some of the tests in the tests/system (systemTests) project for how to launch an Application: you must do it in a "BeforeClass" method. You should have an 'AfterClass' method that calls Platform.exit(). You also need to ensure that all tests run in their own VM, so your unit test either needs to move to the systemTests project (probably the best) under one of the swing packages, or you will need to change the swing project's test setup in build.gradle to add 'forkEvery = 1' (there are not many tests in the swing project, so that should be acceptable for now). 3. As for the fix itself, I now get blurriness at 1.25% on Windows running a simple SwingNode program. Is that expected?
26-07-2016
Upscaling is not needed to send coordinates to sun.swing.JLightweightFrame.
26-07-2016
http://mail.openjdk.java.net/pipermail/openjfx-dev/2016-July/019481.html
26-07-2016