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

8256821: TreeViewSkin/Behavior: misbehavior on switching skin #358

Closed
wants to merge 3 commits into from

Conversation

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Nov 24, 2020

issues with behavior:

  • memory leak due to an key eventHandler that's not removed
  • after dispose, still modifying treeView (anchor) state due to listeners selection that are not removed

issues with skin:

  • memory leak due to behavior leaking
  • memory leak due to cellFactory in flow not removed
  • throws NPE after switching (on modifying root children, refresh) due to listeners not removed

Fixed by cleaning up as needed. Added tests that are failing before and passing after the fix.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256821: TreeViewSkin/Behavior: misbehavior on switching skin

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/358/head:pull/358
$ git checkout pull/358

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 24, 2020

👋 Welcome back fastegal! 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 openjdk bot added the rfr label Nov 24, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 24, 2020

Webrevs

@kevinrushforth kevinrushforth requested a review from arapte Nov 24, 2020
Copy link
Member

@arapte arapte left a comment

Looks good, requested a minor change.

*/
@Test
public void testTreeViewSetRoot() {
TreeView<String> listView = new TreeView<>(createRoot());

This comment has been minimized.

@arapte

arapte Dec 7, 2020
Member

minor: Please rename the variable to treeView in this method and others.

This comment has been minimized.

@kleopatra

kleopatra Dec 7, 2020
Author Collaborator

darn c&p - thanks :)

kleopatra added 2 commits Dec 7, 2020
@arapte
arapte approved these changes Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

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

8256821: TreeViewSkin/Behavior: misbehavior on switching skin

Reviewed-by: arapte

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 2 new commits pushed to the master branch:

  • c8d9c94: 8254049: Update WebView to public suffix list 2020-04-24
  • bd51089: 8256978: GitHub actions: build fails on Linux due to missing package

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 label Dec 7, 2020
@kleopatra
Copy link
Collaborator Author

@kleopatra kleopatra commented Dec 7, 2020

/integrate

@openjdk openjdk bot closed this Dec 7, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 7, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@kleopatra Since your change was applied there have been 3 commits pushed to the master branch:

  • 00a8646: 8247576: Labeled/SkinBase: misbehavior on switching skin
  • c8d9c94: 8254049: Update WebView to public suffix list 2020-04-24
  • bd51089: 8256978: GitHub actions: build fails on Linux due to missing package

Your commit was automatically rebased without conflicts.

Pushed as commit ca0d3d0.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants