On Thu, 26 Jan 2023 18:12:23 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Florian Kirmaier has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains eight commits:
>>
>> - JDK-8269907
>> Added missing changes after merge
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8269907-dirty-and-removed
>>
>> # Conflicts:
>> # modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>> # modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>> - Merge remote-tracking branch 'origin/master'
>> - JDK-8269907
>> Removed the sync methods for the scene, because they don't work when peer
>> is null, and they are not necessary.
>> - JDK-8269907
>> Fixed rare bug, causing bounds to be out of sync.
>> - JDK-8269907
>> We now require the rendering lock when cleaning up dirty nodes. To do so,
>> we moved some code required for snapshot into a reusable method.
>> - JDK-8269907
>> The bug is now fixed in a new way. Toolkit now supports registering
>> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>> - JDK-8269907
>> Fixing dirty nodes and parent removed, when a window is no longer
>> showing. This typically happens with context menus.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 745:
>
>> 743: private void checkCleanDirtyNodes() {
>> 744: if(!cleanupAdded) {
>> 745: if((window.get() == null || !window.get().isShowing()) &&
>> dirtyNodesSize > 0) {
>
> Minor: space after `if`.
> You could also reduce nesting by consolidating the two separate conditions.
I've merged it now to 1 if, with a space!
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2112:
>
>> 2110:
>> **************************************************************************/
>> 2111:
>> 2112: private void windowForSceneChanged(Window oldWindow, Window
>> window) {
>
> This change doesn't seem necessary.
I've reverted the variable name.
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2129:
>
>> 2127: }
>> 2128:
>> 2129: private final ChangeListener<Boolean> sceneWindowShowingListener =
>> (p, o, n) -> {checkCleanDirtyNodes(); } ;
>
> Minor: should be no space before `;`.
> You could also remove the curly braces.
I've removed now the curly braces.
> tests/system/src/test/java/test/javafx/scene/DirtyNodesLeakTest.java line 35:
>
>> 33: import junit.framework.Assert;
>> 34: import org.junit.BeforeClass;
>> 35: import org.junit.Test;
>
> Since this is a new class, you should use the JUnit5 API.
I'm now using the junit5 api.
-------------
PR: https://git.openjdk.org/jfx/pull/584
