On Sun, 1 Jan 2023 15:04:17 GMT, John Hendrikx <jhendr...@openjdk.org> 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<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".
>
> 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<String> a = FXCollections.observableArrayList();
> ObservableList<String> 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<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.
>
> 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
