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 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`. ------------- PR: https://git.openjdk.org/jfx/pull/830
