Allow null font name, Excel uses default font#1778
Conversation
There was a problem hiding this comment.
Pull request overview
Updates XSSF font handling to allow omitting the font name in the generated XML so Excel can apply its own current default font (Aptos/Calibri/user-configured).
Changes:
- Stop auto-populating
XSSFFont.FontNamewithDEFAULT_FONT_NAME; allow it to be unset (null) so<name>is omitted. - Remove explicit default font name assignment in
StylesTable.CreateDefaultFont(). - Make
XSSFTextRun.FontFamilyreturnnullwhen the run has no font family set.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ooxml/XSSF/UserModel/XSSFTextRun.cs | Returns null for FontFamily when unset to reflect missing XML and Excel-default behavior. |
| ooxml/XSSF/UserModel/XSSFFont.cs | Makes FontName nullable and removes default-name assignment from the constructor/setter logic. |
| ooxml/XSSF/Model/StylesTable.cs | Stops setting a default font name when creating the default font in the styles table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| xssfFont.FontHeightInPoints = (XSSFFont.DEFAULT_FONT_SIZE); | ||
| xssfFont.Color = (XSSFFont.DEFAULT_FONT_COLOR);//SetTheme | ||
| xssfFont.FontName = (XSSFFont.DEFAULT_FONT_NAME); | ||
| xssfFont.Color = (XSSFFont.DEFAULT_FONT_COLOR); |
There was a problem hiding this comment.
With FontName now allowed to be null (default font name omitted), StylesTable.FindFont() currently uses font.FontName.Equals(name) in both overloads, which will throw a NullReferenceException when either the stored font has no name (e.g., the default font created here) or when the caller passes name == null. Please switch these comparisons to a null-safe string comparison (e.g., string.Equals(font.FontName, name, StringComparison.Ordinal)).
| xssfFont.Color = (XSSFFont.DEFAULT_FONT_COLOR); | |
| xssfFont.Color = (XSSFFont.DEFAULT_FONT_COLOR); | |
| xssfFont.FontName = XSSFFont.DEFAULT_FONT_NAME; |
| { | ||
| return font.typeface; | ||
| } | ||
| return XSSFFont.DEFAULT_FONT_NAME; | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Changing FontFamily to return null when unset will break existing expectations in tests (e.g., TestXSSFDrawing currently asserts runs[1].FontFamily == XSSFFont.DEFAULT_FONT_NAME). Please update the affected tests to expect null (or explicitly set a font family in the test setup) to match the new behavior of omitting the font name element.
|
/oc fix the issue according to copilot review comments |
- Use string.Equals with Ordinal comparison in StylesTable.FindFont() for null-safe font name comparison - Update TestXSSFDrawing to expect null FontFamily when font name is not set in the XML
|
Fixed both issues:
|
|
This PR is a bit risky. It can break the Excel file |
PR created. The branch
opencode/issue1417-20260401065412has been pushed with commit:56d01bb - Allow XSSF font name to be null, letting Excel use its default font
Changes Made (3 files, +12 -7 lines):
When FontName is not set, the XML won't include a
<name>element, allowing Excel to use its own default font (Aptos, Calibri, or user's preference).Closes #1417
opencode session | github run