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

[Nir Lisker]

On Mon, 12 Jul 2021 22:27:12 GMT, Kevin Rushforth <kcr at 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