On Fri, 26 Aug 2022 00:22:54 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
> It's true that `Parent.getChildren()` must not return `null`, and that we
> shouldn't add null checks to protect against bugs.
>
> However, in this specific instance, `Node.setTreeVisible` is called from the
> constructor of `Node`. At that point in time, the `Parent` constructor has
> not yet run, so its final fields are still unassigned (and therefore `null`).
>
> The same problem exists for `SubScene.getRoot()` a few lines earlier, which
> is specified to never return `null`, but will indeed return `null` if called
> when the `SubScene` is under construction.
>
> But there's a serious problem with the null checking approach: it's calling a
> protected method at an unexpected time, before the constructor of a derived
> class has run. Since that's not what derived classes would expect, I have
> instead opted for a different solution, which includes a
> `underInitialization` flag that is passed to `Node.setTreeVisible`. When the
> flag is set, the calls to `SubScene.getRoot()` and `Parent.getChildren()` are
> elided (they'd return `null` anyway, so there's no point in calling these
> methods).
Can this not be done in a way that doesn't require this `underInitialization`
flag? The call in the constructor to `updateTreeVisible` looks misplaced, or
at least, won't do much; visible is going to be `true`, parent is gonna be
`null`, sub scene is going to be `null`... all that it is going to do is set
`treeVisible` to `true`, so why not just initialize the field that way?
Result of this code for an uninitialized `Node` is going to be a call to
`setTreeVisible(true)`, it won't do anything else:
private void updateTreeVisible(boolean parentChanged) {
boolean isTreeVisible = isVisible(); // is going to be true
final Node parentNode = getParent() != null ? getParent() : //
parentNode is going to be null
clipParent != null ? clipParent :
getSubScene() != null ? getSubScene() : null;
if (isTreeVisible) {
isTreeVisible = parentNode == null || parentNode.isTreeVisible();
// is going to be true
}
// When the parent has changed to visible and we have unsynchronized
visibility,
// we have to synchronize, because the rendering will now pass through
the newly-visible parent
// Otherwise an invisible Node might get rendered
if (parentChanged && parentNode != null && parentNode.isTreeVisible()
&& isDirty(DirtyBits.NODE_VISIBLE)) { // won't enter this if
addToSceneDirtyList();
}
setTreeVisible(isTreeVisible); // called with true
}
Then `setTreeVisible`:
final void setTreeVisible(boolean value) {
if (treeVisible != value) { // always enters if (as initial value of
treeVisible is false, and called with true)
treeVisible = value;
updateCanReceiveFocus(); // doesn't do anything, canReceiveFocus
was false initially and will be false again
focusSetDirty(getScene()); // doesn't do anything, scene == null
if (getClip() != null) { // doesn't do anything, clip == null
getClip().updateTreeVisible(true);
}
if (treeVisible && !isDirtyEmpty()) { // doesn't do anything,
dirty is empty
addToSceneDirtyList();
}
((TreeVisiblePropertyReadOnly) treeVisibleProperty()).invalidate();
// doesn't do anything, there are no listeners yet
if (Node.this instanceof SubScene) {
Node subSceneRoot = ((SubScene)Node.this).getRoot();
if (subSceneRoot != null) { // will always be null, we're
still initializing
// SubScene.getRoot() is only null if it's constructor
// has not finished.
subSceneRoot.setTreeVisible(value &&
subSceneRoot.isVisible());
}
}
}
}
End result of running all that code... `treeVisible` goes from `false` to
`true`.
-------------
PR: https://git.openjdk.org/jfx/pull/841
