On Mon, 6 May 2024 14:14:05 GMT, eduardsdv <d...@openjdk.org> wrote:
> This is an alternative solution to the PR:
> https://github.com/openjdk/jfx/pull/1310.
>
> This solution is based on the invariant that if a node is marked as dirty,
> all ancestors must also be marked as dirty and that if an ancestor is marked
> as clean, all descendants must also be marked as clean.
> Therefore I removed the ``clearDirtyTree()`` method and put its content to
> the ``clearDirty()`` method.
>
> Furthermore, since dirty flag is only used when rendering by ``ViewPainter``,
> it should also be deleted by ``ViewPainter`` only.
> This guarantees:
> 1. that all dirty flags are removed after rendering, and
> 2. that no dirty flags are removed when a node is rendered, e.g. by creating
> a snapshot or printing.
> Therefore I removed all calls of the methods ``clearDirty()`` and
> ``clearDirtyTree()`` from all other classes except the ``ViewerPainter``.
>
> The new version of the ``clearDirty()`` method together with calling it from
> the ``ViewerPainter`` needs to visit far fewer nodes compared to the version
> prior this PR.
>
> The supplied test checks that the nodes are updated even if they are
> partially covered, which led to the error in the version before the PR. The
> test can be started with ``gradlew -PFULL_TEST=true :systemTests:test --tests
> NGNodeDirtyFlagTest``
Providing a few comments on test.
I might need a day or two more for testing/reviewing the fix.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
line 68:
> 66: primaryStage.show();
> 67:
> 68: launchLatch.countDown();
This countDown() should be called after stage is shown to ensure the stage
shown before proceeding.
`primaryStage.setOnShown(e -> Platform.runLater(launchLatch::countDown));`
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
line 75:
> 73: public static void setupOnce() {
> 74: Util.launch(launchLatch, MyApp.class);
> 75: assertEquals(0, launchLatch.getCount());
Util.launch() asserts if launchLatch.countDown() does not occur in 15 seconds,
so this line can be removed.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
line 90:
> 88: StackPane root = myApp.root;
> 89:
> 90: runAndWait(() -> {
All such calls can be changed to use `Util.runAndWait()`
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
line 103:
> 101: });
> 102:
> 103: Thread.sleep(500);
Could use `Util.waitForIdle(Scene)` instead of sleep. It would reduce the test
time and should work. We can consider a sleep if `Util.waitForIdle(Scene)` does
not work. Similarly please see if other sleep calls can be removed.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
line 149:
> 147: throw new RuntimeException(e);
> 148: }
> 149: }
Please use the `Util.runAndWait()` instead of adding this method. So this
method can be removed and all calls can be changed to Util.runAndWait().
Please do remove any imports that become unused afterwards.
tests/system/src/test/java/test/com/sun/prism/impl/NGNodeDirtyFlagTest.java
line 188:
> 186: return result;
> 187: }
> 188: }
Please add an empty line at the end.
-------------
Changes requested by arapte (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1451#pullrequestreview-2147056008
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658155426
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658158260
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658169114
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658173122
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658166372
PR Review Comment: https://git.openjdk.org/jfx/pull/1451#discussion_r1658161537
