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

Thu Jan 6 17:10:24 UTC 2022

On Wed, 5 Jan 2022 12:29:01 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:
> 
>   Apply changes suggested in review and updated copyright years to 2022

The changes look good. I added some minor grammar comments. I think that the API is in a good spot.

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

> 143:     /**
> 144:      * Creates an {@code ObservableValue} that holds the result of applying a
> 145:      * mapping on the value held by this {@code ObservableValue}. The result is

After one of your suggestions, another phrasing for `on the value held by this {@code ObservableValue}` is `on this {@code ObservableValue}'s value`, which is a bit shorter. If you think it helps, you can make this change here and in other places. Fine to leave as it.

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

> 162:      * @param mapper a {@code Function} which converts a given value to a new value, cannot be null
> 163:      * @return an {@code ObservableValue} holding a mapping of this {@code ObservableValue}'s value
> 164:      *     or holds {@code null} when the value is {@code null}; never returns {@code null}

* Comma before "or"
* holds -> holding (or the "holding" in the first line -> "that holds", just needs to be consistent)

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

> 176:      * <p>
> 177:      * For example, mapping a string to an upper case string but leaving it blank
> 178:      * if the input was {@code null}:

* Comma before "but"
* "input was" -> "input is"

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

> 188:      *     holds {@code null}; can be {@code null}
> 189:      * @return an {@code ObservableValue} holding this {@code ObservableValue}'s value
> 190:      *     or the given value when the value is {@code null}; never returns {@code null}

Comma before "or"

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

> 199:      * The result is updated when either this {@code ObservableValue} or the {@code ObservableValue}
> 200:      * resulting from the mapping changes. If this value is {@code null}, no mapping is applied
> 201:      * and the resulting value is {@code null}. If the mapping resulted in {@code null} then

Comma before "then"

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

> 202:      * the resulting value is also {@code null}.
> 203:      * <p>
> 204:      * For example, a property which is only true when a UI element is part of a {@code Scene}

* {@code true}
* "which" -> "that"

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

> 203:      * <p>
> 204:      * For example, a property which is only true when a UI element is part of a {@code Scene}
> 205:      * which is part of a {@code Window} that is currently shown on screen:

"which" -> "that"

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

> 230:      *
> 231:      * @param <U> the type of values held by the resulting {@code ObservableValue}
> 232:      * @param mapper a {@code Function} which converts a given value to an

"which" -> "that"

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

> 234:      * @return an {@code ObservableValue} holding the value of an {@code ObservableValue}
> 235:      *     resulting from a mapping of this {@code ObservableValue}'s value or
> 236:      *     holds {@code null} when the value is {@code null}; never returns {@code null}

* Comma before "or"
* holds -> holding (or the "holding" in the first line -> "that holds", just needs to be consistent)

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

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