On Fri, 24 Sep 2021 16:01:38 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
> Cleanup of Tree-/TableRowSkin to support switching skins > > The misbehavior/s > - memory leaks due to manually registered listeners that were not removed > - side-effects due to listeners still active on old skin (like NPEs) > > Fix > - use skin api for all listener registration (for automatic removal in > dispose) > - do not install listeners that are not needed (fixedCellSize, same as in fix > of ListCellSkin > [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745)) > > Added tests for each listener involved in the fix to guarantee it's still > working and does not have unwanted side-effects after switching skins. > > Note: there are pecularities in row skins (like not updating themselves on > property changes of its control, throwing NPEs when not added to a > VirtualFlow) which are not part of this issue but covered in > [JDK-8274065](https://bugs.openjdk.java.net/browse/JDK-8274065) I have two comments regarding possibility of introducing a regression. Can you please confirm that it does not cause any side effect? Rest of the fix looks ok. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkin.java line 134: > 132: // that when it changes, we can appropriately add / > remove cells that may or may not > 133: // be required (because we remove all cells that are not > visible). > 134: registerChangeListener(getVirtualFlow().widthProperty(), > e -> tableView.requestLayout()); If this listener is removed, then is there a chance of reintroducing [JDK-8144500](https://bugs.openjdk.java.net/browse/JDK-8144500)? Unfortunately, there is no test added for that bug.. so it is difficult to catch regression, if any. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 154: > 152: // that when it changes, we can appropriately add / > remove cells that may or may not > 153: // be required (because we remove all cells that are not > visible). > 154: registerChangeListener(getVirtualFlow().widthProperty(), > e -> treeTableView.requestLayout()); Same comment as in TableRowSkin regarding this listener. ------------- PR: https://git.openjdk.java.net/jfx/pull/632
