On Thu, 27 Jan 2022 21:49:07 GMT, John Hendrikx <jhendr...@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: > > Fix wrong test values The tests look good. I'm happy with the coverage and added one comment about a missing case. My own sanity checks also work as I expect. Will approve when these are addressed as I have already reviewed the API and implementation. modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 42: > 40: > 41: import javafx.beans.property.ObjectProperty; > 42: import javafx.beans.property.SimpleObjectProperty; Unused imports modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 58: > 56: @Nested > 57: class WithNull { > 58: @Test Maybe some line separation between the classes is helpful. Same for other places in this class. modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 356: > 354: } > 355: } > 356: // TODO test for when something is flatMapped to null in > getValue call Is this still relevant? I think that you tested it in line 407 `shouldIgnoreFlatMapsToNull`. What I would like to see is a test when `left`, `right` or `unknown`'s value is set to `null`. What I see is `property`'s value being set to `null`, or the reference of one of the above being set to `null`, but not the value itself. Only when you compose `orElse` on `flatMap` this case is tested (line 604). Is this what you meant? modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 363: > 361: private StringProperty right = new > SimpleStringProperty("RIGHT"); > 362: private StringProperty unknown = new > SimpleStringProperty("UNKNOWN"); > 363: private ObservableValue<String> observableValue = > property.flatMap(v -> "Left".equals(v) ? left : "Right".equals(v) ? right : > unknown); Maybe break this line since it's too long. I think that 120 characters is the maximum length. modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 379: > 377: @Test > 378: void shouldReturnPropertyValuesWithOperationApplied() { > 379: assertEquals("UNKNOWN", observableValue.getValue()); > // initially it is not left or right, so unknown Maybe these comments should be in the `String message` argument? I don't mind either way. modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 457: > 455: right = null; > 456: > 457: property.set("Right"); Isn't `unknown = null;` enough like you did previously? It doesn't really matter, just for consistency. modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 648: > 646: > 647: /** > 648: * Ensures nothing has been observed. "Ensures nothing has been observed since the last check" or something like that because the values get cleared. ------------- PR: https://git.openjdk.java.net/jfx/pull/675
