On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx <[email protected]> wrote:
> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation
Second part of the review. I'm still looking at the changes to sorting. I think
that the problem stems from a more basic flaw that we can possible fix, and
that is that any `ObservableListWrapper` is a `SortedList` even when it's not.
A `SortedList` should require that its elements implement `Comparable`. The
only other class affected is the `Sequential` variant, so the scope here is
small.
While the current fix removes the warnings, I suspect we are just hiding the
flaw that these warnings exist for.
In addition `FXCollections::sort(ObservableList<T>)` can change to
if (list instanceof SortableList<?> sortableList) {
sortableList.sort();
and remove the `@SuppressWarnings("unchecked")`.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
709:
> 707: private static class EmptyObservableList<E> extends AbstractList<E>
> implements ObservableList<E> {
> 708:
> 709: private static final ListIterator<Object> iterator = new
> ListIterator<>() {
Isn't a better fix is to not make this class static and use `<E>`? Other lists
create a new iterator on each invocation, but this is effectively a singleton.
In fact, `EmptyObservableList` doesn't need to be declared because it's called
only in one place, so it can be "inlined" when declaring the `EMPTY_LIST`
field. Maybe this is out of scope.
modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
1640:
> 1638: @Override
> 1639: public Iterator<E> iterator() {
> 1640: return new Iterator<>() {
Here the empty `Set` creates a listener on invocation, unlike in the list case.
Might want to keep a single pattern. I prefer the one with a singleton iterator
because the empty set itself is a singleton. Same comment about considering
"inlining" it.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 94:
> 92: List<?> currentSource = source;
> 93: while(currentSource instanceof TransformationList) {
> 94: currentSource = ((TransformationList<?,
> ?>)currentSource).source;
Can use pattern matching for `instanceof`.
Also, can add a space after `while`.
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 143:
> 141: int idx = getSourceIndex(index);
> 142: while(currentSource != list && currentSource instanceof
> TransformationList) {
> 143: final TransformationList<?, ?> tSource =
> (TransformationList<?, ?>) currentSource;
Sam here.
-------------
PR: https://git.openjdk.org/jfx/pull/972
