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

[Kevin Rushforth]

On Thu, 30 Jun 2022 06:57:03 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
>>       ));
>> 
>>       // Standard JavaFX + Optional:
>>       label.textProperty().bind(Bindings.createStringBinding(
>>           () -> Optinal.ofNullable(employee.get())
>>               .map(Employee::getCompany)
>>               .map(Company::getName)
>>               .orElse(""),
>>          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 five additional commits since the last revision:
> 
>  - Move private binding classes to com.sun.javafx.binding package
>  - Add note to Bindings#select to consider ObservableValue#flatMap
>  - Fix bug invalidation bug in FlatMappedBinding
>    
>    Also fixed a secondary issue where the indirect source of the binding
>    was unsubscribed and resubscribed each time its value was recomputed.
>    
>    Add additional comments to clarify how FlatMappedBinding works.
>    
>    Added test cases for these newly discovered issues.
>  - Fix typos in LazyObjectBinding
>  - Rename observeInputs to observeSources

This looks quite good now. I believe the API is ready to go. I did a little testing and want to do some more tomorrow, and then hope to finish my review. I left a couple comments / questions inline.

Two overall comments:

1. The doc changes in Bindings.java should be added to the Specification section in the CSR.


2. The copyright line for each new file should be:


 * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.


with a comma after the year.

modules/javafx.base/src/main/java/com/sun/javafx/binding/FlatMappedBinding.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved.

That should be `2022,` with a comma after the year in addition to fixing the year.

modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line 90:

> 88:      */
> 89:     static <T> Subscription subscribe(ObservableValue<T> observableValue, Consumer<? super T> subscriber) {
> 90:         ChangeListener<T> listener = (obs, old, current) -> subscriber.accept(current);

Maybe call `Objects.requireNonNull()` on `observableValue` and `subscriber` before creating the listener?

modules/javafx.base/src/main/java/com/sun/javafx/binding/Subscription.java line 110:

> 108:      */
> 109:     static Subscription subscribeInvalidations(ObservableValue<?> observableValue, Runnable runnable) {
> 110:         Objects.requireNonNull(runnable);

Maybe also call `Objects.requireNonNull(observableValue)` here?

modules/javafx.base/src/main/java/javafx/beans/binding/Bindings.java line 443:

> 441:      * Since 19, it is recommended to use {@link ObservableValue#flatMap(java.util.function.Function)}
> 442:      * to select a nested member of an {@link ObservableValue}.
> 443:      * </p>

This should be added to the CSR.

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

PR: https://git.openjdk.org/jfx/pull/675