8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit

Jeanette Winzenburg · Jeanette Winzenburg · commit adcc40d57c0f · 2021-10-29T12:28:37.000Z
Reviewed-by: mhanl, aghaisas
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java
@@ -369,17 +369,18 @@ public void changed(
         super.startEdit();
 
         if (!isEditing()) return;
+
+        indexAtStartEdit = getIndex();
          // Inform the ListView of the edit starting.
         if (list != null) {
             list.fireEvent(new ListView.EditEvent<T>(list,
                     ListView.<T>editStartEvent(),
                     null,
-                    getIndex()));
-            list.edit(getIndex());
+                    indexAtStartEdit));
+            list.edit(indexAtStartEdit);
             list.requestFocus();
         }
 
-        indexAtStartEdit = getIndex();
     }
 
     /** {@inheritDoc} */
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java
@@ -332,6 +332,7 @@ TablePosition<S, T> getEditingCellAtStartEdit() {
         super.startEdit();
 
         if (!isEditing()) return;
+
         editingCellAtStartEdit = new TablePosition<>(table, getIndex(), column);
         if (column != null) {
             CellEditEvent<S,?> editEvent = new CellEditEvent<>(
@@ -343,6 +344,9 @@ TablePosition<S, T> getEditingCellAtStartEdit() {
 
             Event.fireEvent(column, editEvent);
         }
+        if (table != null) {
+            table.edit(editingCellAtStartEdit.getRow(), editingCellAtStartEdit.getTableColumn());
+        }
     }
 
     /** {@inheritDoc} */
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java
@@ -377,17 +377,18 @@ public String getName() {
         super.startEdit();
 
         if (!isEditing()) return;
+
+        treeItemAtStartEdit = getTreeItem();
          // Inform the TreeView of the edit starting.
         if (tree != null) {
             tree.fireEvent(new TreeView.EditEvent<T>(tree,
                     TreeView.<T>editStartEvent(),
-                    getTreeItem(),
+                    treeItemAtStartEdit,
                     getItem(),
                     null));
-
+            tree.edit(treeItemAtStartEdit);
             tree.requestFocus();
         }
-        treeItemAtStartEdit = getTreeItem();
     }
 
      /** {@inheritDoc} */
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java
@@ -349,6 +349,7 @@ TreeTablePosition<S, T> getEditingCellAtStartEdit() {
         super.startEdit();
 
         if (!isEditing()) return;
+
         editingCellAtStartEdit = new TreeTablePosition<>(table, getIndex(), column);
         if (column != null) {
             CellEditEvent<S, T> editEvent = new CellEditEvent<>(
@@ -360,6 +361,9 @@ TreeTablePosition<S, T> getEditingCellAtStartEdit() {
 
             Event.fireEvent(column, editEvent);
         }
+        if (table != null) {
+            table.edit(editingCellAtStartEdit.getRow(), editingCellAtStartEdit.getTableColumn());
+        }
     }
 
     /** {@inheritDoc} */
diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
@@ -31,7 +31,6 @@
 
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import com.sun.javafx.tk.Toolkit;
@@ -541,6 +540,28 @@ public void testEditStartFiresEvent() {
         assertEquals("startEdit must fire", 1, events.size());
     }
 
+    @Test
+    public void testEditStartOnCellUpdatesControl() {
+        setupForEditing();
+        int editingRow = 1;
+        cell.updateIndex(editingRow);
+        TablePosition<?, ?> editingCell = new TablePosition<>(table, editingRow, editingColumn);
+        cell.startEdit();
+        assertEquals("table must be editing at", editingCell, table.getEditingCell());
+    }
+
+    @Test
+    public void testEditStartOnCellNoColumnUpdatesControl() {
+        int editingRow = 1;
+        // note: cell index must be != -1 because table.edit(-1, null) sets editingCell to null
+        cell.updateIndex(editingRow);
+        setupForcedEditing(table, null);
+        TablePosition<?, ?> editingCell = new TablePosition<>(table, editingRow, null);
+        cell.startEdit();
+        assertTrue(cell.isEditing());
+        assertEquals("table must be editing at", editingCell, table.getEditingCell());
+    }
+
     @Test
     public void testEditStartDoesNotFireEventWhileEditing() {
         setupForEditing();
@@ -586,9 +607,7 @@ public void testEditCommitEvent() {
          setupForEditing();
          int editingIndex = 1;
          cell.updateIndex(editingIndex);
-         // FIXME JDK-8187474
-         // should use cell.startEdit for consistency with the following tests
-         table.edit(editingIndex, editingColumn);
+         cell.startEdit();
          TablePosition<?, ?> editingPosition = table.getEditingCell();
          List<CellEditEvent<?, ?>> events = new ArrayList<>();
          editingColumn.setOnEditCommit(events::add);
@@ -714,10 +733,6 @@ public void testStartEditOffRangeMustNotFireStartEdit() {
          assertEquals("must not fire editStart", 0, events.size());
      }
 
-     /**
-      *  Note: this is a false green until JDK-8187474 (update control editing location) is fixed
-      */
-     @Ignore("JDK-8187474")
      @Test
      public void testStartEditOffRangeMustNotUpdateEditingLocation() {
          setupForEditing();
diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java
@@ -599,7 +599,6 @@ public void cleanup() {
         assertNull(tree.getEditingItem());
     }
 
-    @Ignore // TODO file bug!
     @Test public void editCellWithTreeResultsInUpdatedEditingIndexProperty() {
         tree.setEditable(true);
         cell.updateTreeView(tree);
@@ -848,23 +847,21 @@ public void testEditCommitMemoryLeakAfterRemoveEditingItem() {
     public void testStartEditOffRangeMustNotFireStartEdit() {
         tree.setEditable(true);
         cell.updateTreeView(tree);
-        cell.updateIndex(tree.getExpandedItemCount());
+        // update cell's treeItem so there is something to update
+        cell.updateTreeItem(new TreeItem<>("not-contained"));
         List<EditEvent<?>> events = new ArrayList<>();
         tree.addEventHandler(TreeView.editStartEvent(), events::add);
         cell.startEdit();
         assertFalse("sanity: off-range cell must not be editing", cell.isEditing());
         assertEquals("cell must not fire editStart if not editing", 0, events.size());
     }
 
-    /**
-     *  Note: this is a false green until JDK-8187474 (update control editing location) is fixed
-     */
-    @Ignore("JDK-8187474")
     @Test
     public void testStartEditOffRangeMustNotUpdateEditingLocation() {
         tree.setEditable(true);
         cell.updateTreeView(tree);
-        cell.updateIndex(tree.getExpandedItemCount());
+        // update cell's treeItem so there is something to update
+        cell.updateTreeItem(new TreeItem<>("not-contained"));
         cell.startEdit();
         assertFalse("sanity: off-range cell must not be editing", cell.isEditing());
         assertNull("tree editing location must not be updated", tree.getEditingItem());
diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java
@@ -304,15 +304,23 @@ public void cleanup() {
         assertNull(tree.getEditingCell());
     }
 
-    @Ignore // TODO file bug!
     @Test public void editCellWithTreeResultsInUpdatedEditingIndexProperty() {
-        tree.setEditable(true);
-        cell.updateTreeTableView(tree);
+        setupForEditing();
         cell.updateIndex(1);
         cell.startEdit();
         assertEquals(apples, tree.getEditingCell().getTreeItem());
     }
 
+    @Test public void editCellWithTreeNoColumnResultsInUpdatedEditingIndexProperty() {
+        // note: cell index must be != -1 because table.edit(-1, null) sets editingCell to null
+        cell.updateIndex(1);
+        setupForcedEditing(tree, null);
+        cell.startEdit();
+        assertTrue(cell.isEditing());
+        assertNotNull(tree.getEditingCell());
+        assertEquals(apples, tree.getEditingCell().getTreeItem());
+    }
+
 //    @Ignore // TODO file bug!
 //    @Test public void editCellFiresEventOnTree() {
 //        tree.setEditable(true);
@@ -911,9 +919,7 @@ public void testEditCommitEvent() {
          setupForEditing();
          int editingIndex = 1;
          cell.updateIndex(editingIndex);
-         // FIXME JDK-8187474
-         // should use cell.startEdit for consistency with the following tests
-         tree.edit(editingIndex, editingColumn);
+         cell.startEdit();
          TreeTablePosition<?, ?> editingPosition = tree.getEditingCell();
          List<CellEditEvent<?, ?>> events = new ArrayList<>();
          editingColumn.setOnEditCommit(events::add);
@@ -1038,10 +1044,6 @@ public void testStartEditOffRangeMustNotFireStartEdit() {
          assertEquals("cell must not fire editStart if not editing", 0, events.size());
      }
 
-     /**
-      *  Note: this would be a false green until JDK-8187474 (update control editing location) is fixed
-      */
-     @Ignore("JDK-8187474")
      @Test
      public void testStartEditOffRangeMustNotUpdateEditingLocation() {
          setupForEditing();

Original file line number	Diff line number	Diff line change
`@@ -332,6 +332,7 @@ TablePosition<S, T> getEditingCellAtStartEdit() {`
`332`	`332`	`super.startEdit();`
`333`	`333`
`334`	`334`	`if (!isEditing()) return;`
	`335`	`+`
`335`	`336`	`editingCellAtStartEdit = new TablePosition<>(table, getIndex(), column);`
`336`	`337`	`if (column != null) {`
`337`	`338`	`CellEditEvent<S,?> editEvent = new CellEditEvent<>(`
`@@ -343,6 +344,9 @@ TablePosition<S, T> getEditingCellAtStartEdit() {`
`343`	`344`
`344`	`345`	`Event.fireEvent(column, editEvent);`
`345`	`346`	`}`
	`347`	`+ if (table != null) {`
	`348`	`+ table.edit(editingCellAtStartEdit.getRow(), editingCellAtStartEdit.getTableColumn());`
	`349`	`+ }`
`346`	`350`	`}`
`347`	`351`
`348`	`352`	`/** {@inheritDoc} */`
Original file line number	Diff line number	Diff line change
`@@ -349,6 +349,7 @@ TreeTablePosition<S, T> getEditingCellAtStartEdit() {`
`349`	`349`	`super.startEdit();`
`350`	`350`
`351`	`351`	`if (!isEditing()) return;`
	`352`	`+`
`352`	`353`	`editingCellAtStartEdit = new TreeTablePosition<>(table, getIndex(), column);`
`353`	`354`	`if (column != null) {`
`354`	`355`	`CellEditEvent<S, T> editEvent = new CellEditEvent<>(`
`@@ -360,6 +361,9 @@ TreeTablePosition<S, T> getEditingCellAtStartEdit() {`
`360`	`361`
`361`	`362`	`Event.fireEvent(column, editEvent);`
`362`	`363`	`}`
	`364`	`+ if (table != null) {`
	`365`	`+ table.edit(editingCellAtStartEdit.getRow(), editingCellAtStartEdit.getTableColumn());`
	`366`	`+ }`
`363`	`367`	`}`
`364`	`368`
`365`	`369`	`/** {@inheritDoc} */`