On Sat, 6 Feb 2021 18:17:40 GMT, John Hendrikx <jhendr...@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
