On Thu, 29 Sep 2022 15:02:13 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 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 I personally think that it's fine to include the changes to `Node` here. It adds a good amount of value to the new method and can help avoid many memory leaks with this combination. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1415: > 1413: * > 1414: * <p>Note that the {@code Node} does not need to be visible for > this property > 1415: * to be {@code true}. I think that the definition of "shown" is important enough to make it the main focus of the description with something like: Indicates whether or not this {@code Node} is shown. A node is considered shown if it's part of a {@code Window} whose {@link shown #Window::showingProperty} is {@code true}. The {@link visibility #visibleProperty} of the node or its scene do not affect this property. I think that there's no need to repeat the showing conditions for a window. Also we need to be careful not to confuse `shown` with `showing`, which are properties on `ComboBox`, `ChoiceBox` and others. modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1417: > 1415: * to be {@code true}. > 1416: * > 1417: * @defaultValue false I don't think a default value is relevant. It can't be set and the node is not responsible for its value. It's true, though, that by default, since the node does not belong to a window, it's `false`, but I don't know how helpful it is. ------------- Changes requested by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/830
