On Sat, 29 May 2021 12:31:33 GMT, Michael Strauß <[email protected]> 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
