8255572: Axis does not compute preferred height properly when autoRanging is off

[JonathanVusich]

As noted in the corresponding JBS issue, Axis does not properly compute its preferred height when autoRanging is turned off. The simplest fix seems to be changing CategoryAxis so that tickLabelRotation is set to 90 degrees if there is not enough room for the category labels to layout horizontally. This fixes the fact that the axis labels are truncated and also ensures that the chart does not leave unused space from the layout calculations. CategoryAxis is already setting the categorySpacing property when autoRanging is off, so this seems to be an appropriate fix.

---

Progress

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

Issue

• JDK-8255572: Axis does not compute preferred height properly when autoRanging is off

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/342/head:pull/342
$ git checkout pull/342

Update a local copy of the PR:
$ git checkout pull/342
$ git pull https://git.openjdk.java.net/jfx pull/342/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 342

View PR using the GUI difftool:
$ git pr show -t 342

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/342.diff

[bridgekeeper[bot]]

Hi @JonathanVusich, 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 JonathanVusich" 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.

[JonathanVusich]

/signed

[bridgekeeper[bot]]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[mlbridge[bot]]

Webrevs

• 07: Full - Incremental (cc73f8dc)
• 06: Full - Incremental (9d2c8ac9)
• 05: Full - Incremental (d5a3fae961de244b0f1d9f7823b4378ce8218e16)
• 04: Full (dff9ee175c6ea2acfe4269778f576a2728c78e06)
• 03: Full - Incremental (0d11d3414ddd75a5241869e10ad335f37d364c4c)
• 02: Full - Incremental (6d83f79cd922d250c2f6722bfc746cd240d2f25e)
• 01: Full - Incremental (49a5e1f70433970be04de2e6f02561984371c19e)
• 00: Full (e3eb791e050d6bb0792c65efa6b7882fce143af8)

[kevinrushforth]

While this does fix the specific problem, it introduces a new one. If the labels are initially too big, but after resizing the window would now fit, it does not recompute the orientation. This means that you are left with labels that are rotated even when they don't need to be.

[JonathanVusich]

@kevinrushforth Thank you for catching that. Do you think it would be acceptable to simply rotate the labels back to zero if the user expands the window?

[kevinrushforth]

/reviewers 2

[openjdk[bot]]

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

[kevinrushforth]

The fix seems fine to me. I recommend also adding a test for a vertical axis.

[kevinrushforth]

@JonathanVusich will you be able to update the test to add adding a test for a vertical axis? That's the only outstanding change at this point.

[kevinrushforth]

@JonathanVusich Once a review is in progress, please don't force-push your branch without a compelling reason, since that makes it harder to do incremental reviews. In the future, we recommend git merge master rather than git rebase master, since the former doesn't require force-pushing.

[kevinrushforth]

I do see that there are no changes from last time, which is good, so it will be fine for this time.

When ready, you can add the new test by pushing a new commit.

[JonathanVusich]

@kevinrushforth I was not aware of that, thank you for letting me know! I wrote a new test for vertical axes and discovered that there were more issues with axis layout calculations, which I also addressed.

I also found out that the layout tests sometimes fail without a short pause using Util.sleep(). I don't really like having to add sleeps, but I did find that it got rid of any test flakiness.

[kleopatra]

wondering about the system test - what stands in the way to add a simple "normal" unit test?

[kleopatra]

wondering about the system test - what stands in the way to add a simple "normal" unit test?

looks like there might be a bug in StubFontLoader that makes a normal unit test fail w/out the fix: Axis uses a Text field (measure) for measuring size requirements for the tick labels - its font has no nativeFont set in the stub context such that all fields of the bounds of text are always 0, consequently the change in autoRange never kicks in. Hacking around with explicitly setting a tickLabelFont gives us a test that fails before and passes after the fix:

@Test
public void testRotatedStandAlone() {
    Pane root = new Pane();
    Scene scene = new Scene(root);
    Stage stage = new Stage();
    stage.setScene(scene);
    CategoryAxis xAxis = new CategoryAxis(FXCollections.observableArrayList(categories));
    // hack around stubFontLoader bug (?feature)
    xAxis.setTickLabelFont(new Font(8));
    BarChart<String, Number> chart = new BarChart<>(xAxis, new NumberAxis());
    chart.setPrefWidth(400);
    root.getChildren().add(chart);
    stage.show();
    assertEquals(90, xAxis.getTickLabelRotation(), 0.1);
}

Question is why the stubFontLoader fails: its load implementation looks like it should always set the nativeFont fiel to a fitting test font, but it doesn't if the name is a plain "Amble" (vs. a more specialized like "Amble Regular").

[openjdk[bot]]

@JonathanVusich Unknown command signed - for a list of valid commands use /help.

[JonathanVusich]

@kleopatra @kevinrushforth Are the system tests that I have provided sufficient for this PR to move forward or do they need to be rewritten as unit tests?

[kleopatra]

@kleopatra @kevinrushforth Are the system tests that I have provided sufficient for this PR to move forward or do they need to be rewritten as unit tests?

good question :) Personally, I would also add a unit test (even though it requires that hacky bit), it's more likely to be "seen". But also fine as-is, that is having system tests, IMO. Didn't run them, though .. *cough

[kevinrushforth]

We generally prefer unit tests where feasible, but in this case, I think the system tests are fine. I'll review this soon (I had missed your earlier update and then got busy).

[openjdk[bot]]

@JonathanVusich Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[bridgekeeper[bot]]

@JonathanVusich This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@JonathanVusich This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.