8187309: TreeCell must not change tree's data

Reviewed-by: mstrauss, mhanl, aghaisas

Author: Jeanette Winzenburg

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

@@ -414,12 +414,9 @@ public String getName() {
                     newValue));
         }
 
+        // FIXME: JDK-8187314 must respect actual committed value
         // update the item within this cell, so that it represents the new value
-        if (treeItem != null) {
-            treeItem.setValue(newValue);
-            updateTreeItem(treeItem);
-            updateItem(newValue, false);
-        }
+        updateItem(newValue, false);
 
         if (tree != null) {
             // reset the editing item in the TreetView

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

@@ -337,6 +337,8 @@ public TreeView(TreeItem<T> root) {
         MultipleSelectionModel<TreeItem<T>> sm = new TreeViewBitSetSelectionModel<T>(this);
         setSelectionModel(sm);
         setFocusModel(new TreeViewFocusModel<T>(this));
+
+        setOnEditCommit(DEFAULT_EDIT_COMMIT_HANDLER);
     }
 
 
@@ -845,6 +847,11 @@ public final ObjectProperty<EventHandler<EditEvent<T>>> onEditCommitProperty() {
         return onEditCommit;
     }
 
+    private EventHandler<TreeView.EditEvent<T>> DEFAULT_EDIT_COMMIT_HANDLER = t -> {
+        TreeItem<T> editedItem = t.getTreeItem();
+        if (editedItem == null) return;
+        editedItem.setValue(t.getNewValue());
+    };
 
     // --- On Edit Cancel
     private ObjectProperty<EventHandler<EditEvent<T>>> onEditCancel;

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

@@ -51,6 +51,7 @@
 import javafx.scene.control.TreeItem;
 import javafx.scene.control.TreeView;
 import javafx.scene.control.TreeView.EditEvent;
+import javafx.scene.control.cell.TextFieldTreeCell;
 import javafx.scene.control.skin.TreeCellSkin;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils;
@@ -888,6 +889,85 @@ public void testCommitEditMustNotFireCancel() {
         assertEquals("commit must not have fired editCancel", 0, events.size());
     }
 
+//------------- JDK-8187309: fix editing mechanics to comply to specification
+
+    @Test
+    public void testTreeHasDefaultCommitHandler() {
+        assertNotNull("treeView must have default commit handler", tree.getOnEditCommit());
+    }
+
+    @Test
+    public void testDefaultCommitUpdatesData() {
+        TreeItem<String> editingItem = setupForEditing(cell);
+        tree.edit(editingItem);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("value committed", value, editingItem.getValue());
+    }
+
+    @Test
+    public void testDefaultCommitUpdatesCell() {
+        TreeCell<String> cell = TextFieldTreeCell.forTreeView().call(tree);
+        TreeItem<String> editingItem = setupForEditing(cell);
+        tree.edit(editingItem);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("cell text updated to committed value", value, cell.getText());
+    }
+
+    @Test
+    public void testDoNothingCommitHandlerDoesNotUpdateData() {
+        TreeItem<String> editingItem = setupForEditing(cell);
+        String oldValue = editingItem.getValue();
+        // do nothing handler
+        tree.setOnEditCommit(e -> {});
+        tree.edit(editingItem);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("edited value must not be committed", oldValue, editingItem.getValue());
+    }
+
+    @Ignore("JDK-8187314")
+    @Test
+    public void testDoNothingCommitHandlerDoesNotUpdateCell() {
+        TreeCell<String> cell = TextFieldTreeCell.forTreeView().call(tree);
+        TreeItem<String> editingItem = setupForEditing(cell);
+        String oldValue = editingItem.getValue();
+        // do nothing handler
+        tree.setOnEditCommit(e -> {});
+        tree.edit(editingItem);
+        String value = "edited";
+        cell.commitEdit(value);
+        assertEquals("cell text must not have changed", oldValue, cell.getText());
+    }
+
+
+    /**
+     * Sets tree editable, configures the given cell for editing in tree at index 1
+     * and returns the treeItem at that index.
+     */
+    private TreeItem<String> setupForEditing(TreeCell<String> editingCell) {
+        tree.setEditable(true);
+        editingCell.updateTreeView(tree);
+        editingCell.updateIndex(1);
+        return editingCell.getTreeItem();
+    }
+
+    /**
+     * Test test setup.
+     */
+    @Test
+    public void testSetupForEditing() {
+        TreeCell<String> cell = new TreeCell<>();
+        TreeItem<String> cellTreeItem = setupForEditing(cell);
+        assertTrue("sanity: tree must be editable", tree.isEditable());
+        assertEquals("sanity: returned treeItem", cellTreeItem, cell.getTreeItem());
+        assertEquals(1, cell.getIndex());
+        assertEquals("sanity: cell configured with tree's treeItem at index",
+                tree.getTreeItem(cell.getIndex()), cell.getTreeItem());
+        assertNull("sanity: config doesn't change tree state", tree.getEditingItem());
+    }
+
 
     // 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?

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

@@ -1195,7 +1195,8 @@ public void updateItem(String item, boolean empty) {
         treeView.setOnEditStart(t -> {
             rt_29650_start_count++;
         });
-        treeView.setOnEditCommit(t -> {
+        // Note: must add a commit handler to not replace the default (saving) handler
+        treeView.addEventHandler(TreeView.editCommitEvent(), t -> {
             rt_29650_commit_count++;
         });
         treeView.setOnEditCancel(t -> {