On Mon, 1 Feb 2021 04:44:03 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:

>> This is a PoC for performance testing.  
>> 
>> It contains commented out code in PopupWindow and ProgressIndicatorSkin and 
>> two tests are failing because of that.
>> 
>> This code avoids registering two listeners (on Scene and on Window) for each 
>> and every Node to support the aforementioned classes.  The complete change 
>> should update these two classes to add their own listeners instead.
>
> 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

Added couple minor comments and a suggestion.
Rest looks good to me, Observed no failures. It would be a great performance 
improvement for large scene applications.

modules/javafx.graphics/src/test/java/test/javafx/scene/TreeShowingExpressionTest.java
 line 318:

> 316:         assertNull(state.getAndSet(null));
> 317:     }
> 318: }

Please add a new line here at end of file.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java
 line 2:

> 1: /*
> 2:  * Copyright (c) 2013, 2021, Oracle and/or its affiliates. All rights 
> reserved.

It should be changed to -> `Copyright (c) 2021, Oracle `

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().

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

Changes requested by arapte (Reviewer).

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

Reply via email to