PoC: Fluent bindings for ObservableValue

[hjohn]

This is a proof of concept of how fluent bindings could be introduced to JavaFX. The main benefit of fluent bindings are ease of use, type safety and less surprises. Features:

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: #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 or orElseGet.

Conditional Bindings
Bindings can be made conditional using the conditionOn method. A conditional binding retains its last value when its condition is false. Conditional bindings donot observe their source when the condition is false, allowing developers to automatically stop listening to properties when a certain condition is met. A major use of this feature is to have UI components that need to keep models updated which may outlive the UI conditionally update the long lived model only when the UI is showing.

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")
  );
}

void updateLongLivedModelWhileAvoidingMemoryLeaks() {
  // Standard JavaFX: naive, memory leak; UI won't get garbage collected
  listView.getSelectionModel().selectedItemProperty().addListener(
    (obs, old, current) -> longLivedModel.lastSelectedProperty().set(current)
  );

  // Standard JavaFX: no leak, but stops updating after a while
  listView.getSelectionModel().selectedItemProperty().addListener(
    new WeakChangeListener<>(
      (obs, old, current) -> longLivedModel.lastSelectedProperty().set(current)
    )
  );

  // Standard JavaFX: fixed version
  listenerReference = (obs, old, current) -> longLivedModel.lastSelectedProperty().set(current);

  listView.getSelectionModel().selectedItemProperty().addListener(
    new WeakChangeListener<>(listenerReference)
  );

  // Fluent: naive, memory leak... fluent won't solve this...
  listView.getSelectionModel().selectedItemProperty()
      .subscribe(longLivedModel.lastSelectedProperty()::set);

  // Fluent: conditional update when control visible

  // Create a property which is only true when the UI is visible:
  ObservableValue<Boolean> showing = listView.sceneProperty()
      .flatMap(Scene::windowProperty)
      .flatMap(Window::showingProperty)
      .orElse(false);

  // Use showing property to automatically disconnect long lived model
  // allowing garbage collection of the UI:
  listView.getSelectionModel().selectedItemProperty()
    .conditionOn(showing)
    .subscribe(longLivedModel.lastSelectedProperty()::set);

  // Note that the 'showing' property can be provided in multiple ways:
  // - create manually (can be re-used for multiple bindings though)
  // - create with a helper: Nodes.showing(Node node) -> ObservableValue<Boolean>
  // - make it part of the Node class; as the fluent bindings only bind themselves
  //   to their source when needed (lazy binding), this won't create overhead
  //   for each node in the scene
}

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.

I've created tests for this as well, but they're written in JUnit 5, which I noticed is not yet part of OpenJFX. A screenshot:

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/434/head:pull/434
$ git checkout pull/434

To update a local copy of the PR:
$ git checkout pull/434
$ git pull https://git.openjdk.java.net/jfx pull/434/head

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[tbee]

I like this!

About the memory leaks; in Angular it is common that (the equivalent of) an addListener returns a subscription object, which can be closed when the enclosing service or component closes. This will break the strong reference and thus solve the memory leak. Is it an idea to do something similar here? AddListener is void, so it should be possible to have it return a Subscription object without breaking the API. The subscription object knows the exact listener, so it only removes that one.

Of course that solution has the drawback in that you need to maintain an administration so you know what to close. Either a bunch of instance variables or a list, in Angular there is some other trickery but that is technology specific. It all comes down to that you can mark something to need closing at the point of subscribing. The best would be to bind that to an onClose event of the containing object, maybe a little helper method would be good, something like.

this.cascadeClose( listView.getSelectionModel().selectedItemProperty().addListener(
(obs, old, current) -> longLivedModel.lastSelectedProperty().set(current)
));

Or:
listView.getSelectionModel().selectedItemProperty().addListener(
(obs, old, current) -> longLivedModel.lastSelectedProperty().set(current)
).closeWith(this);

[hjohn]

There were `subscribe` methods added which return a `Subscription`. I didn't change the addListener methods as I want to keep the changes to existing code minimal to make this easier to review and perhaps get accepted. The returned subscriptions can be "and"-ed together to make a single subscription which cancels all of them at once. I didn't extend this yet with more API, but ReactFX has many good ideas in that area. For memory leaks however, I really think the `conditionOn` feature that was also in ReactFX is one of the best ways. You can create bindings and subscriptions that way which automatically stop observing when parts of the UI are removed or closed, which then can be garbage collected. This is basically how I do all my memory management now, and rarely do I need to actually remove a listener explicitely. Your `closeWith` idea is similar to `conditionOn`, since it will automatically unsubscribe when a condition is false. You could tie it to the visible property for example, but also to a single boolean property that is set to false when you need all the listeners to stop. A more convenient method could be added (like `conditionOnShowing` which directly accepts a `Node`), or `Node` could get a `showingProperty()`.

…

On 25/03/2021 07:42, Tom Eugelink wrote: I like this! About the memory leaks; in Angular it is common that (the equivalent of) an addListener returns a subscription object, which can be closed when the enclosing service or component closes. This will break the strong reference and thus solve the memory leak. Is it an idea to do something similar here? AddListener is void, so it should be possible to have it return a Subscription object without breaking the API. The subscription object knows the exact listener, so it only removes that one. Of course that solution has the drawback in that you need to maintain an administration so you know what to close. Either a bunch of instance variables or a list, in Angular there is some other trickery but that is technology specific. It all comes down to that you can mark something to need closing at the point of subscribing. The best would be to bind that to an onClose event of the containing object, maybe a little helper method would be good, something like. this.*cascadeClose*( listView.getSelectionModel().selectedItemProperty().addListener( (obs, old, current) -> longLivedModel.lastSelectedProperty().set(current) )); Or: listView.getSelectionModel().selectedItemProperty().addListener( (obs, old, current) -> longLivedModel.lastSelectedProperty().set(current) ).*closeWith*(this); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#434 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHTETMXKRBWNTXSQZLFAV3TFLLL5ANCNFSM4ZP6W6JA>.

[tomsontom]

Changing addListener from void to something different would be binary incompatible change!

[tbee]

Changing addListener from void to something different would be binary incompatible change!

I know, but adding a new method seems like an even worse idea, polluting the API more.

[mstr2]

I really like this idea. But even after a few days of looking at it, I still struggle with the "flatMap" operator. While I understand why you named it like that, it looks a bit foreign to me when it's used to select nested properties. Have you considered using "select" instead, which already has precedence in JavaFX?

Is it possible to have something like an "orDefault" operator, which would be equivalent to "orElse" but with an implicit default value.

[hjohn]

I really like this idea. But even after a few days of looking at it, I still struggle with the "flatMap" operator. While I understand why you named it like that, it looks a bit foreign to me when it's used to select nested properties. Have you considered using "select" instead, which already has precedence in JavaFX?

The name is basically taken from ReactFX, Optional and Stream and is basically short-hand syntax for map(x -> x.someProperty()).map(ObservableValue::getValue). Using select is a valid option to consider. If map and flatMap weren't null-safe, I would definitely have liked to add another method (perhaps select) which saves you the hassle of having to deal with null when selecting properties.

Is it possible to have something like an "orDefault" operator, which would be equivalent to "orElse" but with an implicit default value.

Perhaps yes, but as the PoC doesn't have specific implementations for primitive types it may be a bit tricky to determine what the default should be as I think there is no type information to base this on. I assume you'd want it to be 0 or false for primitive types, and null for everything else.

[tbee]

To me orDefault doesn't give me warm feelings, because it is a bit magical. I'd rather type orElse(0), then know for sure what its value is. I personally also never rely on defaults for any variable, even primitives, I always initialize them.

[mstr2]

To me orDefault doesn't give me warm feelings, because it is a bit magical. I'd rather type orElse(0), then know for sure what its value is. I personally also never rely on defaults for any variable, even primitives, I always initialize them.

That seems like a particular personal preference. Having sensible defaults is a large part of what makes Java as great a language as it is.

[tbee]

Given my preference on initializing everything, it is obvious I have a different opinion on that matter :-)

To me orDefault doesn't give me warm feelings, because it is a bit magical. I'd rather type orElse(0), then know for sure what its value is. I personally also never rely on defaults for any variable, even primitives, I always initialize them.

That seems like a particular personal preference. Having sensible defaults is a large part of what makes Java as great a language as it is.

And given my preference on initializing everything, it is obvious I have a different opinion :-)
But no one is forced to use orDefault, so if it makes people happy...

[tobiasdiez]

I really like this PR and the concepts it proposes. In fact, I'm maintaining a small library (originally due to @TomasMikula) that implements many of these features: https://github.com/tobiasdiez/EasyBind. Please use it as a source of inspiration and code as you see fit.

There are a few notable differences in EasyBind:

• Handling of null values: Sometimes you do want to have explicit control of null values in your mapper. Thus, with EasyBind, we simple apply the mapper to the current value, even if that's null. On the other hand, we introduced ObservableOptionalValue as a convenient wrapper around ObservableValue<Optional<T>> to handle null values similar to Optional.
• For mapping, there are two use cases: Either you have a simple mapper as in text.map(String:toUppercase), or sometimes the mapper returns an observable value itself, which needs to be unpackaged.
• For selection / flatmap chain, you often want to get a bona-fide property back for which you can also set values (and not only use it as the source for a binding).

Maybe it's a good idea to split this PR into smaller ones that only introduce a particular concept (i.e. separate mapper, listener, selection etc).

[hjohn]

On 20/08/2021 10:14, Tobias Diez wrote: I really like this PR and the concepts it proposes. In fact, I'm maintaining a small library (originally due to @TomasMikula <https://github.com/TomasMikula>) that implements many of these features: https://github.com/tobiasdiez/EasyBind. Please use it as a source of inspiration and code as you see fit. There are a few notable differences in EasyBind: * Handling of null values: Sometimes you do want to have explicit control of null values in your mapper. Thus, with EasyBind, we simple apply the mapper to the current value, even if that's null. On the other hand, we introduced |ObservableOptionalValue| as a convenient wrapper around |ObservableValue<Optional<T>>| to handle null values similar to |Optional|.

Yes, this is probably one of the more controversial choices in the proof of concept. In this case the functionality of Optional was directly integrated to make it easier to do map/flatMap chains without having to deal with null. The way EasyBind handles this can certainly be considered as well. I also considered providing a different `map` function that does allow `null`, but felt that it would be hard to think of a good name for it (`mapNullable`?) and it would just offer a second option for something that was already possible.

* For mapping, there are two use cases: Either you have a simple mapper as in |text.map(String:toUppercase)|, or sometimes the mapper returns an observable value itself, which needs to be unpackaged.

I'm not sure I understand, but I think that's what flatMap does in the PoC. It takes a function which returns an ObservableValue. Are you suggesting 'map' should unpackage the value if it happens to be an instance of ObservableValue?

* For selection / flatmap chain, you often want to get a bona-fide property back for which you can also set values (and not only use it as the source for a binding).

Do you have an example case for where this is convenient? I generally only used the most basic functionality of ReactFX, and haven't encountered a scenario where I'd need this. How does this deal with chains that don't result in a property (because a property in the chain is null)?

Maybe it's a good idea to split this PR into smaller ones that only introduce a particular concept (i.e. separate mapper, listener, selection etc).

Yes, I tried to keep it very small (it is only 400 lines) but it can be split further, or at least, not all API needs to be public right away as Nir Lisker also suggested. I've actually been using this PoC for a while now using a simple Maven trick to extend ObservableValue and ObjectBinding without actually having to build JavaFX (the Maven project depends on openjfx, but overlaps the packages with those classes and creates an artifact with those classes replaced). Depending on this new Maven artifact just works with full code completion for the new methods. I'm willing to put all the necessary work in to make this a success, but would need some guidance as to what is needed to push this forward, if it is currently considered at all.

…

--John

[tbee]

On 20/08/2021 10:14, Tobias Diez wrote: > * Handling of null values: Sometimes you do want to have explicit > control of null values in your mapper. Thus, with EasyBind, we > simple apply the mapper to the current value, even if that's null. > On the other hand, we introduced |ObservableOptionalValue| as a > convenient wrapper around |ObservableValue<Optional<T>>| to handle > null values similar to |Optional|. Yes, this is probably one of the more controversial choices in the proof of concept. In this case the functionality of Optional was directly integrated to make it easier to do map/flatMap chains without having to deal with null. The way EasyBind handles this can certainly be considered as well. I also considered providing a different `map` function that does allow `null`, but felt that it would be hard to think of a good name for it (`mapNullable`?) and it would just offer a second option for something that was already possible.

Just brain storming here, but would it be possible to make handling null an option on a stream that can be set to true? .allowNull(true) After which map and all allow null without an api change? Maybe by returning a different implementation when calling the map() etc methods. Possibly even temporarily; .allowNull(true) .map(canBeNull -> ...) .allowNull(false) .map(cannotBeNull -> ...)

[nlisker]

@hjohn I think that this can be closed now :)

[hjohn]

Right :)