On Fri, 15 Jul 2022 09:27:00 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> This PR adds a new (lazy*) property on `Node` which provides a boolean which >> indicates whether or not the `Node` is currently part of a `Scene`, which in >> turn is part of a currently showing `Window`. >> >> It also adds a new fluent binding method on `ObservableValue` dubbed `when` >> (open for discussion, originally I had `conditionOn` here). >> >> Both of these together means it becomes much easier to break strong >> references that prevent garbage collection between a long lived property and >> one that should be shorter lived. A good example is when a `Label` is bound >> to a long lived property: >> >> >> label.textProperty().bind(longLivedProperty.when(label::isShowingProperty)); >> >> The above basically ties the life cycle of the label to the long lived >> property **only** when the label is currently showing. When it is not >> showing, the label can be eligible for GC as the listener on >> `longLivedProperty` is removed when the condition provided by >> `label::isShowingProperty` is `false`. A big advantage is that these >> listeners stop observing the long lived property **immediately** when the >> label is no longer showing, in contrast to weak bindings which may keep >> observing the long lived property (and updating the label, and triggering >> its listeners in turn) until the next GC comes along. >> >> The issue in JBS also describes making the `Subscription` API public, but I >> think that might best be a separate PR. >> >> Note that this PR contains a bugfix in `ObjectBinding` for which there is >> another open PR: https://github.com/openjdk/jfx/pull/829 -- this is because >> the tests for the newly added method would fail otherwise; once it has been >> integrated to jfx19 and then to master, I can take the fix out. >> >> (*) Lazy means here that the property won't be creating any listeners unless >> observed itself, to avoid problems creating too many listeners on >> Scene/Window. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Rename showing property to shown as it is already used in subclasses I reviewed the docs so a CSR can be submitted. Will do the implementation and tests reviews later. The implementation looks fine, but I need to review the subscription model closely. modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 258: > 256: /** > 257: * Returns an {@code ObservableValue} that holds this value whenever > the given > 258: * condition evaluates to {@code true}, otherwise holds the last > value when > evaluates to Maybe "holds"? modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 259: > 257: * Returns an {@code ObservableValue} that holds this value whenever > the given > 258: * condition evaluates to {@code true}, otherwise holds the last > value when > 259: * {@code condition} became {@code false}. The value is updated > whenever this > became `{@code false}` If it was `false` from the start then the observable still holds the initial value. Maybe "was `{@code false}`"? modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 260: > 258: * condition evaluates to {@code true}, otherwise holds the last > value when > 259: * {@code condition} became {@code false}. The value is updated > whenever this > 260: * {@code ObservableValue} changes, unless the condition currently > evaluates Maybe it's a bit clearer to indicate when the value changes rather than when it doesn't: > The value is updated whenever this `{@code ObservableValue}` changes and > `{@code condition}` holds `{@code true}` modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 266: > 264: * {@code condition} evaluates to {@code true}. This allows this > {@code ObservableValue} > 265: * and the conditional {@code ObservableValue} to be garbage > collected if neither is > 266: * otherwise strongly referenced when {@code condition} becomes > {@code false}. What happens if they are GC'd and the conditional becomes `true` later? modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 269: > 267: * <p> > 268: * A currently observed binding will observe its source, which means > it will not be eligible > 269: * for garbage collection while source isn't. However, using {@code > when} this {@code ObservableValue} "while source isn't" -> "while **its** source" "However, using `{@code when}` this `{@code ObservableValue}`" -> "However, when using `{@code when}`, this `{@code ObservableValue}`" modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 271: > 269: * for garbage collection while source isn't. However, using {@code > when} this {@code ObservableValue} > 270: * can still be eligible for garbage collection when the condition > is {@code false} and the > 271: * conditional itself is also eligible for garbage collection. Wasn't this explained in the previous paragraph? modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 274: > 272: * <p> > 273: * Returning {@code null} from the given condition is treated the > same as > 274: * returning {@code false}. I would rephrase to use the "holds" language instead of the "return": > `{@code condition}` holding `{@code null}` is treated the same as it holding > {@code false}` But is this the behavior we want? I would consider throwing when the condition is not a `true` or `false` because it's a boolean condition. `orElse` can be used to create a condition that maps `null` to something else if the user wants to accept `null`s in the condition. I'm not sure if this is really a problem. modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 302: > 300: * ObservableValue<String> globalProperty = new > SimpleStringProperty("A"); > 301: * > 302: * // bind label's text to a global property only when it is shown: "a label's" modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java line 302: > 300: * ObservableValue<String> globalProperty = new > SimpleStringProperty("A"); > 301: * > 302: * // bind label's text to a global property only when it is shown: I would move the comment explanation outside: An example for binding a label's text to a global property only when it is shown: ... Label label = ... ; ObservableValue<String> globalProperty = new SimpleStringProperty("A"); label.textProperty().bind(globalProperty.when(label::isShownProperty)); ... ------------- Changes requested by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/830
