On Sat, 10 Jul 2021 12:33:52 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Maybe we could simply the mental model of the property specification by
>> making it illegal in all cases to use unidirectional and bidirectional
>> bindings at the same time. The specification would be reduced to "it's
>> illegal", instead of having a long explanation that says it's only _likely_
>> to cause an exception.
>>
>> Most attempts to use both kinds of bindings will fail with an exception
>> anyway. Some will fail instantly when calling `bindBidirectional`, some will
>> fail later when calling `setValue`. Some will fail more silently because the
>> exception will happen inside `BidirectionalBinding` and not bring down the
>> application. Only a kind-of-exotic situation will not produce exceptions at
>> all (the situation that you described in the second bullet point).
>>
>> If we were to always fail instantly at the point of calling `bind` or
>> `bindBidirectional`, we would make the misuse of this API visible where it's
>> most relevant, instead of failing later in other parts of the codebase.
>
> The following unit test demonstrated the *current* behavior, though maybe
> it's not the desired one:
>
>
> import static org.junit.Assert.*;
>
> import javafx.beans.property.IntegerProperty;
> import javafx.beans.property.SimpleIntegerProperty;
>
> import org.junit.Before;
> import org.junit.Test;
>
> public class BindingsTest {
>
> private IntegerProperty property1, property2, property3;
>
> @Before
> public void setup() {
> property1 = new SimpleIntegerProperty(1);
> property2 = new SimpleIntegerProperty(2);
> property3 = new SimpleIntegerProperty(3);
> }
>
> @Test
> public void testBidiAndThenUni() {
> property1.bindBidirectional(property2);
> assertEquals(2, property1.get());
> assertEquals(2, property2.get());
> property1.bind(property3);
> assertEquals(3, property1.get());
> assertEquals(3, property2.get());
> assertEquals(3, property3.get());
> }
>
> @Test(expected = RuntimeException.class)
> public void setProperty1() {
> property1.bindBidirectional(property2);
> property1.bind(property3);
>
> property1.set(4);
> }
>
> @Test
> public void setProperty2() {
> property1.bindBidirectional(property2);
> property1.bind(property3);
>
> // ignores exception (but still prints stack trace) at
> //
> com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:181)
> property2.set(5);
> assertEquals(3, property1.get());
> assertEquals(3, property2.get()); // "Bidirectional binding
> failed, setting to the previous value"
> assertEquals(3, property3.get());
> }
>
> @Test
> public void setProperty3() {
> property1.bindBidirectional(property2);
> property1.bind(property3);
>
> property3.set(6);
> assertEquals(6, property1.get());
> assertEquals(6, property2.get());
> assertEquals(6, property3.get());
> }
>
> @Test(expected = RuntimeException.class)
> public void testUniAndThenBidiOnBound() {
> property1.bind(property3);
> property1.bindBidirectional(property2);
> }
>
> @Test
> public void testUniAndThenBidiOnUnbound() {
> property1.bind(property3);
> property2.bindBidirectional(property1);
>
> assertEquals(3, property1.get());
> assertEquals(3, property2.get());
> assertEquals(3, property3.get());
> }
> }
>
>
> My observations:
>
> * `setProperty2()` is weird in that it throws an exception internally which
> is ignored by the JVM, and then reverts the change to the unidirectionally
> unbound property when it finds out that its counterpart can't be changed. It
> makes sense logically, but I don't see a valid use case for this.
> * `setProperty3()` behaves as I expect it to, but this is effectively 2
> chained unidirectional binds: `property3` changes `property1` which changes
> `property2`. This is the same as `testUniAndThenBidiOnUnbound()`.
>
> It would make sense to disallow this mixing of bindings on grounds that this
> is probably not what the developer wants to do. If we opt to let them do it
> but write that it should not be done, we could emit a warning. I'm leaning
> towards to fail-fast approach.
I also like documenting it as illegal, but am hesitant to make any
implementation change in this area for JavaFX 17 during rampdown. So I would
recommend documenting as illegal, but not adding the `@throws` to the docs. We
could still say that an exception might be thrown or could say something like
"results are undefined". Then in JavaFX 18, we can add the `@throws` and change
the implementation to fail fast.
-------------
PR: https://git.openjdk.java.net/jfx/pull/533
