On Fri, 15 Jul 2022 09:27:00 GMT, John Hendrikx <jhendrikx at 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