On Wed, 17 May 2023 15:22:57 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
>> This issue happened because `childSet` member of Parent was modified during
>> `onProposedChange()` call - that call did not recognize negative indexes as
>> invalid, which caused an exception when actually adding the Node to a List.
>>
>> This seemed like the simplest solution which doesn't rework a lot of code
>> underneath. Exceptions coming from a backing list/collection technically are
>> handled by `VetoableListDecorator`'s try-catch clauses, however
>> `VetoableListDecorator` does not provide an interface to react when such an
>> exception happens - without it we cannot revert `childSet` back to its
>> original state.
>
> Lukasz Kostyra has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Add index/null checks to ObservableListWrapper and VetoableListDecorator
> - ParentTest: Expect IndexOutOfBoundsException, add test cases
> - Parent: Remove index check from onProposedChange
I think `ObservableListWrapper#remove(int, int)` also needs a check index
check, or it may end up remove some items and then throw an exception
(exceptions thrown from collections should not modify its state).
modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java
line 116:
> 114: public boolean setAll(Collection<? extends E> col) {
> 115: Objects.requireNonNull(col);
> 116: onProposedChange(Collections.unmodifiableList(new
> ArrayList<>(col)), 0, size());
It's good to make it explicit. It already threw NPE because of `new
ArrayList<>(col)`, so this is not a change.
modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java
line 270:
> 268: @Override
> 269: public boolean removeAll(Collection<?> c) {
> 270: Objects.requireNonNull(c);
Looks like a good change, if `c` was `null` and the backing list was empty,
this would not throw NPE, but now it does.
modules/javafx.graphics/src/test/java/test/javafx/scene/ParentTest.java line
894:
> 892: g.getChildren().remove(0);
> 893: }
> 894:
I think you should also add tests for the `null` cases, if they aren't part of
this test already.
-------------
Changes requested by jhendrikx (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1136#pullrequestreview-1431766296
PR Review Comment: https://git.openjdk.org/jfx/pull/1136#discussion_r1197130391
PR Review Comment: https://git.openjdk.org/jfx/pull/1136#discussion_r1197132309
PR Review Comment: https://git.openjdk.org/jfx/pull/1136#discussion_r1197133728
