On Fri, 14 Oct 2022 00:26:42 GMT, John Hendrikx <jhendr...@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