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

[Jeanette Winzenburg]

On Sat, 3 Oct 2020 11:13:28 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

> 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?

To answer my own question: yes. A failing test with the back-to-front approach (the existing test in ListViewTest
modified in having the last item selected)

    @Test public void test_rt35857Back() {
        ObservableList<String> fxList = FXCollections.observableArrayList("A", "B", "C");
        final ListView<String> listView = new ListView<String>(fxList);
        
        listView.getSelectionModel().select(2);
        
        ObservableList<String> selectedItems =
                listView.getSelectionModel().getSelectedItems();
        assertEquals(1, selectedItems.size());
        assertEquals("C", selectedItems.get(0));
        
        listView.getItems().removeAll(selectedItems);
        assertEquals(2, fxList.size());
        assertEquals("A", fxList.get(0));
        assertEquals("B", fxList.get(1));
    }
    
Feels like coming nearer to the why of the BitSet approach: it guards against the scenario where changing the current
list implicitly changes the list given as parameter - in that case, it's unsafe to query the parameter list while
removing items (c.contains simply reports nonsense).

This may happen whenever the parameter list does some kind of live lookup into the current list (such as f.i.
SelectedItemsReadOnlyObservableList) - which is not as evil as I thought yesterday (did it myself in custom
selectionModels ;) The BitSet solves it by a two-pass approach: first collect all items to remove/retain without (that
is keep the parameter list in a valid state, allowing to safely access it), then do the removal without further
accessing the (then invalid) parameter list.

The fix at that time was a deliberate decision by the designer of the collections, so the context when it happens was
deemed a valid use-case. Looks like we should **not** revert it.

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

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