RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]

[Kevin Rushforth]

On Tue, 30 Jun 2020 18:28:11 GMT, Oliver Schmidtmer <github.com+10960818+Schmidor at openjdk.org> wrote:

>> In edge cases where monitor scaling of 1.25 or 1.75 is active, Math.ceil and Math.round produce different results and
>> EmbeddedScene#getPixels in JFXPanel#paintComponent causes an off-by-one error on the line width and therefore sheared
>> rendering.  The changes were already proposed by the submitter in JBS-8220484.
>> 
>> OCA is signed and send.
>
> Oliver Schmidtmer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Change to Math.ceil and add test

I left specific feedback, mostly on the test, but also one question on the fix.

For the test, make sure you can run it as follows:

gradle --info -PFULL_TEST=true -PUSE_ROBOT=true cleanTest :systemTests:test --tests JFXPanelScaledTest
(presuming you rename it to `JFXPanelScaledTest`)

It should fail without your fix and pass with your fix.

Currently, it will fail because of the call to `setAccessible` (see inline comments).

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 2:

> 1: /*
> 2:  * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

This should be `Copyright (c) 2020, Oracle` ...

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 39:

> 38: import javax.swing.*;
> 39: import java.awt.*;
> 40: import java.awt.image.BufferedImage;

We generally avoid wildcard imports.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 49:

> 48:
> 49: public class JDK8220484Test {
> 50:     static CountDownLatch launchLatch;

We no longer use the pattern of naming our test classes after the bug ID. I recommend a descriptive name for the test
class.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 71:

> 70:         try {
> 71:             if (!launchLatch.await(5 * TIMEOUT, TimeUnit.MILLISECONDS)) {
> 72:                 throw new AssertionFailedError("Timeout waiting for Application to launch (" + (5 * TIMEOUT) + "
> seconds)");

If you add `throws Exception` to the this setup method, then you can replace the entire try / catch block with simply:

    assertTrue("Timeout waiting for Application to launch",
            launchLatch.await(5 * TIMEOUT, TimeUnit.MILLISECONDS));

This is the pattern we use for our newer system tests.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 90:

> 89:         Field fpixelsIm = JFXPanel.class.getDeclaredField("pixelsIm");
> 90:         fpixelsIm.setAccessible(true);
> 91:         BufferedImage pixelsIm = (BufferedImage) fpixelsIm.get(myApp.jfxPanel);

This isn't the pattern we use to access internal fields of a JavaFX class, and won't work.

We typically use the "shim" pattern for such white-box testing. Can you look into adding shims to the `javafx.swing`
module? Many of the other modules already have shims, so you can use that as a pattern. It will require adding a
package-scope `test_getPixelsIm` method to `JFXPanel`.

Alternatively, you can use AWT `Robot` to read the JFXPanel pixels.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 93:

> 92:
> 93:
> 94:         assertEquals(127, pixelsIm.getWidth());

Minor: a single blank line is sufficient.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 103:

> 102:             for (int y = 90; y < 115; y++) {
> 103:                 if(colorOfDiagonal == pixelsIm.getRGB( x, y )) {
> 104:                     fail( "image is skewed" );

Are you sure that an equality test will work on all platforms and configurations? We usually use a tolerance when
comparing colors whose components are other than 0 or 255.

Somewhat related to this, is the expected value of `181` coming from the default value of the button border? It might
be more robust to fill the Scene with a stroked rectangle whose color you control (e.g. set to black). Either that or
set the color of the button border using an inline CSS style so you aren't dependent on the default inherited from the
`modena.css` style sheet.

Minor: add a space after the `if` and remove the extra spaces surrounding the function arguments `x, y`.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 104:

> 103:                 if(colorOfDiagonal == pixelsIm.getRGB( x, y )) {
> 104:                     fail( "image is skewed" );
> 105:                 }

Minor: remove the extra spaces surrounding the function argument.

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 26:

> 25:
> 26: package test.javafx.embed.swing;
> 27:

This test will need to move to the `test.robot.javafx.embed.swing` package since it relies on reading the actual
rendered pixels.

modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/EmbeddedScene.java line 235:

> 234:             scaledWidth = (int) Math.ceil(scaledWidth * texScaleFactorX);
> 235:             scaledHeight = (int) Math.ceil(scaledHeight * texScaleFactorY);
> 236:

Since this is in code that is shared by all embedded scenes, have you verified that FXCanvas (SWT interop) doesn't need
similar changes?

tests/system/src/test/java/test/javafx/embed/swing/JDK8220484Test.java line 114:

> 113:          *
> 114:          */
> 115:         private static final long serialVersionUID = 1L;

Minor: you can remove this empty comment block.

-------------

PR: https://git.openjdk.java.net/jfx/pull/246