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
