Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v4]
On Mon, 6 Jul 2020 17:52:16 GMT, Kevin Rushforth wrote:
>> 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
>
> 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/`
I note that this does work for me. So copying the pattern used by `systemTests`
into the `swt` project might be all
that is needed to get it to work there.
-
PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v4]
On Mon, 6 Jul 2020 18:19:53 GMT, Kevin Rushforth wrote: >> 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). Also, in general we recommend not to force-push to your branch after the review has started, unless there is some compelling reason to do so. It makes it harder for reviewers to look at incremental diffs. - PR: https://git.openjdk.java.net/jfx/pull/246
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v4]
On Mon, 6 Jul 2020 18:22:56 GMT, Oliver Schmidtmer
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
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v4]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/246/files - new: https://git.openjdk.java.net/jfx/pull/246/files/555c5f30..882f95c1 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.02-03 Stats: 268 lines in 6 files changed: 130 ins; 0 del; 138 mod Patch: https://git.openjdk.java.net/jfx/pull/246.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/246/head:pull/246 PR: https://git.openjdk.java.net/jfx/pull/246
