On Fri, 27 Jan 2023 14:51:59 GMT, John Hendrikx <jhendr...@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.
>
> I took another good look at this solution, and I would like to offer an
> alternative as I think this solution is more dealing with symptoms of the
> underlying problem.
>
> I think the underlying problem is:
>
> 1) `Parent` is tracking a `removed` list, which is a list of `Node`. However,
> it only requires the peer for the dirty area calculation. This is a classic
> case of not being specific enough with the information you need. If `Parent`
> is modified to store `NGNode` in its `removed` list, then the problem of
> `removed` keeping references to old nodes disappears as `NGNode` does not
> refer back to `Node`. Note: there is a test method currently in `Parent`
> that returns the list of removed nodes -- these tests would need to be
> modified to work with a list of `NGNode` or some other way should be found to
> check these cases.
>
> 2) `Scene` keeps tracking dirty nodes even when it is not visible. The list
> of dirty nodes could (before this fix) become as big as you want as it was
> never cleared as the pulse listener that synchronizes the nodes never runs
> when `peer == null` -- keep adding and removing new nodes while the scene is
> not shown, and the list of dirty nodes grows indefinitely. This only happens
> if the scene has been shown at least once before, as before that time special
> code kicks in that tries to avoid keeping track of all scene nodes in the
> dirty list.
>
> I think the better solution here would be to reset the scene to go back to
> its initial state (before it was shown) and sync all nodes when it becomes
> visible again. This can be achieved by setting the dirty node list to `null`
> to trigger the logic that normally only triggers on the first show.
> `addToDirtyList` should simply do nothing when `peer == null` .
>
> I believe the `syncPeer` call is smart enough not to update the peer in the
> situation where a scene is hidden and then reshown, even if `Scene` calls it
> again on all its nodes (`syncPeer` in `Node` will check
> `dirtyBits.isEmpty()`).
>
> I'd also highly recommend using `ArrayList` instead of a manual `Node[]
> dirtyNodes` in `Scene`.
@hjohn
The Parent is tracking a removed list, which is a list of Node. However, it
only requires the peer for the dirty area calculation. This is a classic case
of not being specific enough with the information you need. If Parent is
modified to store NGNode in its removed list, then the problem of removed
keeping references to old nodes disappears as NGNode does not refer back to
Node. Note: there is a test method currently in Parent that returns the list of
removed nodes -- these tests would need to be modified to work with a list of
NGNode or some other way should be found to check these cases.
This would just end in an leak of the NGNode, instead of the Node?
There are also some leaks related to NGNode, bug guess I'll have to fix the
more obvious bugs first.
Also, NGNode sometimes refers to the Node. There are a lot of issues in this
area.
## Resetting the Scene
This approach has one problem - it would change the complexity of
showing/hiding a window. This issue typically happened with Popups, which are
regularly shown/hidden. I guess changing the complexity to O(<number-of-node>)
might be ok - but I'm not sure about it.
I don't want to change the dirtyNodes to ArrayList in this PR, i don't want to
mix this kind of refactoring into the Bugfix-PR.
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 492:
>
>> 490: }
>> 491:
>> 492: public void addCleanupListener(TKPulseListener listener) {
>
> This name is misleading. It adds a listener which gets auto removed when
> called (and hence why there is no `removeCleanupListener` counterpart). In
> effect, it works a bit like `runLater`, a one time action to be run on the
> next pulse.
>
> In general, I would like to see more comments/docs to explain what is going
> on.
I've changed it to addCleanupTask
-------------
PR: https://git.openjdk.org/jfx/pull/584
