On Thu, 30 Sep 2021 12:00:16 GMT, Jeanette Winzenburg <faste...@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
