8290765: Remove parent disabled/treeVisible listeners #841

mstr2 · 2022-07-21T04:43:15Z

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

Change must not contain extraneous whitespace
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)
John Hendrikx (@hjohn - Committer)
Nir Lisker (@nlisker - Reviewer)

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 · 2022-07-21T04:43:39Z

👋 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 · 2022-07-21T04:45:27Z

@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 · 2022-07-21T04:56:36Z

Webrevs

kevinrushforth · 2022-07-21T11:33:22Z

/reviewers 2

openjdk · 2022-07-21T11:33:44Z

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

arapte

Tested with some layout and controls. LGTM.

nlisker · 2022-08-25T12:33:51Z

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

+                    if (Node.this instanceof Parent) {
+                        List<Node> children = ((Parent)Node.this).getChildren();
+                        if (children != null) {
+                            for (Node child : children) {
+                                child.updateDisabled();
+                            }
+                        }
+                    }


Because we can use Java 17 now, you can use pattern matching for instanceof. Also, from what I see, getChildren() can never return null. So, we can write

if (Node.this instanceof Parent parent) { parent.getChildren().forEach(child -> child.updateDisabled()); }

Since getChildren() is not final, one can easily override it and return null.
Therefore, this check should still be done here.
Maybe we even need to check that every child is not null, since again I can override getChildren() and return a list that allows null. (The default implementation in Parent throws an exception when adding a null child)

Interestingly, the javadoc of getChildren() states that one should call the super method when overriding (although this can't be enforced). So this is probably a hypothetical case.

It's definitely a hypothetical case. getChildren() is called all over the place in JavaFX without a null check, so I see no reason for null checks here.

Technically correct, although the doc of getChildren() specifically says:

Note to subclasses: if you override this method, you must return from your implementation the result of calling this super method. The actual list instance returned from any getChildren() implementation must be the list owned and managed by this Parent. The only typical purpose for overriding this method is to promote the method to be public.

So considering the case that an overriding method will return null is not practical.

As for a null child, I think that that's also not allowed. Considering that a child can be added at most once to a scenegraph, it would mean that only 1 null child is allowed. There might be more issues with null children. I don't think there's a good reason to check for them.

Yep, right. While it is possible to override getChildren() and return null, it will break everywhere so we can also omit the null check here.

mstr2 · 2022-08-26T00:22:54Z

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 · 2022-08-26T03:43:49Z

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 · 2022-08-26T04:39:46Z

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.

# Conflicts: # modules/javafx.graphics/src/main/java/javafx/scene/Node.java

mstr2 · 2023-01-26T19:06:03Z

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

hjohn · 2023-01-27T22:01:46Z

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 · 2023-01-28T16:24:41Z

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.

nlisker

Looks good. I ran the tests and they pass. Left a few optional comments.

Maybe @kevinrushforth will want to have a look too before you merge.

nlisker · 2023-02-03T22:22:45Z

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

-        //if (PerformanceTracker.isLoggingEnabled()) {
-        //    PerformanceTracker.logEvent("Node.init for [{this}, id=\"{id}\"]");
-        //}
-        updateTreeVisible(false);
-        //if (PerformanceTracker.isLoggingEnabled()) {
-        //    PerformanceTracker.logEvent("Node.postinit " +
-        //                                "for [{this}, id=\"{id}\"] finished");
-        //}


Not sure if these comments are intended to be used for something. I doubt it though.

modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java

modules/javafx.graphics/src/main/java/javafx/scene/Node.java

modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java

openjdk · 2023-02-03T22:38:14Z

@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:

ef9f066: 8138842: TableViewSelectionModel.selectIndices does not select index 0
33f1f62: 8298382: JavaFX ChartArea Accessibility Reader
e1f7d0c: Merge
8f2fac0: 8300705: Update boot JDK to 19.0.2
39d8747: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
a4bc9d1: 8300013: Node.focusWithin doesn't account for nested focused nodes
67c55e5: 8251862: Wrong position of Popup windows at the intersection of 2 screens
fa4884c: 8301604: Replace Collections.unmodifiableList with List.of
301bbd6: 8299977: Update WebKit to 615.1
e234c89: 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 · 2023-02-03T23:02:38Z

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 · 2023-02-03T23:32:14Z

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

nlisker

I missed that you can also change the foreach loop on the treeVisible method too, but it's not important.

arapte

Looks good to me too.

mstr2 · 2023-02-07T15:07:16Z

/integrate

openjdk · 2023-02-07T15:08:57Z

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

ef9f066: 8138842: TableViewSelectionModel.selectIndices does not select index 0
33f1f62: 8298382: JavaFX ChartArea Accessibility Reader
e1f7d0c: Merge
8f2fac0: 8300705: Update boot JDK to 19.0.2
39d8747: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
a4bc9d1: 8300013: Node.focusWithin doesn't account for nested focused nodes
67c55e5: 8251862: Wrong position of Popup windows at the intersection of 2 screens
fa4884c: 8301604: Replace Collections.unmodifiableList with List.of
301bbd6: 8299977: Update WebKit to 615.1
e234c89: 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 · 2023-02-07T15:09:11Z

@mstr2 Pushed as commit 44b8c62.

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

mstr2 added 2 commits July 21, 2022 06:04

Remove Node.disabled and Node.treeVisible listeners

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

34e4a81

added tests

54f03a9

mstr2 marked this pull request as draft July 21, 2022 04:43

openjdk bot added the merge-conflict label Jul 21, 2022

Merge

6065190

mstr2 marked this pull request as ready for review July 21, 2022 04:50

openjdk bot added rfr and removed merge-conflict labels Jul 21, 2022

kevinrushforth requested review from arapte and kevinrushforth July 21, 2022 11:33

arapte approved these changes Aug 25, 2022

View reviewed changes

nlisker reviewed Aug 25, 2022

View reviewed changes

mstr2 added 2 commits August 25, 2022 15:01

Merge branch 'master' into fixes/node-listener-cleanup

354d249

Replace null check by explicit underConstruction flag

a6e01ca

mstr2 added 2 commits January 26, 2023 19:52

Merge branch 'master' into fixes/node-listener-cleanup

6e80ea5

# Conflicts: # modules/javafx.graphics/src/main/java/javafx/scene/Node.java

added javadoc

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

fe80388

Initialize treeVisible field directly

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

02577bb

hjohn approved these changes Jan 28, 2023

View reviewed changes

kevinrushforth requested a review from arapte January 31, 2023 18:38

kevinrushforth removed their request for review January 31, 2023 18:38

nlisker approved these changes Feb 3, 2023

View reviewed changes

openjdk bot added the ready label Feb 3, 2023

improved tests, changed foreach loop

316ec93

openjdk bot removed the ready label Feb 3, 2023

nlisker approved these changes Feb 3, 2023

View reviewed changes

hjohn approved these changes Feb 5, 2023

View reviewed changes

openjdk bot added the ready label Feb 5, 2023

arapte approved these changes Feb 7, 2023

View reviewed changes

openjdk bot added the integrated label Feb 7, 2023

openjdk bot closed this Feb 7, 2023

openjdk bot removed ready rfr labels Feb 7, 2023

mstr2 deleted the fixes/node-listener-cleanup branch April 8, 2023 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8290765: Remove parent disabled/treeVisible listeners #841

8290765: Remove parent disabled/treeVisible listeners #841

mstr2 commented Jul 21, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

mlbridge bot commented Jul 21, 2022 •

edited

Loading

kevinrushforth commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

arapte left a comment

nlisker Aug 25, 2022 •

edited

Loading

Maran23 Aug 25, 2022 •

edited

Loading

kevinrushforth Aug 25, 2022

nlisker Aug 25, 2022 •

edited

Loading

Maran23 Aug 25, 2022

mstr2 commented Aug 26, 2022 •

edited

Loading

nlisker commented Aug 26, 2022

mstr2 commented Aug 26, 2022 •

edited

Loading

mstr2 commented Jan 26, 2023

hjohn commented Jan 27, 2023 •

edited

Loading

mstr2 commented Jan 28, 2023

nlisker left a comment

nlisker Feb 3, 2023

openjdk bot commented Feb 3, 2023 •

edited

Loading

kevinrushforth commented Feb 3, 2023

mstr2 commented Feb 3, 2023

nlisker left a comment

arapte left a comment

mstr2 commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

8290765: Remove parent disabled/treeVisible listeners #841

8290765: Remove parent disabled/treeVisible listeners #841

Conversation

mstr2 commented Jul 21, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

mlbridge bot commented Jul 21, 2022 • edited Loading

Webrevs

kevinrushforth commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

arapte left a comment

Choose a reason for hiding this comment

nlisker Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Maran23 Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

kevinrushforth Aug 25, 2022

Choose a reason for hiding this comment

nlisker Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Maran23 Aug 25, 2022

Choose a reason for hiding this comment

mstr2 commented Aug 26, 2022 • edited Loading

nlisker commented Aug 26, 2022

mstr2 commented Aug 26, 2022 • edited Loading

mstr2 commented Jan 26, 2023

hjohn commented Jan 27, 2023 • edited Loading

mstr2 commented Jan 28, 2023

nlisker left a comment

Choose a reason for hiding this comment

nlisker Feb 3, 2023

Choose a reason for hiding this comment

openjdk bot commented Feb 3, 2023 • edited Loading

kevinrushforth commented Feb 3, 2023

mstr2 commented Feb 3, 2023

nlisker left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

mstr2 commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

mstr2 commented Jul 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 21, 2022 •

edited

Loading

nlisker Aug 25, 2022 •

edited

Loading

Maran23 Aug 25, 2022 •

edited

Loading

nlisker Aug 25, 2022 •

edited

Loading

mstr2 commented Aug 26, 2022 •

edited

Loading

mstr2 commented Aug 26, 2022 •

edited

Loading

hjohn commented Jan 27, 2023 •

edited

Loading

openjdk bot commented Feb 3, 2023 •

edited

Loading