On Wed, 10 May 2023 06:54:35 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 10 additional
>> commits since the last revision:
>>
>> - removed monkey tester changes
>> - Merge remote-tracking branch 'origin/master' into 8307538.refresh
>> - whitespace
>> - updated test
>> - variable tree cell fix
>> - fixed size selector
>> - list view page
>> - fix and test
>> - skin menu
>> - refresh button
>
> 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.
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190070346
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1190071689
