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

[Nir Lisker]

On Sat, 10 Jul 2021 01:44:43 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 111:
>> 
>>> 109:      * established. However, doing so is not a meaningful use of this API, because the
>>> 110:      * unidirectionally bound property will cause any attempt to change the value of any of
>>> 111:      * the properties to fail with an exception.
>> 
>> I don't think this quite matches the implementation. What I've observed is:
>> 
>> * If a unidirectional binding already exists for this property, calling this method to attempt to establish a bidirectional binding will immediately cause an exception to happen.
>> * If a bidirectional binding already exists (as a result of calling this method), subsequently calling the bind() method to set up a unidirectional binding seems to "partially" remove the bidirectional binding. Setting the other bidirectionally bound property no longer updates this property, but setting this (or the newly bound property) does set the other bidirectionally bound property. I very much doubt we want to specify this, though.
>> 
>> More to the point, what we want to convey is that a property should not be both bound unidirectionally and bidirectionally. So I'd like to see a stronger "don't do that" sort of statement. The docs should state that an application should not call this method if a unidirectional binding already exists for either `this` property or the `other` property (rather than saying that it's possible, but discouraged or not meaningful). I think it would be fine to say that it is likely to cause an exception. 
>> 
>> Similarly, the docs for `bind` should say something about not calling that method if `this` property is bidirectionally bound to any other property.
>
> 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.

-------------

PR: https://git.openjdk.java.net/jfx/pull/533