On Sun, 23 May 2021 10:13:48 GMT, Jeanette Winzenburg <faste...@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
