On Fri, 14 Oct 2022 16:17:24 GMT, Andy Goryachev <[email protected]> wrote:
>>> 1. it looks like your proposal will work, as long as the `active` property
>>> is not referenced outside of its owner (in our discussion, Skin). If that
>>> is true, as soon as you set active=false, the listener is disconnected and
>>> the skin, the active property, and the lambda can be collected. Thus, as
>>> you correctly explained, we need to create a large aggregate with two flat
>>> maps in order to avoid the memory leak (whether you hide this complexity
>>> behind shownProperty is irrelevant).
>>
>> Yes, the `shownProperty` is there purely for convenience, and more useful
>> for regular controls, not the skin scenario I think. I have this helper
>> that I use for this purpose at the moment:
>>
>> public static ObservableValue<Boolean> showing(Node node) {
>> return node.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false);
>> }
>>
>>> So yes, it will work, as long as the developer makes no mistakes and does
>>> not wire the thing directly (a mistake I readily made)
>>
>> Yes, that's certainly true, I think the `shownProperty` will help with that
>> though, as I think it will prevent people from storing the flatMap aggregate
>> and reusing it, and perhaps then reusing it for the wrong controls. Reuse
>> however can still be fine for groups of listeners of several controls that
>> go together. If your control's lifecycle is tied to a group, container or
>> dialog, then it is perfectly fine to use one of their shown properties.
>>
>>> 2. as a side note, I would discourage a pattern where Nodes are reused and
>>> need to be reconnected. at least in my applications, I never do that, but
>>> I can see situation when this might be useful.
>>
>> I'm not in favor of that either, and I don't think I ever do it, but it is
>> allowed by JavaFX, and so if you did, then the listeners would be restored
>> as they were, and since you had to have a reference to this reused control
>> still, nothing will have been GC'd yet.
>>
>>> 3. is when() a good name? it sort of implies a time-domain criterion
>>> instead of when a boolean becomes true (whenTrue? whenAllowed?) i could be
>>> wrong here.
>>
>> ReactFX used `conditionOn` and had a special version `conditionOnShowing`
>> which accepts a `Node` (this would however create a circular reference
>> between projects base and graphics). I proposed `when` as it is nice and
>> short, inspired by the recent developments in the area of switch
>> expressions. Even better would be `while` IMHO, but that is unfortunately a
>> reserved keyword. Still, when works reasonably well: "Listen to changes of
>> this long lived property **when** this condition holds". While would
>> definitely be better or perhaps "as long as" or "whenever" :)
>>
>> I don't think we should add "true" in the name, no other conditionals do
>> this (like `Stream#filter` or `List#removeIf`).
>>
>> `filter` itself is also not an option, as this has a different meaning which
>> we may implement in the future. `filter` would allow you to remove certain
>> values, which would set the value to empty:
>>
>> textProperty.filter(text ->
>> !isRudeWord()).orElse("<censored>").addListener(...);
>
> thank you for clarifications.
>
> If I were to choose, I'd pick `conditionOn` as it implies a boolean.
>
> Also, since I've fallen into this trap when reviewing this PR, I think it
> might be a good idea to explain how to avoid memory leak by using a local
> property in `conditionOn/when` so as not to create a memory leak.
@andy-goryachev-oracle
About avoiding the memory leak. The documentation for `when` currently says:
> The returned {@code ObservableValue} only observes this value when {@code
> condition} holds {@code true}. **This allows this {@code ObservableValue} and
> the conditional {@code ObservableValue} to be garbage collected if neither is
> otherwise strongly referenced when {@code condition} holds {@code false}.**
> This is in contrast to the general behavior of bindings, where the binding is
> only eligible for garbage collection when not observed itself.
The second sentence I think explains the reason that it should be a property
with a similar lifecycle.
Do you think I should expand on this further?
-------------
PR: https://git.openjdk.org/jfx/pull/830
