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

[Nir Lisker]

On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation

This is the first half of the review. Will finish the classes under the `javafx` package in the 2nd iteration.

modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalContentBinding.java line 81:

> 79:         if ((obj1 instanceof ObservableList<?> list1) && (obj2 instanceof ObservableList<?> list2)) {
> 80:             @SuppressWarnings("unchecked")
> 81:             final ListContentBinding<Object> binding = new ListContentBinding<>((ObservableList<Object>) list1, (ObservableList<Object>) list2);

Although the previous code has the same problem, this is sketchy. The two lists can be of different types while `ListContentBinding` requires the same type. This is a result of the `Bindings` public API that takes two `Objects`, so all type information is lost. Is it worth adding a comment about this since suppressing the warning can be understood as "trust me, this is fine".

modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalContentBinding.java line 81:

> 79:         if ((obj1 instanceof ObservableList<?> list1) && (obj2 instanceof ObservableList<?> list2)) {
> 80:             @SuppressWarnings("unchecked")
> 81:             final ListContentBinding<Object> binding = new ListContentBinding<>((ObservableList<Object>) list1, (ObservableList<Object>) list2);

By the way, for these cases I usually use `var` as I find it has less clutter:

final var binding = new ListContentBinding<Object>(...)


You don't have to change this.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java line 87:

> 85:         if ((obj1 instanceof List<?> list1) && (obj2 instanceof ObservableList<?> list2)) {
> 86:             @SuppressWarnings("unchecked")
> 87:             ListContentBinding<Object> binding = new ListContentBinding<>((List<Object>) list1);

Another place where I would personally use `var`.

modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java line 89:

> 87:             ListContentBinding<Object> binding = new ListContentBinding<>((List<Object>) list1);
> 88: 
> 89:             list2.removeListener(binding);

Another problem inherited from the existing code. What if the `obj2` is a `List` and `obj1` is an `ObservableList`? The docs don't say anything about the order.

Same question as before about adding a comment addressing the case that the two lists are not of the same type.

modules/javafx.base/src/main/java/com/sun/javafx/collections/MappingChange.java line 38:

> 36:     private List<F> removed;
> 37: 
> 38:     private static final Map<?, ?> NOOP_MAP = new Map<>() {

I think that we can do better with a bit of refactoring. The `Map` interface here is just `java.util.Function`. We can get rid of it and use `Function` instead. The `map` method call in `getRemoved` will be replaced with `apply`. The call sites needs just a little adjustment:
* In `TableView::TableViewArrayListSelectionModel` the `cellToIndicesMap` needs to change its type to `Function`, but it is also unused (didn't look what it was supposed to be for), so no issue there.
* In `TableView`, the method `fireCustomSelectedCellsListChangeEvent` needs to change its 2nd argument in the `MappingChange` constructor to `Function.indentity()` or something like `f -> f`.
* Same changes for `TreeTableView`.

I think that we can also use JavaFX's `Callback`, though I never use it if I can use `Function`.

modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java line 238:

> 236:     public Iterator<E> iterator() {
> 237:         final ObservableList<E> list = get();
> 238:         return (list == null)? ListExpression.<E>emptyList().iterator() : list.iterator();

Might as well add a space before `?` in these lines if you want.

modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java line 406:

> 404:     private static <E> ObservableList<E> emptyList() {
> 405:         return (ObservableList<E>) EMPTY_LIST;
> 406:     }

I would move this below the declaration of `EMPTY_LIST`.

modules/javafx.base/src/main/java/javafx/beans/binding/MapExpression.java line 326:

> 324:     private static <K, V> ObservableMap<K, V> emptyMap() {
> 325:         return (ObservableMap<K, V>) EMPTY_MAP;
> 326:     }

Same

modules/javafx.base/src/main/java/javafx/beans/binding/SetExpression.java line 331:

> 329:     private static <E> ObservableSet<E> emptySet() {
> 330:         return (ObservableSet<E>) EMPTY_SET;
> 331:     }

Same

-------------

PR: https://git.openjdk.org/jfx/pull/972