RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v10]

[John Hendrikx]

On Thu, 17 Mar 2022 20:09:23 GMT, Michael Strauß <mstrauss at 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