On Fri, 1 Jul 2022 15:16:24 GMT, John Hendrikx <[email protected]> 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/50192012...d66f2ba6 > I have yet another question. The following test passes for `Bindings.select`, > but fails for `ObservableValue.flatMap`: > > Is this by design? If so, I think this can lead to subtle and hard to > diagnose bugs, and should be documented at the very least. This is by design, but it is a break from current JavaFX behaviour to ensure there are no surprises when making much more intensive use of mappings. In current JavaFX's, mappings are used sparingly as they are fairly limited and akward to use. For example, there is a `concat` function for `StringExpression`, but other useful functions like `lowerCase`, `prepend`, `trim`, `substring`, etc. are missing. There is `Bindings#when` to akwardly simulate a ternary expression, and there are arithmetic functions which, like `BigDecimal`, are quite akward to use. With the introduction of an arbitrary mapping function `map`, we can expect such mappings to be used much more often and I think that it is therefore important that they are predictable and are not subject to the whim of the garbage collector. Let me try to explain. Please have a look at these six tests: // #1 Baseline JavaFX bind on property: JMemoryBuddy.memoryTest(test -> { StringProperty labelText = new SimpleStringProperty(this, "labelText"); StringProperty model = new SimpleStringProperty(this, "model"); ObservableValue<String> expression = model; labelText.bind(expression); test.setAsReferenced(labelText); test.setAsReferenced(model); test.assertNotCollectable(expression); // makes sense, as it is the same as model }); // #2 Baseline JavaFX listener on property: JMemoryBuddy.memoryTest(test -> { StringProperty labelText = new SimpleStringProperty(this, "labelText"); StringProperty model = new SimpleStringProperty(this, "model"); ObservableValue<String> expression = model; expression.addListener((obs, old, current) -> labelText.set(current)); test.setAsReferenced(labelText); test.setAsReferenced(model); test.assertNotCollectable(expression); // makes sense, as it is the same as model }); // #3 Baseline JavaFX bind on concatted result: JMemoryBuddy.memoryTest(test -> { StringProperty labelText = new SimpleStringProperty(this, "labelText"); StringProperty model = new SimpleStringProperty(this, "model"); ObservableValue<String> expression = model.concat("%"); labelText.bind(expression); test.setAsReferenced(labelText); test.setAsReferenced(model); test.assertNotCollectable(expression); // expression is being used, please don't collect }); // #4 Baseline JavaFX listener on concatted result: JMemoryBuddy.memoryTest(test -> { StringProperty labelText = new SimpleStringProperty(this, "labelText"); StringProperty model = new SimpleStringProperty(this, "model"); ObservableValue<String> expression = model.concat("%"); expression.addListener((obs, old, current) -> labelText.set(current)); test.setAsReferenced(labelText); test.setAsReferenced(model); test.assertCollectable(expression); // !!!! -- listener stops working, expression was collected! }); // #5 Fluent bindings with bind: JMemoryBuddy.memoryTest(test -> { StringProperty labelText = new SimpleStringProperty(this, "labelText"); StringProperty model = new SimpleStringProperty(this, "model"); ObservableValue<String> expression = model.map(x -> x + "%"); labelText.bind(expression); test.setAsReferenced(labelText); test.setAsReferenced(model); test.assertNotCollectable(expression); // expression is being used, please don't collect }); // #6 Fluent bindings with listener: JMemoryBuddy.memoryTest(test -> { StringProperty labelText = new SimpleStringProperty(this, "labelText"); StringProperty model = new SimpleStringProperty(this, "model"); ObservableValue<String> expression = model.map(x -> x + "%"); expression.addListener((obs, old, current) -> labelText.set(current)); test.setAsReferenced(labelText); test.setAsReferenced(model); test.assertNotCollectable(expression); // expression is being used, please don't collect }); It contains six tests, using different variations of observing a model and updating a value when it changes: - Using a binding or listener - Observe a property directly or a derived value - For the derived case, using JavaFX or Fluent Bindings These tests all pass, but test #4 is the odd one out. It is the only one that allows the expression to be garbage collected, which will likely come as a big surprise to the user. Nothing in the docs of `concat` or `addListener` clarifies this behaviour. The documentation of `addListener` mentions it makes a strong reference, but that does not explain why this is happening and may even lead to more confusion as it is not the listener the user added that is the culprit; it is the listener added by `concat` that is being GC'd. Note that in these tests this seems relatively innocent, but in a real application the listener added on expression will stop working randomly and the label stops getting updates. Consider if it was written as: model.concat("%").addListener((obs, old, current) -> LOGGER.info("Download progress is at: " + current)); The above would break after a while, while fluent versions would keep working: model.map(x -> x + "%").addListener((obs, old, current) -> LOGGER.info("Download progress is at: " + current)); model.map(x -> "Download progres is at: " + x + "%").addListener((obs, old, current) -> LOGGER.info(current)); Also note that the below version (listening directly on `model`) also keeps working: model.addListener((obs, old, current) -> LOGGER.info("Download progress is at: " + current + "%")); As we'd expect fluent mappings to be used much more in normal code, the difference in behaviours between an unmapped value and a mapped value IMHO becomes hard to justify. A simple code change where the value is defined (by adding a mapping to it) could lead to existing listeners to stop working: ObservableValue<String> progressProperty() { return directProperty; } vs: ObservableValue<String> progressProperty() { return directProperty.concat("%"); // dangerous change! } vs: ObservableValue<String> progressProperty() { return directProperty.map(x -> x + "%"); // safe change } Coming back to your test; I agree that it is strange behaviour when compared to the `Bindings#select` case. But the strange behaviour is IMHO caused by JavaFX's reliance on a rather unpredictable mechanism in the first place: weak references and their interaction with the GC. A listener "stub" is actually left behind after `otherProperty` is GC'd, which unfortunately cannot be cleaned up immediately as the GC has no callback mechanism to notify you when a weak reference was collected (aside from polling a `ReferenceQueue`). Instead, JavaFX is relying here on a later invalidation to clean up this stub -- if an invalidation never occurs, JavaFX leaks these stubs: // Sample program that shows stub leakage: public static void main(String[] args) throws InterruptedException { StringProperty x = new SimpleStringProperty(); // Startup Thread.sleep(20000); for(int i = 0; i < 10000000; i++) { new SimpleStringProperty().bind(x); } // Bindings added System.gc(); // 400 MB of uncollectable stubs does not get GC'd Thread.sleep(20000); // Triggers "stub" clean-up (which fails miserably because of lots of array copying) x.set("A"); System.gc(); Thread.sleep(20000); }  All this interacts with how the fluent bindings are implemented. They observe their source when they themselves are observed. This is done using normal listeners (not weak listeners) to make sure there are no surprises for users (relying on GC is IMHO always surprising behaviour). If they were to use weak listeners, test cases like #6 and the `LOGGER` examples would fail. ------------- PR: https://git.openjdk.org/jfx/pull/675
