RFR: 8196065: ListChangeListener getRemoved() returns items that were not removed. [v6]

[Michael Strauß]

On Sun, 23 May 2021 10:13:48 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
>> 
>>  - Merge branch 'master' into fixes/JDK-8196065
>>    
>>    # Conflicts:
>>    #	modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java
>>  - Merge branch 'master' into fixes/JDK-8196065
>>  - Merge branch 'master' into fixes/JDK-8196065
>>    
>>    # Conflicts:
>>    #	modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
>>  - Cleanup
>>  - Fixed clear-and-select change notification
>>  - Add failing tests
>>  - Cleanup
>>  - Added tests
>>  - Fix incorrect index when multiple remove changes are handled in SelectedItemsReadOnlyObservableList
>>  - Add failing test
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableList.java line 54:
> 
>> 52:             while (c.next()) {
>> 53:                 if (c.wasReplaced()) {
>> 54:                     List<E> removed = getRemovedElements(c, totalRemovedSize);
> 
> hmm, don't quite understand all the fuss (but might be overlooking something, has been a while since I did a deeper dig into the selection issues ;) 
> 
> - we have a copy of the items (itemsRefs) with the same coordinates we receive from the change
> - this list is in sync with the indices, that is it contains the items at the respective selectedIndices
> - currently, that's done in a bulk setting at the end of the listener code
> 
> Why not sync our copy while working through the change? Doing so, would automatically collect the state of our own change, or what am I missing?

This was my first thought, too. However, it doesn't work because `SelectedItemsReadOnlyObservableList` doesn't truthfully report its changes, so we can't simply replay its reported changes onto our copy of the items list. For example, consider the following simple piece of code:


ListView<String> listView = new ListView<>(FXCollections.observableArrayList("foo", "bar"));
MultipleSelectionModel<String> model = listView.getSelectionModel();
model.setSelectionMode(SelectionMode.SINGLE);
model.getSelectedIndices().addListener((ListChangeListener<? super Integer>)System.out::println);
model.select(0);
model.select(1);


You'd expect the following output:

{ [0] added at 0 }
{ [0] replaced by [1] at 0 }


However, the actual output is:

{ [0] added at 0 }
{ [1] added at 0 }


While that is a bug, it almost seems like it is a bug _by design_. `MultipleSelectionModelBase` actually contains lots of code to suppress change notifications. In this particular example, a crucial change notification is suppressed in `MultipleSelectionModelBase:405`:

if (getSelectionMode() == SINGLE) {
    startAtomic();
    quietClearSelection();
    stopAtomic();
}
selectedIndices.set(row);


The reason why `MultipleSelectionModelBase` interferes with `selectedIndices` change notifications is to prevent surfacing all kinds of invalid intermediate selection states.

I tried to fix this by making `selectedIndices` changes transactional, where each of the `MultipleSelectionModel` operations would be a transaction that silently accumulates changes (clear, set, and so on), and at the end of a transaction, the changes would be distilled down into a single change notification.

However, in doing so, I ended up rewriting large parts of `MultipleSelectionModelBase`. This seemed like too big of a change. Also, I noticed that at one point you prototyped a new implementation of `SelectionModel`... maybe starting from scratch is better than adding transactions on top of the existing implementation.

> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableListTest.java line 120:
> 
>> 118:         assertEquals(change(replaced(0, range("foo"), range("bar"))), changes.get(0));
>> 119:     }
>> 120: 
> 
> Another thingy I don't quite understand, sry ;) 
> 
> What do you want to test here? We have:
> 
> selectedItems before: [foo, bar]
> selectedItems after: [bar, foo]
> 
> so we need a change (the "should be" snippet in the description of this method)
> 
> replaced [foo, bar] by [bar, foo] at 0  
> 
> we get: 
> 
> replaced [foo] by [bar] at 0
> 
> which is an incorrect notification (because it's incomplete, doesn't notify at all about the replace at 1) covered by a passing test .. feels fishy. The other way round: if selectedItems cannot react to such a use-case with a correct notification, it might be broken?

Yes, `selectedItems` is broken. I agree that having an objectively incorrect test pass is fishy, but the alternative would be to not have the test at all, and lose the "free" heads-up if at some point in the future the underlying issue is fixed. Do you think it's better to have the correct test, but ignore it?

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

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