RFR: 8290040: Provide simplified deterministic way to manage listeners [v9]

[Michael Strauß]

On Thu, 1 Dec 2022 17:38:01 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:
> 
>   Adjust Node
>   
>   - Fixed javadoc
>   - Added comment for code that avoid eager instantiation
>   - Changed `isShown` to use property if it is available

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1413:

> 1411:      * Indicates whether or not this {@code Node} is shown. A node is considered shown if it's
> 1412:      * part of a {@code Scene} that is part of a {@code Window} whose
> 1413:      * {@link Window#showingProperty showing property} is {@code true}. The {@link Node#visibleProperty visibility}

I think you should choose one of the following:
`...whose {@link Window#showingProperty} is...`
`...whose {@link Window#showingProperty showing} property is...`

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1420:

> 1418:      * <p>
> 1419:      * This property can also be used to start animations when the node is shown, and to stop them
> 1420:      * when it is no longer shown.

This sentence sounds like `shownProperty()` is quite deeply connected to animations, which it isn't. Maybe you can clarify with "For example, ..."?

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1437:

> 1435:     }
> 1436: 
> 1437:     public final ReadOnlyBooleanProperty shownProperty() {

This property probably should have been named `showing` to align it with `Window.showing`, but the "good name" is already taken by `ComboBox` and other controls. Just out of interest, have you considered alternatives to `shown`?

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1443:

> 1441:                 .flatMap(Window::showingProperty);  // note: the null case is handled by the delegate with an orElse(false)
> 1442: 
> 1443:             shown = new ReadOnlyBooleanDelegate(Node.this, "shown", ov);

`shownProperty()` might be used quite a lot, have you considered using a specialized implementation that doesn't need several intermediate observable values?

modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1453:

> 1451:         private final ObservableValue<Boolean> delegate;
> 1452:         private final Object bean;
> 1453:         private final String name;

You could omit the `bean` and `name` fields by making this a non-static inner class and returning `Node.this` and `"shown"` directly from `getBean()` and `getName()` respectively. In that case, you could also make this class an anonymous implementation of `ReadOnlyBooleanProperty`. This pattern is used quite extensively in JavaFX, since it can potentially save a few bytes of memory.

-------------

PR: https://git.openjdk.org/jfx/pull/830