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

[Kevin Rushforth]

On Mon, 6 Jul 2020 18:22:56 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 refreshed the contents of this pull request, and previous commits have been removed. The
> incremental views will show differences compared to the previous content of the PR. The pull request contains two new
> commits since the last revision:
>  - Similar changes and test for FXCanvas
>  - more solid test with shim and style independent color

The test is looking better now. And the fix to `FXPanel` looks correct as well, although needs to be tested.

I left a few comments relating to the tests. I haven't looked at the SWT test in detail, but will do that later. I also
still need to test this on multiple platforms (I have a concern about platforms other than Windows due to assumptions
the test is making).

build.gradle line 3611:

> 3610:         testCompile project(":swt")
> 3611:         testCompile name: SWT_FILE_NAME
> 3612:     }

We can't have a dependency on `swt` from any other project. SWT tests need to be confined to
`modules/javafx.swt/src/test/java/`

build.gradle line 3614:

> 3613:
> 3614:     def dependentProjects = [ 'base', 'graphics', 'controls', 'media', 'web', 'swing', 'fxml', 'swt' ]
> 3615:     commonModuleSetup(project, dependentProjects)

Similarly, this needs to be reverted.

modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 934:

> 933:     // Package scope method for testing
> 934:     final BufferedImage test_getPixelsIm(){
> 935:         return pixelsIm;

Minor: add a space before the `{`

tests/system/src/test/java/test/robot/javafx/embed/swt/FXCanvasScaledTest.java line 26:

> 25:
> 26: package test.robot.javafx.embed.swt;
> 27:

This should move to `test.javafx.embed.swt` underneath `modules/javafx.swt/src/test/java`.

Note that the tests in the `javafx.swt` module are not currently enabled (I haven't looked recently into what it would
take to do so), so you might need to verify it manually.

tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelScaledTest.java line 74:

> 73:         if (!launchLatch.await(5 * TIMEOUT, TimeUnit.MILLISECONDS)) {
> 74:             throw new AssertionFailedError("Timeout waiting for Application to launch (" + (5 * TIMEOUT) + "
> seconds)"); 75:         }

As a suggestion, this can further be simplified to:

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

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

> 89:         assertEquals(127, pixelsIm.getWidth());
> 90:         assertEquals(127, pixelsIm.getHeight());
> 91:

Where does the `127` come from? If this is derived from the size of the JFXPanel * scale, it isn't likely to work on
platforms other than Windows (it certainly won't work on Mac, and I suspect not on Linux, but it needs to be tested).
Also, unless there is a padding of 1 pixel (in user space coordinates), wouldn't it be 125?

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

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