On Thu, 17 Mar 2022 20:09:23 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Process review comments (2)
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java
> line 40:
>
>> 38: *
>> 39: * For example:<p>
>> 40: * <pre>Subscription s = property.subscribe(System.out::println)</pre>
>
> I believe our recommended pattern for example code is:
>
> <pre>{@code
> ...
> }</pre>
Fixed this everywhere.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java
> line 67:
>
>> 65: */
>> 66: default Subscription and(Subscription other) {
>> 67: Objects.requireNonNull(other);
>
> This exception could be documented with `@throws NullPointerException if
> {@code other} is null`
I've updated the docs a bit -- it hasn't received much attention because this
is not going to be API for now
> modules/javafx.base/src/main/java/javafx/beans/value/FlatMappedBinding.java
> line 68:
>
>> 66: };
>> 67: }
>> 68: }
>
> Several files are missing newlines after the last closing brace. Do we
> enforce this?
>
> Also, if there's a newline after the first line of a class declaration,
> shouldn't there also be a newline before the last closing brace?
Let me add those new lines at the end of files (everywhere) as Github is also
flagging it with an ugly red marker. I tend to unconsciously remove them
myself on longer files as it looks weird in editors to have an unused line at
the bottom.
As for the newline before the last closing brace, that doesn't seem to be done
a lot in the current code base. I've added those newlines at the top as it
seems fairly consistent in the code base, although I'm not a fan as I use empty
lines only to separate things when there is no clear separation already (like
an opening brace).
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 146:
>
>> 144: * Creates an {@code ObservableValue} that holds the result of
>> applying a
>> 145: * mapping on this {@code ObservableValue}'s value. The result is
>> updated
>> 146: * when this {@code ObservableValue}'s value changes. If this value
>> is
>
> I think a lot of the new documentation in this class sacrifices
> understandability for precision in trying to deal with the difference between
> "this ObservableValue" and "this ObservableValue's value".
>
> However, my feeling is that that's not helping users who are trying to
> understand the purpose of the new APIs.
> What do you think about a simplified version like this:
> `Creates a new {@ObservableValue} that applies a mapping function to this
> {@code ObservableValue}. The result is updated when this {@code
> ObservableValue} changes.`
>
> Sure, it's not literally mapping _this ObservableValue instance_, but would
> this language really confuse readers more that the precise language?
>
> Another option might be to combine both:
> `Creates a new {@ObservableValue} that applies a mapping function to this
> {@code ObservableValue}. More precisely, it creates a new {@code
> ObservableValue} that holds the result of applying a mapping function to the
> value of this {@code ObservableValue}.`
Yeah, agreed, it is a bit annoying to have to deal with the fact that these
classes are wrappers around an actual value and having to refer to them as such
to be "precise". I'm willing to make another pass at all of these to change
the wording. What do you think @nlisker ?
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 153:
>
>> 151: * <pre>
>> 152: * var text = new SimpleStringProperty("abcd");
>> 153: * ObservableValue<String> upperCase =
>> text.map(String::toUpperCase);
>
> Escaping `<` and `>` is not necessary if the code block is wrapped in
> `{@code}`
Also fixed everywhere
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 197:
>
>> 195: /**
>> 196: * Creates an {@code ObservableValue} that holds the value of an
>> {@code ObservableValue}
>> 197: * resulting from applying a mapping on this {@code
>> ObservableValue}'s value. The result
>
> While technically correct, I think the first sentence should focus more on
> the purpose of this method.
>
> How about something like this:
> `Creates a new {@code ObservableValue} that holds the value of a nested
> {@code ObservableValue} by applying a mapping function to extract the nested
> {@code ObservableValue}.`
>
> That's not as precise, but it makes the purpose much more clear.
I've changed this to use your wording as I think it does read much better.
Perhaps also possible:
Creates a new {@code ObservableValue} that holds the value of a nested
{@code ObservableValue} supplied
by the given mapping function.
?
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 227:
>
>> 225: * the showing of that window, will update the boolean value {@code
>> isShowing}.
>> 226: * <p>
>> 227: * This method is preferred over {@link
>> javafx.beans.binding.Bindings Bindings#select} methods
>
> This links to the `Bindings` class instead of a `select` method. Shouldn't it
> link directly to one of the relevant methods?
I changed this, it now links to the correct spot (for me)
> modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java
> line 685:
>
>> 683: * before adding it.
>> 684: *
>> 685: * @param observableValue
>
> You can probably remove `@param` if there's no further documentation.
I've fixed these javadoc comments.
-------------
PR: https://git.openjdk.java.net/jfx/pull/675
