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

[Michael Strauß]

On Thu, 10 Mar 2022 17:49:38 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> This is an implementation of the proposal in https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker (@nlisker) have been working on.  It's a complete implementation including good test coverage.  
>> 
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller API footprint.  Compared to the PoC this is lacking public API for subscriptions, and does not include `orElseGet` or the `conditionOn` conditional mapping.
>> 
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested properties with `flatMap`.
>> 
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when not themselves observed. This allows for easier garbage collection (once the last observer is removed, a chain of bindings will stop observing their parents) and less listener management when dealing with nested properties.  Furthermore, this allows inclusion of such bindings in classes such as `Node` without listeners being created when the binding itself is not used (this would allow for the inclusion of a `treeShowingProperty` in `Node` without creating excessive listeners, see this fix I did in an earlier PR: https://github.com/openjdk/jfx/pull/185)
>> 
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional` when the value they would be mapping is `null`. This makes mapping nested properties with `flatMap` trivial as the `null` case does not need to be taken into account in a chain like this: `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.  Instead a default can be provided with `orElse`.
>> 
>> Some examples:
>> 
>>     void mapProperty() {
>>       // Standard JavaFX:
>>       label.textProperty().bind(Bindings.createStringBinding(() -> text.getValueSafe().toUpperCase(), text));
>> 
>>       // Fluent: much more compact, no need to handle null
>>       label.textProperty().bind(text.map(String::toUpperCase));
>>     }
>> 
>>     void calculateCharactersLeft() {
>>       // Standard JavaFX:
>>       label.textProperty().bind(text.length().negate().add(100).asString().concat(" characters left"));
>> 
>>       // Fluent: slightly more compact and more clear (no negate needed)
>>       label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + " characters left"));
>>     }
>> 
>>     void mapNestedValue() {
>>       // Standard JavaFX:
>>       label.textProperty().bind(Bindings.createStringBinding(
>>         () -> employee.get() == null ? ""
>>             : employee.get().getCompany() == null ? ""
>>             : employee.get().getCompany().getName(),
>>         employee
>>       ));
>> 
>>       // Fluent: no need to handle nulls everywhere
>>       label.textProperty().bind(
>>         employee.map(Employee::getCompany)
>>                 .map(Company::getName)
>>                 .orElse("")
>>       );
>>     }
>> 
>>     void mapNestedProperty() {
>>       // Standard JavaFX:
>>       label.textProperty().bind(
>>         Bindings.when(Bindings.selectBoolean(label.sceneProperty(), "window", "showing"))
>>           .then("Visible")
>>           .otherwise("Not Visible")
>>       );
>> 
>>       // Fluent: type safe
>>       label.textProperty().bind(label.sceneProperty()
>>         .flatMap(Scene::windowProperty)
>>         .flatMap(Window::showingProperty)
>>         .orElse(false)
>>         .map(showing -> showing ? "Visible" : "Not Visible")
>>       );
>>     }
>> 
>> Note that this is based on ideas in ReactFX and my own experiments in https://github.com/hjohn/hs.jfx.eventstream.  I've come to the conclusion that this is much better directly integrated into JavaFX, and I'm hoping this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Process review comments (2)

The code looks good. I've left some inline comments.

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>

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`

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?

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}.`

modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 151:

> 149:      * <p>
> 150:      * For example, mapping a string to an upper case string:
> 151:      * <pre>

<pre>{@code
    ...
}</pre>

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}`

modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 180:

> 178:      * <pre>
> 179:      * var text = new SimpleStringProperty("abcd");
> 180:      * ObservableValue<String> upperCase = text.map(String::toUpperCase).orElse("");

Escaping `<` and `>` is not necessary within

<pre>{@code
    ...
}</pre>

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.

modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 206:

> 204:      * that is part of a {@code Window} that is currently shown on screen:
> 205:      * <pre>
> 206:      * ObservableValue<Boolean> isShowing = listView.sceneProperty()

Escaping `<` and `>` is not necessary within

<pre>{@code
    ...
}</pre>

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?

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.

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

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