Repository navigation
Allow tick spacing hints per Y-axis group - #1035
Conversation
timmolter
left a comment
timmolter
left a comment
There was a problem hiding this comment.
Thanks for this, the approach is sound and the tests are thorough. I ran the xchart module tests locally on this branch (175 tests, all passing) and the per-group plumbing through Axis_Y and the Number/Callback calculators looks right. Logarithmic axes ignoring the hint and slaves following the master are the correct calls.
A few things to address before merge, detailed inline:
- A group hint larger than the available tick space produces an axis with no ticks at all, and the demo sits right at that edge.
- The formatter left several comment blocks in
AxisTickCalculator_with orphan fragments. - Consider the
YAxisGroupnaming convention the other per-group styler methods use.
| return; | ||
| } | ||
| if (axisDirection == Direction.Y && tickSpace < styler.getYAxisTickMarkSpacingHint()) { | ||
| if (axisDirection == Direction.Y && tickSpace < getTickMarkSpacingHint()) { |
There was a problem hiding this comment.
With a per-group hint this early return becomes much easier to hit. The whole point of a group hint for a 0/1 axis is to set it large, and as soon as the hint exceeds the available tick space the axis renders with no ticks and no labels at all.
I rendered the chart from TestForIssue797 at a few heights:
| chart height | plot height | hint | group 1 labels |
|---|---|---|---|
| 600 | 523 | 400 | 0, 1 |
| 500 | 423 | 400 | (none) |
| 400 | 323 | 400 | (none) |
| 300 | 223 | 200 | 0, 1 |
So the demo works at its built-in size and goes blank the moment the window is shrunk a little. That is pre-existing behavior for the global hint, but a user setting a group hint has explicitly asked for sparse ticks, not for none. When a group hint is set and tickSpace < hint, could we fall back to ticks at minValue and maxValue instead of returning empty? Alternatively clamp the effective hint to tickSpace before the gridStep calculation. Either way, a test for the "hint larger than plot" case would be good.
|
|
||
| // for very short plots, squeeze some more ticks in than normal into the Y-Axis | ||
| if (axisDirection == Direction.Y && tickSpace < 160) { | ||
| if (axisDirection == Direction.Y && tickSpace < 160 && !hasYAxisGroupSpacingHint()) { |
There was a problem hiding this comment.
Agree that an explicit group hint should win over the small-plot squeeze. A one-line comment here saying so would help the next reader, since both this and the equally-spaced bypass above are silent behavior changes that only kick in when a group hint exists.
| // ticks must not become visible labels. Gridlines and tick marks already clip to the plot bounds, | ||
| // but the tick-label renderer clips to the taller axis-column bounds, so an out-of-range label -- | ||
| // grid step below minValue and the loop bound runs two grid steps past maxValue); those | ||
| // overshoot |
There was a problem hiding this comment.
The formatter reflowed this comment block and left orphan fragments on their own lines (overshoot, bounds,, --, the, tickSpace].) through line 394 and again around line 406. Could you rewrap these by hand so the prose reads cleanly? The formatter won't fix comment wrapping on its own.
| * global hint again | ||
| * @return this styler | ||
| */ | ||
| public AxesChartStyler setYAxisTickMarkSpacingHint( |
There was a problem hiding this comment.
Naming suggestion: the existing per-group methods in this class use a YAxisGroup prefix (setYAxisGroupPosition, setYAxisGroupTickMarksColorMap, setYAxisGroupTickLabelsColorMap). Following that here would give setYAxisGroupTickMarkSpacingHint(int, Integer) and getYAxisGroupTickMarkSpacingHint(int). It also avoids having an overload pair where the no-arg getter returns int and the one-arg getter returns a nullable Integer, which is easy to misread at a call site.
| chart.getStyler().setYAxisMin(1, 0.0); | ||
| chart.getStyler().setYAxisMax(1, 1.0); | ||
| chart.getStyler().setYAxisTickMarkSpacingHint(40); | ||
| chart.getStyler().setYAxisTickMarkSpacingHint(1, 400); |
There was a problem hiding this comment.
A hint of 400 only leaves room for ticks when the plot is at least 400 pixels tall, which at this chart size is a margin of about 120 pixels. Resizing the Swing window a little shorter blanks the right axis entirely. Until the calculator handles that case (see my comment in AxisTickCalculator_), 200 gives 0, 0.5, 1 at 600 tall and 0, 1 at 300 tall, which is a more forgiving demo.
|
Thanks for checking the resized charts. Fixed in b60061c.
Java 17 |
2 participants
Adds per-group tick spacing hints for linear numeric Y-axes. A chart can keep detailed ticks on one axis and use wider spacing on another:
Passing
nulltosetYAxisGroupTickMarkSpacingHintrestores the global setting. Explicit group hints take precedence over equally spaced data and the short-plot squeeze, and work with custom label formatters. Merged axes follow their master's ticks; logarithmic and categorical tick rules are unchanged. The hint is in pixels, not an interval in data units.When a group hint exceeds the available tick space, the minimum and maximum ticks are retained, provided positive space remains and the formatter produces distinct endpoint labels. Global-hint behavior is unchanged. The demo uses a 200-pixel hint.
Tests cover independent groups, removing overrides, custom formatting, small plots, oversized hints, resizing, merged axes, CategoryChart rendering, and X-axis isolation. The resize regression checks a 400-pixel group hint at chart heights 600, 500, 400, 300, then 600 again.
Validation: Java 17
mvn clean verifypassed (178 tests, no failures/errors/skips). Changed Java files pass a direct google-java-format 1.23.0 check; the repository skips its Maven formatter. Rendered the demo at all four heights with hints of 200 and 400, and visually checked the full-height and reduced-height outputs.Fixes #797.