RFR: 8264127: ListCell editing status is true, when index changes while editing [v6]

[Jeanette Winzenburg]

On Thu, 22 Apr 2021 11:00:43 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> Fixing ListCell editing status is true, when index changes while editing.
>
> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8264127:
>   Fixed another index case based on code review

Fix and tests are not yet quite complete - see my inline comments :)

A general suggestion for the tests: I would prefer to have them consistent in both toEditing and offEditing (or whileEditing)

The transitions in the direction `cell-not-editing` to `cell-editing` are factored into a common method `assertChangeIndexToEditing(...)` and have test methods calling that common method with varied concrete indices (f.i `0 ->1`, `-1 -> 1` etc). The other way round - `cell-editing` to `cell-not-editing` - should be factored in a similar way, with the same index pairs. The advantages, IMO: guaranteed same setup and test completeness for both directions and easy read of the variations

modules/javafx.controls/src/main/java/javafx/scene/control/ListCell.java line 549:

> 547:                 cancelEdit();
> 548:             }
> 549:         } else {

minor: not a big fan of local fields, but .. if there already _is_ a local field, we should use it instead of calling the method again (here: editing vs. isEditing) 

minor: the code comment does no longer fit the code

major: this will change the editing state of the list - the call to cancelEdit must be wrapped into un/setting the updateEditingIndex flag (see the else-if block later in the method)

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

> 843:             events.add(e);
> 844:         });
> 845:         list.setEditable(true);

missing setup of focus to -1

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

> 848:         list.edit(1);
> 849:         cell.updateIndex(0);
> 850:         assertTrue(!cell.isEditing());

let's not confuse the code reader: `assertTrue(!boolean)` should be `assertFalse(boolean)`

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

> 849:         cell.updateIndex(0);
> 850:         assertTrue(!cell.isEditing());
> 851:         assertEquals("Should still be editing 1", list.getEditingIndex(), 1);

the assert should go the other way round: assert(expected, actual)

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

> 850:         assertTrue(!cell.isEditing());
> 851:         assertEquals("Should still be editing 1", list.getEditingIndex(), 1);
> 852:         assertTrue("cell re-use must trigger cancel events", events.size() == 1);

don't "hide" information in an assert: `assertEquals(1, events.size())`

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

> 865:     @Test
> 866:     public void testChangeIndexToEditing3_jdk_8264127() {
> 867:         assertChangeIndexToEditing(1, -1);

hmm .. don't quite understand this: the list's editing state shouldn't be changed and here it is never editing. What's the intention?

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

> 893:         } else {
> 894:             // -1 represents "not editing" for the listview and "no index" for the list cell.
> 895:             assertTrue(!cell.isEditing());

same as above

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

> 894:             // -1 represents "not editing" for the listview and "no index" for the list cell.
> 895:             assertTrue(!cell.isEditing());
> 896:             assertTrue("cell re-use must trigger edit events", events.size() == 0);

same as above

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

> 909:         assertTrue("sanity: cell must be editing", cell.isEditing());
> 910:         cell.updateIndex(-1); // change cell index to negative
> 911:         assertFalse("cell must not be editing if cell index is " + cell.getIndex(), cell.isEditing());

missing: assert that a cancelEvent is fired and the list editing state is unchanged (they fail until updateEditing is fixed)

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

Changes requested by fastegal (Committer).

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