RFR: 8274433: All Cells: misbehavior of startEdit

[Marius Hanl]

On Thu, 30 Sep 2021 12:00:16 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

> The misbehavior happens if (super) startEdit didn't succeed (== !cell.isEditing):
> 
> - must not fire editStart event
> - must not update control's editing location
> 
> fix is to back out of startEdit if super.startEdit doesn't switch the cell into editing mode
> 
> Added tests that failed/passed before/after the fix. Note that for Tree-, Table-, TreeTableCell one of the added tests would be a false green due to those cells not updating the editing location on its control [JDK-8187474](https://bugs.openjdk.java.net/browse/JDK-8187474), so it's ignore until that's fixed.

Change looks good, I just left some comments on the tests. :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java line 860:

> 858:         cell.updateListView(list);
> 859:         cell.updateIndex(list.getItems().size());
> 860:         List<EditEvent> events = new ArrayList<>();

Very minor: You can use EditEvent<Object> here so you don't use the raw generic (and also on the other locations). But I guess that's just syntax sugar.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java line 874:

> 872:         cell.startEdit();
> 873:         assertFalse("sanity: off-range cell must not be editing", cell.isEditing());
> 874:         assertEquals("list editing location must not be updated", - 1, list.getEditingIndex());

`-1` would look better here (without the space inbetween) given the fact this is not a mathematical context

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java line 726:

> 724:          int editingRow = table.getItems().size();
> 725:          cell.updateIndex(editingRow);
> 726:          List<CellEditEvent> events = new ArrayList<>();

`events` is effectively unused here, maybe you forgot the assert here?

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeCellTest.java line 870:

> 868:         cell.startEdit();
> 869:         assertFalse("sanity: off-range cell must not be editing", cell.isEditing());
> 870:         assertEquals("editing location", null, tree.getEditingItem());

Minor: This can be replaced with `assertNull`, which would also be a bit more readable :)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableCellTest.java line 1051:

> 1049:          cell.startEdit();
> 1050:          assertFalse("sanity: off-range cell must not be editing", cell.isEditing());
> 1051:          assertEquals("editing location", null, tree.getEditingCell());

Minor: This can be replaced with `assertNull`. :)

-------------

PR: https://git.openjdk.java.net/jfx/pull/636