On Fri, 1 Jul 2022 15:16:24 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 >> )); >> >> // 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 27 additional > commits since the last revision: > > - Merge branch 'openjdk:master' into feature/fluent-bindings > - Add null checks in Subscription > - Update copyrights > - 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 > - Expand flatMap javadoc with additional wording from Optional#flatMap > - Add since tags to all new API > - ... and 17 more: https://git.openjdk.org/jfx/compare/cacb6847...d66f2ba6 > I'm not entirely in agreement here, but also not entirely disagreeing. I > mean, if there was a way to make this work perfectly, then I agree that this > "automatic" unsubscribing is a great mechanism. However, because you are > relying on the GC the code is actually subtly broken right from the start -- > you just don't notice it so easily: > > 1. [...] > 2. [...] > 3. The issues I highlighted in my earlier post. Why is there a difference in > how listeners work when it is a property vs an expression? Why does it work > differently when using a binding? Replacing a binding with a listener and > vice versa can have surprising consequences. I think a model with the following properties would be more consistent and easier to understand than that we currently have: 1. An `ObservableValue` is eligible for collection if it is not reachable from user code, independent of whether it has listeners. 2. A derived `ObservableValue` keeps its base value alive, i.e. the base value is reachable through the derived value. The second property ensures that it will be sufficient to keep a reference to the last `ObservableValue` in an expressions like `caption.map(String::toUpperCase).map(String::toLowerCase()` without losing any of the intermediate values. It is true that manually added listeners may suffer from unexpected collection, but something has to give. It seems like a reasonable trade-off to make listeners more complicated than binding expressions (which also hopefully discourages the use of manually added listeners). I would also argue that it is easier to explain that it's not the _listener_ that's different in properties v. expressions, it's the _reachability_ of the `ObservableValue` that's different. > Just to make sure we are on same page, it leaks a bit more memory than it > would if it was using `concat` or a regular property, as the listener stub is > leaked + the mapping itself. The label is not leaked. The leak "fixes" itself > (in both cases) once caption is updated: the left over listener stub which > `label` added gets removed, which in turn causes an unsubscribe on the > mapping, which in turn unsubscribes itself from `caption`. The mapping is > then eligible for GC. Thanks for this clarification, I wasn't aware that the leaked listener is actually removed when the source property is changed. That makes the issue a little bit less concerning, but I'd still prefer a solution that doesn't require the source property to change. ------------- PR: https://git.openjdk.org/jfx/pull/675
