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

[John Hendrikx]

On Fri, 5 Feb 2021 14:43:25 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> John Hendrikx has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - Add missing TreeShowingExpressionTest which also tests SubScene
>>  - Also dispose listeners on Window/Showing in dispose
>>  - Fix review comments
>>  - Update copyrights
>
> 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.

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

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