On Mon, 8 May 2023 18:55:38 GMT, Andy Goryachev <[email protected]> wrote:
>> Fixed a memory leak in TreeTableView by using WeakInvalidationListener
>> (which is the right approach in this particular situation) - the leak is
>> specific to TreeTableRowSkin.
>>
>> Added a unit test.
>>
>> Added Refresh buttons and Skin menu to the Monkey Tester (will expand these
>> to other controls as a part next MT update).
>
> 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 101:
> 99: control.indexProperty().addListener(new
> WeakInvalidationListener((x) -> {
> 100: updateCells = true;
> 101: }));
I don't think this needed changing, the index property is directly available on
`TreeTableRow` and so shouldn't cause GC issues.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
line 108:
> 106: // determined to be unnecessary and led to performance
> issues such as
> 107: // those detailed in JDK-8143266
> 108: }));
As above, I don't think this needs to be weak.
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.
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?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189429842
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189430267
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189434672
PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189438781
