8290040: Provide simplified deterministic way to manage listeners

[hjohn]

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

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)
• Change requires a CSR request to be approved

Issues

• JDK-8290040: Provide simplified deterministic way to manage listeners
• JDK-8297719: Provide simplified deterministic way to manage listeners (CSR)

Reviewers

• Nir Lisker (@nlisker - Reviewer)
• Andy Goryachev (@andy-goryachev-oracle - Committer)
• Kevin Rushforth (@kevinrushforth - Reviewer)
• Michael Strauß (@mstr2 - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/830/head:pull/830
$ git checkout pull/830

Update a local copy of the PR:
$ git checkout pull/830
$ git pull https://git.openjdk.org/jfx pull/830/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 830

View PR using the GUI difftool:
$ git pr show -t 830

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/830.diff

[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.

[mlbridge]

Webrevs

• 13: Full - Incremental (7318af2b)
• 12: Full (24872f09)
• 11: Full - Incremental (75ccca6b)
• 10: Full - Incremental (94b50b5d)
• 09: Full - Incremental (67c277cb)
• 08: Full - Incremental (5c22fdd0)
• 07: Full - Incremental (46f206aa)
• 06: Full - Incremental (ea546998)
• 05: Full (4fa45c76)
• 04: Full - Incremental (23538520)
• 03: Full - Incremental (13c6e39a)
• 02: Full - Incremental (07e9d88a)
• 01: Full - Incremental (fd19fc36)
• 00: Full (a70161cb)

[kevinrushforth]

/csr
/reviewers 3

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@hjohn please create a CSR request for issue JDK-8290040 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

[nlisker]

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.

[mlbridge]

Mailing list message from John Hendrikx on openjfx-dev:

Please ignore the explanation below it is incorrect (I forgot that
conditional and new observable reference each other).

I've updated the comment on Github, not sure if these edits will show on
the mailinglist.

--John

On 01/09/2022 15:14, John Hendrikx wrote:

[nlisker]

The docs of when look good now (I left one minor comment). I actually didn't think about the name of the method, compared to ReactFX's conditionOn for example, but this is good enough until others review.

I haven't looked at the addition to Node closely yet. I will look at the implementation and tests of the method itself first because Node is a use case.

[nlisker]

Implementation and tests look good. Left a few minor comments.

in ObjectBinding.java, the change in a previous PR was integrated. I think that you need to merge master to not show this as a change in this PR.

I will have a look at the changes to Node soon. I'm not sure if they need to be in this PR, but I personally don't mind. Will also do some sanity checks manually to see that the binding functions the way I think it should (as I have done in the previous fluent binding PR), but I don't foresee any issues.

[hjohn]

Implementation and tests look good. Left a few minor comments.

in ObjectBinding.java, the change in a previous PR was integrated. I think that you need to merge master to not show this as a change in this PR.

I've merged in master.

I will have a look at the changes to Node soon. I'm not sure if they need to be in this PR, but I personally don't mind. Will also do some sanity checks manually to see that the binding functions the way I think it should (as I have done in the previous fluent binding PR), but I don't foresee any issues.

It would be good to have changes in Node as well as they help with the goal of this issue (deterministic ways to manage listeners), but not required. If we take them out, I'll have to update one of the examples that assumes it is part of this PR.

[hjohn]

I think that ObservableValue.when is good to go, but the case for Node.shown seems less clear.

Applications might want to bind the active scope of an ObservableValue to any number of conditions, for example:

• whether a node is connected to a Scene
• ... that is connected to a window
• ... that is currently showing
• whether a node is currently visible
• ... and it is also not hidden by other controls or outside the viewport
• all of the above combined

The proposed shown property picks one of these semantics and promotes it to a first-class API on Node. But it doesn't add any functionality that can't be provided by libraries or applications, since it's essentially just a convenience method for

sceneProperty()
    .flatMap(Scene::windowProperty)
    .flatMap(Window::showingProperty)

In the past, a form of this property already existed as part of Node in the NodeHelper class, which was almost always created. It was not public API and had the name treeShowing. I removed this as part of #185 because it caused big performance issues because it was not a lazy property. I then implemented the required functionality directly in PopupWindow and ProgressIndicatorSkin. The performance problems were not related to the existence of the extra property, but because it was eagerly registering listeners on a many-to-one dependency (all nodes would register on scene and window, causing huge listener lists on the latter two).

As I think the property is still useful (internally as well as publicly) putting it back in a form that won't cause performance issues seemed reasonable. However, good arguments can be made to leave it out.

Currently there are three concepts within Node (all private):

• windowShowing - true when the Node is part of a Scene that is part of a showing Window
• treeVisible - true when the Node is visible and all its parents are visible
• treeShowing (removed) - the combination of windowShowing and treeVisible

windowShowing is not a real property currently, the shown property is basically making it available to all.

treeVisible is a real property, but it's private API part of the NodeHelper instance that is part of Node -- it does not suffer the performance problems that treeShowing had because it only registers on its parent, spreading the load of the listeners it needs (they don't all end up on Scene or Window) -- it may still be worth it to optimize this one though by switching to lazy listeners.

treeShowing as said was the combination of the previous two.

My idea here was to make windowShowing public (as shown), and perhaps later treeVisible. The most interesting options will then no longer be only available for internal use, but for everyone. The combination of the two need not be a separate property. This can be handled with a double when or some kind of and construct:

   property.when(label::shownProperty).when(label::treeVisibleProperty)

The bar for adding convenience methods to Node should be high, and I'm not sure that shown clears the bar. My main objection would be that it's quite easy to add this (and other useful convenience methods) in a way that's not clearly worse, for example using a collection of static methods:

public static class NodeUtil {
    public static ObservableValue<Boolean> shown(Node node) {
        ObservableValue<Boolean> shown = (ObservableValue<Boolean>)node.getProperties().get("shown");
        if (shown == null) {
            shown = node.sceneProperty()
                    .flatMap(Scene::windowProperty)
                    .flatMap(Window::showingProperty);
            node.getProperties().put("shown", shown);
        }
        return shown;
    }

    public static ObservableValue<Boolean> visiblyShown(Node node) {
        ...
    }

    public static ObservableValue<Boolean> visiblyShownOnScreen(Node node) {
        ...
    }   
}

When put into use, it's doesn't look terrible compared to a Node.shown property:

label.textProperty().bind(longLivedProperty.when(label::shownProperty));
label.textProperty().bind(longLivedProperty.when(NodeUtil.shown(label));

This suggests to me that we should discuss the Node additions separately from ObservableValue.when.

I suppose so; my main reason for making it a lot more visible is because of how important I think it is for deterministic listener management in UI's. The reasoning here is that if it is very visible it is discovered more easily, and it can be made good use of in examples to help people do the right thing -- combined with the fact that there apparently is some internal need for such properties as well. Then again, I agree that a helper class is not a too big of a compromise (I'd go for Nodes in the spirit of Bindings because I dislike classes with generic terms like Util or Helper).

It's primary use case is to remove the need for weak listeners; it's not common that people remove listeners when things become invisible or are covered by something else -- when a control is invisible, it's main cost (rendering) is already gone, any updates that would affect its appearance will be deferred anyway until it becomes visible, so removing the listeners in those cases is not that urgent.

So I think listeners are usually only removed when a piece of UI is going to be disposed permanently (this is also how weak listeners work, as they can't work in any other way). The closest I've managed to get to "is going to be disposed" is to use scene.window.isShowing, but even in those cases listeners may be removed too early (although not a problem as they'll be re-added if somehow that piece of UI didn't get GC'd and is shown again later). As the primary use case is to remove the reliance on weak listeners I think the shown property (in whatever form) is what we all need, and cases involving actual visibility are far less important.

[kevinrushforth]

In order to move this forward, please revert the changes in javafx.graphics and file a follow-up Enhancement in JBS that we can use to separately discuss adding such a property to Node.

I also think it might be cleanest to remove the second example from the when API docs as a result.

Once you have reverted the changes in Node, please update the CSR to reflect this. You can then move the CSR to Proposed.

[hjohn]

In order to move this forward, please revert the changes in javafx.graphics and file a follow-up Enhancement in JBS that we can use to separately discuss adding such a property to Node.

I also think it might be cleanest to remove the second example from the when API docs as a result.

Once you have reverted the changes in Node, please update the CSR to reflect this. You can then move the CSR to Proposed.

I updated the CSR to remove all references to the changes in Node, and moved it to proposed.

[kevinrushforth]

The update to revert javafx.graphics looks good. I'll finish reviewing soon. Can you merge in the latest master? Your PR branch is a couple months out of date, and it would be helpful to get a more recent GHA run.

[kevinrushforth]

Looks good. The additional tests for when look to be quite thorough and are very easy to follow. I left one minor formatting comment. I'll reapprove if you fix that.

[andy-goryachev-oracle]

lgtm

[openjdk]

@hjohn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290040: Provide simplified deterministic way to manage listeners

Reviewed-by: nlisker, angorya, kcr, mstrauss

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@nlisker, @andy-goryachev-oracle, @kevinrushforth, @mstr2) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[hjohn]

/integrate

[openjdk]

@hjohn
Your change (at version 7318af2) is now ready to be sponsored by a Committer.

[nlisker]

/sponsor

[openjdk]

Going to push as commit adfc022.
Since your change was applied there has been 1 commit pushed to the master branch:

• f217d5e: 8298200: Clean up raw type warnings in javafx.beans.property.* and com.sun.javafx.property.*

Your commit was automatically rebased without conflicts.

[openjdk]

@nlisker @hjohn Pushed as commit adfc022.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.