On Sun, 31 Oct 2021 17:05:52 GMT, Michael Strauß <mstra...@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
