On Wed, 4 Jan 2023 05:28:10 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> 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.
@nlisker I think I made all modifications requested, is there anything else
that should be done here? Do you want the sort changes to be made or an issue
be filed for it?
-------------
PR: https://git.openjdk.org/jfx/pull/972
