[jfx17] RFR: 8268642: Expand the javafx.beans.property.Property documentation [v2]

[Kevin Rushforth]

On Sat, 10 Jul 2021 12:33:52 GMT, Nir Lisker <nlisker at 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