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

[Kevin Rushforth]

On Thu, 24 Jun 2021 01:53:53 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> * Expand the `Property.bind` and `Property.bindBidirectional` documentation
>> * Change the name of the formal parameter of `Property.bind` to "source" (currently, it is inconsistently named "observable", "rawObservable" or "newObservable")
>
> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
> 
>   changes as per review

Overall this looks good with a few minor comments and one substantive one around specifying the behavior of binding the same property both unidirectionally and bidirectionally.

modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 44:

> 42:     /**
> 43:      * Establishes a unidirectional binding between this property (the <i>bound property</i>)
> 44:      * and an {@link ObservableValue} (the <i>binding source</i>).

Minor: We generally recommend using the `<em>` tag for emphasis instead of the `<i>` tag.

modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 91:

> 89:      * After establishing the binding, the values of both properties are synchronized: any change
> 90:      * to the value of one property will immediately result in the value of the other property being
> 91:      * changed accordingly.

I recommend indicating which one is initially used. Maybe something like "When the binding is first established, the value of this property is changed to the current value of the other property."

modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 97:

> 95:      * since a bidirectional binding allows for the values of both properties to be changed
> 96:      * by calling {@link #setValue(Object)}, neither of the properties is considered to be a
> 97:      * <i>bound property</i> that returns {@code true} from {@link #isBound()}.

It might be clearer to say this as:


    neither of the properties is considered to be a
    <em>bound</em> property, which means that {@link #isBound()} will return {@code false}.

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.

modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 125:

> 123:      * <p>
> 124:      * Bidirectional bindings can be removed by calling this method on either of the two properties:
> 125:      * <pre><code>

Minor: Our typical pattern for example code is:


     * <pre>{@code
     * code sample
     * ...
     * }</pre>

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

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