On Fri, 14 Oct 2022 18:30:43 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains ten commits: >> >> - Merge remote-tracking branch 'origin/master' into >> feature/conditional-bindings >> >> # Conflicts: >> # >> modules/javafx.base/src/test/java/test/javafx/beans/value/LazyObjectBindingTest.java >> - Fix review comments >> - Add missing new line >> - Small wording fixes >> - Update javadoc of "when" with better phrasing >> - Rename showing property to shown as it is already used in subclasses >> - Add conditional bindings to ObservableValue >> - Change explanatory comment to block comment >> - Fix bug where ObjectBinding returns null when revalidated immediately >> >> Introduced with the fluent bindings PR > > 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.conditionOn(slider::shownProperty).map(toNumber).subscribe(slider.valueProperty()::setValue); slider.valueProperty().map(fromNumber).subscribe(value::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. ------------- PR: https://git.openjdk.org/jfx/pull/830
