[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v16]
From: Kevin Rushforth <kcr () openjdk ! org>
Date: 2022-06-30 23:54:02
Message-ID: 4SyeeOKmJpOlJV_5pxkAn9fjjjUVLL84t7NYzoA--is=.2f8ca931-cf15-4cd9-a407-a0094373c930 () github ! com
[Download RAW message or body]
On Thu, 30 Jun 2022 06:57:03 GMT, John Hendrikx <jhendrikx@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic