RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin

[Ajit Ghaisas]

On Fri, 24 Sep 2021 16:01:38 GMT, Jeanette Winzenburg <fastegal at 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