On Fri, 2 Oct 2020 18:20:18 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>>> the reason BitSet was introduced was to ensure that the elements are
>>> removed from this List in reverse order (prior to
>>> that fix, they were removed in forward order with the loop index being
>>> messed up).
>>
>> But why do they need to be removed in reverse order to begin with? The super
>> implementation does forward removal, or
>> just use `removeIf`.
>
> It might not matter any more (presuming that it was done correctly), but it
> seems safer to leave the logic as-is to
> match the existing behavior.
the problem was (and still is) in MultipleSelectionModelBase:
selectedItems = new
SelectedItemsReadOnlyObservableList<T>(selectedIndices, () -> getItemCount()) {
@Override protected T getModelItem(int index) {
return MultipleSelectionModelBase.this.getModelItem(index);
}
};
meaning that the selectedItems change during the removal of items (that's plain
wrong!) which shows when removing the
items from the start:
// removeAll before fix of
for (int i = 0; i < size(); ++i) {
if (c.contains(get(i))) {
remove(i);
--i;
modified = true;
}
}
doing so is basically valid (note the decremented i). Just the selectedItems
look into the underlying model, so **c**
is changing during the removal which is a no-go.
Returning to the old (pre
[JDK-8093144](https://bugs.openjdk.java.net/browse/JDK-8093144)) still makes
the tests
against it fail (ListViewTest.test_rt35857 f.i.).
Still wondering why the detour over the BitSet was choosen as fix (vs. the more
natural remove-from-last). The listView
test is passing for the bitSet and for the back-to-front approach. Can we
imagine a context where the broken
selectedItems impl would add wreckage to the latter?
-------------
PR: https://git.openjdk.java.net/jfx/pull/305
