RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]

[John Hendrikx]

On Wed, 4 Jan 2023 05:28:10 GMT, Nir Lisker <nlisker at 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