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

8242548: Wrapped labeled controls using -fx-line-spacing cut text off #173

Closed
wants to merge 2 commits into from

Conversation

@hjohn
Copy link
Collaborator

hjohn commented Apr 12, 2020

This is a solution for 8242548. There was zero test coverage, so I added a few tests for this as well.


Progress

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

Issue

  • JDK-8242548: Wrapped labeled controls using -fx-line-spacing cut text off

Reviewers

  • Ajit Ghaisas (aghaisas - Reviewer)
  • Kevin Rushforth (kcr - Reviewer)

Download

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

@bridgekeeper bridgekeeper bot added the oca label Apr 12, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 12, 2020

Hi @hjohn, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user hjohn" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@hjohn hjohn changed the title WIP: 9064482: Honor line spacing in Labeled reflow calculation 9064482: (WIP) Honor line spacing in Labeled reflow calculation Apr 12, 2020
@hjohn hjohn changed the title 9064482: (WIP) Honor line spacing in Labeled reflow calculation 8242548: (WIP) Honor line spacing in Labeled reflow calculation Apr 13, 2020
@hjohn hjohn force-pushed the hjohn:fix/9064482 branch from cfe224c to 86b2f01 Apr 13, 2020
@hjohn hjohn changed the title 8242548: (WIP) Honor line spacing in Labeled reflow calculation 8242548: Honor line spacing in Labeled reflow calculation Apr 13, 2020
@bridgekeeper bridgekeeper bot removed the oca label Apr 14, 2020
@openjdk openjdk bot added the rfr label Apr 14, 2020
@mlbridge
Copy link

mlbridge bot commented Apr 14, 2020

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 14, 2020

@aghaisas can you review this?

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 21, 2020

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review Apr 21, 2020
@openjdk
Copy link

openjdk bot commented Apr 21, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 21, 2020

@aghaisas can you also review this?

@aghaisas
Copy link

aghaisas commented Apr 23, 2020

I tested with the test program in the bug and can confirm that this PR fixes the reported issue.

I found that the unit test (together with stub) passes without the code fix as well.
I think, this is a limitation of our test framework that we need to do code fix + stub fix as well. I find that you have done these changes.

@hjohn hjohn force-pushed the hjohn:fix/9064482 branch 2 times, most recently from 8d6084e to 6b401bc Apr 23, 2020
@hjohn hjohn force-pushed the hjohn:fix/9064482 branch from 6b401bc to 20ccd12 Apr 23, 2020
@hjohn
Copy link
Collaborator Author

hjohn commented Apr 23, 2020

I added a comment clarification and passed the line spacing to the ellipsis height calculation as well (just in case there is a multi line ellipsis).

The test indeed doesn't quite cover the change, the StubTextLayout is not advanced enough to actually do line wrapping calculations (it ignores wrap height), and would need to be updated.

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 23, 2020

@hjohn Updates to a PR under review should be done as additional commits. Please do not force push to your branch. This makes it more difficult for reviewers, since the incremental diffs from the last time it was reviewed are lost. A force push of your branch should only be used in extraordinary circumstances.

@kevinrushforth
Copy link
Member

kevinrushforth commented Apr 28, 2020

The code change and test look fine. Please change the PR title to match the JBS issue title, so:

8242548: Wrapped labeled controls using -fx-line-spacing cut text off
@hjohn hjohn changed the title 8242548: Honor line spacing in Labeled reflow calculation 8242548: Wrapped labeled controls using -fx-line-spacing cut text off Apr 28, 2020
@openjdk
Copy link

openjdk bot commented May 1, 2020

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

8242548: Wrapped labeled controls using -fx-line-spacing cut text off

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

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

  • 7b06190: 8242548: Wrapped labeled controls using -fx-line-spacing cut text off
  • 435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
  • c14cc44: 8244417: support static build for Windows
  • b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
  • b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
  • 0f87d20: 8244487: One Windows 10 SDK file missing from FX build
  • 4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2
  • 236e2d6: 8244421: Wrong scrollbar position on touch enabled devices
  • 0385563: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi
  • 2e90076: 8242507: Add support for Visual Studio 2019
  • ... and 34 more: https://git.openjdk.java.net/jfx/compare/b1fdc45f01ce7374fecf4705867fdb4baf6142a2...master

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

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 (@aghaisas, @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 May 1, 2020
@aghaisas
Copy link

aghaisas commented May 12, 2020

@hjohn, this PR is ready to be merged.
You need to comment /integrate on this PR as instructed by the bot above. I will sponsor it once you do it.

@hjohn
Copy link
Collaborator Author

hjohn commented May 12, 2020

/integrate

@openjdk openjdk bot added the sponsor label May 12, 2020
@openjdk
Copy link

openjdk bot commented May 12, 2020

@hjohn
Your change (at version 8fa8f3f) is now ready to be sponsored by a Committer.

@hjohn
Copy link
Collaborator Author

hjohn commented May 12, 2020

Sorry I figured only a committer was allowed to use that command.

@aghaisas
Copy link

aghaisas commented May 13, 2020

/sponsor

@openjdk openjdk bot closed this May 13, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels May 13, 2020
@openjdk
Copy link

openjdk bot commented May 13, 2020

@aghaisas @hjohn The following commits have been pushed to master since your change was applied:

  • 435671e: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
  • c14cc44: 8244417: support static build for Windows
  • b14e085: 8244735: Error on iOS passing keys with unicode values greater than 255
  • b0d66d0: 8242508: Upgrade to Visual Studio 2019 version 16.5.3
  • 0f87d20: 8244487: One Windows 10 SDK file missing from FX build
  • 4ec163d: 8242001: ChoiceBox: must update value on setting SelectionModel, part2
  • 236e2d6: 8244421: Wrong scrollbar position on touch enabled devices
  • 0385563: 8243255: Font size is large in JavaFX app with enabled Monocle on Raspberry Pi
  • 2e90076: 8242507: Add support for Visual Studio 2019
  • 39d9c3b: 8244110: NPE in MenuButtonSkinBase change listener
  • 99f7747: 8241999: ChoiceBox: incorrect toggle selected for uncontained
  • 1cae6a8: 8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards
  • 2b9eb52: 8237504: Update copyright header for files modified in 2020
  • 3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
  • 8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
  • 66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
  • e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
  • e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
  • ceb3fce: 8087555: [ChoiceBox] uncontained value not shown
  • 818ac00: 8175358: Memory leak when moving MenuButton into another Scene
  • 91d4c8b: 8241737: TabPaneSkin memory leak on replacing selectionModel
  • 48476eb: 8241582: Infinite animation does not start from the end when started with a negative rate
  • dedf7cb: 8242490: Upgrade to gcc 9.2 on Linux
  • 5e9fb82: 8242577: Cell selection fails on iOS most of the times
  • 69e4266: 8242489: ChoiceBox: initially toggle not sync'ed to selection
  • 1d88180: 8243112: Skip failing test SVGTest.testSVGRenderingWithPattern
  • ec8608f: 8223298: SVG patterns are drawn wrong
  • e82046e: 8242530: [macos] Some audio files miss spectrum data when another audio file plays first
  • 7044cef: 8241476: Linux build warnings issued on gcc 9
  • 9d50c4c: Merge
  • 4d69a0d: 8241629: [macos10.15] Long startup delay playing media over https on Catalina
  • ef37669: Merge
  • f2bca9f: Merge
  • 6900d29: Merge
  • e91bec4: Merge
  • 66a8f49: Merge
  • fde42da: Merge
  • e21fd1f: Merge
  • 443c845: Merge
  • 31e63de: Merge
  • 14c6938: 8236798: Enhance FX scripting support
  • bfb2d0e: Merge
  • 39f6127: Merge

Your commit was automatically rebased without conflicts.

Pushed as commit 7b06190.

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.