Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v5]
On Sun, 12 Feb 2023 21:49:11 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix review comments This looked good to me, please go ahead and integrate. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v5]
On Sun, 12 Feb 2023 21:49:11 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix review comments @arapte do you want to take a look still? - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v5]
On Sun, 12 Feb 2023 21:49:11 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Fix review comments Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v5]
> Packages fixed: > - com.sun.javafx.binding > - com.sun.javafx.collections > - javafx.beans > - javafx.beans.binding > - javafx.collections > - javafx.collections.transformation John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Fix review comments - Changes: - all: https://git.openjdk.org/jfx/pull/972/files - new: https://git.openjdk.org/jfx/pull/972/files/8814a990..40abc6e6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=972=04 - incr: https://webrevs.openjdk.org/?repo=jfx=972=03-04 Stats: 52 lines in 2 files changed: 17 ins; 24 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/972.diff Fetch: git fetch https://git.openjdk.org/jfx pull/972/head:pull/972 PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v5]
On Sun, 12 Feb 2023 18:35:40 GMT, Nir Lisker wrote: >> Can make these consistent if the approach is agreed upon. > > So let's make the list and the set use an instance singleton pattern, like > the empty list does. Fixed - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v4]
On Sun, 12 Feb 2023 16:31:35 GMT, Nir Lisker wrote: >> John Hendrikx has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Merge branch 'master' of https://git.openjdk.org/jfx into >> feature/fix-raw-type-warnings-in-base-2 >> - Undo changes in SortHelper >> - Simplify MappingChange by using Function >> - Clean up expression classes repeated null checks >> - Use instanceof with pattern matching in a few spots >> - Use static method for no-op map >> - Fix raw type warnings in base >> >>Packages fixed: >>- com.sun.javafx.binding >>- com.sun.javafx.collections >>- javafx.beans >>- javafx.beans.binding >>- javafx.collections >>- javafx.collections.transformation > > modules/javafx.base/src/main/java/javafx/collections/transformation/FilteredList.java > line 133: > >> 131: return getPredicate(); >> 132: } >> 133: return (Predicate) ALWAYS_TRUE; > > Do we actually need the constant `ALWAYS_TRUE`? I think that just inlining > `return e -> true;` is fine and we can remove the field, the cast, and the > warning suppression. What do you think? I guess this is fine, as for example `Function#identity()` also just returns `t -> t` instead of using a static final. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v4]
On Sun, 1 Jan 2023 15:25:01 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
>> 1640:
>>
>>> 1638: @Override
>>> 1639: public Iterator 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.
>
> Can make these consistent if the approach is agreed upon.
So let's make the list and the set use an instance singleton pattern, like the
empty list does.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v4]
On Wed, 8 Feb 2023 15:05:46 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > John Hendrikx has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge branch 'master' of https://git.openjdk.org/jfx into > feature/fix-raw-type-warnings-in-base-2 > - Undo changes in SortHelper > - Simplify MappingChange by using Function > - Clean up expression classes repeated null checks > - Use instanceof with pattern matching in a few spots > - Use static method for no-op map > - Fix raw type warnings in base > >Packages fixed: >- com.sun.javafx.binding >- com.sun.javafx.collections >- javafx.beans >- javafx.beans.binding >- javafx.collections >- javafx.collections.transformation modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 235: > 233: } > 234: > 235: private static ObservableMap EMPTY_OBSERVABLE_MAP = new > EmptyObservableMap<>(); This can be a `final` field. Same for the list and set ones. Not sure why it wasn't declared as such from the start. modules/javafx.base/src/main/java/javafx/collections/transformation/FilteredList.java line 133: > 131: return getPredicate(); > 132: } > 133: return (Predicate) ALWAYS_TRUE; Do we actually need the constant `ALWAYS_TRUE`? I think that just inlining `return e -> true;` is fine and we can remove the field, the cast, and the warning suppression. What do you think? - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v4]
> Packages fixed: > - com.sun.javafx.binding > - com.sun.javafx.collections > - javafx.beans > - javafx.beans.binding > - javafx.collections > - javafx.collections.transformation John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'master' of https://git.openjdk.org/jfx into feature/fix-raw-type-warnings-in-base-2 - Undo changes in SortHelper - Simplify MappingChange by using Function - Clean up expression classes repeated null checks - Use instanceof with pattern matching in a few spots - Use static method for no-op map - Fix raw type warnings in base Packages fixed: - com.sun.javafx.binding - com.sun.javafx.collections - javafx.beans - javafx.beans.binding - javafx.collections - javafx.collections.transformation - Changes: - all: https://git.openjdk.org/jfx/pull/972/files - new: https://git.openjdk.org/jfx/pull/972/files/106fc11b..8814a990 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=972=03 - incr: https://webrevs.openjdk.org/?repo=jfx=972=02-03 Stats: 221805 lines in 6977 files changed: 129959 ins; 45549 del; 46297 mod Patch: https://git.openjdk.org/jfx/pull/972.diff Fetch: git fetch https://git.openjdk.org/jfx pull/972/head:pull/972 PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v3]
> Packages fixed: > - com.sun.javafx.binding > - com.sun.javafx.collections > - javafx.beans > - javafx.beans.binding > - javafx.collections > - javafx.collections.transformation John Hendrikx has updated the pull request incrementally with one additional commit since the last revision: Simplify MappingChange by using Function - Changes: - all: https://git.openjdk.org/jfx/pull/972/files - new: https://git.openjdk.org/jfx/pull/972/files/24278e96..106fc11b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=972=02 - incr: https://webrevs.openjdk.org/?repo=jfx=972=01-02 Stats: 31 lines in 3 files changed: 4 ins; 21 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/972.diff Fetch: git fetch https://git.openjdk.org/jfx pull/972/head:pull/972 PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 16:08:15 GMT, John Hendrikx wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > 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 looked at it again. I think it would be best to do the sorting-related changes separately. The rest looks good. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Wed, 4 Jan 2023 05:28:10 GMT, Nir Lisker 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 > void
> sort(ObservableList list) {
> sort(list, null); // or Comparator.naturalOrder() instead of null
> }
>
> public static void sort(ObservableList list, Comparator
> c) {
> if (list instanceof SortableList) {
> list.sort(c);
> } else {
> List 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 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 extends List, Observable {
>
> @Override
> default void sort(Comparator 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
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 16:08:15 GMT, John Hendrikx wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> 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 > void sort(ObservableList
list) {
sort(list, null); // or Comparator.naturalOrder() instead of null
}
public static void sort(ObservableList list, Comparator
c) {
if (list instanceof SortableList) {
list.sort(c);
} else {
List 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 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 extends List, Observable {
@Override
default void sort(Comparator 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.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Tue, 3 Jan 2023 00:24:25 GMT, Nir Lisker wrote:
>> What would go wrong if they're not the same type? `ListContentBinding`
>> doesn't (and can't) enforce it and doesn't care either way. The whole
>> function fails silently if types don't match. Also `ListContentBinding` is
>> a private class and so I'd expect this code to be aware of how it works and
>> what is/isn't safe to do.
>>
>> I personally think this entire class is unfinished. It fails miserably in
>> edge cases without so much as a warning to the user. Take this for example:
>>
>> public static void main(String[] args) {
>> ObservableList a = FXCollections.observableArrayList();
>> ObservableList b = FXCollections.observableArrayList();
>> Bindings.bindContentBidirectional(a, b);
>> Bindings.bindContentBidirectional(a, b);
>> a.add("A");
>> System.out.println(a + " : " + b);
>> }
>>
>> Prints:
>>
>> [A, A, A, A] : [A, A, A]
>>
>> No mention about this in the API docs at all. It breaks even worse when you
>> make circular bindings `[a,b], [b,c], [c,a]` (stack traces get logged to the
>> console complaining about `IndexOutOfBoundsException`).
>>
>> I've created a solution that rejects double bindings and circular bindings,
>> but it's a bit out of scope for this. I think however that it is worth
>> adding, and considering that the current behavior is broken when doing any
>> of such things, not a big API break if instead we throw an exception.
>
> You are right, nothing would go wrong.
>
> I agree that the behavior is undesired and should be fixed in another issue.
> I was thinking of adding more specific overloads that make sense to the
> public API and deprecating the method that takes everything, making it throw.
Adding more specific unbind functions may be a good idea, but I don't think we
can change `unbind` to throw :)
I've added concept code here (https://github.com/openjdk/jfx/pull/988) that
rejects situations that would lead to lists becoming unsynced or behave badly
in other ways.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Tue, 3 Jan 2023 00:26:21 GMT, Nir Lisker wrote: >> Yes, looks like this is quite broken. This wouldn't have gone unnoticed so >> long if unbind would just throw an exception when nothing could be unbound; >> silently failing is never a good idea. > > Can you file an issue for this if it's not filed already? I've filed https://bugs.openjdk.org/browse/JDK-8299521 - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Wed, 28 Dec 2022 20:16:51 GMT, Nir Lisker 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
>
> modules/javafx.base/src/main/java/com/sun/javafx/collections/MappingChange.java
> line 38:
>
>> 36: private List 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`.
Changed as suggested; I removed the unused fields.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 15:04:17 GMT, John Hendrikx wrote:
>> 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 binding = new
>>> ListContentBinding<>((ObservableList) list1,
>>> (ObservableList) 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".
>
> What would go wrong if they're not the same type? `ListContentBinding`
> doesn't (and can't) enforce it and doesn't care either way. The whole
> function fails silently if types don't match. Also `ListContentBinding` is a
> private class and so I'd expect this code to be aware of how it works and
> what is/isn't safe to do.
>
> I personally think this entire class is unfinished. It fails miserably in
> edge cases without so much as a warning to the user. Take this for example:
>
> public static void main(String[] args) {
> ObservableList a = FXCollections.observableArrayList();
> ObservableList b = FXCollections.observableArrayList();
> Bindings.bindContentBidirectional(a, b);
> Bindings.bindContentBidirectional(a, b);
> a.add("A");
> System.out.println(a + " : " + b);
> }
>
> Prints:
>
> [A, A, A, A] : [A, A, A]
>
> No mention about this in the API docs at all. It breaks even worse when you
> make circular bindings `[a,b], [b,c], [c,a]` (stack traces get logged to the
> console complaining about `IndexOutOfBoundsException`).
>
> I've created a solution that rejects double bindings and circular bindings,
> but it's a bit out of scope for this. I think however that it is worth
> adding, and considering that the current behavior is broken when doing any of
> such things, not a big API break if instead we throw an exception.
You are right, nothing would go wrong.
I agree that the behavior is undesired and should be fixed in another issue. I
was thinking of adding more specific overloads that make sense to the public
API and deprecating the method that takes everything, making it throw.
>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java
>> line 89:
>>
>>> 87: ListContentBinding binding = new
>>> ListContentBinding<>((List) 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.
>
> Yes, looks like this is quite broken. This wouldn't have gone unnoticed so
> long if unbind would just throw an exception when nothing could be unbound;
> silently failing is never a good idea.
Can you file an issue for this if it's not filed already?
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Sun, 1 Jan 2023 15:23:16 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
>> 709:
>>
>>> 707: private static class EmptyObservableList extends
>>> AbstractList implements ObservableList {
>>> 708:
>>> 709: private static final ListIterator iterator = new
>>> ListIterator<>() {
>>
>> Isn't a better fix is to not make this class static and use ``? 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.
>
> I considered all that, and don't mind changing it if we're in agreement.
>
> The reason I didn't is that it would subtly change the iterator (it would
> have an unnecessary reference to its containing class).
In my opinion that's fine. The iterator might not use its reference to the
list, but it's using its type information.
As for creating a new iterator on every invocation (like empty set does) or
returning the same instance (like empty list does), I don't mind that much and
I can't imagine it mattering in terms of performance.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
> Packages fixed: > - com.sun.javafx.binding > - com.sun.javafx.collections > - javafx.beans > - javafx.beans.binding > - javafx.collections > - javafx.collections.transformation 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 - Changes: - all: https://git.openjdk.org/jfx/pull/972/files - new: https://git.openjdk.org/jfx/pull/972/files/cee36600..24278e96 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=972=01 - incr: https://webrevs.openjdk.org/?repo=jfx=972=00-01 Stats: 243 lines in 5 files changed: 3 ins; 172 del; 68 mod Patch: https://git.openjdk.org/jfx/pull/972.diff Fetch: git fetch https://git.openjdk.org/jfx pull/972/head:pull/972 PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages [v2]
On Fri, 30 Dec 2022 16:21:10 GMT, Nir Lisker wrote:
>> I'm fine with that; the first two are equivalent, but in some cases I need
>> to add the type witness to avoid a warning and you can only do that by
>> adding the class name as well (ie. `emptyList()` is not allowed, but
>> `ListExpression.emptyList()` is.
>
> Why not use `FXCollections.emptyObservableList()` directly? If it's too
> long, a convenvenience method can be used:
>
> private static ObservableList emptyList() {
> return FXCollections.emptyObservableList();
> }
>
> No need to hold the instance here again in a way that makes it lose its type
> and do the same cast (and supress the warning) that `FXCollections` already
> does.
>
> Same with the other collections.
I've removed the repeated checks from these classes.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Fri, 30 Dec 2022 20:53:01 GMT, Nir Lisker wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
> 709:
>
>> 707: private static class EmptyObservableList extends AbstractList
>> implements ObservableList {
>> 708:
>> 709: private static final ListIterator iterator = new
>> ListIterator<>() {
>
> Isn't a better fix is to not make this class static and use ``? 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.
I considered all that, and don't mind changing it if we're in agreement.
The reason I didn't is that it would subtly change the iterator (it would have
an unnecessary reference to its containing class).
> modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line
> 1640:
>
>> 1638: @Override
>> 1639: public Iterator 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.
Can make these consistent if the approach is agreed upon.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Wed, 28 Dec 2022 19:11:38 GMT, Nir Lisker wrote: >> Packages fixed: >> - com.sun.javafx.binding >> - com.sun.javafx.collections >> - javafx.beans >> - javafx.beans.binding >> - javafx.collections >> - javafx.collections.transformation > > modules/javafx.base/src/main/java/com/sun/javafx/binding/ContentBinding.java > line 89: > >> 87: ListContentBinding binding = new >> ListContentBinding<>((List) 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. Yes, looks like this is quite broken. This wouldn't have gone unnoticed so long if unbind would just throw an exception when nothing could be unbound; silently failing is never a good idea. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Wed, 28 Dec 2022 18:57:23 GMT, Nir Lisker wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> 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 binding = new
>> ListContentBinding<>((ObservableList) list1,
>> (ObservableList) 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".
What would go wrong if they're not the same type? `ListContentBinding` doesn't
(and can't) enforce it and doesn't care either way. The whole function fails
silently if types don't match. Also `ListContentBinding` is a private class
and so I'd expect this code to be aware of how it works and what is/isn't safe
to do.
I personally think this entire class is unfinished. It fails miserably in edge
cases without so much as a warning to the user. Take this for example:
public static void main(String[] args) {
ObservableList a = FXCollections.observableArrayList();
ObservableList b = FXCollections.observableArrayList();
Bindings.bindContentBidirectional(a, b);
Bindings.bindContentBidirectional(a, b);
a.add("A");
System.out.println(a + " : " + b);
}
Prints:
[A, A, A, A] : [A, A, A]
No mention about this in the API docs at all. It breaks even worse when you
make circular bindings `[a,b], [b,c], [c,a]` (stack traces get logged to the
console complaining about `IndexOutOfBoundsException`).
I've created a solution that rejects double bindings and circular bindings, but
it's a bit out of scope for this. I think however that it is worth adding, and
considering that the current behavior is broken when doing any of such things,
not a big API break if instead we throw an exception.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx 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)` 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 extends AbstractList
> implements ObservableList {
> 708:
> 709: private static final ListIterator iterator = new
> ListIterator<>() {
Isn't a better fix is to not make this class static and use ``? 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 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
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Mon, 26 Dec 2022 14:32:52 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java
>> line 238:
>>
>>> 236: public Iterator iterator() {
>>> 237: final ObservableList list = get();
>>> 238: return (list == null)?
>>> ListExpression.emptyList().iterator() : list.iterator();
>>
>> You're using three slightly different ways of referring to the empty list:
>> * `ListExpression.emptyList()`
>> * `emptyList()`
>> * `EMPTY_LIST`
>>
>> What do you think about using the first option in all cases?
>
> I'm fine with that; the first two are equivalent, but in some cases I need to
> add the type witness to avoid a warning and you can only do that by adding
> the class name as well (ie. `emptyList()` is not allowed, but
> `ListExpression.emptyList()` is.
Why not use `FXCollections.emptyObservableList()` directly? If it's too
long, a convenvenience method can be used:
private static ObservableList emptyList() {
return FXCollections.emptyObservableList();
}
No need to hold the instance here again in a way that makes it lose its type
and do the same cast (and supress the warning) that `FXCollections` already
does.
Same with the other collections.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx 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 binding = new
> ListContentBinding<>((ObservableList) list1, (ObservableList)
> 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 binding = new
> ListContentBinding<>((ObservableList) list1, (ObservableList)
> list2);
By the way, for these cases I usually use `var` as I find it has less clutter:
final var binding = new ListContentBinding(...)
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 binding = new
> ListContentBinding<>((List) 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 binding = new
> ListContentBinding<>((List) 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 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 iterator() {
> 237: final ObservableList list = get();
> 238: return (list == null)? ListExpression.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 ObservableList emptyList() {
> 405: return (ObservableList) 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 ObservableMap emptyMap() {
> 325: return (ObservableMap) EMPTY_MAP;
> 326: }
Same
modules/javafx.base/src/main/java/javafx/beans/binding/SetExpression.java line
331:
> 329: private static ObservableSet emptySet() {
> 330: return (ObservableSet) EMPTY_SET;
> 331: }
Same
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Mon, 19 Dec 2022 18:00:30 GMT, Kevin Rushforth wrote: > I spot-checked it and it seems OK, but I'll let Nir and Ambarish review it in > more detail. One question I had was around the changes to remove one of the > overloads of `mergeSort` (the one without a `Comparator` arg) from > `SortHelper.java`: I presume you have verified that you won't ever get an NPE > due to a null `Comparator`? I checked all the call sites, and they never call it with a `null` as they check either just before or in the previous method if the comparator is `null` before going into that path, so the code that was removed wasn't (currently) in use (perhaps it was in the past). The caller is now responsible for calling either a version that uses an explicit comparator (with the appropriate generic signature), or one that relies on the natural order of elements (ie. the elements implement `Comparable`). I documented the methods to make sure (future) callers know what to expect. - PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Mon, 26 Dec 2022 03:26:14 GMT, Michael Strauß wrote:
>> Packages fixed:
>> - com.sun.javafx.binding
>> - com.sun.javafx.collections
>> - javafx.beans
>> - javafx.beans.binding
>> - javafx.collections
>> - javafx.collections.transformation
>
> modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java
> line 238:
>
>> 236: public Iterator iterator() {
>> 237: final ObservableList list = get();
>> 238: return (list == null)? ListExpression.emptyList().iterator()
>> : list.iterator();
>
> You're using three slightly different ways of referring to the empty list:
> * `ListExpression.emptyList()`
> * `emptyList()`
> * `EMPTY_LIST`
>
> What do you think about using the first option in all cases?
I'm fine with that; the first two are equivalent, but in some cases I need to
add the type witness to avoid a warning and you can only do that by adding the
class name as well (ie. `emptyList()` is not allowed, but
`ListExpression.emptyList()` is.
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx wrote:
> Packages fixed:
> - com.sun.javafx.binding
> - com.sun.javafx.collections
> - javafx.beans
> - javafx.beans.binding
> - javafx.collections
> - javafx.collections.transformation
modules/javafx.base/src/main/java/javafx/beans/binding/ListExpression.java line
238:
> 236: public Iterator iterator() {
> 237: final ObservableList list = get();
> 238: return (list == null)? ListExpression.emptyList().iterator()
> : list.iterator();
You're using three slightly different ways of referring to the empty list:
* `ListExpression.emptyList()`
* `emptyList()`
* `EMPTY_LIST`
What do you think about using the first option in all cases?
-
PR: https://git.openjdk.org/jfx/pull/972
Re: RFR: JDK-8298528: Clean up raw type warnings in base in bindings and collections packages
On Sun, 11 Dec 2022 20:12:17 GMT, John Hendrikx wrote: > Packages fixed: > - com.sun.javafx.binding > - com.sun.javafx.collections > - javafx.beans > - javafx.beans.binding > - javafx.collections > - javafx.collections.transformation I spot-checked it and it seems OK, but I'll let Nir and Ambarish review it in more detail. One question I had was around the changes to remove one of the overloads of `mergeSort` (the one without a `Comparator` arg) from `SortHelper.java`: I presume you have verified that you won't ever get an NPE due to a null `Comparator`? - PR: https://git.openjdk.org/jfx/pull/972
