8267094: TreeCell: cancelEvent must return correct editing location

Jeanette Winzenburg · Jeanette Winzenburg · commit e6cf1df477a7 · 2021-06-10T11:36:32.000Z
Reviewed-by: aghaisas
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
@@ -351,6 +351,8 @@ public String getName() {
      * Public API                                                              *
      *                                                                         *
      **************************************************************************/
+    // treeItem at time of startEdit - fix for JDK-8267094
+    private TreeItem<T> treeItemAtStartEdit;
 
     /** {@inheritDoc} */
     @Override public void startEdit() {
@@ -384,6 +386,7 @@ public String getName() {
 
             tree.requestFocus();
         }
+        treeItemAtStartEdit = getTreeItem();
     }
 
      /** {@inheritDoc} */
@@ -424,6 +427,7 @@ public String getName() {
             // It would be rude of us to request it back again.
             ControlUtils.requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild(tree);
         }
+        treeItemAtStartEdit = null;
     }
 
     /** {@inheritDoc} */
@@ -435,6 +439,9 @@ public String getName() {
         super.cancelEdit();
 
         if (tree != null) {
+            TreeItem<T> editingItem = treeItemAtStartEdit;
+            T value = editingItem != null ? editingItem.getValue() : null;
+
             // reset the editing index on the TreeView
             if (updateEditingIndex) tree.edit(null);
 
@@ -446,10 +453,11 @@ public String getName() {
 
             tree.fireEvent(new TreeView.EditEvent<T>(tree,
                     TreeView.<T>editCancelEvent(),
-                    getTreeItem(),
-                    getItem(),
+                    editingItem,
+                    value,
                     null));
         }
+        treeItemAtStartEdit = null;
     }
 
     /** {@inheritDoc} */
diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellEditingTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellEditingTest.java
@@ -31,7 +31,6 @@
 import java.util.List;
 
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -85,21 +84,6 @@ public void testCancelOffEditingIndex() {
         assertEquals("sanity: tree editing unchanged", editingItem, tree.getEditingItem());
         assertEquals("sanity: editingIndex unchanged", editingIndex, tree.getRow(editingItem));
         assertEquals("cell must have fired edit cancel", 1, events.size());
-    }
-
-    /**
-     * Extracted from testCancelOffEditingIndex to formally ignore
-     * FIXME: move the assert to the other method, once the issue is solved
-     */
-    @Ignore("JDK-8267094")
-    @Test
-    public void testCancelOffEditingIndexEventIndex() {
-        cell.updateIndex(editingIndex);
-        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
-        tree.edit(editingItem);
-        List<EditEvent<String>> events = new ArrayList<>();
-        tree.setOnEditCancel(events::add);
-        cell.updateIndex(cellIndex);
         assertEquals("cancel on updateIndex from " + editingIndex + " to " + cellIndex + "\n  ",
                 editingItem, events.get(0).getTreeItem());
     }
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
@@ -25,6 +25,22 @@
 
 package test.javafx.scene.control;
 
+import java.lang.ref.WeakReference;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import com.sun.javafx.tk.Toolkit;
+
+import static javafx.scene.control.ControlShim.*;
+import static org.junit.Assert.*;
+import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
+import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
+
 import javafx.beans.InvalidationListener;
 import javafx.collections.ListChangeListener;
 import javafx.scene.control.FocusModel;
@@ -34,14 +50,10 @@
 import javafx.scene.control.TreeCell;
 import javafx.scene.control.TreeItem;
 import javafx.scene.control.TreeView;
+import javafx.scene.control.TreeView.EditEvent;
 import javafx.scene.control.skin.TreeCellSkin;
-import org.junit.Before;
-import org.junit.Ignore;
-import org.junit.Test;
-
-import static javafx.scene.control.ControlShim.*;
-import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
-import static org.junit.Assert.*;
+import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
+import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
 
 public class TreeCellTest {
     private TreeCell<String> cell;
@@ -56,6 +68,7 @@ public class TreeCellTest {
     private TreeItem<String> apples;
     private TreeItem<String> oranges;
     private TreeItem<String> pears;
+    private StageLoader stageLoader;
 
     @Before public void setup() {
         cell = new TreeCell<String>();
@@ -70,6 +83,11 @@ public class TreeCellTest {
         root.setExpanded(true);
     }
 
+    @After
+    public void cleanup() {
+        if (stageLoader != null) stageLoader.dispose();
+    }
+
     /*********************************************************************
      * Tests for the constructors                                        *
      ********************************************************************/
@@ -686,6 +704,146 @@ public class TreeCellTest {
         assertFalse(cell.isEditing());
     }
 
+    @Test
+    public void testEditCancelEventAfterCancelOnCell() {
+        tree.setEditable(true);
+        cell.updateTreeView(tree);
+        int editingIndex = 1;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        cell.updateIndex(editingIndex);
+        tree.edit(editingItem);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        cell.cancelEdit();
+        assertEquals(1, events.size());
+        assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem());
+    }
+
+    @Test
+    public void testEditCancelEventAfterCancelOnTree() {
+        tree.setEditable(true);
+        cell.updateTreeView(tree);
+        int editingIndex = 1;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        cell.updateIndex(editingIndex);
+        tree.edit(editingItem);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        tree.edit(null);
+        assertEquals(1, events.size());
+        assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem());
+    }
+
+    @Test
+    public void testEditCancelEventAfterCellReuse() {
+        tree.setEditable(true);
+        cell.updateTreeView(tree);
+        int editingIndex = 1;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        cell.updateIndex(editingIndex);
+        tree.edit(editingItem);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        cell.updateIndex(0);
+        assertEquals(1, events.size());
+        assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem());
+    }
+
+    @Test
+    public void testEditCancelEventAfterCollapse() {
+        stageLoader = new StageLoader(tree);
+        tree.setEditable(true);
+        int editingIndex = 1;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        tree.edit(editingItem);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        root.setExpanded(false);
+        Toolkit.getToolkit().firePulse();
+        assertEquals(1, events.size());
+        assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem());
+    }
+
+    @Test
+    public void testEditCancelEventAfterModifyItems() {
+        stageLoader = new StageLoader(tree);
+        tree.setEditable(true);
+        int editingIndex = 2;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        tree.edit(editingItem);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        root.getChildren().add(0, new TreeItem<>("added"));
+        Toolkit.getToolkit().firePulse();
+        assertEquals(1, events.size());
+        assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem());
+    }
+
+    /**
+     * Test that removing the editing item implicitly cancels an ongoing
+     * edit and fires a correct cancel event.
+     */
+    @Test
+    public void testEditCancelEventAfterRemoveEditingItem() {
+        stageLoader = new StageLoader(tree);
+        tree.setEditable(true);
+        int editingIndex = 2;
+        TreeItem<String> editingItem = tree.getTreeItem(editingIndex);
+        tree.edit(editingItem);
+        List<EditEvent<String>> events = new ArrayList<>();
+        tree.setOnEditCancel(events::add);
+        root.getChildren().remove(editingItem);
+        Toolkit.getToolkit().firePulse();
+        assertNull("removing item must cancel edit on tree", tree.getEditingItem());
+        assertEquals(1, events.size());
+        assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem());
+    }
+
+    /**
+     * Test that removing the editing item does not cause a memory leak.
+     */
+    @Test
+    public void testEditCancelMemoryLeakAfterRemoveEditingItem() {
+        stageLoader = new StageLoader(tree);
+        tree.setEditable(true);
+        // the item to test for being gc'ed
+        TreeItem<String> editingItem = new TreeItem<>("added");
+        WeakReference<TreeItem<?>> itemRef = new WeakReference<>(editingItem);
+        root.getChildren().add(0, editingItem);
+        Toolkit.getToolkit().firePulse();
+        tree.edit(editingItem);
+        root.getChildren().remove(editingItem);
+        Toolkit.getToolkit().firePulse();
+        assertNull("removing item must cancel edit on tree", tree.getEditingItem());
+        editingItem = null;
+        attemptGC(itemRef);
+        assertEquals("treeItem must be gc'ed", null, itemRef.get());
+    }
+
+    /**
+     * Test that removing a committed editing item does not cause a memory leak.
+     */
+    @Test
+    public void testEditCommitMemoryLeakAfterRemoveEditingItem() {
+        stageLoader = new StageLoader(tree);
+        tree.setEditable(true);
+        // the item to test for being gc'ed
+        TreeItem<String> editingItem = new TreeItem<>("added");
+        WeakReference<TreeItem<?>> itemRef = new WeakReference<>(editingItem);
+        root.getChildren().add(0, editingItem);
+        int editingIndex = tree.getRow(editingItem);
+        Toolkit.getToolkit().firePulse();
+        tree.edit(editingItem);
+        TreeCell<String> editingCell = (TreeCell<String>) VirtualFlowTestUtils.getCell(tree, editingIndex);
+        editingCell.commitEdit("added changed");
+        root.getChildren().remove(editingItem);
+        Toolkit.getToolkit().firePulse();
+        assertNull("removing item must cancel edit on tree", tree.getEditingItem());
+        editingItem = null;
+        attemptGC(itemRef);
+        assertEquals("treeItem must be gc'ed", null, itemRef.get());
+    }
+
     // When the tree view item's change and affects a cell that is editing, then what?
     // When the tree cell's index is changed while it is editing, then what?
 
