On Thu, 13 Oct 2022 23:25:05 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> let me try to create a unit test (and I could be wrong, of course).
>
>> In the context of this PR, I can see how a memory leak can be created when a
>> developer has the `active` property as long lived and that would prevent the
>> whole chain from being collected. An example would be trying to attach to
>> the Window.showingProperty(), adding listeners that reference Nodes that are
>> supposed to be GC'd when removed from the said window (dynamically creating
>> detail panes as response to selection event, for example).
>
> What would be the purpose of having a long lived `active` property? If it
> has any purpose, then the properties it is controlling must stay referenced,
> so yes, that would prevent the chain from being GC'd otherwise it could
> suddenly break your application.
>
> Just like this application breaks currently because of weak listeners:
>
> label.textProperty().concat("A").addListener((src, old, current) ->
> System.out.println(current));
>
> The above code will stop working at a random time due to the weak listener
> created by `concat`. The same code without `concat` will keep working...
> this is very hard to explain to users.
>
> Now, if you had used `map`, this wouldn't happen. This is because `map` uses
> a normal listener, since it is observed itself. If the above is what the
> user wants to do, then who are we to break this when a GC happens?
>
> You also mentioned the `Window.showingProperty`. You have to use the correct
> path to access this property. You don't want to make it depend on whether
> the window is showing. No, you want to make it depend on whether the label
> has a scene which is part of a showing window:
>
> ObservableValue<Boolean> showing = label.sceneProperty()
> .flatMap(Scene::windowProperty)
> .flatMap(Window::showingProperty)
> .orElse(false);
>
> Or short version:
>
> ObservableValue<Boolean> showing = label.shownProperty();
>
> Now, the listener on your label is added like this:
>
> label.textProperty().when(showing).addListener( xyz );
>
> Or inlined:
>
> label.textProperty().when(label::shownProperty).addListener( xyz );
>
> When you remove this label, it's Scene is cleared. `showing` will therefore
> become `false`, and in turn the listener will stop functioning.
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.
-------------
PR: https://git.openjdk.org/jfx/pull/830
