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

8232811: Dialog's preferred size no longer accommodates multi-line strings #63

Closed
wants to merge 8 commits into from

Conversation

@tsayao
Copy link

tsayao commented Dec 9, 2019

https://bugs.openjdk.java.net/browse/JDK-8232811

This one was hard to tackle.

./gradlew -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests test.robot.javafx.scene.dialog.DialogWithOwnerSizingTest

Progress

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

Issue

JDK-8232811: Dialog's preferred size no longer accommodates multi-line strings

Approvers

  • Kevin Rushforth (kcr - Reviewer)
  • Ambarish Rapte (arapte - Reviewer)
tsayao and others added 4 commits Nov 3, 2019
Merge
Merge from upstream
Thiago Milczarek Sayão
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 9, 2019

👋 Welcome back tsayao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Dec 9, 2019
@mlbridge
Copy link

mlbridge bot commented Dec 9, 2019

Webrevs

tsayao added 2 commits Dec 10, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 12, 2019

This will need two reviewers.

@kevinrushforth kevinrushforth self-requested a review Dec 12, 2019
Copy link
Member

kevinrushforth left a comment

The fix looks good (with a minor formatting comment). The test needs a couple of changes.

Thread.sleep(500);
Assert.assertEquals(dialog.getDialogPane().getWidth(), dialog2.getDialogPane().getWidth(), 0.0);
Assert.assertEquals(dialog.getDialogPane().getHeight(), dialog2.getDialogPane().getHeight(), 0.0);
hide();

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Dec 17, 2019

Member

When running the test on Linux without your fix, it gets the expected assertion failure, but then crashes. It looks like there is some interaction between the dialog still being open and the eventual call to stage.hide() and Platform.exit. I'll file a separate bug for this, since it is unrelated to your fix (I can make it happen with or without your fix).

I recommend putting the call to hide() in a try ... finally.

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 18, 2019

@arapte - can you be the second reviewer on this?

Copy link

arapte left a comment

The fix itself looks good. suggested some changes in test and a query.
Please take a look.

Thiago Milczarek Sayão added 2 commits Dec 19, 2019
@arapte
arapte approved these changes Dec 20, 2019
Copy link

arapte left a comment

Looks good to me. Test passes on all three platforms.

@openjdk openjdk bot removed the rfr label Dec 20, 2019
@openjdk
Copy link

openjdk bot commented Dec 20, 2019

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

8232811: Dialog's preferred size no longer accommodates multi-line strings

Reviewed-by: kcr, arapte
  • 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 /solves command.

Since the source branch of this PR was last updated there have been 9 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge master into your branch first.

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, @arapte) 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 Dec 20, 2019
Copy link
Member

kevinrushforth left a comment

Looks good.

I can sponsor this fix.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 2, 2020

@tsayao this is ready for you to integrate.

@tsayao
Copy link
Author

tsayao commented Jan 2, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Jan 2, 2020

@tsayao
Your change (at version 1d40aa9) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Jan 2, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jan 2, 2020

/sponsor

@openjdk openjdk bot closed this Jan 2, 2020
@openjdk openjdk bot added the integrated label Jan 2, 2020
@openjdk openjdk bot removed sponsor ready labels Jan 2, 2020
@openjdk
Copy link

openjdk bot commented Jan 2, 2020

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

  • 4c6ebfb: 8236484: Compile error in monocle dispman
  • 8367e1a: 8130738: Add tabSize property to Text and TextFlow
  • 69e4ef3: 8235627: Blank stages when running JavaFX app in a macOS virtual machine
  • 5e0fb91: 8235364: Update copyright header for files modified in 2019
  • 935e99d: 8207957: TableSkinUtils should not contain actual code implementation
  • 4e005e4: 8227808: Make GTK3 libraries mandatory for building on Linux
  • 1140d34: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
  • d2d44b4: 8207759: VK_ENTER not consumed by TextField when default Button exists
  • fc539b5: 8223296: NullPointerException in GlassScene.java at line 325

Your commit was automatically rebased without conflicts.

Pushed as commit 1952606.

@mlbridge
Copy link

mlbridge bot commented Jan 6, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: 1952606
Author: Thiago Milczarek Sayao <tsayao at openjdk.org>
Committer: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-01-02 17:01:13 +0000
URL: https://git.openjdk.java.net/jfx/commit/1952606a

8232811: Dialog's preferred size no longer accommodates multi-line strings

Reviewed-by: kcr, arapte

! modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp
+ tests/system/src/test/java/test/robot/javafx/scene/dialog/DialogWithOwnerSizingTest.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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