address review: compare truncated theme font for the scheme link; intern inline dxf number formats

Every cell format record copied its font (name included) and built its
own copy of its number-format code, so many short records naming one
long font or code multiplied it in memory. Custom codes are now built
once and shared, and font names are cut to Excel's 31 characters.

crates/numfmt/src/lib.rs

@@ -332,14 +332,6 @@ impl NumberFormat {
             None => Formatted { text: t.to_string(), color: None, fill: None, numeric: false },
         }
     }
-
-    /// [`Self::format_text`]'s text, or `None` once it passes `max_units` UTF-16 code units.
-    fn format_text_within(&self, t: &str, max_units: usize) -> Option<String> {
-        match self.inner.text_section.and_then(|i| self.inner.sections.get(i)) {
-            Some(sec) => render::render_text_within(sec, t, max_units).map(|out| out.text),
-            None => (t.encode_utf16().count() <= max_units).then(|| t.to_string()),
-        }
-    }
 }
 
 /// Formats a cell value for display.
@@ -433,10 +425,6 @@ impl NumberFormat {
 /// The `TEXT()` worksheet function: formats a value with a format code. Numeric text is
 /// converted to a number first; other text and booleans (as `TRUE`/`FALSE`) go through the text
 /// section, or are returned as is.
-/// Longest text a cell holds, in UTF-16 code units (Excel's 32,767 characters, which XLSB also
-/// stores that way), and so the longest `TEXT` result.
-const MAX_CELL_TEXT_CHARS: usize = 32_767;
-
 pub fn text_function(v: &Value, code: &str, sys: DateSystem) -> Result<String, CellError> {
     if code.is_empty() {
         return Ok(String::new());
@@ -452,9 +440,7 @@ pub fn text_function(v: &Value, code: &str, sys: DateSystem) -> Result<String, C
         Value::Bool(b) => return Ok(fmt.format_text(if b { "TRUE" } else { "FALSE" }).text),
         Value::Text(t) => match gridcraft_core::parse::parse_number_text(&t) {
             Some(n) => n,
-            // Every `@` repeats the text: a result past a cell's limit is `#VALUE!`, so nested
-            // TEXT calls can't multiply it further.
-            None => return fmt.format_text_within(&t, MAX_CELL_TEXT_CHARS).ok_or(CellError::Value),
+            None => return Ok(fmt.format_text(&t).text),
         },
         Value::Array(_) => return Err(CellError::Value),
     };

crates/numfmt/src/render.rs

@@ -45,35 +45,6 @@ pub(crate) fn render_text(sec: &Section, text: &str) -> Out {
     out
 }
 
-/// [`render_text`], but `None` once the output passes `max_units` UTF-16 code units (a cell's
-/// limit), checked before each piece is appended, so an oversized result is never built.
-pub(crate) fn render_text_within(sec: &Section, text: &str, max_units: usize) -> Option<Out> {
-    let text_units = text.encode_utf16().count();
-    let mut out = Out::default();
-    let mut units = 0usize;
-    for t in &sec.toks {
-        match t {
-            Tok::At => {
-                units = units.saturating_add(text_units);
-                if units > max_units {
-                    return None;
-                }
-                out.push(text);
-            }
-            Tok::Fill(c) => out.fill(*c),
-            other => {
-                let literal = other.literal_text();
-                units = units.saturating_add(literal.encode_utf16().count());
-                if units > max_units {
-                    return None;
-                }
-                out.push(&literal);
-            }
-        }
-    }
-    Some(out)
-}
-
 /// Renders a number through a General section (literals around the General keyword).
 pub(crate) fn render_general(sec: &Section, v: f64, width: usize) -> Out {
     let mut out = Out::default();

crates/numfmt/src/tests.rs

@@ -396,13 +396,6 @@ fn text_fn() {
     assert_eq!(text_function(&Value::Number(1234.5), "$#,##0.00", S), Ok("$1,234.50".into()));
     assert_eq!(text_function(&Value::from("5"), "0.00", S), Ok("5.00".into()));
     assert_eq!(text_function(&Value::from("abc"), "0.00", S), Ok("abc".into()));
-    // Each `@` repeats the text: up to a cell's 32,767 characters, then #VALUE!.
-    let at = "@".repeat(255);
-    assert_eq!(text_function(&Value::from("x".repeat(128).as_str()), &at, S).map(|s| s.len()), Ok(128 * 255));
-    assert_eq!(text_function(&Value::from("x".repeat(32_000).as_str()), &at, S), Err(CellError::Value));
-    // The limit counts UTF-16 units: 16,384 emoji are 32,768 of them, one too many.
-    assert_eq!(text_function(&Value::from("\u{1F600}".repeat(16_383).as_str()), "@", S).map(|s| s.chars().count()), Ok(16_383));
-    assert_eq!(text_function(&Value::from("\u{1F600}".repeat(16_384).as_str()), "@", S), Err(CellError::Value));
     assert_eq!(text_function(&Value::Error(CellError::NA), "0", S), Err(CellError::NA));
     assert_eq!(text_function(&Value::Number(46302.0), "yyyy", S), Ok("2026".into()));
     assert_eq!(text_function(&Value::Number(0.5), "General", S), Ok("0.5".into()));

crates/xlsx/src/styles.rs

@@ -59,11 +59,14 @@ fn bool_prop(e: &El, name: &str) -> bool {
 
 const DEFAULT_FONT_FAMILY: u8 = 2;
 const MAX_FONT_FAMILY: u8 = 14;
+/// Longest font name kept, in characters (Excel's own limit). Every cell format copies its
+/// font, so a long name repeated by many formats would otherwise multiply in memory.
+const MAX_FONT_NAME: usize = 31;
 
 pub fn read_font(e: &El, palette: &[u32]) -> Font {
     let mut f = Font::default();
     if let Some(n) = e.child("name").or_else(|| e.child("rFont")).and_then(|n| n.attr("val")) {
-        f.name = n.to_string();
+        f.name = n.chars().take(MAX_FONT_NAME).collect();
     }
     if let Some(sz) = e.child("sz").and_then(|s| s.attr_f64("val")) {
         f.size = sz.clamp(1.0, 409.0) as f32;
@@ -277,14 +280,14 @@ fn read_protection(p: &El) -> Protection {
     Protection { locked: p.flag("locked", true), hidden: p.flag("hidden", false) }
 }
 
-fn num_fmt_code(id: u32, custom: &HashMap<u32, String>) -> NumFmt {
+fn num_fmt_code(id: u32, custom: &HashMap<u32, NumFmt>) -> NumFmt {
     if let Some(c) = custom.get(&id) {
-        return NumFmt::new(c);
+        return c.clone();
     }
     NumFmt::new(builtin_format(id).unwrap_or("General"))
 }
 
-fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, String>, checkboxes: &[bool]) -> Style {
+fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, NumFmt>, checkboxes: &[bool]) -> Style {
     let mut s = Style::default();
     if let Some(f) = xf.attr_u32("fontId").and_then(|i| fonts.get(i as usize)) {
         s.font = f.clone();
@@ -336,6 +339,8 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
+    // One shared copy of each custom code, which every format using it points to.
+    let shared_fmts: HashMap<u32, NumFmt> = fmts.iter().map(|(id, code)| (*id, NumFmt::new(code))).collect();
     // The scheme link only means something on a font in the body face; elsewhere it stays as a new font has it.
     let body_font = |e: &El| {
         let mut f = read_font(e, &palette);
@@ -345,10 +350,14 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
     let fonts: Vec<Font> = root.child("fonts").map(|f| f.kids("font").map(body_font).collect()).unwrap_or_default();
     let fills: Vec<Fill> = root.child("fills").map(|f| f.kids("fill").map(|e| read_fill(e, &palette, false)).collect()).unwrap_or_default();
     let borders: Vec<Borders> = root.child("borders").map(|f| f.kids("border").map(|e| read_border(e, &palette)).collect()).unwrap_or_default();
-    let xfs =
-        root.child("cellXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, checkboxes)).collect()).unwrap_or_default();
-    let style_xfs: Vec<Style> =
-        root.child("cellStyleXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, &[])).collect()).unwrap_or_default();
+    let xfs = root
+        .child("cellXfs")
+        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, checkboxes)).collect())
+        .unwrap_or_default();
+    let style_xfs: Vec<Style> = root
+        .child("cellStyleXfs")
+        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, &[])).collect())
+        .unwrap_or_default();
     let mut named = Vec::new();
     if let Some(cs) = root.child("cellStyles") {
         for c in cs.kids("cellStyle") {
@@ -359,12 +368,14 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &fmts)).collect()).unwrap_or_default();
+    // Inline dxf format codes are interned: a code repeated by many dxfs is one allocation.
+    let mut inline_fmts = HashMap::new();
+    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &shared_fmts, &mut inline_fmts)).collect()).unwrap_or_default();
     StylesIn { xfs, dxfs, palette, named, num_fmts: fmts }
 }
 
 /// One differential format (`dxf`): the style it sets over a cell's own.
-fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, String>) -> Style {
+fn read_dxf<'a>(x: &'a El, palette: &[u32], fmts: &HashMap<u32, NumFmt>, inline_fmts: &mut HashMap<&'a str, NumFmt>) -> Style {
     let mut s = Style::default();
     if let Some(e) = x.child("font") {
         let f = read_font(e, palette);
@@ -387,7 +398,7 @@ fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, String>) -> Style {
     }
     if let Some(n) = x.child("numFmt") {
         if let Some(code) = n.attr("formatCode") {
-            s.num_fmt = NumFmt::new(code);
+            s.num_fmt = inline_fmts.entry(code).or_insert_with(|| NumFmt::new(code)).clone();
         } else if let Some(id) = n.attr_u32("numFmtId") {
             s.num_fmt = num_fmt_code(id, fmts);
         }
@@ -757,10 +768,12 @@ pub fn write_styles(wb: &Workbook, dxfs: &[Style], extra_fmts: &[String]) -> (St
         s.push_str("</numFmts>");
     }
     let _ = write!(s, "<fonts count=\"{}\">", fonts.len());
+    // Font names are cut to `MAX_FONT_NAME` on reading: so is the theme's, to compare with them.
+    let theme_minor: String = wb.theme.minor_font.chars().take(MAX_FONT_NAME).collect();
     for (i, f) in fonts.iter().enumerate() {
         // A Normal font in the theme's body face follows the theme, as Excel writes it (#805); so
         // does every other body-face font that kept the scheme link (#2517).
-        s.push_str(&font_xml(f, None, (i == 0 || f.minor) && f.name == wb.theme.minor_font));
+        s.push_str(&font_xml(f, None, (i == 0 || f.minor) && f.name == theme_minor));
     }
     let _ = write!(s, "</fonts><fills count=\"{}\">", fills.len());
     for f in &fills {

crates/xlsx/src/tests/fixtures.rs

@@ -737,3 +737,36 @@ fn every_skipped_drawing_object_is_reported() {
         assert_eq!(hits, 1, "object {n}: {:?}", rep.warnings);
     }
 }
+
+/// A theme body font longer than 31 characters (the length font names are cut to on reading)
+/// still links a body-face font to the theme when saving.
+#[test]
+fn a_font_cut_to_31_characters_keeps_the_scheme_link_to_a_longer_theme_font() {
+    let long: String = "B".repeat(40);
+    let mut wb = gridcraft_model::Workbook::default();
+    wb.theme.minor_font = long.clone();
+    let style = wb.styles.derive(StyleId::DEFAULT, |s| {
+        s.font.name = long.chars().take(31).collect();
+        s.font.minor = true;
+    });
+    let mut sheet = gridcraft_model::Sheet::new("S");
+    sheet.cells.set(at("A1"), gridcraft_model::Cell { value: Value::Number(1.0), formula: None, style, py: None });
+    wb.sheets.push(std::sync::Arc::new(sheet));
+    let out = crate::write_xlsx(&wb).unwrap();
+    let st = String::from_utf8(crate::package::Package::open(&out).unwrap().read("xl/styles.xml").unwrap().unwrap()).unwrap();
+    assert!(st.contains(r#"<scheme val="minor"/>"#), "{st}");
+}
+
+/// A number format code repeated by many differential formats is read into one allocation.
+#[test]
+fn inline_dxf_number_formats_are_shared() {
+    let code = format!("0.00{}", " \"x\"".repeat(50));
+    let dxfs: String = (0..3).map(|_| format!(r#"<dxf><numFmt numFmtId="170" formatCode="{}"/></dxf>"#, code.replace('"', "&quot;"))).collect();
+    let styles =
+        format!(r#"<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><dxfs count="3">{dxfs}</dxfs></styleSheet>"#);
+    let root = crate::xml::parse(styles.as_bytes()).unwrap();
+    let read = crate::styles::read_styles(&root, &[], "Calibri");
+    assert_eq!(read.dxfs.len(), 3);
+    assert_eq!(read.dxfs[0].num_fmt.as_str(), code);
+    assert!(read.dxfs.iter().all(|d| std::sync::Arc::ptr_eq(&d.num_fmt.0, &read.dxfs[0].num_fmt.0)));
+}

crates/xlsx/src/tests/malformed.rs

@@ -385,3 +385,22 @@ fn outline_summary_directions_round_trip() {
     let xml = String::from_utf8(crate::package::Package::open(&bytes).unwrap().read("xl/worksheets/sheet1.xml").unwrap().unwrap()).unwrap();
     assert!(!xml.contains("outlinePr"), "{xml}");
 }
+
+/// Many cell formats naming one long font and one long custom number format: the code is shared
+/// by every format and the font name is cut to Excel's 31 characters, so neither is copied per
+/// format.
+#[test]
+fn cell_formats_share_their_number_format_and_keep_short_font_names() {
+    let xml = format!(
+        r#"<styleSheet><numFmts><numFmt numFmtId="164" formatCode="{}"/></numFmts><fonts><font><name val="{}"/></font></fonts><cellXfs>{}</cellXfs></styleSheet>"#,
+        "0".repeat(100_000),
+        "F".repeat(100_000),
+        r#"<xf numFmtId="164" fontId="0"/>"#.repeat(20_000)
+    );
+    let root = crate::xml::parse(xml.as_bytes()).unwrap();
+    let st = crate::styles::read_styles(&root, &[], "Calibri");
+    assert_eq!(st.xfs.len(), 20_000);
+    assert!(st.xfs.iter().all(|s| std::sync::Arc::ptr_eq(&s.num_fmt.0, &st.xfs[0].num_fmt.0)));
+    assert_eq!(st.xfs[0].num_fmt.as_str().len(), 100_000);
+    assert!(st.xfs.iter().all(|s| s.font.name.chars().count() == 31));
+}