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

[Ambarish Rapte]

On Sat, 29 May 2021 12:31:33 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> The documentation for `ObservableListBase.nextRemove` states that a single change always refers to the current state of the list, which likely means that multiple disjoint removed ranges need to be applied in order, otherwise the next change's `getFrom` doesn't refer to the correct index.
>> 
>> `SelectedItemsReadOnlyObservableList` doesn't apply removals to `itemsRefList`, which means that subsequent removals will refer to the wrong index when retrieving the removed elements. This PR fixes the calculation of the current index.
>
> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix tests

Fix looks good to me. Provided few minor comments.

modules/javafx.base/src/test/java/test/javafx/collections/MockListObserver.java line 174:

> 172:         assertFalse(tooManyCalls);
> 173:         assertEquals(1, calls.size());
> 174:     }

Minor: The `check1` method can be changed to internally call `checkN`.

modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java line 101:

> 99:             @Override public int getTo() {
> 100:                 checkState();
> 101:                 return from;

How about reverting this change ?

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableListTest.java line 66:

> 64:         selectedIndices.addAll(0, 1, 2, 3, 4);
> 65:         assertEquals(1, changes.size());
> 66:         assertEquals(change(added(0, "foo", "bar", "baz", "qux", "quz")), changes.get(0));

I would recommend to use the exact string in the expected parameter instead of the helper function. 
`assertEquals("{ [foo, bar, baz, qux, quz] added at 0 }", changes.get(0));`
This way improves readability.

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableListTest.java line 107:

> 105:      */
> 106:     @Test
> 107:     @Ignore("see JDK-8267951")

Generally we use only the bug ID here -> @Ignore("JDK-8267951")

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

Changes requested by arapte (Reviewer).

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