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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JonathanVusich
Copy link

@JonathanVusich JonathanVusich commented Oct 29, 2020

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

  • Change must not contain extraneous whitespace
  • 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 bridgekeeper bot added the oca label Oct 29, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 29, 2020

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
Copy link
Author

@JonathanVusich JonathanVusich commented Oct 29, 2020

/signed

@bridgekeeper bridgekeeper bot added the oca-verify label Oct 29, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 29, 2020

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!

@bridgekeeper bridgekeeper bot removed oca oca-verify labels Nov 11, 2020
@openjdk openjdk bot added the rfr label Nov 11, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 11, 2020

Webrevs

@kevinrushforth kevinrushforth requested a review from aghaisas Nov 11, 2020
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Can you merge in the latest jfx/master? That way the GitHub actions build/test will be run (although it won't run the new headful test). You also need a copyright header as noted below.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Nov 24, 2020

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
Copy link
Author

@JonathanVusich JonathanVusich commented Nov 25, 2020

@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 kevinrushforth self-requested a review Dec 5, 2020
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 12, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Dec 12, 2020

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

}
if (!isAutoRanging()) setTickLabelRotation(tickLabelRotation);
Copy link
Collaborator

@aghaisas aghaisas Dec 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I thought this setTickLabelRotation() call violates the method javadoc above. On a second look, I found that the javadoc is applicable only if AutoRanging() is true.
Method calls - calculateNewSpacing() and calculateNewFirstPos() also set properties if AutoRanging is false. Hence, this fix seems OK to me.

Copy link
Collaborator

@kleopatra kleopatra Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I thought this setTickLabelRotation() call violates the method javadoc above. On a second look, I found that the javadoc is applicable only if AutoRanging() is true.
Method calls - calculateNewSpacing() and calculateNewFirstPos() also set properties if AutoRanging is false. Hence, this fix seems OK to me.

hmm ... indeed they do, unexpected for me, reading the must-not-effect-the-state in the javadoc ;) Sounds like a doc error, if the intention was to clearly state that the method shouldn't do the auto-ranging itself?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Dec 24, 2020

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

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 11, 2021

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

@JonathanVusich JonathanVusich force-pushed the jonathan/fix-chart-axis-labels branch from 0d11d34 to dff9ee1 Compare Feb 1, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 1, 2021

@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
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 1, 2021

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
Copy link
Author

@JonathanVusich JonathanVusich commented Feb 1, 2021

@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
Copy link
Collaborator

@kleopatra kleopatra commented Feb 2, 2021

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

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Feb 3, 2021

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
Copy link

@openjdk openjdk bot commented Mar 17, 2021

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

@JonathanVusich
Copy link
Author

@JonathanVusich JonathanVusich commented Mar 29, 2021

@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
Copy link
Collaborator

@kleopatra kleopatra commented Mar 30, 2021

@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
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 22, 2021

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

Copy link
Member

@kevinrushforth kevinrushforth left a comment

I think it looks pretty good. I left one question about what happens if an app explicitly sets a rotation of, say, 45 degrees. I also left a couple other minor comments.

// try to rotate the text to increase the density
if (side.isHorizontal() && tickLabelRotation != 90) {
tickLabelRotation = 90;
if (tickLabelRotation != 90) {
Copy link
Member

@kevinrushforth kevinrushforth Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference between the new algorithm and the old is that the new doesn't take into account which side you are on. If the only two values are 0 or 90, then it wouldn't matter. But what happens when someone sets a 45 degree rotation (or 30)?

Copy link
Author

@JonathanVusich JonathanVusich Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's complicated. The algorithm that calculates the required space for the tick labels does not take into account the tick label rotation. I did not look into adding this at this time because that opens up a lot of other potential pathways that seem broader in scope. With these changes, if the application detects that there is not enough space to display the text at the current rotation (which is assumed to be either 90 or 0 degrees), we rotate it by 90 degrees and remeasure. If that second remeasuring shows that the tick labels can be fully displayed at that rotation, the tick label rotation is overwritten to either 0 or 90 degrees. Theoretically if a user defines a set tick label rotation, it can be overwritten under these circumstances. However, I think this is reasonable given that when autoRanging is disabled a number of other properties are also overwritten in a similar manner.

Copy link
Member

@kevinrushforth kevinrushforth Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is complicated, and I agree doesn't work entirely correctly today, but this change breaks setting the axis tick label rotation completely, even in case where will fit without forcing the rotation to 90.

To reproduce this problem, run the Ensemble8 app with your fix, select the Bar Chart sample, and attempt to change the X axis tickLabelRotation, either using the slider or entering a number in the text field. You will be unable to change the value since the code you added will overwrite it.

If you run it without your fix, you can change the X axis tickLabelRotation. It won't take effect right away (that's a bug), but if you uncheck and then recheck tickLabelsVisible it will re-layout the chart and the axis labels will be drawn rotated.

With auto-ranging selected, if you run with your fix, you can change the axis, but it won't actually do anything, even in the case where the label fits.

Without your fix, you can rotate the labels with the slider just fine.

So while I agree that improving the situation when a rotation is set could be done as a follow-up bug, we need a solution that won't break existing use cases that are working.

Copy link
Author

@JonathanVusich JonathanVusich Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinrushforth Thank you for pointing this out, I was not aware of the Ensemble8 app that provided a testbed for this functionality. I will dig into this again and figure out a better solution.

On a somewhat unrelated note to this PR, are there any instructions for how to get the JavaFX repository set up in an IDE? I have had a terrible time dealing with compile warnings in my IntelliJ IDE even though I can run everything successfully through Gradle on the command line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr
4 participants