On Thu, 24 Jun 2021 01:53:53 GMT, Michael Strauß <[email protected]> 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
