-
Bug
-
Resolution: Fixed
-
P4
-
8u40
-
8u40b23, win7
There are several variant, related to incorrect handling of a list change event that has multiple sub-changes
1. the selected index isn't always updated
2. even if updated, selectedIndex fires too many events: must be one per change, must not fire for each sub-change
3. an unchanged selectedItem must not fire any notification
Technically, the basic error for 1. and 2. might be in itemsContentObserver, which does something like:
while (c.next()) {
if (removedBeforeSelected) {
// must not access the list during subchanges!
// the list's change is already completed, that is oldLast - someShift
// is off range
clearAndSelect(getSelectedIndex() - someShift);
}
}
Instead the shift must be calculated during the loop and selection only updated after, something like (pseudo-code, untested ;-)
int shift = 0;
while (c.next()) {
if (removeBeforeSelected) {
shift += c.getRemovedSize();
} else {
break;
}
}
if (shift > 0)
clearAndSelect(getSelectedIndex() - shift);
There might be some additional destructive interference from the ListSelectionModel of the popup though, didn't dig (my own implementation is too different from core to be sure about the exact reason for the failure).
Similar incorrect notifications - the bullets 2 and 3 above) can be seen in list, table (probably treeTable as well) and tree. Similar incorrect index value (bullet 1) only in table.
failing test snippets
/**
* Bullet 1: selected index must be updated
* Corner case: last selected. Fails for core
*/
@Test
public void testSelectedAtLastOnDisjointRemoveItemsAbove() {
int last = items.size() - 1;
getSelectionModel().select(last);
// disjoint remove of 2 elements above the last selected
// removeAll(1, 3);
items.removeAll(items.get(1), items.get(3));
int expected = last - 2;
assertEquals("selected index after disjoint removes above",
expected, getSelectionModel().getSelectedIndex());
}
/**
* Variant of 1: if selectedIndex is not updated,
* the old index is no longer valid
* for accessing the items.
*/
@Test
public void testAccessSelectedAtLastOnDisjointRemoveItemsAbove() {
int last = items.size() - 1;
getSelectionModel().select(last);
// disjoint remove of 2 elements above the last selected
items.removeAll(items.get(1), items.get(3));
int selected = getSelectionModel().getSelectedIndex();
if (selected > -1)
items.get(selected);
}
/**
* Bullet 2: selectedIndex notification count
*
* Note that we don't use the corner case of having the last index selected
* (which fails already on updating the index)
*/
@Test
public void testSelectedIndexNotificationOnDisjointRemovesAbove() {
int last = items.size() - 2;
getSelectionModel().select(last);
assertEquals(last, getSelectionModel().getSelectedIndex());
ChangeReport report = new ChangeReport(getSelectionModel().selectedIndexProperty());
// disjoint remove of 2 elements above the last selected
items.removeAll(items.get(1), items.get(3));
assertEquals("sanity: selectedIndex must be shifted by -2", last - 2,
getSelectionModel().getSelectedIndex());
assertEquals("must fire single event on removes above", 1,
report.getEventCount());
}
/**
* Bullet 3: unchanged selectedItem must not fire change
*/
@Test
public void testSelectedItemNotificationOnDisjointRemovesAbove() {
int last = items.size() - 2;
Object lastItem = items.get(last);
getSelectionModel().select(last);
assertEquals(lastItem, getSelectionModel().getSelectedItem());
ChangeReport report = new ChangeReport(getSelectionModel().selectedItemProperty());
// disjoint remove of 2 elements above the last selected
items.removeAll(items.get(1), items.get(3));
assertEquals("sanity: selectedItem unchanged", lastItem,
getSelectionModel().getSelectedItem());
assertEquals("must not fire on unchanged selected item", 0, report.getEventCount());
}
1. the selected index isn't always updated
2. even if updated, selectedIndex fires too many events: must be one per change, must not fire for each sub-change
3. an unchanged selectedItem must not fire any notification
Technically, the basic error for 1. and 2. might be in itemsContentObserver, which does something like:
while (c.next()) {
if (removedBeforeSelected) {
// must not access the list during subchanges!
// the list's change is already completed, that is oldLast - someShift
// is off range
clearAndSelect(getSelectedIndex() - someShift);
}
}
Instead the shift must be calculated during the loop and selection only updated after, something like (pseudo-code, untested ;-)
int shift = 0;
while (c.next()) {
if (removeBeforeSelected) {
shift += c.getRemovedSize();
} else {
break;
}
}
if (shift > 0)
clearAndSelect(getSelectedIndex() - shift);
There might be some additional destructive interference from the ListSelectionModel of the popup though, didn't dig (my own implementation is too different from core to be sure about the exact reason for the failure).
Similar incorrect notifications - the bullets 2 and 3 above) can be seen in list, table (probably treeTable as well) and tree. Similar incorrect index value (bullet 1) only in table.
failing test snippets
/**
* Bullet 1: selected index must be updated
* Corner case: last selected. Fails for core
*/
@Test
public void testSelectedAtLastOnDisjointRemoveItemsAbove() {
int last = items.size() - 1;
getSelectionModel().select(last);
// disjoint remove of 2 elements above the last selected
// removeAll(1, 3);
items.removeAll(items.get(1), items.get(3));
int expected = last - 2;
assertEquals("selected index after disjoint removes above",
expected, getSelectionModel().getSelectedIndex());
}
/**
* Variant of 1: if selectedIndex is not updated,
* the old index is no longer valid
* for accessing the items.
*/
@Test
public void testAccessSelectedAtLastOnDisjointRemoveItemsAbove() {
int last = items.size() - 1;
getSelectionModel().select(last);
// disjoint remove of 2 elements above the last selected
items.removeAll(items.get(1), items.get(3));
int selected = getSelectionModel().getSelectedIndex();
if (selected > -1)
items.get(selected);
}
/**
* Bullet 2: selectedIndex notification count
*
* Note that we don't use the corner case of having the last index selected
* (which fails already on updating the index)
*/
@Test
public void testSelectedIndexNotificationOnDisjointRemovesAbove() {
int last = items.size() - 2;
getSelectionModel().select(last);
assertEquals(last, getSelectionModel().getSelectedIndex());
ChangeReport report = new ChangeReport(getSelectionModel().selectedIndexProperty());
// disjoint remove of 2 elements above the last selected
items.removeAll(items.get(1), items.get(3));
assertEquals("sanity: selectedIndex must be shifted by -2", last - 2,
getSelectionModel().getSelectedIndex());
assertEquals("must fire single event on removes above", 1,
report.getEventCount());
}
/**
* Bullet 3: unchanged selectedItem must not fire change
*/
@Test
public void testSelectedItemNotificationOnDisjointRemovesAbove() {
int last = items.size() - 2;
Object lastItem = items.get(last);
getSelectionModel().select(last);
assertEquals(lastItem, getSelectionModel().getSelectedItem());
ChangeReport report = new ChangeReport(getSelectionModel().selectedItemProperty());
// disjoint remove of 2 elements above the last selected
items.removeAll(items.get(1), items.get(3));
assertEquals("sanity: selectedItem unchanged", lastItem,
getSelectionModel().getSelectedItem());
assertEquals("must not fire on unchanged selected item", 0, report.getEventCount());
}