On Sun, 11 Dec 2022 20:12:17 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
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
