Repository navigation
Improve XLSX clustered column chart visual parity - #156
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new right-side legend rendering can lead to duplicated legends (existing default legend plus the new right legend), which is a correctness/layout issue that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves XLSX clustered column/bar chart rendering parity (targeting LibreOffice behavior) by parsing additional OOXML chart metadata and updating rendering/layout logic, plus adding a focused regression test and updating the checked-in benchmark reports/manifest to include the new fixture.
Changes:
- Parse chart legend position, built-in chart style, and
<c:gapWidth>from OOXML and plumb them throughExcelChartInfo. - Update chart rendering to (a) apply a style-10 frame/title layout when the chart fits on-page, (b) render a right-side legend when explicitly requested, (c) honor
gapWidthfor bar sizing, and (d) adjust narrow-positive-range autoscaling. - Add/strengthen regression assertions for axis bounds, bar spacing, series name legend text, and style-10 title bolding; update benchmark report artifacts to include the new case.
File summaries
| File | Description |
|---|---|
| tests/MiniPdf.Tests/ExcelToPdfConverterTests.cs | Strengthens the clustered non-zero bar/column chart regression test with gap/axis/title/legend assertions. |
| src/MiniPdf/ExcelReader.cs | Parses chart style, legend position, and gap width from chart XML into ExcelChartInfo. |
| src/MiniPdf/ExcelToPdfConverter.cs | Updates chart rendering/layout (style 10 framing, right-side legend, axis scaling tweak, gapWidth sizing). |
| tests/Issue_Files/reports_xlsx/comparison_report.md | Updates the checked-in benchmark report to include the new XLSX fixture and updated summary sections. |
| tests/Issue_Files/reports_xlsx/comparison_report.json | Adds the new case’s JSON report entry (scores, pages, diff metadata). |
| tests/Issue_Files/reports_xlsx/comparison_manifest.json | Adds the new case to the report manifest and normalizes formatting. |
Review details
Suppressed comments (2)
src/MiniPdf/ExcelToPdfConverter.cs:2894
- The right-legend layout uses the series loop index to compute entryY; if some series names are empty, this leaves vertical gaps (and can push later entries outside the allocated legend area sooner). Track a separate entry index that only increments when an entry is actually rendered.
{
var seriesName = chart.Series[index].Name;
if (string.IsNullOrEmpty(seriesName)) continue;
var entryY = legendY - index * (labelFontSize + 6f);
page.AddRectangle(legendX, entryY, 7f, 7f, ChartColors[index % ChartColors.Length]);
src/MiniPdf/ExcelToPdfConverter.cs:2806
- legendWidth is capped at 32pt / 10% chart width, but the legend text is drawn without clipping and can extend past the chart (and even the page when the chart is near the right edge). It would be safer to size legendWidth based on the measured max series-name width (plus swatch/padding) and clamp it to the available width, so plotRight and the legend stay within bounds.
&& chart.Series.Any(series => !string.IsNullOrEmpty(series.Name))
&& chartFitsPage;
var legendWidth = showRightLegend ? Math.Min(32f, width * 0.1f) : 0f;
var plotLeft = x + padding + (useStyle10Layout ? 47f : 40f);
var plotRight = x + width - padding - 10f - legendWidth;
- Files reviewed: 6/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2801
to
+2803
| var showRightLegend = chart.LegendPosition is "r" or "tr" | ||
| && chart.Series.Any(series => !string.IsNullOrEmpty(series.Name)) | ||
| && chartFitsPage; |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change parses chart style, legend position, and gap width from XLSX chart XML. It applies these values to chart layout, legends, bar sizing, and axis scaling. Tests and LibreOffice comparison reports cover a clustered non-zero column chart. ChangesXLSX chart parity
Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant XLSXChartXml
participant ExcelReader
participant ExcelToPdfConverter
participant ComparisonTests
XLSXChartXml->>ExcelReader: provide style, legend position, and gap width
ExcelReader->>ExcelToPdfConverter: pass ExcelChartInfo metadata
ExcelToPdfConverter->>ComparisonTests: render chart PDF
ComparisonTests->>ComparisonTests: validate layout, labels, spacing, and title
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
c:gapWidthfor vertical and horizontal bar chartsFixes #155
Validation
dotnet test tests/MiniPdf.Tests— 189 passed, 0 failedscripts/Run-Benchmark_issues.ps1 -Filter "XlsxIssue152_ClusteredNonZeroBarChart" -Engine libre -SkipInstall -HeatmapsSummary by CodeRabbit
New Features
Tests