On Sun, 1 Jan 2023 16:08:15 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> John Hendrikx has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Clean up expression classes repeated null checks
> - Use instanceof with pattern matching in a few spots
I took a closer look at the sorting-related classes. I think that the design
there is not ideal. Fixing it might be out of scope for this PR. I wouldn't
mind fixing it myself, by the way.
The way the JDK treats soring on lists is with a single `sort(Comparable c)`
method that, when receiving `null`, assumes that the elements are `Comparable`
and throws if not. `Collections` has 2 static sorting methods that help enforce
this condition, where the one that doesn't take a `Comparator` passes `null` to
the one that does.
JavaFX tries to emulate this. `FXCollections` has the same 2 methods for
`ObservableList`, but one doesn't call the other. The asymmetry comes from
`SortableList` (which doesn't live up to its name - non-sortable lists can also
implement it). It defines a 0-argument method and a `Comparator`-argument
method as different behaviors, instead of one calling the other. This is
deferred to its implementations, `ObservableListWrapper` and
`ObservableSequentialListWrapper`, which make the differentiation by, again,
calling 2 different methods on `SortHelper`.
I suggest that the argument check be made at the lowest level, like in the JDK
(inside `Arrays` sort method). First, `FXCollections` can change to:
public static <T extends Comparable<? super T>> void sort(ObservableList<T>
list) {
sort(list, null); // or Comparator.naturalOrder() instead of null
}
public static <T> void sort(ObservableList<T> list, Comparator<? super T>
c) {
if (list instanceof SortableList) {
list.sort(c);
} else {
List<T> newContent = new ArrayList<>(list);
newContent.sort(c);
list.setAll(newContent);
}
}
`SortableList` then removes its `sort()` method, and now it's just overriding
`List::sort(Comparator)` without doing anything with it, so it can be removed
too, and it's left as a marker interface for the special sorting in
`SortHelper` (I haven't understood yet why it's better , I need to dig there
more):
public interface SortableList {}
Now `ObservableListWrapper` and `ObservableSequentialListWrapper` also remove
`sort()`, and override `List::sort(Comparator)` directly and in accordance with
its contract, passing the (possibly `null`) comparator to `SortHelper`, which
is akin to the JDK's `Arrays::sort` method.
Now I need to look into `SortHelper` to see what it does exactly and how to
change it. I think it doesn't need the `sort(List<T> list)` method too now
because it doesn't really enforce that the elements are `Comparable`, it will
naturally throw if not.
---
In my opinion, this is the design flaw: `ObservableList` should have overridden
`List`'s sort method to specify that its sorting is done as one change:
public interface ObservableList<E> extends List<E>, Observable {
@Override
default void sort(Comparator<? super E> c) {
var newContent = new ArrayList<>(this);
newContent.sort(c);
setAll(newContent);
}
Then we wouldn't need the `SortableList` marker because `FXCollections` could
just call this method directly (like `Collections` does, which means that
`FXCollections`'s methods are not needed anymore, though we can't remove them),
and whichever implementation wants to do a more efficient sorting, like
`ObservableListWrapper`, can override again.
This will be a behavioral change, though, because the generated change events
differ.
-------------
PR: https://git.openjdk.org/jfx/pull/972
