On Mon, 1 Feb 2021 23:12:08 GMT, Jonathan Vusich
<github.com+31666175+jonathanvus...@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
