On Fri, 14 Oct 2022 20:53:01 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1420: >> >>> 1418: * @since 20 >>> 1419: */ >>> 1420: private ReadOnlyBooleanProperty shown; >> >> I would recommend against adding this property, as it adds a pointer to >> every Node regardless of whether it's needed or not. >> >> Another reason not to add is that, at least in my experience, there is >> rarely a need to base the logic on whether a particular Node is shown or not >> - instead, the logic to disconnect (one or more) listeners should be based >> on a Window/Dialog or a container pane. And for that I think a better >> solution might be - sorry for self plug - something like ListenerHelper >> https://github.com/openjdk/jfx/pull/908 >> >> And another thing - Window and Dialog are not Nodes, but they do have >> `showing` property. >> >> I would suggest either remove the changes pertaining to Node, or perhaps >> provide a utility method to create the property and store it in >> Node.getProperties() or Window.getProperties() (sadly, there is no >> Dialog.getProperties()) > > I agree with your first point, but I think limiting any changes to `Node` > because it would add another field is not a good enough argument. It would > depend on its merits. > > The second point, there is much more to the `shownProperty` then you might > think at first glance. In combination with `when`, it offers a standard way > to manage life cycles for controls. It is a very natural fit to tie the > lifecycle of bindings and listeners to whether a control is actually > currently shown. You may think that a Listener helper class would be just as > flexible, but it isn't. The helper is not available everywhere (unless you > propose to integrate it into Node), which means when constructing complicated > UI's which may have smaller groups of controls that are created by other > classes, methods or factories, that you may have to pass along an instance of > your helper to all these to make sure they register their bindings and > listeners with it (or perhaps to link multiple Listener helper classes > together in some kind of hierarchy, that matches the control groupings that > might have separate life cycles). > > The `shownProperty` does not have this problem. Since you can tie the > lifecycle to your own small piece of the UI (ie, a factory that creates a > `BorderPane` with several controls, can tie all these to the lifecycle of the > `BorderPane`). When this `BorderPane` is just a piece of UI used as part of > a larger whole, its lifecycle still works as expected -- if it is not shown, > its listeners disappear. If the whole window is destroyed, then the same > happens. If however the Pane next to it is destroyed, only their listeners > disappear. With a helper system, you'd need to coordinate this somehow, > perhaps having that `BorderPane` return its helper, so the large UI can tie > it to its helper, so when the main helper is called to unsubscribe, that all > its child helpers are also unsubscribed... > > Another advantage is that a listener does not have to start working right > away. When setting up a new pane of controls, many subscriptions may need to > be made. When using `when(control::shownProperty)` these listeners don't > immediately become active (potentially while still constructing and > initializing your control). Only when the control is fully constructed, and > added to an active Scene and Window do all the listeners become active. This > can be important when listening to external properties that change often and > may even change during construction. > > In ReactFX it the `shownProperty` was deemed important enough to go one step > further and even create a separate method on what would be `ObservableValue` > called `conditionOnShowing`. > > In short, `when` and the `shownProperty` is basically used every time you > create a binding or add a listener between two properties that have a > different lifecycle, like between a model and a control. This completely > removes the responsibility of having to track these bindings or listeners > separately; it becomes automatic. Subscribing all at once when it becomes > first shown becomes automatic (and you immediately get the first value), > unsubscribing when it goes invisible is automatic, and resubscribing if a > control becomes shown again is completely transparent. > > In my projects, I've completely eliminated memory leaks without a single call > to `removeListener` or `unbind` anywhere. > There are no explicit weak listeners (and not just because they're incredibly > cumbersome to use), nor is there anywhere where subscriptions or bindings are > being tracked (aside from the `when` construct). I'm very picky about memory > leaks, so I have a Scene scanner, that maintains weak references to all Nodes > at all times, and warns me if any are not getting GC'd after not being part > of a Scene for a while. In 99% of the cases, I forgot a `when`, where in > plain JavaFX you'd be forced to use a weak listener. > > The simple rule to follow is, whenever you bind or listen to or on a > property, if they have different lifecycles, then use a `when`, otherwise > bind or listen directly. > > Some examples of how it is used: > > Below is a very common pattern when binding any property that has a different > lifecycle than its target, this is one of many: > > longLivedModel.selectedItem > .when(this::shownProperty) // this = BorderPane, and the code is > part of its constructor) > .addListener((obs, old, current) -> { > // code that starts a transition from the previous item to the > next item > } > ); > > Below some binding examples where several properties are bound to long lived > versions: > > ObservableValue<Boolean> shown = pane.shownProperty(); > > pane.model.work.bind(presentation.root.when(shown)); > pane.model.missingFraction.bind(presentation.missingFraction.when(shown)); > pane.model.watchedFraction.bind(presentation.watchedFraction.when(shown)); > > Below a "bidirectional binding" for a `Slider` control, where one direction > is long lived. The binding vanishes on its own when the UI that is using it > is closed -- this is part of a UI generator, so this code has no idea where > the UI might end up being used or for how long, but it can still manage the > lifecycle easily: > > > longLivedValue.when(slider::shownProperty).map(toNumber).subscribe(slider.valueProperty()::setValue); > > slider.valueProperty().map(fromNumber).subscribe(longLivedValue::setValue); > > Another example to make sure a `Timeline` is stopped/started automatically > based on visibility (Timeline's will keep references around if not stopped): > > center.shownProperty() // center here is a Pane > .addListener((obs, old, visible) -> { > if(visible) timeline.playFromStart(); > else timeline.stop(); > }); > > So, I think it certainly has some merit to include it :) > >> And another thing - Window and Dialog are not Nodes, but they do have >> showing property. > > I'm unsure what you are saying here. I don't think we need to tie anything > directly to a Window or Dialog, only to controls they might contain. Also > the `shownProperty` is not useful for properties not associated with > controls; depending on what the properties are used for people may have to > create their own `active` property or something else that identifies the > lifecycle of those properties. For control properties though, their > lifecycle is quite clear and I think it makes great sense to tie to whether > they're actually visible or not. What I meant is you can add it in Node.getProperties() and not to add a pointer to every Node everywhere. If it's used, the actual properties will be created in a few containers. ------------- PR: https://git.openjdk.org/jfx/pull/830
