RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
Nir Lisker
nlisker at openjdk.java.net
Tue Jan 4 03:32:24 UTC 2022
On Wed, 15 Dec 2021 11:43:36 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:
>
> Upgrade tests to JUnit 5
This is the review of the API. I suggested also adding the examples in the POC or similar to the relevant methods.
I have already commented on the implementation while we were discussing it in the mailing list, so that review will be minimal.
I will review the tests soon, but after a quick look they seem fine.
Don't worry about the CSR for now. When all reviewers agree on the API you can copy the final result to the CSR.
Unrelated to the review, will it makes sense in the future to make all bindings lazily register listeners like `LazyObjectBinding`, perhaps when we introduce `Subscription`?
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.
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.
modules/javafx.base/src/main/java/javafx/beans/value/LazyObjectBinding.java line 77:
> 75:
> 76: /**
> 77: * Called after a listener was added to start observing inputs, if they're not observed already.
No need for a comma before "if".
modules/javafx.base/src/main/java/javafx/beans/value/LazyObjectBinding.java line 99:
> 97:
> 98: /**
> 99: * Called after a listener was removed to stop observing inputs, if this was the last listener
No need for a comma before "if".
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>
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.
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
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>
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 164:
> 162: *
> 163: * @param constant an alternative value to use when this {@code ObservableValue}
> 164: * holds {@code null}, can be null
`... holds {@code null}; can be {@code null}`
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
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.
modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 184:
> 182: * @param <U> the type of values held by the resulting {@code ObservableValue}
> 183: * @param mapper a {@link Function} which converts a given value to an
> 184: * {@code ObservableValue}, cannot be null
`a {@code Function} that converts a given value to an {@code ObservableValue}; cannot be {@code null}`
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
-------------
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/675
More information about the openjfx-dev
mailing list