On Mon, 14 Jun 2021 17:06:34 GMT, Michael Strauß <mstra...@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")
Looks good in general. I left some inline comments which are mostly optional.
modules/javafx.base/src/main/java/javafx/beans/property/BooleanPropertyBase.java
line 165:
> 163: if (source == null) {
> 164: throw new NullPointerException("Cannot bind to null");
> 165: }
I recommend using `Objects.requireNonNull(source, "Cannot bind to null");`
while these are being changed. Same for other files in this PR.
modules/javafx.base/src/main/java/javafx/beans/property/BooleanPropertyBase.java
line 167:
> 165: }
> 166:
> 167: final ObservableBooleanValue newObservable = (source instanceof
> ObservableBooleanValue) ? (ObservableBooleanValue) source
This line is longer than 120 characters. You can break it into
final ObservableBooleanValue newObservable = (source instanceof
ObservableBooleanValue) ?
(ObservableBooleanValue) source : new ValueWrapper(source);
modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 47:
> 45: * <p>
> 46: * After establishing the binding, the value of the bound property is
> synchronized with the value
> 47: * of the binding source. If the value of the binding source changes,
> the change is immediately
I would align the phrasing here with the one from bidirectional binding:
After establishing the binding, the value of the bound property is synchronized
with the value
of the binding source: any change to the value of the binding source will
immediately result in the value
of the bound property being changed accordingly.
modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 68:
> 66:
> 67: /**
> 68: * Removes a unidirectional binding that was established with {@link
> #bind(ObservableValue)}.
Why "**a** unidirectional binding" and not "**the**"? "a" implies that there
can be more than one binding at the same time. At least that's how I read it.
modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 115:
> 113: * @param other the other property
> 114: * @throws NullPointerException if {@code other} is {@code null}
> 115: * @throws IllegalArgumentException if {@code other} is {@code this}
Since there are many references to unidirectional bindings, it might be wise to
add an `@see` for the `bind` method. It could also be done with an `@link` on
the first occurrence of "unidirectional binding".
modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 120:
> 118:
> 119: /**
> 120: * Removes a bidirectional binding that was established with {@link
> #bindBidirectional(Property)}.
"the" (it's possible to have multiple bidi bindings, but only 1 to the same
`other` property).
-------------
PR: https://git.openjdk.java.net/jfx/pull/533
