On Thu, 1 Dec 2022 17:38:01 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: > > Adjust Node > > - Fixed javadoc > - Added comment for code that avoid eager instantiation > - Changed `isShown` to use property if it is available modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1413: > 1411: * Indicates whether or not this {@code Node} is shown. A node is > considered shown if it's > 1412: * part of a {@code Scene} that is part of a {@code Window} whose > 1413: * {@link Window#showingProperty showing property} is {@code true}. > The {@link Node#visibleProperty visibility} I think you should choose one of the following: `...whose {@link Window#showingProperty} is...` `...whose {@link Window#showingProperty showing} property is...` modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1420: > 1418: * <p> > 1419: * This property can also be used to start animations when the node > is shown, and to stop them > 1420: * when it is no longer shown. This sentence sounds like `shownProperty()` is quite deeply connected to animations, which it isn't. Maybe you can clarify with "For example, ..."? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1437: > 1435: } > 1436: > 1437: public final ReadOnlyBooleanProperty shownProperty() { This property probably should have been named `showing` to align it with `Window.showing`, but the "good name" is already taken by `ComboBox` and other controls. Just out of interest, have you considered alternatives to `shown`? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1443: > 1441: .flatMap(Window::showingProperty); // note: the null > case is handled by the delegate with an orElse(false) > 1442: > 1443: shown = new ReadOnlyBooleanDelegate(Node.this, "shown", ov); `shownProperty()` might be used quite a lot, have you considered using a specialized implementation that doesn't need several intermediate observable values? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1453: > 1451: private final ObservableValue<Boolean> delegate; > 1452: private final Object bean; > 1453: private final String name; You could omit the `bean` and `name` fields by making this a non-static inner class and returning `Node.this` and `"shown"` directly from `getBean()` and `getName()` respectively. In that case, you could also make this class an anonymous implementation of `ReadOnlyBooleanProperty`. This pattern is used quite extensively in JavaFX, since it can potentially save a few bytes of memory. ------------- PR: https://git.openjdk.org/jfx/pull/830
