Skip to content

8187309: TreeCell must not change tree's data #724

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.

Sign up for GitHub

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

Closed
kleopatra wants to merge 1 commit into from
Closed

8187309: TreeCell must not change tree's data #724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeCell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

@aghaisas aghaisas Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are adding the default edit commit handler to TreeView. Although it seems to be correct, this default handler is likely to be overwritten if the user code already has a setOnEditCommit() call. This is shown by the test_rt_29650() failure in TreeviewTest.java which you have corrected.

The changes done in this PR makes TreeView similar to ListView and TableView in terms of having the default edit commit handler.

Unfortunately, I do not see how can we avoid user code accidentally overwriting the default edit commit handler. Documenting this possibility seems to be the only way ahead.
Any other idea?

Copy link
Collaborator Author

@kleopatra kleopatra Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the change might break application code, though that code would be buggy. Actually, the behavior as implemented now, already is documented :)

It is very important to note that if you call setOnEditCommit(javafx.event.EventHandler) with your own EventHandler, then you will be removing the default handler. Unless you then handle the writeback to the property (or the relevant data source), nothing will happen.

so: if they have a custom handler that doesn't save the data, they were violating the specification (though were getting away with it due to the bug).

Personally, I think that we cannot keep backward compatibility to bugs.

Copy link
Collaborator

@aghaisas aghaisas Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I see that it is already well documented.

}


Expand Down Expand Up @@ -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;
Expand Down
80 changes: 80 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}


Copy link
Member

@Maran23 Maran23 Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor: Empty line

/**
* 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.
Copy link
Member

@Maran23 Maran23 Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I would rephrase that a bit. Something like:
Tests the cell editing setup.

Copy link
Collaborator Author

@kleopatra kleopatra Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops .. forgot to change this (and the other comment) before integration - sry

*/
@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?
Expand Down
3 changes: 2 additions & 1 deletion modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 -> {
Expand Down