8251483: TableCell: NPE on modifying item's list

Reviewed-by: aghaisas, mstrauss

modules/javafx.controls/src/main/java/javafx/scene/control/IndexedCell.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -72,12 +72,17 @@ public IndexedCell() {
      *                                                                         *
      **************************************************************************/
 
+    private int oldIndex = -1;
+
     // --- Index
     private ReadOnlyIntegerWrapper index = new ReadOnlyIntegerWrapper(this, "index", -1) {
         @Override protected void invalidated() {
-            boolean active = ((get() % 2) == 0);
+            int newIndex = get();
+            boolean active = ((newIndex % 2) == 0);
             pseudoClassStateChanged(PSEUDO_CLASS_EVEN,  active);
             pseudoClassStateChanged(PSEUDO_CLASS_ODD,  !active);
+
+            indexChanged(oldIndex, newIndex);
         }
     };
 
@@ -112,19 +117,25 @@ public IndexedCell() {
      * Note: This function is intended to be used by experts, primarily
      *       by those implementing new Skins. It is not common
      *       for developers or designers to access this function directly.
-     * @param i the index associated with this indexed cell
+     * @param newIndex the index associated with this indexed cell
      */
-    public void updateIndex(int i) {
-        final int oldIndex = index.get();
-        index.set(i);
-        indexChanged(oldIndex, i);
+    public void updateIndex(int newIndex) {
+        oldIndex = index.get();
+
+        if (oldIndex == newIndex) {
+            // When the index wasn't changed the index property will not be invalidated,
+            // therefore indexChanged() is not called, so we will manually call it here.
+            indexChanged(oldIndex, newIndex);
+        } else {
+            index.set(newIndex);
+        }
     }
 
     /**
      * This method is called whenever the index is changed, regardless of whether
      * the new index is the same as the old index.
-     * @param oldIndex
-     * @param newIndex
+     * @param oldIndex the old index
+     * @param newIndex the new index
      */
     void indexChanged(int oldIndex, int newIndex) {
         // no-op

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java

@@ -29,6 +29,7 @@
 import java.util.ArrayList;
 import java.util.List;
 
+import javafx.beans.property.SimpleStringProperty;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -362,6 +363,33 @@ protected void updateItem(String item, boolean empty) {
         stageLoader = new StageLoader(table);
     }
 
+    /**
+     * The item of the {@link TableRow} should not be null, when the {@link TableCell} is not empty.
+     * See also: JDK-8251483
+     */
+    @Test
+    public void testRowItemIsNotNullForNonEmptyCell() {
+        TableColumn<String, String> tableColumn = new TableColumn<>();
+        tableColumn.setCellValueFactory(cc -> new SimpleStringProperty(cc.getValue()));
+        tableColumn.setCellFactory(col -> new TableCell<>() {
+            @Override
+            protected void updateItem(String item, boolean empty) {
+                super.updateItem(item, empty);
+
+                if (!empty) {
+                    assertNotNull(getTableRow().getItem());
+                }
+            }
+        });
+        table.getColumns().add(tableColumn);
+
+        stageLoader = new StageLoader(table);
+
+        // Will create a new row and cell.
+        table.getItems().add("newItem");
+        Toolkit.getToolkit().firePulse();
+    }
+
     /**
      * Table: Editable<br>
      * Row: Not editable<br>

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java

@@ -25,6 +25,7 @@
 
 package test.javafx.scene.control;
 
+import javafx.beans.property.SimpleStringProperty;
 import javafx.scene.control.skin.TreeTableCellSkin;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
@@ -703,6 +704,33 @@ protected void updateItem(String item, boolean empty) {
         stageLoader = new StageLoader(tree);
     }
 
+    /**
+     * The item of the {@link TreeTableRow} should not be null, when the {@link TreeTableCell} is not empty.
+     * See also: JDK-8251483
+     */
+    @Test
+    public void testRowItemIsNotNullForNonEmptyCell() {
+        TreeTableColumn<String, String> treeTableColumn = new TreeTableColumn<>();
+        treeTableColumn.setCellValueFactory(cc -> new SimpleStringProperty(cc.getValue().getValue()));
+        treeTableColumn.setCellFactory(col -> new TreeTableCell<>() {
+            @Override
+            protected void updateItem(String item, boolean empty) {
+                super.updateItem(item, empty);
+
+                if (!empty) {
+                    assertNotNull(getTableRow().getItem());
+                }
+            }
+        });
+        tree.getColumns().add(treeTableColumn);
+
+        stageLoader = new StageLoader(tree);
+
+        // Will create a new row and cell.
+        tree.getRoot().getChildren().add(new TreeItem<>("newItem"));
+        Toolkit.getToolkit().firePulse();
+    }
+
     /**
      * Table: Editable<br>
      * Row: Not editable<br>