RFR: 8274061: Tree-/TableRowSkin: misbehavior on switching skin [v2]

[Jeanette Winzenburg]

On Sun, 31 Oct 2021 17:05:52 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   re-added forgotten code comments
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 299:
> 
>> 297:     @Override protected ObjectProperty<Node> graphicProperty() {
>> 298:         TreeTableRow<T> treeTableRow = getSkinnable();
>> 299:         // FIXME: illegal access if skinnable is null
> 
> What is the purpose of removing the null check, but leaving getSkinnable() in there?
> Given the signature of the method, it seems that it shouldn't ever return `null` in any case.

none except me having been sloppy ;) - removed both the access and the left-over code comment from my dev version). And yeah, true: guarding against null skinnable is always an indication of something being wrong (skinnable can be null only after dispose - after that, its methods must not be used)

For fun, here's the evolution of  `graphicProperty()` (which was `getGraphic()` early on):

Initially the treeItem is accessed directly from the skinnable: the null guard already is wrong (and might hide problems elsewhere), but doesn't look so

    @Override protected Node getGraphic() {
        TreeTableRow<T> treeTableRow = getSkinnable();
        if (treeTableRow == null) return null;

        TreeItem<T> treeItem = treeTableRow.getTreeItem();
        if (treeItem == null) return null;

        return treeItem.getGraphic();
    }

At some time ([JDK-8124119](https://bugs.openjdk.java.net/browse/JDK-8124119) was RT-28098) the direct access was replaced by keeping an alias to the treeItem. Now the skinnable is not needed, and the null guard still is wrong :) 

     @Override protected Node getGraphic() {
        TreeTableRow<T> treeTableRow = getSkinnable();
        if (treeTableRow == null) return null;
        if (treeItem == null) return null;

        return treeItem.getGraphic();
    }

-------------

PR: https://git.openjdk.java.net/jfx/pull/632