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

[Jeanette Winzenburg]

On Wed, 21 Apr 2021 14:22:21 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

> 
> 
> Finally found time to update the PR.
> 
>     1. As suggested, I've moved the updateIndex method below the if.
> 

looks good :)

>     2. The test from 1 to -1 is a bit more complicated. Because -1 represents "not editing" the test would be differnt. I've adapted the test to check for "not editing" in the case of -1. With this change the test is green.
> 

looks like I haven't been clear enough in my recent comments: the list is always really editing (state transitions of list's editingIndex and their effects on a cell at that index seem to be working and have tests), that is `editingIndex > 0`. What I meant is updating the cell index from that editingIndex to -1, here is a test that's still failing

    @Test
    public void testUpdateCellIndexOffEditing() {
        list.setEditable(true);
        cell.updateListView(list);
        int editingIndex = 1; // list editing
        cell.updateIndex(editingIndex);
        list.edit(editingIndex); 
        // here we are certain that the cell is in editing state
        assertTrue("sanity: cell must be editing", cell.isEditing());
        cell.updateIndex(-1); // change cell index to negative
        assertFalse("cell must not be editing if cell index is " + cell.getIndex(), cell.isEditing());
    }

For a fix, have a look at [JDK-8264127](https://bugs.openjdk.java.net/browse/JDK-8264127) - I think it's similar for ListCell updateEditing, though its logic block clouds it a bit (instead of immediately backing out for cellIndex == -1, it doesn't enter the if.

> 
> Honestly im not really aware of the Ordering of the methods. I didn't spend a lot of time understanding the inner works of all the controls - im usually more focused on fixing memory leaks.
> 
> About the initial focus, i see 3 options:
> 
>     * set it to -1 in testChangeIndexToEditing1_jdk_8264127
> 
>     * set it to -1 in assertChangeIndexToEditing
> 
>     * set it to -1 in the setup method.
>       All 3 options work. Im fine with keeping it at the concrete test, because it's the only test who needs it, but im fine with any of the 3 options.

if you intend to keep the tests in the big-fat ListCellTest, then option 3 would be a bit brittle, IMO: it might lead to unexpected behavior in the other/future tests. Keeping focus on -1 should be done in each of the tests here (otherwise changes in default focus might - or not - break the tests), so I would suggest option 2 and in all whileEditing methods.

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

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