RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]

[Andy Goryachev]

On Fri, 14 Oct 2022 00:26:42 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> sorry for including multiple topics in one comment.
>> 
>> 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).  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)
>> 
>> 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.
>> 
>> 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.
>
>> 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.

-------------

PR: https://git.openjdk.org/jfx/pull/830