On Mon, 12 Jul 2021 22:27:12 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> 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.
Fine with me.
I didn't look at the current tests, but if it helps, we can add the one I wrote.
-------------
PR: https://git.openjdk.java.net/jfx/pull/533
