RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]

[Kevin Rushforth]

On Mon, 1 Feb 2021 23:12:08 GMT, Jonathan Vusich <github.com+31666175+JonathanVusich at openjdk.org> wrote:

>> 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.
>
> Jonathan Vusich has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Add tests for vertical axis as well
>  - Improve layout calculations for rotated text

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.

modules/javafx.controls/src/main/java/javafx/scene/chart/CategoryAxis.java line 365:

> 363:             double requiredLengthToDisplay = calculateRequiredSize(side.isVertical(), tickLabelRotation);
> 364:             if (requiredLengthToDisplay > length) {
> 365:                 if (tickLabelRotation != 90) {

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

modules/javafx.controls/src/main/java/javafx/scene/chart/CategoryAxis.java line 366:

> 364:             if (requiredLengthToDisplay > length) {
> 365:                 if (tickLabelRotation != 90) {
> 366:                     var rotatedRequiredLengthToDisplay = calculateRequiredSize(side.isVertical(), 90);

I prefer an explicit type here (double, I think) so I don't have to go look at the method to know what type it is without going and finding the method.

modules/javafx.controls/src/main/java/javafx/scene/chart/CategoryAxis.java line 373:

> 371:             } else {
> 372:                 if (tickLabelRotation != 0) {
> 373:                     var unrotatedRequiredLengthToDisplay = calculateRequiredSize(side.isVertical(), 0);

Same comment as above about using double rather than var.

tests/system/src/test/java/test/javafx/scene/chart/ChartXAxisLayoutTest.java line 65:

> 63:             stage = primaryStage;
> 64:             rootPane = new VBox();
> 65:             stage.setScene(new Scene(rootPane, 600, 800));

I recommend setting the x and y position of the stage to 0 here.

tests/system/src/test/java/test/javafx/scene/chart/ChartYAxisLayoutTest.java line 65:

> 63:             stage = primaryStage;
> 64:             rootPane = new VBox();
> 65:             stage.setScene(new Scene(rootPane, 800, 600));

I recommend setting the x and y position of the stage to 0 here.

-------------

PR: https://git.openjdk.java.net/jfx/pull/342