RFR: 8252935: Add treeShowing listener only when needed [v4]

[Ambarish Rapte]

On Sat, 6 Feb 2021 18:17:40 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java line 89:
>> 
>>> 87:         NodeHelper.treeVisibleProperty(node).addListener(windowShowingChangedListener);
>>> 88: 
>>> 89:         nodeSceneChangedListener.changed(null, null, node.getScene());  // registers listeners on Window/Showing
>> 
>> We do not follow this pattern of calling the changed() or invalidated() methods. Instead we directly add the necessary calls at the time of initialisation and disposal. I think the pattern should be followed here too.
>> For reference, the pattern is followed in all Skin classes. For example [here](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L148) in ButtonSkin, we do not make a call to `sceneChangeListener.changed()`, instead the necessary calls are directly made on [next lines](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L151) and then [opposite](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L179) calls in dispose().
>> 
>> For this line 89 needs to be replaced by following code, (with additional null checks)
>> node.getScene().windowProperty().addListener(sceneWindowChangedListener);
>> node.getScene().getWindow().showingProperty().addListener(windowShowingChangedListener);
>> updateTreeShowing();
>> 
>> and similarly they should be removed in dispose().
>
> Isn't it quite error prone to repeat this logic again (especially with all the null cases), not to mention that you would need to test the code for the initial case (with/without Scene, with/without Window), the "in use" case and again for the disposal case?
> 
> I personally use helpers for this kind of property chaining as it is far to easy to get wrong:
> 
>     public Binding<Boolean> isShowing(Node node) {
>         Values.of(node.sceneProperty())
>            .flatMap(s -> Values.of(s.windowProperty()))
>            .flatMap(w -> Values.of(w.showingProperty()))
>            .orElse(false)
>            .toBinding();
>     }
> 
> The implementation here takes care of `null` and automatically tracks property changes and is type safe to boot.  I think JavaFX in general could really benefit from this, as I've seen this chaining logic repeated a lot.

My concern is about having a similar way of doing something. It would keep the code uniform. We have been following the earlier pattern under a cleanup task [JDK-8241364](https://bugs.openjdk.java.net/browse/JDK-8241364). Several bugs under this task are being fixed in earlier way.
May be we can discuss the new way of handling properties under a separate issue and plan to modify all such instances at once. Does that sound ok ?

-------------

PR: https://git.openjdk.java.net/jfx/pull/185