On Mon, 10 Jan 2022 21:09:15 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 grammar mistakes and did some small rephrases The API looks good to me. After you get the other reviewers to look at it you can update the CSR. As for the fluent binding tests: * The `private` fields in `ObservableValueFluentBindingsTest` can be `final`. * Most tests that I have seen in JavaFX use the assert overloads that include a message that explains what the value should be or what it means if the assertion failed. I don't know how much of a requirement it is. I can help write these if you want. * There is a utility class `JMemoryBuddy` that was added to JavaFX to test for memory leaks. This would have been helpful in the GC tests, but it seems that the class is not suited too well for this case (it requires you to `null` you own reference etc.). I think the way you did it is fine, but maybe that class should be updated (in a different patch) to be more welcoming for these checks. I would also add tests of chaining the observables in common use cases. I wrote some myself to test it for `map`: ObservableValue<String> observableValue1 = property.map(v -> v + "Z"); ObservableValue<String> observableValue2 = observableValue1.map(v -> v + "X"); assertEquals("AZ", observableValue1.getValue()); assertEquals("AZX", observableValue2.getValue()); property.set("B"); assertEquals("BZ", observableValue1.getValue()); assertEquals("BZX", observableValue2.getValue()); for `orElse`: ObservableValue<String> observableValue1 = property.map(v -> v + "Z"); ObservableValue<String> observableValue2 = observableValue1.orElse("K"); assertEquals("AZ", observableValue1.getValue()); assertEquals("AZ", observableValue2.getValue()); property.set("B"); assertEquals("BZ", observableValue1.getValue()); assertEquals("BZ", observableValue2.getValue()); property.set(null); assertNull(observableValue1.getValue()); assertEquals("K", observableValue2.getValue()); You can clean these up and use them or write your own. `flatMap` should also have one. I can clean mine up and post it if it helps (I did some dirty testing there). modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 189: > 187: * holds {@code null}; can be {@code null} > 188: * @return an {@code ObservableValue} holding this {@code > ObservableValue}'s value, > 189: * or the given value it is {@code null}; never returns {@code > null} `or the given value it is {@code null}` missing "when" or "if"? modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 271: > 269: @Test > 270: void shouldReturnPropertyValuesWithOperationApplied() { > 271: assertEquals((Integer) 65, > observableValue.getValue()); I don't think that the cast is needed as autoboxing will take care of it. Fine to leave as-is. ------------- PR: https://git.openjdk.java.net/jfx/pull/675
