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

8247163: JFXPanel throws exception on click when no Scene is set #248

Conversation

@FlorianKirmaier
Copy link
Member

FlorianKirmaier commented Jun 9, 2020

Fixing exception when clicking on JFXPanel when no Scene is set.

quick test: ./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests: test --tests *javafx.embed.swing.JFXPan* --info

It's a regression from my previous PR: #25


Progress

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

Issue

  • JDK-8247163: JFXPanel throws exception on click when no Scene is set

Reviewers

  • Kevin Rushforth (kcr - Reviewer)

Download

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

8247163:
Fixing exception when clicking on JFXPanel when no Scene is set.
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 9, 2020

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jun 9, 2020
@mlbridge
Copy link

mlbridge bot commented Jun 9, 2020

Webrevs

@FlorianKirmaier FlorianKirmaier changed the title 8247163: Fixing exception when clicking on JFXPanel when no Scene is set 8247163: JFXPanel throws exception on click when no Scene is set Jun 9, 2020
Copy link
Member

kevinrushforth left a comment

The fix looks correct to me (I haven't tested it yet). I left a minor comment inline.

@@ -449,8 +449,10 @@ protected void processMouseEvent(MouseEvent e) {
// The extra simulated mouse pressed event is removed by making the JavaFX scene focused.
// It is safe, because in JavaFX only the method "setFocused(true)" is called,
// which doesn't have any side-effects when called multiple times.
int focusCause = AbstractEvents.FOCUSEVENT_ACTIVATED;
stagePeer.setFocused(true, focusCause);
if(stagePeer != null) {

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 11, 2020 Member

Minor: space after the if

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Jun 16, 2020 Author Member

I've added the space.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 11, 2020

The fix and test both look good. Approved pending the minor format change (no need for a second reviewer).

8247163:
Added space based on code review
@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Jun 16, 2020

Great, I've added the requested minor format change!

@openjdk
Copy link

openjdk bot commented Jun 17, 2020

@FlorianKirmaier This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8247163: JFXPanel throws exception on click when no Scene is set

Reviewed-by: kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 8 commits pushed to the master branch:

  • 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
  • fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
  • bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
  • b200891: 8244824: TableView : Incorrect German translation
  • b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
  • afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
  • 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
  • ba501ef: 8246357: Allow static build of webkit library on linux

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 54e2507f0d69c07363d58aeb066932919a6c8cc6.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jun 17, 2020
@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Jun 17, 2020

/integrate

@openjdk openjdk bot added the sponsor label Jun 17, 2020
@openjdk
Copy link

openjdk bot commented Jun 17, 2020

@FlorianKirmaier
Your change (at version 6df4635) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 17, 2020

/sponsor

@openjdk openjdk bot closed this Jun 17, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels Jun 17, 2020
@openjdk
Copy link

openjdk bot commented Jun 17, 2020

@kevinrushforth @FlorianKirmaier The following commits have been pushed to master since your change was applied:

  • 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
  • fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
  • bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
  • b200891: 8244824: TableView : Incorrect German translation
  • b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
  • afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
  • 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
  • ba501ef: 8246357: Allow static build of webkit library on linux

Your commit was automatically rebased without conflicts.

Pushed as commit 1727dfa.

@openjdk openjdk bot removed the rfr label Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.