On Thu, 22 Sep 2022 11:04:46 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: > > Add missing new line 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 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. modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java line 12: > 10: private final ObservableValue<Boolean> nonNullCondition; > 11: > 12: private Subscription subscription; Isn't it better to use an `EMPTY` subscription, like `FlatMap` does? modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java line 16: > 14: public ConditionalBinding(ObservableValue<T> source, > ObservableValue<Boolean> condition) { > 15: this.source = Objects.requireNonNull(source, "source"); > 16: this.nonNullCondition = Objects.requireNonNull(condition, > "condition").orElse(false); The message should probably match the other classes: "source cannot be null". modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java line 41: > 39: protected T computeValue() { > 40: if (isObserved() && isActive()) { > 41: if(subscription == null) { Space after `if`. modules/javafx.base/src/test/java/test/javafx/beans/value/LazyObjectBindingTest.java line 65: > 63: > 64: binding.addListener(obs -> { > 65: assertEquals("A", binding.get()); Can be converted to an expression. modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java line 52: > 50: > 51: public class ObservableValueFluentBindingsTest { > 52: private int invalidations; Empty line after class declaration. ------------- Changes requested by nlisker (Reviewer). PR: https://git.openjdk.org/jfx/pull/830
