8290765: Remove parent disabled/treeVisible listeners

[mstr2]

Node adds InvalidationListeners to its parent's disabled and treeVisible properties and calls its own updateDisabled() and updateTreeVisible(boolean) methods when the property values change.

These listeners are not required, since Node can easily call the updateDisabled() and updateTreeVisible(boolean) methods on its children, saving the memory cost of maintaining listeners and bindings.

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8290765: Remove parent disabled/treeVisible listeners

Reviewers

• Ambarish Rapte (@arapte - Reviewer) 🔄 Re-review required (review applies to 60651900)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/841/head:pull/841
$ git checkout pull/841

Update a local copy of the PR:
$ git checkout pull/841
$ git pull https://git.openjdk.org/jfx pull/841/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 841

View PR using the GUI difftool:
$ git pr show -t 841

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/841.diff

[bridgekeeper[bot]]

:wave: Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fixes/node-listener-cleanup
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[mlbridge[bot]]

Webrevs

• 04: Full - Incremental (316ec939)
• 03: Full - Incremental (02577bba)
• 02: Full (fe803883)
• 01: Full (a6e01cac)
• 00: Full (60651900)

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[mstr2]

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).

[nlisker]

I haven't looked at the code yet, but in general it's not considered a good idea to expose an object before it's instantiated. Not sure if we have a choice here though.

[mstr2]

I haven't looked at the code yet, but in general it's not considered a good idea to expose an object before it's instantiated. Not sure if we have a choice here though.

I agree, that's why the underInitialization flag is used to prevent calling getRoot() and getChildren(), which would leak a partially constructed instance to derived classes.

[mstr2]

I've resolved some merge conflicts with the latest master branch. @arapte can you re-review?

[hjohn]

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()) {  // always enters
            addToSceneDirtyList();  // doesn't do anything, scene == null
        }
        ((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.

[mstr2]

Can this not be done in a way that doesn't require this underInitialization flag?

That's a good idea, and a also a good observation that the constructor only really sets treeVisible to true. I've updated the PR accordingly, which removes some of the complexity. There's no need for additional unit tests, as the expected behavior is already covered by NodeTest.testIsTreeVisible.

[openjdk[bot]]

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290765: Remove parent disabled/treeVisible listeners

Reviewed-by: arapte, jhendrikx, nlisker

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 12 new commits pushed to the master branch:

• ef9f0664484f7a3c2a9ebfbd81e1d23ef65218ed: 8138842: TableViewSelectionModel.selectIndices does not select index 0
• 33f1f629c5df9f8e03e81e360730536cde0a8f53: 8298382: JavaFX ChartArea Accessibility Reader
• e1f7d0c42be3839df7679ae45d74907ef33edfbe: Merge
• 8f2fac06152d3332e169f5b5389ac2ba84d18bc2: 8300705: Update boot JDK to 19.0.2
• 39d874712205f96befe4af07a332f1e747f3ecc2: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
• a4bc9d1a69e56cab92d3dc34cfff49c5cb524443: 8300013: Node.focusWithin doesn't account for nested focused nodes
• 67c55e51e063d2f9e291ae8dc4877843fa51119c: 8251862: Wrong position of Popup windows at the intersection of 2 screens
• fa4884c3c43c43da8dd555441ca5658898e807cb: 8301604: Replace Collections.unmodifiableList with List.of
• 301bbd6409d463c4f789e2b3dcb6b2ea200d7373: 8299977: Update WebKit to 615.1
• e234c89a253ece4962803a05d6ef77f0a7a80a82: 8237505: RadioMenuItem in ToggleGroup: deselected on accelerator
• ... and 2 more: https://git.openjdk.org/jfx/compare/294e82e636a74aab73491c4dea9a31a97c389353...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[kevinrushforth]

I took a quick look and it seems fine to me.

I recommend to wait for a day or two to see if @arapte has any additional comments on the changes (since his review is stale).

[mstr2]

I've extended the tests to check for the other direction as well. @hjohn @nlisker can you re-review?

[mstr2]

/integrate

[openjdk[bot]]

Going to push as commit 44b8c620082e26a23c48b3c0feb96c17e7aa7d5a. Since your change was applied there have been 12 commits pushed to the master branch:

• ef9f0664484f7a3c2a9ebfbd81e1d23ef65218ed: 8138842: TableViewSelectionModel.selectIndices does not select index 0
• 33f1f629c5df9f8e03e81e360730536cde0a8f53: 8298382: JavaFX ChartArea Accessibility Reader
• e1f7d0c42be3839df7679ae45d74907ef33edfbe: Merge
• 8f2fac06152d3332e169f5b5389ac2ba84d18bc2: 8300705: Update boot JDK to 19.0.2
• 39d874712205f96befe4af07a332f1e747f3ecc2: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
• a4bc9d1a69e56cab92d3dc34cfff49c5cb524443: 8300013: Node.focusWithin doesn't account for nested focused nodes
• 67c55e51e063d2f9e291ae8dc4877843fa51119c: 8251862: Wrong position of Popup windows at the intersection of 2 screens
• fa4884c3c43c43da8dd555441ca5658898e807cb: 8301604: Replace Collections.unmodifiableList with List.of
• 301bbd6409d463c4f789e2b3dcb6b2ea200d7373: 8299977: Update WebKit to 615.1
• e234c89a253ece4962803a05d6ef77f0a7a80a82: 8237505: RadioMenuItem in ToggleGroup: deselected on accelerator
• ... and 2 more: https://git.openjdk.org/jfx/compare/294e82e636a74aab73491c4dea9a31a97c389353...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@mstr2 Pushed as commit 44b8c620082e26a23c48b3c0feb96c17e7aa7d5a.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.