Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations [v2]
On Tue, 19 Mar 2024 21:37:50 GMT, Kevin Rushforth wrote: > I also suspect that Karthik was running in that mode. You can check the value > of the `XDG_SESSION_TYPE` env var to see. All robot tests that read the > screen will fail on Wayland until > [JDK-8326712](https://bugs.openjdk.org/browse/JDK-8326712) is fixed (should > be done reasonably soon, sometime in the JavaFX 23 time frame). Yes it was because of Wayland. The test looks good now. - PR Comment: https://git.openjdk.org/jfx/pull/1403#issuecomment-2008766427
Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations [v2]
On Tue, 19 Mar 2024 13:16:48 GMT, Lukasz Kostyra wrote: >> There were two different problems with this test's stability and HiDPI and >> both were fixed with this change. >> >> The whole reason for this tests lack of stability comes with how Windows >> calculates window position when using HiDPI. When window's X/Y coordinates >> are not provided by the application, they are (generally speaking) randomly >> selected by Windows. X/Y are picked as two integers, and afterwards the >> HiDPI scaling is applied. With fractional scaling like 125% this can cause >> X/Y coordinates to have fractional values. >> >> When performing a screen capture, Robot takes a generous estimate of >> window's position and dimensions. This is done by taking provided X/Y >> coordinates (in case of this test, stage's X/Y coords), multiplying them by >> scaling (ex. 1.25) and then flooring the result. Similar estimation is done >> to opposite coordinates - we take X/Y coords, add width/height to them >> respectively, multiply them by scaling and ceiling them. As a result, we >> have minX/Y and maxX/Y coords of capture region, which is used to calculate >> capture region's width and height. The first problem with test's stability >> is right here - the test did not take this calculation and floor/ceil >> operations into account, which with appropriately randomized window >> coordinates could cause discrepancy between expected width/height and >> actual. This logic that is used by `getScreenCapture()` seems to be >> necessary and good, so to remedy the problem I replicated those calculations >> on test side. >> >> Another problem came after that - fractional X/Y values caused capture to >> not always line up with pixel layout, which made the 1-pixel border of >> capture sometimes incorrect (an average of stage's MAGENTA color and >> whatever background happened to be under the displayed stage). This means we >> cannot exactly expect those pixels to have exactly MAGENTA color when the >> system has fractional HiDPI enabled. To help that, the test now doesn't >> check for correctness of the 1-pixel border of captured image. I feel this >> still gives us enough confidence that the Robot screen capture works >> properly, while stabilizing the test on all systems. >> >> I briefly considered an alternative approach to hard-set X/Y values but >> doing it this way gives us a bit more robustness, makes the test independent >> of Stage's X/Y coordinates which matter quite a lot. >> >> Verified this change on macOS and especially on Windows - ran the test on >> Windows 50 times each on 125% and 100% scaling and noticed no intermittent >> failures. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Fix cast formatting Marked as reviewed by kpk (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1403#pullrequestreview-1948052893
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:34:17 GMT, Nir Lisker wrote: >> The only issue with this style is - it's hard to set breakpoints. >> In this case it's not that important. >> Thank you! > > It's a matter of style mostly, so I don't really mind it. The debug issue can > be remedied by adding breakpoints inside Lambdas, which all IDEs support I > think (Eclipse and IntelliJ for sure). "inside lambdas" - inside the referenced methods you mean? yes, but if that method is frequently called it's not what I want. There is no way to set a breakpoint at the moment of lambda invocation, I think, at least not in Eclipse. - PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531211388
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:28:54 GMT, Andy Goryachev wrote:
>> tests/system/src/test/java/test/util/Util.java line 383:
>>
>>> 381: runAndWait(() -> {
>>> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
>>> 383: w.hide();
>>
>> I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't
>> get modified (it's a one-time copy).
>> The iteration can also be
>>
>> List.copyOf(Window.getWindows()).forEach(Window::hide);
>>
>> if you want.
>
> The only issue with this style is - it's hard to set breakpoints.
> In this case it's not that important.
> Thank you!
It's a matter of style mostly, so I don't really mind it. The debug issue can
be remedied by adding breakpoints inside Lambdas, which all IDEs support I
think (Eclipse and IntelliJ for sure).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531207452
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:23:46 GMT, Nir Lisker wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> for each
>
> tests/system/src/test/java/test/util/Util.java line 383:
>
>> 381: runAndWait(() -> {
>> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
>> 383: w.hide();
>
> I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't
> get modified (it's a one-time copy).
> The iteration can also be
>
> List.copyOf(Window.getWindows()).forEach(Window::hide);
>
> if you want.
The only issue with this style is - it's hard to set breakpoints.
In this case it's not that important.
Thank you!
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531200964
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
> Changing `Util.shutdown()` to hide **all** showing Windows and then call > `Platform.exit()`. > This simplifies the tests and ensures a clean shutdown. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: for each - Changes: - all: https://git.openjdk.org/jfx/pull/1407/files - new: https://git.openjdk.org/jfx/pull/1407/files/2887d654..5773ebfd Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1407&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1407&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1407.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1407/head:pull/1407 PR: https://git.openjdk.org/jfx/pull/1407
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows
On Mon, 18 Mar 2024 22:53:03 GMT, Andy Goryachev wrote:
> Changing `Util.shutdown()` to hide **all** showing Windows and then call
> `Platform.exit()`.
> This simplifies the tests and ensures a clean shutdown.
tests/system/src/test/java/test/util/Util.java line 383:
> 381: runAndWait(() -> {
> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
> 383: w.hide();
I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't
get modified (it's a one-time copy).
The iteration can also be
List.copyOf(Window.getWindows()).forEach(Window::hide);
if you want.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531195323
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows
On Mon, 18 Mar 2024 22:53:03 GMT, Andy Goryachev wrote: > Changing `Util.shutdown()` to hide **all** showing Windows and then call > `Platform.exit()`. > This simplifies the tests and ensures a clean shutdown. Looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1407#pullrequestreview-1947463852
Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations [v2]
On Tue, 19 Mar 2024 13:16:48 GMT, Lukasz Kostyra wrote: >> There were two different problems with this test's stability and HiDPI and >> both were fixed with this change. >> >> The whole reason for this tests lack of stability comes with how Windows >> calculates window position when using HiDPI. When window's X/Y coordinates >> are not provided by the application, they are (generally speaking) randomly >> selected by Windows. X/Y are picked as two integers, and afterwards the >> HiDPI scaling is applied. With fractional scaling like 125% this can cause >> X/Y coordinates to have fractional values. >> >> When performing a screen capture, Robot takes a generous estimate of >> window's position and dimensions. This is done by taking provided X/Y >> coordinates (in case of this test, stage's X/Y coords), multiplying them by >> scaling (ex. 1.25) and then flooring the result. Similar estimation is done >> to opposite coordinates - we take X/Y coords, add width/height to them >> respectively, multiply them by scaling and ceiling them. As a result, we >> have minX/Y and maxX/Y coords of capture region, which is used to calculate >> capture region's width and height. The first problem with test's stability >> is right here - the test did not take this calculation and floor/ceil >> operations into account, which with appropriately randomized window >> coordinates could cause discrepancy between expected width/height and >> actual. This logic that is used by `getScreenCapture()` seems to be >> necessary and good, so to remedy the problem I replicated those calculations >> on test side. >> >> Another problem came after that - fractional X/Y values caused capture to >> not always line up with pixel layout, which made the 1-pixel border of >> capture sometimes incorrect (an average of stage's MAGENTA color and >> whatever background happened to be under the displayed stage). This means we >> cannot exactly expect those pixels to have exactly MAGENTA color when the >> system has fractional HiDPI enabled. To help that, the test now doesn't >> check for correctness of the 1-pixel border of captured image. I feel this >> still gives us enough confidence that the Robot screen capture works >> properly, while stabilizing the test on all systems. >> >> I briefly considered an alternative approach to hard-set X/Y values but >> doing it this way gives us a bit more robustness, makes the test independent >> of Stage's X/Y coordinates which matter quite a lot. >> >> Verified this change on macOS and especially on Windows - ran the test on >> Windows 50 times each on 125% and 100% scaling and noticed no intermittent >> failures. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Fix cast formatting Fix looks good to me. I confirm that the test no longer fails on my Windows system (running at 1.25x scale). - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1403#pullrequestreview-1947446088
Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations [v2]
On Fri, 15 Mar 2024 11:37:53 GMT, Karthik P K wrote: >> Lukasz Kostyra has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix cast formatting > > Tested the changes in Mac, Windows and Linux. The changes fixes the issues in > Mac and Windows. > I ran the test in Linux VM and it failed with and without the fix with error: > > RobotTest > testScreenCapture FAILED > junit.framework.AssertionFailedError: expected: rgba(255,0,255,255) but > was: rgba(0,0,0,255) > at > test.robot.javafx.scene.RobotTest.assertColorEquals(RobotTest.java:848) > at > test.robot.javafx.scene.RobotTest.testScreenCapture(RobotTest.java:774) > > > Changing the scale also did not make any difference. Not sure if it is > related to anything in VM. > It is a Ubuntu 22.04 VM > > Added a minor comment inline. > @karthikpandelu I just tried this on Ubuntu 22.04 VM and everything seems to > work fine. (0,0,0,255) returned from capture suggests capture failed - could > it be your VM is running on Wayland instead of X11? I also just ran this, and it runs fine for me on an Ubuntu 22.04 VM. Since Wayland is the default in 22.04, I also suspect that Karthik was running in that mode. You can check the value of the `XDG_SESSION_TYPE` env var to see. All robot tests that read the screen will fail on Wayland until [JDK-8326712](https://bugs.openjdk.org/browse/JDK-8326712) is fixed (should be done reasonably soon, sometime in the JavaFX 23 time frame). - PR Comment: https://git.openjdk.org/jfx/pull/1403#issuecomment-2008178337
Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations [v2]
On Tue, 19 Mar 2024 13:16:48 GMT, Lukasz Kostyra wrote: >> There were two different problems with this test's stability and HiDPI and >> both were fixed with this change. >> >> The whole reason for this tests lack of stability comes with how Windows >> calculates window position when using HiDPI. When window's X/Y coordinates >> are not provided by the application, they are (generally speaking) randomly >> selected by Windows. X/Y are picked as two integers, and afterwards the >> HiDPI scaling is applied. With fractional scaling like 125% this can cause >> X/Y coordinates to have fractional values. >> >> When performing a screen capture, Robot takes a generous estimate of >> window's position and dimensions. This is done by taking provided X/Y >> coordinates (in case of this test, stage's X/Y coords), multiplying them by >> scaling (ex. 1.25) and then flooring the result. Similar estimation is done >> to opposite coordinates - we take X/Y coords, add width/height to them >> respectively, multiply them by scaling and ceiling them. As a result, we >> have minX/Y and maxX/Y coords of capture region, which is used to calculate >> capture region's width and height. The first problem with test's stability >> is right here - the test did not take this calculation and floor/ceil >> operations into account, which with appropriately randomized window >> coordinates could cause discrepancy between expected width/height and >> actual. This logic that is used by `getScreenCapture()` seems to be >> necessary and good, so to remedy the problem I replicated those calculations >> on test side. >> >> Another problem came after that - fractional X/Y values caused capture to >> not always line up with pixel layout, which made the 1-pixel border of >> capture sometimes incorrect (an average of stage's MAGENTA color and >> whatever background happened to be under the displayed stage). This means we >> cannot exactly expect those pixels to have exactly MAGENTA color when the >> system has fractional HiDPI enabled. To help that, the test now doesn't >> check for correctness of the 1-pixel border of captured image. I feel this >> still gives us enough confidence that the Robot screen capture works >> properly, while stabilizing the test on all systems. >> >> I briefly considered an alternative approach to hard-set X/Y values but >> doing it this way gives us a bit more robustness, makes the test independent >> of Stage's X/Y coordinates which matter quite a lot. >> >> Verified this change on macOS and especially on Windows - ran the test on >> Windows 50 times each on 125% and 100% scaling and noticed no intermittent >> failures. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Fix cast formatting Thanks for fixing this - was a thorn in my side when developing the Robot API. - PR Comment: https://git.openjdk.org/jfx/pull/1403#issuecomment-2008008533
Re: RFR: 8327482: Fix missing image resource in HelloImage toy app
On Tue, 19 Mar 2024 16:05:50 GMT, Lukasz Kostyra wrote: > Replaced existing `slowImageURL` address to a new image (+ some extra bits > related to it). Image has been taken from open-source repository of Duke > images and licensed under BSD license: > https://wiki.openjdk.org/display/duke/Gallery Looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1410#pullrequestreview-1947122603
[jfx22u] Integrated: 8324326: Update ICU4C to 74.2
On Tue, 19 Mar 2024 16:32:55 GMT, Hima Bindu Meda wrote: > Clean Backport This pull request has now been integrated. Changeset: 32378354 Author:Hima Bindu Meda URL: https://git.openjdk.org/jfx22u/commit/323783541140bea2119e2e59d0bb2d8d1fb37c8a Stats: 10765 lines in 123 files changed: 3601 ins; 1545 del; 5619 mod 8324326: Update ICU4C to 74.2 Backport-of: ad3d44e27f8ffb90aad81497f0bba2b00f7a49aa - PR: https://git.openjdk.org/jfx22u/pull/20
[jfx22u] Integrated: 8324326: Update ICU4C to 74.2
Clean Backport - Commit messages: - Backport ad3d44e27f8ffb90aad81497f0bba2b00f7a49aa Changes: https://git.openjdk.org/jfx22u/pull/20/files Webrev: https://webrevs.openjdk.org/?repo=jfx22u&pr=20&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324326 Stats: 10765 lines in 123 files changed: 3601 ins; 1545 del; 5619 mod Patch: https://git.openjdk.org/jfx22u/pull/20.diff Fetch: git fetch https://git.openjdk.org/jfx22u.git pull/20/head:pull/20 PR: https://git.openjdk.org/jfx22u/pull/20
Re: RFR: 8328399: Add hs_err_pid* to .gitignore
On Mon, 18 Mar 2024 23:31:03 GMT, Andy Goryachev wrote: > Add hs_err_pid* to .gitignore > > Q: Any other files? Marked as reviewed by aghaisas (Reviewer). Oops... I saw the JBS Description now. It is already ignored. - PR Review: https://git.openjdk.org/jfx/pull/1408#pullrequestreview-1946762826 PR Comment: https://git.openjdk.org/jfx/pull/1408#issuecomment-2007612920
Re: RFR: 8328399: Add hs_err_pid* to .gitignore
On Mon, 18 Mar 2024 23:31:03 GMT, Andy Goryachev wrote: > Q: Any other files? May be `.DS_Store` file - PR Comment: https://git.openjdk.org/jfx/pull/1408#issuecomment-2007598297
RFR: 8327482: Fix missing image resource in HelloImage toy app
Replaced existing `slowImageURL` address to a new image (+ some extra bits related to it). Image has been taken from open-source repository of Duke images and licensed under BSD license: https://wiki.openjdk.org/display/duke/Gallery - Commit messages: - Change slow image link to new one Changes: https://git.openjdk.org/jfx/pull/1410/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1410&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327482 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1410.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1410/head:pull/1410 PR: https://git.openjdk.org/jfx/pull/1410
RFR: 8325566: [TestBug] Util.shutdown() to hide ALL the scenes
Changing `Util.shutdown()` to hide **all** showing Windows and then call `Platform.exit()`. This simplifies the tests and ensures a clean shutdown. - Commit messages: - copy - 8325566: [TestBug] Util.shutdown() to hide ALL the scenes Changes: https://git.openjdk.org/jfx/pull/1407/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1407&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325566 Stats: 124 lines in 59 files changed: 1 ins; 6 del; 117 mod Patch: https://git.openjdk.org/jfx/pull/1407.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1407/head:pull/1407 PR: https://git.openjdk.org/jfx/pull/1407
RFR: 8328399: Add hs_err_pid* to .gitignore
Add hs_err_pid* to .gitignore Q: Any other files? - Commit messages: - 8328399: Add hs_err_pid* to .gitignore Changes: https://git.openjdk.org/jfx/pull/1408/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1408&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328399 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1408.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1408/head:pull/1408 PR: https://git.openjdk.org/jfx/pull/1408
Re: RFR: 8323511 Scrollbar Click jumps inconsistent amount of pixels [v2]
On Tue, 19 Mar 2024 12:02:14 GMT, Marius Hanl wrote: > But is it possible to scroll to an index that is negative? Yes, both the negative value (``-1``) and the positive value, which is greater than the items count (to be precise ``last item index + 1``), are possible and indicate the direction in which the scrolling takes place. The ``last item index + 1`` is already handled correctly. We only need to fix the handling of ``-1`` index. Can you explain your second question in more detail? - PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1530382582
Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations [v2]
> There were two different problems with this test's stability and HiDPI and > both were fixed with this change. > > The whole reason for this tests lack of stability comes with how Windows > calculates window position when using HiDPI. When window's X/Y coordinates > are not provided by the application, they are (generally speaking) randomly > selected by Windows. X/Y are picked as two integers, and afterwards the HiDPI > scaling is applied. With fractional scaling like 125% this can cause X/Y > coordinates to have fractional values. > > When performing a screen capture, Robot takes a generous estimate of window's > position and dimensions. This is done by taking provided X/Y coordinates (in > case of this test, stage's X/Y coords), multiplying them by scaling (ex. > 1.25) and then flooring the result. Similar estimation is done to opposite > coordinates - we take X/Y coords, add width/height to them respectively, > multiply them by scaling and ceiling them. As a result, we have minX/Y and > maxX/Y coords of capture region, which is used to calculate capture region's > width and height. The first problem with test's stability is right here - the > test did not take this calculation and floor/ceil operations into account, > which with appropriately randomized window coordinates could cause > discrepancy between expected width/height and actual. This logic that is used > by `getScreenCapture()` seems to be necessary and good, so to remedy the > problem I replicated those calculations on test side. > > Another problem came after that - fractional X/Y values caused capture to not > always line up with pixel layout, which made the 1-pixel border of capture > sometimes incorrect (an average of stage's MAGENTA color and whatever > background happened to be under the displayed stage). This means we cannot > exactly expect those pixels to have exactly MAGENTA color when the system has > fractional HiDPI enabled. To help that, the test now doesn't check for > correctness of the 1-pixel border of captured image. I feel this still gives > us enough confidence that the Robot screen capture works properly, while > stabilizing the test on all systems. > > I briefly considered an alternative approach to hard-set X/Y values but doing > it this way gives us a bit more robustness, makes the test independent of > Stage's X/Y coordinates which matter quite a lot. > > Verified this change on macOS and especially on Windows - ran the test on > Windows 50 times each on 125% and 100% scaling and noticed no intermittent > failures. Lukasz Kostyra has updated the pull request incrementally with one additional commit since the last revision: Fix cast formatting - Changes: - all: https://git.openjdk.org/jfx/pull/1403/files - new: https://git.openjdk.org/jfx/pull/1403/files/da7c97e4..6c14114b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1403&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1403&range=00-01 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1403.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1403/head:pull/1403 PR: https://git.openjdk.org/jfx/pull/1403
Re: RFR: 8207379: Robot screen capture test fails with HiDPI at some screen locations
On Fri, 15 Mar 2024 11:37:53 GMT, Karthik P K wrote: >> There were two different problems with this test's stability and HiDPI and >> both were fixed with this change. >> >> The whole reason for this tests lack of stability comes with how Windows >> calculates window position when using HiDPI. When window's X/Y coordinates >> are not provided by the application, they are (generally speaking) randomly >> selected by Windows. X/Y are picked as two integers, and afterwards the >> HiDPI scaling is applied. With fractional scaling like 125% this can cause >> X/Y coordinates to have fractional values. >> >> When performing a screen capture, Robot takes a generous estimate of >> window's position and dimensions. This is done by taking provided X/Y >> coordinates (in case of this test, stage's X/Y coords), multiplying them by >> scaling (ex. 1.25) and then flooring the result. Similar estimation is done >> to opposite coordinates - we take X/Y coords, add width/height to them >> respectively, multiply them by scaling and ceiling them. As a result, we >> have minX/Y and maxX/Y coords of capture region, which is used to calculate >> capture region's width and height. The first problem with test's stability >> is right here - the test did not take this calculation and floor/ceil >> operations into account, which with appropriately randomized window >> coordinates could cause discrepancy between expected width/height and >> actual. This logic that is used by `getScreenCapture()` seems to be >> necessary and good, so to remedy the problem I replicated those calculations >> on test side. >> >> Another problem came after that - fractional X/Y values caused capture to >> not always line up with pixel layout, which made the 1-pixel border of >> capture sometimes incorrect (an average of stage's MAGENTA color and >> whatever background happened to be under the displayed stage). This means we >> cannot exactly expect those pixels to have exactly MAGENTA color when the >> system has fractional HiDPI enabled. To help that, the test now doesn't >> check for correctness of the 1-pixel border of captured image. I feel this >> still gives us enough confidence that the Robot screen capture works >> properly, while stabilizing the test on all systems. >> >> I briefly considered an alternative approach to hard-set X/Y values but >> doing it this way gives us a bit more robustness, makes the test independent >> of Stage's X/Y coordinates which matter quite a lot. >> >> Verified this change on macOS and especially on Windows - ran the test on >> Windows 50 times each on 125% and 100% scaling and noticed no intermittent >> failures. > > Tested the changes in Mac, Windows and Linux. The changes fixes the issues in > Mac and Windows. > I ran the test in Linux VM and it failed with and without the fix with error: > > RobotTest > testScreenCapture FAILED > junit.framework.AssertionFailedError: expected: rgba(255,0,255,255) but > was: rgba(0,0,0,255) > at > test.robot.javafx.scene.RobotTest.assertColorEquals(RobotTest.java:848) > at > test.robot.javafx.scene.RobotTest.testScreenCapture(RobotTest.java:774) > > > Changing the scale also did not make any difference. Not sure if it is > related to anything in VM. > It is a Ubuntu 22.04 VM > > Added a minor comment inline. @karthikpandelu I just tried this on Ubuntu 22.04 VM and everything seems to work fine. (0,0,0,255) returned from capture suggests capture failed - could it be your VM is running on Wayland instead of X11? > tests/system/src/test/java/test/robot/javafx/scene/RobotTest.java line 758: > >> 756: // Below calculations follow how getScreenCapture should >> calculate screen capture dimensions. This >> 757: // is to make this code consistent and stable on HiDPI systems. >> 758: int stageX = (int)stage.getX(); > > Minor: Do you think space should be added after `(int)` similar to `(double)`? It would be good for consistent formatting. I'll add those. - PR Comment: https://git.openjdk.org/jfx/pull/1403#issuecomment-2007138448 PR Review Comment: https://git.openjdk.org/jfx/pull/1403#discussion_r1530334956
Re: RFR: 8323511 Scrollbar Click jumps inconsistent amount of pixels [v2]
On Sun, 17 Mar 2024 17:59:07 GMT, Johan Vos wrote: >> As I was also involved in fixing this error, I can answer. >> >> The `getAvailableCell()` method creates a new cell if none exists for the >> specified index (-1). >> The `getCell()` returns the `accumCell` in this case, which is completely >> sufficient for our case, because we only need the dimensions of the cell >> here. The cell itself will not be added to the scene-graph. > > That sounds right. > @Maran23 does this answer your question? Yes. But is it possible to scroll to an index that is negative? (<0) And what do we estimate then? - PR Review Comment: https://git.openjdk.org/jfx/pull/1326#discussion_r1530233902
