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

[Andy Goryachev]

On Thu, 29 Sep 2022 15:02:13 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 with a new target base due to a merge or a rebase. The pull request now contains ten commits:
> 
>  - Merge remote-tracking branch 'origin/master' into
>    feature/conditional-bindings
>    
>    # Conflicts:
>    #	modules/javafx.base/src/test/java/test/javafx/beans/value/LazyObjectBindingTest.java
>  - Fix review comments
>  - Add missing new line
>  - Small wording fixes
>  - Update javadoc of "when" with better phrasing
>  - Rename showing property to shown as it is already used in subclasses
>  - Add conditional bindings to ObservableValue
>  - Change explanatory comment to block comment
>  - Fix bug where ObjectBinding returns null when revalidated immediately
>    
>    Introduced with the fluent bindings PR

Changes requested by angorya (Author).

modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 1162:

> 1160:                 }
> 1161:             }
> 1162:         }

Is it possible to add the scenario that simulates configurations you so beautifully illustrated in the comments in this PR?  For example, simulate the removal of the "Skin" (without involving a real Skin implementation, just re-create the classes and properties in the same configuration)?

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

> 1418:      * @since 20
> 1419:      */
> 1420:     private ReadOnlyBooleanProperty shown;

I would recommend against adding this property, as it adds a pointer to every Node regardless of whether it's needed or not.

Another reason not to add is that, at least in my experience, there is rarely a need to base the logic on whether a particular Node is shown or not - instead, the logic to disconnect (one or more) listeners should be based on a Window/Dialog or a container pane.  And for that I think a better solution might be - sorry for self plug - something like ListenerHelper https://github.com/openjdk/jfx/pull/908

And another thing - Window and Dialog are not Nodes, but they do have `showing` property.

I would suggest either remove the changes pertaining to Node, or perhaps provide a utility method to create the property and store it in Node.getProperties() or Window.getProperties() (sadly, there is no Dialog.getProperties())

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

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