From ab2da1e413e93c7654540d0b3bae592398f35d61 Mon Sep 17 00:00:00 2001 From: kleopatra Date: Thu, 27 May 2021 16:55:51 +0200 Subject: [PATCH 1/2] 8267094: TreeCell: cancelEvent must return correct editing location --- .../java/javafx/scene/control/TreeCell.java | 10 +- .../scene/control/TreeCellEditingTest.java | 16 --- .../javafx/scene/control/TreeCellTest.java | 100 ++++++++++++++++-- 3 files changed, 101 insertions(+), 25 deletions(-) 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 index 9353e9c92db..5d9fddff86b 100644 --- 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 treeItemAtStartEdit; /** {@inheritDoc} */ @Override public void startEdit() { @@ -384,6 +386,7 @@ public String getName() { tree.requestFocus(); } + treeItemAtStartEdit = getTreeItem(); } /** {@inheritDoc} */ @@ -435,6 +438,9 @@ public String getName() { super.cancelEdit(); if (tree != null) { + TreeItem editingItem = treeItemAtStartEdit; + T value = editingItem != null ? editingItem.getValue() : null; + // reset the editing index on the TreeView if (updateEditingIndex) tree.edit(null); @@ -446,8 +452,8 @@ public String getName() { tree.fireEvent(new TreeView.EditEvent(tree, TreeView.editCancelEvent(), - getTreeItem(), - getItem(), + editingItem, + value, null)); } } 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 index cfbaa8b502e..55b07496614 100644 --- 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 editingItem = tree.getTreeItem(editingIndex); - tree.edit(editingItem); - List> 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 index b205c30c372..e3f4361af4d 100644 --- 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,18 @@ package test.javafx.scene.control; +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 static javafx.scene.control.ControlShim.*; +import static org.junit.Assert.*; +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 +46,9 @@ 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; public class TreeCellTest { private TreeCell cell; @@ -56,6 +63,7 @@ public class TreeCellTest { private TreeItem apples; private TreeItem oranges; private TreeItem pears; + private StageLoader stageLoader; @Before public void setup() { cell = new TreeCell(); @@ -70,6 +78,11 @@ public class TreeCellTest { root.setExpanded(true); } + @After + public void cleanup() { + if (stageLoader != null) stageLoader.dispose(); + } + /********************************************************************* * Tests for the constructors * ********************************************************************/ @@ -686,6 +699,79 @@ public class TreeCellTest { assertFalse(cell.isEditing()); } + @Test + public void testEditCancelEventAfterCancelOnCell() { + tree.setEditable(true); + cell.updateTreeView(tree); + int editingIndex = 1; + TreeItem editingItem = tree.getTreeItem(editingIndex); + cell.updateIndex(editingIndex); + tree.edit(editingItem); + List> 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 editingItem = tree.getTreeItem(editingIndex); + cell.updateIndex(editingIndex); + tree.edit(editingItem); + List> 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 editingItem = tree.getTreeItem(editingIndex); + cell.updateIndex(editingIndex); + tree.edit(editingItem); + List> 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 editingItem = tree.getTreeItem(editingIndex); + tree.edit(editingItem); + List> events = new ArrayList<>(); + tree.setOnEditCancel(events::add); + root.setExpanded(false); + 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 editingItem = tree.getTreeItem(editingIndex); + tree.edit(editingItem); + List> events = new ArrayList<>(); + tree.setOnEditCancel(events::add); + root.getChildren().add(0, new TreeItem<>("added")); + assertEquals(1, events.size()); + assertEquals("editing location of cancel event", editingItem, events.get(0).getTreeItem()); + } + // 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? From ea3b705fd93fc436ac0e609239eeda249a7301c1 Mon Sep 17 00:00:00 2001 From: kleopatra Date: Tue, 1 Jun 2021 12:29:59 +0200 Subject: [PATCH 2/2] fixed memory leak introduced in previous version --- .../java/javafx/scene/control/TreeCell.java | 2 + .../javafx/scene/control/TreeCellTest.java | 72 +++++++++++++++++++ 2 files changed, 74 insertions(+) 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 index 5d9fddff86b..8cf141f2bd4 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java @@ -427,6 +427,7 @@ public String getName() { // It would be rude of us to request it back again. ControlUtils.requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild(tree); } + treeItemAtStartEdit = null; } /** {@inheritDoc} */ @@ -456,6 +457,7 @@ public String getName() { value, null)); } + treeItemAtStartEdit = null; } /** {@inheritDoc} */ 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 index e3f4361af4d..d8a73e0d753 100644 --- 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,7 @@ package test.javafx.scene.control; +import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.List; @@ -33,8 +34,11 @@ 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; @@ -49,6 +53,7 @@ import javafx.scene.control.TreeView.EditEvent; import javafx.scene.control.skin.TreeCellSkin; import test.com.sun.javafx.scene.control.infrastructure.StageLoader; +import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils; public class TreeCellTest { private TreeCell cell; @@ -754,6 +759,7 @@ public void testEditCancelEventAfterCollapse() { List> 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()); } @@ -768,10 +774,76 @@ public void testEditCancelEventAfterModifyItems() { List> 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 editingItem = tree.getTreeItem(editingIndex); + tree.edit(editingItem); + List> 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 editingItem = new TreeItem<>("added"); + WeakReference> 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 editingItem = new TreeItem<>("added"); + WeakReference> itemRef = new WeakReference<>(editingItem); + root.getChildren().add(0, editingItem); + int editingIndex = tree.getRow(editingItem); + Toolkit.getToolkit().firePulse(); + tree.edit(editingItem); + TreeCell editingCell = (TreeCell) 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?