On Thu, 22 Apr 2021 11:00:43 GMT, Florian Kirmaier <fkirma...@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
