On Tue, 9 Feb 2021 10:31:57 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> 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 ? > > hmm ... might appear convenient (in very controlled contexts) but looks like > a precondition violation: the sender of the change must not be null > (concededly not explicitly spec'ed but logically implied, IMO) > > so would tend to _not_ see this as blueprint for a general pattern fx code > base So, what you are suggesting is to replace the above single line to this: Scene scene = node.getScene(); if (scene != null) { Window window = scene.getWindow(); scene.addListener(nodeSceneChangedListener); if (window != null) { window.showingProperty().addListener(windowShowingChangedListener); } } updateTreeShowing(); And something similarly wordy in dispose again -- and donot forget that I would then need to also duplicate the testing code to make sure all these branches are covered in both the constructor and in dispose again. Are you sure? I could extract the methods instead if you donot like me passing in `null` for the `ObservableValue`, it would then look like: sceneChanged(null, node.getScene()); // register chain sceneChanged(node.getScene(), null); // unregister chain No further changes in testing required as it is all covered and proven to work correctly. Also I should mention that this is not Skin code in case it was missed. ------------- PR: https://git.openjdk.java.net/jfx/pull/185
