On Fri, 2 Dec 2022 09:56:42 GMT, John Hendrikx <jhendr...@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: > > Improve wording in javadoc and comments > 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 > > ```java > 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: > > ```java > 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. ------------- PR: https://git.openjdk.org/jfx/pull/830
