On Wed, 21 Apr 2021 14:22:21 GMT, Florian Kirmaier <fkirma...@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
