8290040: Provide simplified deterministic way to manage listeners

[hjohn]

trafficstars

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.

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] 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

Issue

• JDK-8290040: Provide simplified deterministic way to manage listeners

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[bot]]

:wave: 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[bot]]

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[bot]]

@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[bot]]

@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).

[mlbridge[bot]]

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:

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

[andy-goryachev-oracle]

I am late to the party (should have commented in #675) - "Subscription" is way too generic and overloaded word for a functional interface. Any one who is using real Subscriptions will get confused ;-)

Probably too late to change now.

[hjohn]

I am late to the party (should have commented in #675) - "Subscription" is way too generic and overloaded word for a functional interface. Any one who is using real Subscriptions will get confused ;-)

Probably too late to change now.

No, you're not too late. The Subscription class is already part of JFX 19, but it is not public.

Subscription was the term using in ReactFX and similar frameworks, and was quite clear, but if you have a suggestion for a name that is even better suited than it can still be changed.

[andy-goryachev-oracle]

I would expect to get some items (messages, real time streams) from a Subscription. Even your javadoc for unsubscribe() says "Cancels this subscription."

In this case, something along the lines of ICancellable.cancel() might be better, or

IDisconnectable.disconnect() as in https://github.com/openjdk/jfx/blob/af77693c4c7e778f27f03063bb0256b579de3bb9/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/IDisconnectable.java

[hjohn]

I made another image that shows what happens when you have a Label that is listening on a long lived property that is protected by using a when(label::shownProperty). The reference in the green circle disappears once the label is not part of a scene anymore that is part of a window that is currently showing:

[andy-goryachev-oracle]

Poll: would you rather name the method when or onCondition ?

[hjohn]

I think the most important open point for this PR is the name for the new operation to be added to ObservableValue. Currently in this PR it is called when.

Some background first to help with the decision; all fluent methods follow this basic pattern:

 source.operation( ... ) -> new target observable

The proposed operation has a ObservabeValue<Boolean> parameter as its condition. It allows you to create a new observable that has the same value as its source, unless the associated condition is false, in which case it contains the last seen value of the source (it does not change to null or empty, so an orElse cannot be used to change the value when the condition is false.).

The proposed operation differs from the other three available fluent methods (map, flatMap and orElse) that in order to determine its result it does not need to continuously observe its source -- the source value has no bearing on whether the target will or will not reflect the source value, only the condition controls this. This is unlike map for example, which must evaluate the source value to determine the target value, and also unlike a filter method (as seen in Optional or Stream) which must evaluate the source value to see if it passes the filter predicate in order to determine whether the target uses the source value or becomes empty.

Because the source value is not needed to reach a decision for this new proposed operation, there is no need to constantly observe the source value for changes. When the conditional is false, the observer can be removed until such time that the conditional becomes true. This is in contrast to map, orElse or a potential future filter operation which have no choice but to observe their source at all times in order to correctly calculate their target value.

There is also a clear intention that the condition changes at some point in the future and that it need not be a one time change (when condition becomes true again, the target again starts tracking the source value); if the condition never changes there is no need to use this new operation as one can just use the source observable (if condition is always true) or it becomes a constant value (if condition is always false).

A quick overview of the differences between the existing and new operation:

Time	x	condition	x.map(x -> x * 2)	x.filter(x -> x % 2 == 0)	x.operation(condition)
0	2	true	4	2	2
1	2	false	4	2	2
2	3	false	6	null (empty)	2
3	3	true	6	null (empty)	3
4	4	true	8	4	4

Options for the name of this new operation

The full signature for the newly proposed operation is:

    ObservableValue<T> __operation__(ObservableValue<Boolean> condition)

Possible options:

1. while, if
2. conditionOn (from ReactFX)
3. skipUntil + takeWhile or skipWhile + takeUntil (from ReactiveX)
4. When variations: when, updateWhen, whenever

while and if

Although while would fit the bill reasonably nice ("target becomes source value while condition (is true)"), it is a reserved keyword. while does have a time component to it, but does not so nicely communicate that the condition may become true again; one would have to clarify this with "target becomes source value whenever the while condition (is true)".

if is also a reserved keyword, but does a slightly better job than while: "target becomes source value if condition (is true)". It does however not communicate that the condition is likely to change in the future, which is the whole point of this new operation.

conditionOn

ReactFX uses this term for the proposed operation. In english it could perhaps be formulated as: "target becomes source value on the condition that condition (is true)". This name seems a bit superfluous. The parameter of the proposed operation is already called condition and other similar methods don't re-iterate this fact in their naming either.

skipUntil + takeWhile or skipWhile + takeUntil

In ReactiveX there is a similar operation for dealing with its streams. First note, there is an important difference between ObservableValue and ReactiveX's Observable: an ObservableValue always has a current value that can be queried and operated upon at any time. An Observable in ReactiveX has no current value, and when applying operations to its streams, these operations can only be applied if the source observable emits an event. ReactiveX is closer match for Java's Stream while ObservableValue is a closer match to Java's Optional.

The operations allowed by ReactiveX cannot be used to achieve a similar effect as the proposed method by combining them (x.skipUntil(c).takeWhile(c)). This is because the operations allow one condition change only, which is pretty clear from their names. A better name that would match our operation would be takeWhenever(c) which is another variation of using "when".

When variations: when, updateWhen, whenever

Earlier, if proved to be quite a good match: "target becomes source value if condition (is true)", but did not communicate that well that the condition is almost certain to change in the future (as using the new operation would be pointless otherwise). In English, "when" distinguishes itself from "if" for something that is almost certain to happen at some point in the future.

All of these are a good match. In the context of creating observables that are bound to each other (and thus update on changes of their source), there is little need to reiterate this in the name; all of the fluent methods will update their target in some fashion. In English, whenever is used for repeated occurrences, while when can mean a single occurrence (but doesn't preclude it AFAIK). This mean I think that both of these would be great matches. I would be inclined to go for the more compact when.

This has been a long story; I hope we can reach a consensus on a name, perhaps people can respond with a ranked set of preferences.

[andy-goryachev-oracle]

I am not a native speaker, but to me whenever sounds event better than conditionOn. I might be wrong, but it does suggest that the binding becomes active again when the condition turns true.

[hjohn]

As per the discussion on the mailing list (https://mail.openjdk.org/pipermail/openjfx-dev/2022-November/036707.html) we've decided to go for when as the name for the new method in ObservableValue.

I've created a CSR now: https://bugs.openjdk.org/browse/JDK-8297719

[hjohn]

@andy-goryachev-oracle @mstr2 would one of you be willing to review this one as well? It should be close to its final state now.

[kevinrushforth]

The API and docs for ObservableValue::when look like they are in good shape, so I'll review them next week.

My only overall comment is that this PR should be limited to the new API, implementation, and tests in javafx.base. Although it's helpful to review them together, the changes to Node to add a new shown property is an interesting use case that should stand on its own merits and should be done as a follow-on Enhancement in a separate PR.

[hjohn]

The API and docs for ObservableValue::when look like they are in good shape, so I'll review them next week.

My only overall comment is that this PR should be limited to the new API, implementation, and tests in javafx.base. Although it's helpful to review them together, the changes to Node to add a new shown property is an interesting use case that should stand on its own merits and should be done as a follow-on Enhancement in a separate PR.

Okay, that would mean removing the examples from when that use the shownProperty, and I suppose adding them back in later. The ticket was specifically worded to make it easier to deterministically manage listeners, which at least for UI controls, the shown property is an important part of.

If you think its realistic, I can file a new ticket, split it off so hopefully both can be part of the same release?

[mstr2]

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)

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.

[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 https://github.com/openjdk/jfx/pull/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.

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

[openjdk[bot]]

@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[bot]]

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

[nlisker]

/sponsor

[openjdk[bot]]

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

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

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@nlisker @hjohn Pushed as commit adfc022060087490d0a620f38104a826411820ae.

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