From bc5a0a213913ffafe896bcf6cdeb8ea18df829f5 Mon Sep 17 00:00:00 2001 From: Jeanette Winzenburg Date: Tue, 30 Nov 2021 12:15:48 +0100 Subject: [PATCH] 8187307: LisingtView, TableView, TreeView: receives editCancel event when edit is committed --- .../java/javafx/scene/control/ListCell.java | 17 +++++++------ .../java/javafx/scene/control/TableCell.java | 25 ++++++++++--------- .../java/javafx/scene/control/TreeCell.java | 16 ++++++------ .../javafx/scene/control/TreeTableCell.java | 25 ++++++++++--------- .../javafx/scene/control/ListCellTest.java | 20 +++++++++++++++ .../javafx/scene/control/TableCellTest.java | 23 ++++++++++++++++- .../javafx/scene/control/TreeCellTest.java | 21 ++++++++++++++++ .../scene/control/TreeTableCellTest.java | 23 ++++++++++++++++- 8 files changed, 129 insertions(+), 41 deletions(-) diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java index b21ab33954f..b8659ecbda2 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java @@ -386,8 +386,16 @@ public void changed( /** {@inheritDoc} */ @Override public void commitEdit(T newValue) { if (! isEditing()) return; - ListView list = getListView(); + // inform parent classes of the commit, so that they can switch us + // out of the editing state. + // This MUST come before the updateItem call below, otherwise it will + // call cancelEdit(), resulting in both commit and cancel events being + // fired (as identified in RT-29650) + super.commitEdit(newValue); + + ListView list = getListView(); + // JDK-8187307: fire the commit after updating cell's editing state if (list != null) { // Inform the ListView of the edit being ready to be committed. list.fireEvent(new ListView.EditEvent(list, @@ -396,13 +404,6 @@ public void changed( list.getEditingIndex())); } - // inform parent classes of the commit, so that they can switch us - // out of the editing state. - // This MUST come before the updateItem call below, otherwise it will - // call cancelEdit(), resulting in both commit and cancel events being - // fired (as identified in RT-29650) - super.commitEdit(newValue); - // update the item within this cell, so that it represents the new value updateItem(newValue, false); diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java index 4cc51c1a48f..43b3b8374ad 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/TableCell.java @@ -353,26 +353,27 @@ TablePosition getEditingCellAtStartEdit() { @Override public void commitEdit(T newValue) { if (!isEditing()) return; + // inform parent classes of the commit, so that they can switch us + // out of the editing state. + // This MUST come before the updateItem call below, otherwise it will + // call cancelEdit(), resulting in both commit and cancel events being + // fired (as identified in RT-29650) + super.commitEdit(newValue); + final TableView table = getTableView(); + // JDK-8187307: fire the commit after updating cell's editing state if (getTableColumn() != null) { // Inform the TableColumn of the edit being ready to be committed. CellEditEvent editEvent = new CellEditEvent<>( - table, - editingCellAtStartEdit, - TableColumn.editCommitEvent(), - newValue - ); + table, + editingCellAtStartEdit, + TableColumn.editCommitEvent(), + newValue + ); Event.fireEvent(getTableColumn(), editEvent); } - // inform parent classes of the commit, so that they can switch us - // out of the editing state. - // This MUST come before the updateItem call below, otherwise it will - // call cancelEdit(), resulting in both commit and cancel events being - // fired (as identified in RT-29650) - super.commitEdit(newValue); - // update the item within this cell, so that it represents the new value updateItem(newValue, false); 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 2ef0a76c84e..b3911bd749f 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 @@ -394,8 +394,17 @@ public String getName() { /** {@inheritDoc} */ @Override public void commitEdit(T newValue) { if (! isEditing()) return; + + // inform parent classes of the commit, so that they can switch us + // out of the editing state. + // This MUST come before the updateItem call below, otherwise it will + // call cancelEdit(), resulting in both commit and cancel events being + // fired (as identified in RT-29650) + super.commitEdit(newValue); + final TreeItem treeItem = getTreeItem(); final TreeView tree = getTreeView(); + // JDK-8187307: fire the commit after updating cell's editing state if (tree != null) { // Inform the TreeView of the edit being ready to be committed. tree.fireEvent(new TreeView.EditEvent(tree, @@ -405,13 +414,6 @@ public String getName() { newValue)); } - // inform parent classes of the commit, so that they can switch us - // out of the editing state. - // This MUST come before the updateItem call below, otherwise it will - // call cancelEdit(), resulting in both commit and cancel events being - // fired (as identified in RT-29650) - super.commitEdit(newValue); - // update the item within this cell, so that it represents the new value if (treeItem != null) { treeItem.setValue(newValue); diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java b/modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java index b33da392869..8ccd8a50214 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java +++ b/modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableCell.java @@ -370,26 +370,27 @@ TreeTablePosition getEditingCellAtStartEdit() { @Override public void commitEdit(T newValue) { if (!isEditing()) return; + // inform parent classes of the commit, so that they can switch us + // out of the editing state. + // This MUST come before the updateItem call below, otherwise it will + // call cancelEdit(), resulting in both commit and cancel events being + // fired (as identified in RT-29650) + super.commitEdit(newValue); + final TreeTableView table = getTreeTableView(); + // JDK-8187307: fire the commit after updating cell's editing state if (getTableColumn() != null) { // Inform the TreeTableColumn of the edit being ready to be committed. CellEditEvent editEvent = new CellEditEvent( - table, - editingCellAtStartEdit, - TreeTableColumn.editCommitEvent(), - newValue - ); + table, + editingCellAtStartEdit, + TreeTableColumn.editCommitEvent(), + newValue + ); Event.fireEvent(getTableColumn(), editEvent); } - // inform parent classes of the commit, so that they can switch us - // out of the editing state. - // This MUST come before the updateItem call below, otherwise it will - // call cancelEdit(), resulting in both commit and cancel events being - // fired (as identified in RT-29650) - super.commitEdit(newValue); - // update the item within this cell, so that it represents the new value updateItem(newValue, false); diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java index b2dc1465f69..a968f8b06e8 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java @@ -874,6 +874,26 @@ public void testStartEditOffRangeMustNotUpdateEditingLocation() { assertEquals("list editing location must not be updated", -1, list.getEditingIndex()); } + @Test + public void testCommitEditMustNotFireCancel() { + list.setEditable(true); + // JDK-8187307: handler that resets control's editing state + list.setOnEditCommit(e -> { + int index = e.getIndex(); + list.getItems().set(index, e.getNewValue()); + list.edit(-1); + }); + cell.updateListView(list); + int editingIndex = 1; + cell.updateIndex(editingIndex); + list.edit(editingIndex); + List> events = new ArrayList<>(); + list.setOnEditCancel(events::add); + String value = "edited"; + cell.commitEdit(value); + assertEquals("sanity: value committed", value, list.getItems().get(editingIndex)); + assertEquals("commit must not have fired editCancel", 0, events.size()); + } // When the list view item's change and affects a cell that is editing, then what? // When the list cell's index is changed while it is editing, then what? diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java index d1cf4642b44..2754f74efb1 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java @@ -600,7 +600,28 @@ public void testEditStartEventAfterStartOnTable() { } //------------- commitEdit - // fix of JDK-8271474 changed the implementation of how the editing location is evaluated + + @Test + public void testCommitEditMustNotFireCancel() { + setupForEditing(); + // JDK-8187307: handler that resets control's editing state + editingColumn.setOnEditCommit(e -> { + table.getItems().set(e.getTablePosition().getRow(), e.getNewValue()); + table.edit(-1, null); + }); + int editingRow = 1; + cell.updateIndex(editingRow); + table.edit(editingRow, editingColumn); + List> events = new ArrayList<>(); + editingColumn.setOnEditCancel(events::add); + String value = "edited"; + cell.commitEdit(value); + assertEquals("sanity: value committed", value, table.getItems().get(editingRow)); + assertEquals("commit must not have fired editCancel", 0, events.size()); + } + + +// fix of JDK-8271474 changed the implementation of how the editing location is evaluated @Test public void testEditCommitEvent() { 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 7d023e0625b..2c2ff29aae0 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 @@ -867,6 +867,27 @@ public void testStartEditOffRangeMustNotUpdateEditingLocation() { assertNull("tree editing location must not be updated", tree.getEditingItem()); } + @Test + public void testCommitEditMustNotFireCancel() { + tree.setEditable(true); + int editingIndex = 1; + TreeItem editingItem = tree.getTreeItem(editingIndex); + // JDK-8187307: handler that resets control's editing state + tree.setOnEditCommit(e -> { + editingItem.setValue(e.getNewValue()); + tree.edit(null); + }); + cell.updateTreeView(tree); + cell.updateIndex(editingIndex); + List> events = new ArrayList<>(); + tree.setOnEditCancel(events::add); + tree.edit(editingItem); + String value = "edited"; + cell.commitEdit(value); + assertEquals("sanity: value committed", value, tree.getTreeItem(editingIndex).getValue()); + assertEquals("commit must not have fired editCancel", 0, events.size()); + } + // 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? diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java index c177b768204..9a2dd0807f5 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java @@ -912,7 +912,28 @@ public void testEditStartEventAfterStartOnTable() { } //------------- commitEdit - // fix of JDK-8271474 changed the implementation of how the editing location is evaluated + + @Test + public void testCommitEditMustNotFireCancel() { + setupForEditing(); + // JDK-8187307: handler that resets control's editing state + editingColumn.setOnEditCommit(e -> { + TreeItem treeItem = tree.getTreeItem(e.getTreeTablePosition().getRow()); + treeItem.setValue(e.getNewValue()); + tree.edit(-1, null); + }); + int editingRow = 1; + cell.updateIndex(editingRow); + tree.edit(editingRow, editingColumn); + List> events = new ArrayList<>(); + editingColumn.setOnEditCancel(events::add); + String value = "edited"; + cell.commitEdit(value); + assertEquals("sanity: value committed", value, tree.getTreeItem(editingRow).getValue()); + assertEquals("commit must not have fired editCancel", 0, events.size()); + } + +// fix of JDK-8271474 changed the implementation of how the editing location is evaluated @Test public void testEditCommitEvent() {