Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 26 Apr 2021 21:06:41 GMT, Kevin Rushforth wrote: >> 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. > > 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. @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. - PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 26 Apr 2021 13:14:20 GMT, Jonathan Vusich
wrote:
>> 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)?
>
> 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.
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.
-
PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Fri, 23 Apr 2021 23:28:49 GMT, Kevin Rushforth wrote:
>> 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
>
> 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)?
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. Currently, 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.
-
PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 1 Feb 2021 23:12:08 GMT, Jonathan Vusich
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
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 1 Feb 2021 23:12:08 GMT, Jonathan Vusich 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 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). - PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 29 Mar 2021 22:19:08 GMT, Jonathan Vusich wrote: > > > @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 - PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Wed, 3 Feb 2021 17:08:17 GMT, Jeanette Winzenburg
wrote:
>> wondering about the system test - what stands in the way to add a simple
>> "normal" unit test?
>
>>
>>
>> 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 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").
@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?
-
PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Tue, 2 Feb 2021 12:02:17 GMT, Jeanette Winzenburg
wrote:
>
>
> 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 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").
-
PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 1 Feb 2021 23:13:02 GMT, Jonathan Vusich wrote: >> 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. > > @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. wondering about the system test - what stands in the way to add a simple "normal" unit test? - PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
On Mon, 1 Feb 2021 21:37:44 GMT, Kevin Rushforth wrote: >> @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. > > 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. @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. - PR: https://git.openjdk.java.net/jfx/pull/342
Re: RFR: 8255572: Axis does not compute preferred height properly when autoRanging is off [v6]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/342/files - new: https://git.openjdk.java.net/jfx/pull/342/files/dff9ee17..d5a3fae9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=342=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=342=04-05 Stats: 134 lines in 3 files changed: 120 ins; 1 del; 13 mod Patch: https://git.openjdk.java.net/jfx/pull/342.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/342/head:pull/342 PR: https://git.openjdk.java.net/jfx/pull/342
