On Tue, 25 Feb 2025 19:05:23 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> The `VetoableListDecorator` class does not override the default
>> implementation of `List.sort()`. The default implementation copies the
>> elements into an array, sorts the array, and then calls `List.set()` in a
>> loop to sort the List **without removing the old elements first**. This
>> leads to the `VetoableListDecorator` intercepting the call to `set()` and
>> seeing a "duplicate child" being added.
>>
>> The solution is to implement `SortableList.doSort()` with the following
>> protocol:
>> 1. Make a copy of the list and sort it.
>> 2. Present the sorted list with `onProposedChange` for a potential veto.
>> 3. If successful, use `setAll()` to swap out the current list with the
>> sorted list.
>
> Michael Strauß has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - merge
> - Merge branch 'master' into fixes/vetoable-list-decorator-sort
>
> # Conflicts:
> #
> modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java
> - factor out setAll implementation
> - Implement sorting for VetoableListDecorator
> - failing tests
Marked as reviewed by angorya (Reviewer).
modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java
line 127:
> 125:
> 126: private boolean setAllImpl(List<E> unmodifiableList) {
> 127: onProposedChange(unmodifiableList, 0, size());
Have you thought of moving all the checks and wrapping into `setAllImpl()`?
Regardless of that, the current code is good.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1674#pullrequestreview-2648772428
PR Review Comment: https://git.openjdk.org/jfx/pull/1674#discussion_r1974168926
