On Mon, 6 Jul 2020 18:22:56 GMT, Oliver Schmidtmer
<github.com+10960818+schmi...@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
