Re: [jfx15] RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v6]
On Mon, 20 Jul 2020 18:11:34 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 updated the pull request incrementally with two > additional commits since the last revision: > > - run test only on windows > - Typo Marked as reviewed by psadhukhan (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/246
Re: [jfx15] RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v6]
On Mon, 20 Jul 2020 18:11:34 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 updated the pull request incrementally with two > additional commits since the last revision: > > - run test only on windows > - Typo Looks good. @prsadhuk can you also review this? - Marked as reviewed by kcr (Lead). 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% [v5]
On Wed, 8 Jul 2020 22:18:39 GMT, Kevin Rushforth wrote: >> Oliver Schmidtmer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> move swt test > > The test looks good with one minor typo in the constant field name (see > below). I verified that it fails on Linux and > Mac (as I suspected), so you will need to limit the test to running on > Windows. I think this is ready to target to `jfx15`, so go ahead and do that. I'll finish my review after that. - 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% [v6]
> 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 two additional commits since the last revision: - run test only on windows - Typo - Changes: - all: https://git.openjdk.java.net/jfx/pull/246/files - new: https://git.openjdk.java.net/jfx/pull/246/files/6b0d58a0..ea1ff37d Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.05 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.04-05 Stats: 9 lines in 2 files changed: 4 ins; 0 del; 5 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
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v5]
On Tue, 7 Jul 2020 12:34:17 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 updated the pull request incrementally with one
> additional commit since the last revision:
>
> move swt test
The test looks good with one minor typo in the constant field name (see below).
I verified that it fails on Linux and
Mac (as I suspected), so you will need to limit the test to running on Windows.
modules/javafx.swt/src/test/java/test/javafx/embed/swt/FXCanvasScaledTest.java
line 56:
> 55: /* Base size, so that with a scaling of 125% there are different
> results for Math.round and Math.ceil */
> 56: final static int TAGET_BASE_SIZE = 101;
> 57:
Spelling error: `TAGET` --> `TARGET`
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelScaledTest.java
line 63:
> 62: /* Base size, so that with a scaling of 125% there are different
> results for Math.round and Math.ceil */
> 63: final static int TAGET_BASE_SIZE = 101;
> 64:
Spelling error: `TAGET` --> `TARGET`
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelScaledTest.java
line 68:
> 67: @BeforeClass
> 68: public static void setupOnce() throws Exception {
> 69: System.setProperty("sun.java2d.uiScale.enabled", "true");
You can add an `assumeTrue(PlatformUtil.isWindows());` here to ensure that the
test only runs on Windows.
-
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 Tue, 7 Jul 2020 10:53:18 GMT, Oliver Schmidtmer wrote: >> 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? > > I'm using a base size of 101, so that there is a difference between Math.ceil > and Math.round for width*1.25. > 100 is only the initial size. The Timer changes the bounds first to 201 and > then to 101. Without resizing the issue > does not appear. I believe somewhere in the Pulse-Thread? there is also a > creation/resizing of the Buffer using > Math.ceil. I'm trying to clarify that now using constants. > My main concern is that the conversion vom JFX to Swing (and SWT, as you > mentioned) is consistent, so there are no > wrong line offsets when reading the buffer outside of JFX. OK. You can update the PR with your findings. - 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 Tue, 7 Jul 2020 12:15:11 GMT, Oliver Schmidtmer wrote: >> 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. > > I assumed I should add it to the robot package, as it is also a visual test. > I've moved it to the swt module I verified that the SWT test works well, when manually enabled in build.gradle and if it runs in isolation. As for putting it in a `robot` sub-package, that seems helpful, but since it won't be enabled anyway, is optional. We can file a follow-up issue to enable SWT tests again (as long as they aren't enabled by default. My thought is to change the default value of `SWT_TEST` to false, and then enable the tests on platforms other than macOS when the `SWT_TEST` is set to `true` along with `FULL_TEST`). - 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 23:31:28 GMT, Kevin Rushforth wrote:
>> 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.
I assumed I should add it to the robot package, as it is also a visual test.
I've moved it to the swt module
-
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% [v5]
> 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: move swt test - Changes: - all: https://git.openjdk.java.net/jfx/pull/246/files - new: https://git.openjdk.java.net/jfx/pull/246/files/882f95c1..6b0d58a0 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.03-04 Stats: 275 lines in 5 files changed: 137 ins; 131 del; 7 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
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v4]
On Mon, 6 Jul 2020 18:00:08 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. > > 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. The swt tests are hard disabled by `enabled = false // FIXME: JIGSAW -- support this with modules` in the gradle.build If I manually change that for running the single test now in the swt module, the test is working fine. - 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:18:20 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. > > 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? I'm using a base size of 101, so that there is a difference between Math.ceil and Math.round for width*1.25. 100 is only the initial size. The Timer changes the bounds first to 201 and then to 101. Without resizing the issue does not appear. I believe somewhere in the Pulse-Thread? there is also a creation/resizing of the Buffer using Math.ceil. I'm trying to clarify that now using constants. My main concern is that the conversion vom JFX to Swing (and SWT, as you mentioned) is consistent, so there are no wrong line offsets when reading the buffer outside of JFX. - 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 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
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Fri, 3 Jul 2020 00:01:10 GMT, Kevin Rushforth wrote:
>> Oliver Schmidtmer has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Change to Math.ceil and add test
>
> 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.
I've added a shim. I would like to check the backing image directly.
> 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`.
done, a rectangle with a defined background & border color. That is definitely
more stable.
-
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% [v3]
> 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: 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/994ca03c..555c5f30 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.01-02 Stats: 358 lines in 5 files changed: 201 ins; 155 del; 2 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
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Tue, 30 Jun 2020 18:28:11 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 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:
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175% [v2]
On Tue, 30 Jun 2020 18:21:52 GMT, Oliver Schmidtmer wrote: >> In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks >> for -ve/+ve max INTEGER but I guess that is >> internal class to FX so it's ok to use Math.round. Approval pending test >> creation. > > While both might work, as long as there is no mixed usage of round and ceil, > Math.ceil seems to be better. > > I'm not sure if the timed waiting for the resizes is the best way for > ensuring that the buffer is resized to the new > bounds, so I'm open for suggestions. To me at least it seems to create a > reproducible sheared output after the second > resize in the test case and not anymore after changing the calculations to > Math.ceil. This fix might be a candidate for JavaFX 15, so I recommend to _not_ merge the master branch. If we don't spot anything of concern during the review, then we might ask you to retarget your PR to the `jfx15` branch. - 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% [v2]
On Wed, 17 Jun 2020 07:32:46 GMT, Prasanta Sadhukhan wrote: >> Oliver Schmidtmer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change to Math.ceil and add test > > In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks for > -ve/+ve max INTEGER but I guess that is > internal class to FX so it's ok to use Math.round. Approval pending test > creation. While both might work, as long as there is no mixed usage of round and ceil, Math.ceil seems to be better. I'm not sure if the timed waiting for the resizes is the best way for ensuring that the buffer is resized to the new bounds, so I'm open for suggestions. To me at least it seems to create a reproducible sheared output after the second resize in the test case and not anymore after changing the calculations to Math.ceil. - 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% [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/246/files - new: https://git.openjdk.java.net/jfx/pull/246/files/5eda49bf..994ca03c Webrevs: - full: https://webrevs.openjdk.java.net/jfx/246/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/246/webrev.00-01 Stats: 161 lines in 3 files changed: 155 ins; 0 del; 6 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
Re: RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
On Wed, 3 Jun 2020 15:46:36 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. In 2D, we normally use sun.java2d.pipe.Region.clipRound as it also checks for -ve/+ve max INTEGER but I guess that is internal class to FX so it's ok to use Math.ceil. Approval pending test creation. - 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%
On Fri, 12 Jun 2020 20:43:18 GMT, Kevin Rushforth wrote: >> @Schmidor in general, we don't use the `/issue` command to identify other >> issues that happen to be fixed by a commit. >> Rather, if they are the same bug, or if one is caused by the other, we would >> closed the other one as a duplicate. This >> seems like a good thing for me to add to the >> [CONTRIBUTING](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md) >> guidelines. > > Even though this is a seemingly simple fix, I'd like @prsadhuk to also review > it. > > @Schmidor Can you do the following? > > 1. Provide an evaluation as to why this is the correct fix, and indicate what > testing you have done to ensure no > regressions in behavior. Changes to rounding in width and height computations > tend to be trickier than they might seem. > 2. Provide a test as part of this fix. Ideally, this would be an automated > test, in the system tests project (under > `tests/system`), and should set the `glass.win.uiScale` system property to > `1.25` before running the test. There are a > few examples you can follow under tests/system that create a JFrame with a > JFXPanel and use Robot to verify the > rendered image. If it turns out that an automated is not feasible, a manual > test would be an acceptable alternative. 3. > Enter the `/issue remove 8230007` command to remove that issue from this PR. > I've already closed > [JDK-8230007](https://bugs.openjdk.java.net/browse/JDK-8230007) in JBS as a > duplicate. I'll look into a more detailed description with code references and try to create a test. The backbuffer of the Stage is created with Math.round(baseSize*uiScale) and for Swing the DataBuffer is referenced into an BufferdImage, where the bounds are calculated with Math.ceil(baseSize*uiScale). So when the base-width is 101px and uiscale is 1.25, the unrounded size is 126.25. The real buffer-width is then 126px, but rendered in swing using 127px. So there is an offset of one pixel per line and the resulting image is skewed. - 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%
On Wed, 3 Jun 2020 16:07:24 GMT, Kevin Rushforth 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. > > @Schmidor in general, we don't use the `/issue` command to identify other > issues that happen to be fixed by a commit. > Rather, if they are the same bug, or if one is caused by the other, we would > closed the other one as a duplicate. This > seems like a good thing for me to add to the > [CONTRIBUTING](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md) > guidelines. Even though this is a seemingly simple fix, I'd like @prsadhuk to also review it. @Schmidor Can you do the following? 1. Provide an evaluation as to why this is the correct fix, and indicate what testing you have done to ensure no regressions in behavior. Changes to rounding in width and height computations tend to be trickier than they might seem. 2. Provide a test as part of this fix. Ideally, this would be an automated test, in the system tests project (under `tests/system`), and should set the `glass.win.uiScale` system property to `1.25` before running the test. There are a few examples you can follow under tests/system that create a JFrame with a JFXPanel and use Robot to verify the rendered image. If it turns out that an automated is not feasible, a manual test would be an acceptable alternative. 3. Enter the `/issue remove 8230007` command to remove that issue from this PR. I've already closed [JDK-8230007](https://bugs.openjdk.java.net/browse/JDK-8230007) in JBS as a duplicate. - 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%
On Wed, 3 Jun 2020 15:46:36 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. @Schmidor in general, we don't use the `/issue` command to identify other issues that happen to be fixed by a commit. Rather, if they are the same bug, or if one is caused by the other, we would closed the other one as a duplicate. This seems like a good thing for me to add to the [CONTRIBUTING](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md) guidelines. - PR: https://git.openjdk.java.net/jfx/pull/246
RFR: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
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. - Commit messages: - 8220484: calculate buffer size of JFXPanel#createResizePixelBuffer as EmbeddedScene#getPixels does Changes: https://git.openjdk.java.net/jfx/pull/246/files Webrev: https://webrevs.openjdk.java.net/jfx/246/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8220484 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
