RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper [v4]

[Jeanette Winzenburg]

On Fri, 2 Oct 2020 18:20:18 GMT, Kevin Rushforth <kcr at 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