Skip to content

Implement Grouped/Merged Y-Axes Feature (Issue #916)#917

Open
ngmikeng wants to merge 6 commits into
knowm:developfrom
ngmikeng:feature/issue-916-grouped-y-axes
Open

Implement Grouped/Merged Y-Axes Feature (Issue #916)#917
ngmikeng wants to merge 6 commits into
knowm:developfrom
ngmikeng:feature/issue-916-grouped-y-axes

Conversation

@ngmikeng
Copy link
Copy Markdown

@ngmikeng ngmikeng commented May 2, 2026

xchart-grouped-yaxes-demo
ngmikeng and others added 4 commits May 1, 2026 20:59
- Implemented merged Y-axis functionality allowing multiple series with different value ranges to share a single axis line.
- Introduced new methods in Styler for merging Y-axis groups and determining visual group IDs.
- Enhanced Axis and AxisPair classes to manage master/slave relationships for merged axes.
- Added synchronization for tick locations and labels between master and slave axes.
- Created MergedYAxisDemo to demonstrate the new feature.
- Added unit tests for merged Y-axes functionality to ensure correctness and backward compatibility.

Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
@timmolter
Copy link
Copy Markdown
Member

Here's the full analysis of PR #917:

PR #917 — Implement Grouped/Merged Y-Axes
Status: MERGEABLE / CLEAN / CI GREEN ✓ — but has several issues that should be fixed before merging.

What it does
Adds opt-in mergeYAxisGroups(int...) support so multiple logical Y-axis groups can share a single visible axis line while retaining independent scales. Includes a colocate-slave mode (setMergedAxisColocateSlaveLabels) that stacks slave tick labels on the master's column.

Issues Found
🔴 Critical
Styler.java (base class) polluted with axes-only state.
Three new fields (yAxisGroupMergeMap, mergedAxisColocateSlaveLabels, mergedAxisColocatedSlaveLabelsGap) and their API methods were added to the base Styler, which is shared by Pie, Dial, Radar, HeatMap, etc. These fields are meaningless for non-axes charts. Per project rules: "Never add state to the base Styler class unless it truly applies to every chart type." They belong in AxesChartStyler.
🟠 Moderate
Demo file violates naming and structure conventions.

Named MergedYAxisDemo.java → must be TestForIssue916.java (implements issue #916)
Missing the required public static XYChart getChart() method — only has main(), so it can't be invoked from unit tests headlessly.
Left-side colocate mode is asymmetric with the right side.
In Step 3 (left-side painting in AxisPair.java), the colocate branch only skips slave width estimation but there's no matching colocate painting block. The right side has a full if (colocate) branch with masterAxis.addColocatedSlave(...). The left side never calls addColocatedSlave — meaning colocate mode silently does nothing on the left side.

Raw types in AxisTickLabels.java.
List colocatedSlaves = yAxis.getColocatedSlaves() and Axis slaveAxis = (Axis) colocatedSlaves.get(s) use raw types. The Axis class already declares List for colocatedSlaves — the raw usage should be parameterized.

🟡 Minor
Fully-qualified java.util.Collections.sort(...) in AxisPair.java instead of adding an import — breaks the import consistency style.

BitmapEncoder.java change appears to be formatting-only (no functional bug fix), yet it's bundled into this PR.

No multi-chart regression test. The PR only tests XYChart. Since AxisPair.java / Styler.java / PlotSurface_AxesChart.java are touched (all high-risk ripple zones), CategoryChart and BubbleChart should also be smoke-tested.

ngmikeng added 2 commits May 18, 2026 23:37
- Merged-axis fields/methods moved from Styler.java → AxesChartStyler.java
- MergedYAxisDemo.java deleted; `TestForIssue916.java` created
- Left-side colocate symmetry confirmed (already present)
- Raw types in AxisTickLabels.java and Axis.java eliminated
- `Collections.sort` fully qualified in AxisPair.java → import added
- CategoryChart + BubbleChart smoke tests added to MergedYAxisTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants