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

8247576: Labeled/SkinBase: misbehavior on switching skin #355

Closed
wants to merge 2 commits into from

Conversation

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Nov 19, 2020

Cleanup of LabeledSkinBase to allow for switching skins

  • removed null check in listener on graphic's layoutBounds
  • added removal of listener in dispose

Progress

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

Issue

  • JDK-8247576: Labeled/SkinBase: misbehavior on switching skin

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 19, 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 19, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 19, 2020

Webrevs

@kleopatra
Copy link
Collaborator Author

@kleopatra kleopatra commented Nov 19, 2020

hmm .. failing windows build, why?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 19, 2020

Not sure why it failed, but it's definitely not anything related to your change. I'll take a look at it, but in the mean time reviewers of this PR can ignore the failure.

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

@kevinrushforth kevinrushforth commented Nov 19, 2020

I get the same build failure if I push to a branch that is even with master. Likely something changed in the GitHub configuration that we will need to adapt to.

@kleopatra
Copy link
Collaborator Author

@kleopatra kleopatra commented Nov 19, 2020

Not sure why it failed, but it's definitely not anything related to your change. I'll take a look at it, but in the mean time reviewers of this PR can ignore the failure.

thanks :)

Copy link
Member

@arapte arapte left a comment

Looks good, added a minor comment to remove a FIXME note

@@ -272,6 +283,7 @@ protected void updateChildren() {
getChildren().setAll(text);
}
} else {
// FIXME: this listener must be removed in dispose!

This comment has been minimized.

@arapte

arapte Dec 7, 2020
Member

minor: the FIXME note should be removed.

This comment has been minimized.

@kleopatra

kleopatra Dec 7, 2020
Author Collaborator

thanks for spotting it :)

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

8247576: Labeled/SkinBase: 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 7 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
  • a394fab: 8199592: Control labels truncated at certain DPI scaling levels
  • 2fe8677: 8256184: Openjfx build broken (Eclipse)
  • 8e03018: 8256649: Parameterized tests must not use instances as parameters
  • 767a1fd: 8256686: GitHub actions: build fails due to upgraded MSVC compiler
  • a128952: 8177945: Single cell selection flickers when adding data to TableView

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 7 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
  • a394fab: 8199592: Control labels truncated at certain DPI scaling levels
  • 2fe8677: 8256184: Openjfx build broken (Eclipse)
  • 8e03018: 8256649: Parameterized tests must not use instances as parameters
  • 767a1fd: 8256686: GitHub actions: build fails due to upgraded MSVC compiler
  • a128952: 8177945: Single cell selection flickers when adding data to TableView

Your commit was automatically rebased without conflicts.

Pushed as commit 00a8646.

💡 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
3 participants