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

Wed Jan 5 12:29:12 UTC 2022

On Sun, 2 Jan 2022 20:18:02 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Upgrade tests to JUnit 5
>
> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java line 193:
> 
>> 191:      *
>> 192:      * @return {@code true} when this binding currently has one or more
>> 193:      *     listeners, otherwise {@code false}
> 
> Maybe the method description
> `Checks if the binding has at least one listener registered on it.`
> is more straightforward, especially since it is a first-sentence summary. The `@return` description can contain the info on what is returned. As for that, maybe
> `{@code true} if this binding currently has one or more listeners registered on it, otherwise {@code false}`
> is more precise.
> I'm not sure if "registered on" or "registered to" is better, not that I think it matters.
> 
> I would also like to see an explanation of how the method should/can be used by subclasses, but it looks to be tied to `Subscription`, which isn't added yet, so I don't have a good idea on this.

It is not strictly tied to `Subscription`, the method is required to determine when `LazyObjectBinding` must register listeners on its dependencies and when it can remove them again (basically when it stops being lazy or when it can become lazy again).

> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java line 204:
> 
>> 202:      *
>> 203:      * @return {@code true} if this binding is allowed to become valid, otherwise
>> 204:      *     {@code false}
> 
> Typo: overriden -> overridden
> 
> I would add a a first-sentence summary and an explanation as to why a binding would not allow it. I would write something like
> 
> Checks if the binding is allowed to become valid. Overriding classes can prevent a binding from becoming
> valid. This is useful when ____.
> <p>
> The default implementation always allows bindings to become valid.

I've made your suggested changes and added this explanation: "This is useful in subclasses which do not always listen for invalidations of their dependencies and prefer to recompute the current value instead. This can also be useful if caching of the current computed value is not desirable."

Furthermore, I noticed I forgot to make the code changes that prevent caching of the value when the binding is invalid -- bindings currently cache their value even when invalid, which could lead to situations where something is still being referenced in an invalid binding that should have been GC'd.

> modules/javafx.base/src/main/java/javafx/beans/value/MappedBinding.java line 34:
> 
>> 32: 
>> 33: class MappedBinding<S, T> extends LazyObjectBinding<T> {
>> 34:     private final ObservableValue<S> source;
> 
> We usually have an empty line below the class declaration. Not sure if this is enforced. Same for the other classes.

I've added them everywhere.

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 146:
> 
>> 144:      * Returns an {@link ObservableValue} which provides a mapping of the value
>> 145:      * held by this {@code ObservableValue}, and provides {@code null} when this
>> 146:      * {@code ObservableValue} holds {@code null}.
> 
> No need to `@link` since we are in the same class already.
> 
> While this description is very similar to that of ReactFX, I would prefer a shorter summary of what the method does as its first sentence, and this will allow to unpack the details more cleanly. Maybe something like:
> 
> Creates an {@code ObservableValue} that holds the result of applying a mapping on the value held
> by this {@code ObservableValue}. The result is updated (lazily) when the value held by this
> {@code ObservableValue} changes. If this value is {@code null}, the resulting value is also {@code null}
> (the mapping is not applied).
> <p>
> For example, mapping a lower case string to an uppercase string, and to a check if the string is blank:
> <pre>
>     var lowercase = new SimpleStringProperty("abcd");
>     ObservableValue<String> uppercase = lowercase.map(String::toUpperCase); // "ABCD"
>     ObservableValue<Boolean> blank = lowercase.map(String::isBlank);        // false
>     lowercase.set(" ");
>     uppercase.getValue(); // " "
>     blank.getValue();     // true
> </pre>

Copied these suggestions with some slight modifications, and I simplified the example a little bit.

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 149:
> 
>> 147:      *
>> 148:      * @param <U> the type of values held by the resulting {@code ObservableValue}
>> 149:      * @param mapper a {@link Function} which converts a given value to a new value, cannot be null
> 
> No need to `@link` since its linked in the method argument list when the docs are generated.
> `null` should be in `@code`.
> 
> Worth thinking about adding the types of the conversion:
> `...which converts a given value of type {@code T} to a new value of type {@code U}`
> but I'm not sure it's necessary.

The type parameters already have their own documentation lines, and I think `Function` itself clearly describes how this works already.

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 152:
> 
>> 150:      * @return an {@link ObservableValue} which provides a mapping of the value
>> 151:      *     held by this {@code ObservableValue}, and provides {@code null} when
>> 152:      *     this {@code ObservableValue} holds {@code null}, never null
> 
> No need for `@link`.
> 
> Since we already explained how the mapping works, maybe we can be more brief here:
> 
> an {@code ObservableValue} that holds the result of the mapping of the value
> held by this {@code ObservableValue}; never {@code null} itself

I think that `@return` should mention that the returned observable can hold `null`, how about: 

    an {@code ObservableValue} that holds a mapping of this {@code ObservableValue}'s value
    or holds {@code null} when the value is {@code null}; never returns {@code null}

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 161:
> 
>> 159:      * Returns an {@link ObservableValue} which provides a mapping of the value
>> 160:      * held by this {@code ObservableValue}, or {@code constant} when this
>> 161:      * {@code ObservableValue} holds {@code null}.
> 
> Similarly to `map`, but I would refrain from using the "mapping" terminology since we are not dealing with a map (function) on the surface:
> 
> 
> Creates an {@code ObservableValue} that holds the value held by this {@code ObservableValue},
> or a given value if this value is {@code null}. The result is updated (lazily) when the value held by this
> {@code ObservableValue} changes. This method can be combined with {@link #map(Function)} to
> handle {@code null} values.
> <p>
> For example, mapping a lower case string to an uppercase string, and to a check if the string is blank,
> with alternatives for a {@code null} value:
> <pre>
>     var lowercase = new SimpleStringProperty("abcd");
>     ObservableValue<String> uppercase = lowercase.map(String::toUpperCase).orElse(""); // "ABCD"
>     ObservableValue<Boolean> blank = lowercase.map(String::isBlank).orElse(true);      // false
>     lowercase.set(null);
>     uppercase.getValue(); // ""
>     blank.getValue();     // true
> </pre>

Copied this with some minor changes (I removed "lazily").  Simplified the example a bit.

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 167:
> 
>> 165:      * @return an {@link ObservableValue} which provides a mapping of the value
>> 166:      *     held by this {@code ObservableValue}, or {@code constant} when this
>> 167:      *     {@code ObservableValue} holds {@code null}, never null
> 
> an {@code ObservableValue} that holds the value held by this {@code ObservableValue}, or the given
> value if it is {@code null}; never {@code null} itself

Would this be clear enough:

    an {@link ObservableValue} that holds this {@code ObservableValue}'s value
    or the given value when the value is {@code null}; never returns {@code null}

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 180:
> 
>> 178:      *
>> 179:      * Returning {@code null} from {@code mapper} will result in an
>> 180:      * {@code ObservableValue} which holds {@code null}.
> 
> This one is hard for me to explain. It comes out convoluted regardless of how I phrase it.
> 
> 
> Creates an {@code ObservableValue} that holds the value of the {@code ObservableValue} resulting
> from applying a mapping on the value held by this {@code ObservableValue}. The result is updated (lazily)
> when either the values held by this {@code ObservableValue} or the value held by the {@code ObservableValue}
> in the mapping changes. If this value or the value returned by the mapping are {@code null}, the held value
> is also {@code null}.
> <p>
> For example, a property which is only true when the UI is visible:
> <pre>
>     ObservableValue<Boolean> allShowing = listView.sceneProperty()
>         .flatMap(Scene::windowProperty)
>         .flatMap(Window::showingProperty)
>         .orElse(false);
> </pre>
> Changes in the values of any of: the scene of {@code listView}, the window of that scene, or the showing of that
> window, will updated the result boolean value {@code allShowing}.
> <p>
> This method is preferred over {@link javafx.beans.binding.Bindings Bindings#select} methods since it is type
> safe and handles {@code null}s in the call chain more easily.

I think this reads okay. I made some minor changes to clarify the mapper is not called when this observable value holds null.  I also adding some more lines to the example, and I removed "and handles {@code null}s in the call chain more easily" as I think that's not really true (`select` handles nulls fine, although it (used to) prints warnings I think).

> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 188:
> 
>> 186:      *     {@code ObservableValue} given by applying {@code mapper} on the value
>> 187:      *     held by this {@code ObservableValue}, and is {@code null} when this
>> 188:      *     {@code ObservableValue} holds {@code null}, never null
> 
> an {@code ObservableValue} that holds the value of the {@code ObservableValue} resulting
> from applying a mapping on the value held by this {@code ObservableValue}; never {@code null} itself

How about:

    an {@code ObservableValue} holding the value of an {@code ObservableValue}
    resulting from a mapping of this {@code ObservableValue}'s value or
    holds {@code null} when the value is {@code null}; never returns {@code null}

They are tough to describe; if you don't like the `or holds {@code null}` parts I can remove those from all the functions.

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

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