On Wed, 10 May 2023 15:20:25 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
>> line 334:
>>
>>> 332: private void updateTreeItem() {
>>> 333: unregisterInvalidationListeners(graphicProperty());
>>> 334: treeItem = (getSkinnable() == null) ? null :
>>> getSkinnable().getTreeItem();
>>
>> This is not needed if the weak listener change is reverted in the
>> constructor. The cause for it being `null` is that the listener is now no
>> longer removed by `dispose` where before (with `ListenerHelper` or
>> `registerXXXListener`) it would have been.
>>
>> Also, I think when `dispose` is called, this skin should still do its utmost
>> best to remove all listeners immediately.
>
> The problem is, `dispose()` is never called for cells discarded by the
> refresh() method.
>
> In the absence of explicit release, these cells are still listening - so
> using a weak listener in this case is, I think, the right approach. Adding
> explicit release per your (absolutely valid) suggestion is a good idea, but
> much, much more complicated task.
Even if you are using weak listeners, `dispose` should remove them.
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
>> line 335:
>>
>>> 333:
>>> 334: treeTableView.refresh();
>>> 335: Toolkit.getToolkit().firePulse();
>>
>> I think cells should also be collectable in other situations, like replacing
>> the row factory. Is it worth adding a test for that?
>
> this test reflects the exact failure scenario described in the ticket.
If it's easy to add another test for additional missing cases that would
provide better coverage for your fix, it seems worth doing.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190136063
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190138025
