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

Reply via email to