Fix legacy XLSX drawing and merged-cell rendering - #130
Merged
Merged
Conversation
Honor authored dimensions and vertical centering, render safe VML/OLE previews, preserve stacked text and cross-page merged cells, and make issue benchmark manifests BOM tolerant. Refs #129
|
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 (14)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe XLSX reader now parses rotated text, vertical page centering, and legacy VML images. The PDF converter renders stacked text, merged-cell continuations, centered worksheets, and positioned previews. Tests and benchmark reports cover the updated output. ChangesXLSX rendering pipeline
STKaiti font normalization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant XLSX
participant ExcelReader
participant ExcelToPdfConverter
participant PdfPage
XLSX->>ExcelReader: Read styles, page setup, and legacy VML
ExcelReader->>ExcelToPdfConverter: Provide rotation, centering, and image metadata
ExcelToPdfConverter->>PdfPage: Render worksheet content and previews
ExcelToPdfConverter->>PdfPage: Apply worksheet centering and block translation
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Legacy VML image ingestion can admit non-PNG/JPG payloads which downstream rendering will mis-declare as PNG, risking corrupted output or runtime failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR targets Microsoft 365 compatibility for the Issue202609031340.xlsx fixture by improving XLSX layout fidelity (default column widths, vertical centering), legacy VML/OLE preview image extraction (including EMF rasterization without executing OLE payloads), stacked-text handling (textRotation="255"), and vertically merged-cell continuation across automatic page breaks. It also updates benchmark tooling/artifacts to tolerate UTF-8 BOM manifests and adds focused regression coverage and refreshed benchmark evidence.
Changes:
- Add XLSX parsing/rendering support for vertical centering, stacked text (
textRotation=255), legacy VML image anchors, and cross-page vertical merge continuations. - Extend legacy image ingestion to rasterize EMF/WMF previews on Windows, and normalize font name mapping for
stkaiti. - Update benchmark loader to accept UTF-8 BOM JSON and refresh issue benchmark artifacts + add focused regression test.
File summaries
| File | Description |
|---|---|
| tests/MiniPdf.Tests/XlsxIssueFileTests.cs | Adds a focused regression test for layout, centering, stacked text rotation, and (Windows-only) OLE preview images. |
| tests/MiniPdf.Benchmark/compare_pdfs.py | Loads BOM-prefixed JSON manifests and updates report API typing. |
| tests/Issue_Files/reports_xlsx/comparison_report.md | Refreshes rendered comparison report content, labels, and adds the new case + heatmaps. |
| tests/Issue_Files/reports_xlsx/comparison_report.json | Adds the new case metrics/details to the machine-readable report. |
| tests/Issue_Files/reports_xlsx/comparison_manifest.json | Updates manifest content/format (including BOM) and adds the new issue case. |
| src/MiniPdf/PdfWriter.cs | Extends font-name normalization to map STKaiti variants consistently. |
| src/MiniPdf/PdfTextBlock.cs | Adds TranslateY helper used for vertical centering adjustments. |
| src/MiniPdf/PdfPage.cs | Implements page-level vertical centering using computed content bounds and worksheet-range tracking. |
| src/MiniPdf/ExcelToPdfConverter.cs | Wires vertical centering + stacked text support into rendering, adds merged-cell continuation and VML image placement handling. |
| src/MiniPdf/ExcelReader.cs | Parses textRotation, vertical centering flag, reads legacy VML drawing images (including EMF->PNG conversion), and extends embedded image metadata. |
| src/MiniPdf/DocxReader.cs | Makes EMF/WMF rasterization helper reusable with configurable raster height. |
Review details
- Files reviewed: 11/25 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
+3699
to
+3713
| var extension = Path.GetExtension(mediaPath).TrimStart('.').ToLowerInvariant(); | ||
| if (extension is "emf" or "wmf") | ||
| { | ||
| var cropLeft = ReadVmlCrop(imageData?.Attribute("cropleft")?.Value); | ||
| var cropTop = ReadVmlCrop(imageData?.Attribute("croptop")?.Value); | ||
| var cropRight = ReadVmlCrop(imageData?.Attribute("cropright")?.Value); | ||
| var cropBottom = ReadVmlCrop(imageData?.Attribute("cropbottom")?.Value); | ||
| var visibleHeightFraction = Math.Max(0.01f, 1f - cropTop - cropBottom); | ||
| var rasterHeight = (int)Math.Ceiling(heightPt.Value * 300f / 72f / visibleHeightFraction); | ||
| var converted = DocxReader.TryConvertMetafileToPng(data, widthEmu, heightEmu, | ||
| cropLeft, cropTop, cropRight, cropBottom, rasterHeight); | ||
| if (converted == null) continue; | ||
| data = converted; | ||
| extension = "png"; | ||
| } |
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.
Fix Microsoft 365 compatibility for the
Issue202609031340.xlsxrendering case.Changes
textRotation="255")Validation
dotnet build --configuration Release --nologodotnet test tests\MiniPdf.Tests --configuration Release --nologo(185 passed)scripts\Run-Benchmark_issues.ps1 -Filter "Issue202609031340" -SkipReference -SkipInstall -Heatmaps0.9524; overall0.9341diff_bbox=None)Compatibility and security
System.Drawingpath.Closes #129
Summary by CodeRabbit
New Features
Bug Fixes