On Wed, 26 Oct 2022 07:56:52 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: > > Fix javadoc error I think the most important open point for this PR is the name for the new operation to be added to `ObservableValue`. Currently in this PR it is called `when`. Some background first to help with the decision; all fluent methods follow this basic pattern: source.operation( ... ) -> new target observable The proposed operation has a `ObservabeValue<Boolean>` parameter as its condition. It allows you to create a new observable that has the same value as its source, unless the associated condition is `false`, in which case it contains the last seen value of the source (it does **not** change to `null` or empty, so an `orElse` cannot be used to change the value when the condition is `false`.). The proposed operation differs from the other three available fluent methods (`map`, `flatMap` and `orElse`) that in order to determine its result it does not need to continuously observe its source -- the source value has no bearing on whether the target will or will not reflect the source value, only the condition controls this. This is unlike `map` for example, which **must** evaluate the source value to determine the target value, and also unlike a `filter` method (as seen in `Optional` or `Stream`) which must evaluate the source value to see if it passes the filter predicate in order to determine whether the target uses the source value or becomes empty. Because the source value is not needed to reach a decision for this new proposed operation, there is no need to constantly observe the source value for changes. When the conditional is `false`, the observer can be removed until such time that the conditional becomes `true`. This is in contrast to `map`, `orElse` or a potential future `filter` operation which have no choice but to observe their source at all times in order to correctly calculate their target value. There is also a clear intention that the condition changes at some point in the future and that it need not be a one time change (when condition becomes `true` again, the target again starts tracking the source value); if the condition never changes there is no need to use this new operation as one can just use the source observable (if condition is always `true`) or it becomes a constant value (if condition is always `false`). A quick overview of the differences between the existing and new operation: |Time|x|condition|x.map(x -> x * 2)|x.filter(x -> x % 2 == 0)|x.operation(condition)| |:-:|:-:|:-:|:-:|:-:|:-:| |0|2|true|4|2|2| |1|2|false|4|2|2| |2|3|false|6|null (empty)|2| |3|3|true|6|null (empty)|3| |4|4|true|8|4|4| ## Options for the name of this new operation The full signature for the newly proposed operation is: ObservableValue<T> __operation__(ObservableValue<Boolean> condition) Possible options: 1. `while`, `if` 2. `conditionOn` (from ReactFX) 3. `skipUntil` + `takeWhile` or `skipWhile` + `takeUntil` (from ReactiveX) 4. When variations: `when`, `updateWhen`, `whenever` ### `while` and `if` Although `while` would fit the bill reasonably nice ("target becomes source value **while** condition (is true)"), it is a reserved keyword. `while` does have a time component to it, but does not so nicely communicate that the condition may become `true` again; one would have to clarify this with "target becomes source value whenever the **while** condition (is true)". `if` is also a reserved keyword, but does a slightly better job than `while`: "target becomes source value **if** condition (is true)". It does however not communicate that the condition is likely to change in the future, which is the whole point of this new operation. ### `conditionOn` ReactFX uses this term for the proposed operation. In english it could perhaps be formulated as: "target becomes source value on the condition that condition (is true)". This name seems a bit superfluous. The parameter of the proposed operation is already called `condition` and other similar methods don't re-iterate this fact in their naming either. ### `skipUntil` + `takeWhile` or `skipWhile` + `takeUntil` In ReactiveX there is a similar operation for dealing with its streams. First note, there is an important difference between `ObservableValue` and ReactiveX's `Observable`: an `ObservableValue` always has a current value that can be queried and operated upon at any time. An `Observable` in ReactiveX has no current value, and when applying operations to its streams, these operations can only be applied if the source observable emits an event. ReactiveX is closer match for Java's `Stream` while `ObservableValue` is a closer match to Java's `Optional`. The operations allowed by ReactiveX cannot be used to achieve a similar effect as the proposed method by combining them (`x.skipUntil(c).takeWhile(c)`). This is because the operations allow one condition change only, which is pretty clear from their names. A better name that would match our operation would be `takeWhenever(c)` which is another variation of using "when". ### When variations: `when`, `updateWhen`, `whenever` Earlier, `if` proved to be quite a good match: "target becomes source value **if** condition (is true)", but did not communicate that well that the condition is almost certain to change in the future (as using the new operation would be pointless otherwise). In English, "when" distinguishes itself from "if" for something that is almost certain to happen at some point in the future. All of these are a good match. In the context of creating observables that are bound to each other (and thus update on changes of their source), there is little need to reiterate this in the name; all of the fluent methods will update their target in some fashion. In English, `whenever` is used for repeated occurrences, while `when` can mean a single occurrence (but doesn't preclude it AFAIK). This mean I think that both of these would be great matches. I would be inclined to go for the more compact `when`. This has been a long story; I hope we can reach a consensus on a name, perhaps people can respond with a ranked set of preferences. ------------- PR: https://git.openjdk.org/jfx/pull/830
