Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8267094: TreeCell: cancelEvent must return correct editing location #524

Closed
wants to merge 2 commits into from
Closed

8267094: TreeCell: cancelEvent must return correct editing location #524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab2da1e
8267094: TreeCell: cancelEvent must return correct editing location
kleopatra May 27, 2021
ea3b705
fixed memory leak introduced in previous version
kleopatra Jun 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java
View file
Open in desktop
Expand Up @@ -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() {
Expand Down Expand Up @@ -384,6 +386,7 @@ public String getName() {

tree.requestFocus();
}
treeItemAtStartEdit = getTreeItem();
}

/** {@inheritDoc} */
Expand Down Expand Up @@ -424,6 +427,7 @@ public String getName() {
// It would be rude of us to request it back again.
ControlUtils.requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild(tree);
}
treeItemAtStartEdit = null;
}

/** {@inheritDoc} */
Expand All @@ -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);

Expand All @@ -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} */
Expand Down
16 changes: 0 additions & 16 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellEditingTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
Expand Down
172 changes: 165 additions & 7 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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>();
Expand All @@ -70,6 +83,11 @@ public class TreeCellTest {
root.setExpanded(true);
}

@After
public void cleanup() {
if (stageLoader != null) stageLoader.dispose();
}

/*********************************************************************
* Tests for the constructors *
********************************************************************/
Expand Down Expand Up @@ -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?

Expand Down