RFR: 8290765: Remove parent disabled/treeVisible listeners [v2]

[John Hendrikx]

On Fri, 26 Aug 2022 00:22:54 GMT, Michael Strauß <mstrauss at 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