On Thu, 14 Mar 2024 16:06:55 GMT, Lukasz Kostyra <lkost...@openjdk.org> 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.
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)`?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1403#pullrequestreview-1938661604
PR Review Comment: https://git.openjdk.org/jfx/pull/1403#discussion_r1526104191
