Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8290765: Remove parent disabled/treeVisible listeners #841

Closed
wants to merge 9 commits into from

Conversation

mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Jul 21, 2022

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

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

@mstr2 mstr2 marked this pull request as draft July 21, 2022 04:43
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 21, 2022

👋 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
Copy link

openjdk bot commented Jul 21, 2022

@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

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jul 21, 2022
@mstr2 mstr2 marked this pull request as ready for review July 21, 2022 04:50
@openjdk openjdk bot added rfr Ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Jul 21, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 21, 2022

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jul 21, 2022

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

Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with some layout and controls. LGTM.

Comment on lines 1912 to 1919
if (Node.this instanceof Parent) {
List<Node> children = ((Parent)Node.this).getChildren();
if (children != null) {
for (Node child : children) {
child.updateDisabled();
}
}
}
Copy link
Collaborator

@nlisker nlisker Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

Copy link
Member

@Maran23 Maran23 Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@nlisker nlisker Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

mstr2 commented Aug 26, 2022

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
Copy link
Collaborator

nlisker commented Aug 26, 2022

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
Copy link
Collaborator Author

mstr2 commented Aug 26, 2022

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
Copy link
Collaborator Author

mstr2 commented Jan 26, 2023

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

@hjohn
Copy link
Collaborator

hjohn commented Jan 27, 2023

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
Copy link
Collaborator Author

mstr2 commented Jan 28, 2023

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.

@kevinrushforth kevinrushforth removed their request for review January 31, 2023 18:38
Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines -2601 to -2608
//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");
//}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@openjdk
Copy link

openjdk bot commented Feb 3, 2023

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

@openjdk openjdk bot added the ready Ready to be integrated label Feb 3, 2023
@kevinrushforth
Copy link
Member

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

@openjdk openjdk bot removed the ready Ready to be integrated label Feb 3, 2023
@mstr2
Copy link
Collaborator Author

mstr2 commented Feb 3, 2023

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

Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@openjdk openjdk bot added the ready Ready to be integrated label Feb 5, 2023
Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too.

@mstr2
Copy link
Collaborator Author

mstr2 commented Feb 7, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Feb 7, 2023

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 openjdk bot added the integrated Pull request has been integrated label Feb 7, 2023
@openjdk openjdk bot closed this Feb 7, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 7, 2023
@openjdk
Copy link

openjdk bot commented Feb 7, 2023

@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 mstr2 deleted the fixes/node-listener-cleanup branch April 8, 2023 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
6 participants