On Fri, 14 Oct 2022 00:26:42 GMT, John Hendrikx <[email protected]> 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
