Merge pull request #2557 from Silvertree2010:fix/2517-a-new-font-derived-from-the-them

crates/engine/tests/it/font_scheme_minor.rs

@@ -0,0 +1,29 @@
+//! A font a formatting command derives from the theme's body font keeps following it (#2517).
+
+use std::io::Read;
+
+use gridcraft_engine::Session;
+use serde_json::json;
+
+fn styles_xml(session: &Session) -> String {
+    let bytes = gridcraft_xlsx::write_xlsx(&session.doc().unwrap().wb).unwrap();
+    let mut zip = zip::ZipArchive::new(std::io::Cursor::new(bytes)).unwrap();
+    let mut xml = String::new();
+    zip.by_name("xl/styles.xml").unwrap().read_to_string(&mut xml).unwrap();
+    xml
+}
+
+#[test]
+fn derived_body_font_keeps_minor_scheme() {
+    let mut session = Session::new();
+    session.new_workbook();
+    session.execute("home.bold", json!({"range": "A1"})).unwrap();
+    session.execute("home.fontSize", json!({"range": "B1", "size": 14})).unwrap();
+    let xml = styles_xml(&session);
+    let fonts = xml.split("<fonts").nth(1).unwrap().split("</fonts>").next().unwrap();
+    let fonts: Vec<&str> = fonts.split("<font>").skip(1).collect();
+    assert!(fonts.len() >= 3, "{xml}");
+    for f in &fonts {
+        assert!(f.contains("<scheme val=\"minor\"/>"), "every body-font font follows the theme: {f}");
+    }
+}

crates/engine/tests/it/main.rs

@@ -11,6 +11,7 @@ mod custom_filter_blanks;
 mod custom_view_freeze;
 mod dynamicmetadata;
 mod extreme_selection_moves;
+mod font_scheme_minor;
 mod format_as_table_restyle;
 mod hidden_sheet_navigation;
 mod ink_roundtrip;

crates/model/src/style.rs

@@ -193,6 +193,14 @@ pub struct Font {
     /// Font family class (1 roman, 2 swiss, 3 modern…); `None` is swiss, the default.
     #[serde(default)]
     pub family: Option<u8>,
+    /// The font follows the theme's body font (`<scheme val="minor"/>`) while its face is the
+    /// theme's minor font. Fonts derived from it (bold, a colour, a size) keep the link.
+    #[serde(default = "default_minor")]
+    pub minor: bool,
+}
+
+fn default_minor() -> bool {
+    true
 }
 
 impl Eq for Font {}
@@ -200,7 +208,7 @@ impl Hash for Font {
     fn hash<H: Hasher>(&self, h: &mut H) {
         self.name.hash(h);
         self.size.to_bits().hash(h);
-        (self.bold, self.italic, self.underline, self.strike, self.color, self.vert, self.charset, self.family).hash(h);
+        (self.bold, self.italic, self.underline, self.strike, self.color, self.vert, self.charset, self.family, self.minor).hash(h);
     }
 }
 
@@ -217,6 +225,7 @@ impl Default for Font {
             vert: VertAlign::Baseline,
             charset: None,
             family: None,
+            minor: true,
         }
     }
 }

crates/xlsx/src/read.rs

@@ -143,7 +143,13 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> {
     let mut wb = Workbook::new();
     wb.sheets.clear();
 
-    // Styles and theme. Cell checkboxes live in the feature property bags, which styles refer to.
+    // Theme, then styles: a font's link to the theme's body font is read against it.
+    if let Some(r) = find_rel(&wb_rels, "theme").cloned()
+        && let Some(x) = cx.optional_xml(&r.target)?
+    {
+        wb.theme = crate::theme::read_theme(&x);
+    }
+    // Cell checkboxes live in the feature property bags, which styles refer to.
     let mut checkboxes = Vec::new();
     if let Some(r) = find_rel(&wb_rels, "FeaturePropertyBag").cloned()
         && let Some(x) = cx.optional_xml(&r.target)?
@@ -152,7 +158,7 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> {
     }
     if let Some(r) = find_rel(&wb_rels, "styles").cloned() {
         if let Some(x) = cx.optional_xml(&r.target)? {
-            let StylesIn { xfs, dxfs, palette, named, num_fmts } = read_styles(&x, &checkboxes);
+            let StylesIn { xfs, dxfs, palette, named, num_fmts } = read_styles(&x, &checkboxes, &wb.theme.minor_font);
             cx.num_fmts = num_fmts;
             // Cell format 0 is the Normal cell format: everything on it (font, number format,
             // fill, borders, alignment, protection) is what a cell without its own format shows
@@ -168,11 +174,6 @@ pub fn read_xlsx(bytes: &[u8]) -> Result<(Workbook, ReadReport), IoError> {
     } else {
         cx.warn("no styles part");
     }
-    if let Some(r) = find_rel(&wb_rels, "theme").cloned()
-        && let Some(x) = cx.optional_xml(&r.target)?
-    {
-        wb.theme = crate::theme::read_theme(&x);
-    }
     // Shared strings, streamed item by item. Phonetic (furigana) runs are kept next to the text
     // they annotate, so an open + save does not delete them.
     if let Some(r) = find_rel(&wb_rels, "sharedStrings").cloned() {

crates/xlsx/src/styles.rs

@@ -93,6 +93,7 @@ pub fn read_font(e: &El, palette: &[u32]) -> Font {
         .and_then(|c| c.attr_u32("val"))
         .and_then(|v| u8::try_from(v).ok())
         .filter(|v| *v <= MAX_FONT_FAMILY && *v != DEFAULT_FONT_FAMILY);
+    f.minor = e.child_val("scheme") == Some("minor");
     f.color = opt_color(e.child("color"), palette);
     // Text 1 (dk1) is what "automatic" font colour means in practice.
     if f.color == Color::Theme(1, 0) {
@@ -312,7 +313,8 @@ fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &
 
 /// `checkboxes`: for each `xfComplement` index, whether it is a checkbox (see
 /// [`crate::feature_bags::checkbox_complements`]); empty when the file has no feature property bags.
-pub fn read_styles(root: &El, checkboxes: &[bool]) -> StylesIn {
+/// `minor_font`: the theme's body font, which fonts with `<scheme val="minor"/>` follow.
+pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn {
     let mut palette: Vec<u32> = INDEXED.to_vec();
     if let Some(ix) = root.path(&["colors", "indexedColors"]) {
         let custom: Vec<u32> = ix
@@ -334,7 +336,13 @@ pub fn read_styles(root: &El, checkboxes: &[bool]) -> StylesIn {
             }
         }
     }
-    let fonts: Vec<Font> = root.child("fonts").map(|f| f.kids("font").map(|e| read_font(e, &palette)).collect()).unwrap_or_default();
+    // 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);
+        f.minor |= f.name != minor_font;
+        f
+    };
+    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 =
@@ -373,7 +381,8 @@ pub fn read_styles(root: &El, checkboxes: &[bool]) -> StylesIn {
                         st.size = has_size && size == def.size;
                         st.name = has_name && name == def.name;
                         st.scheme = e.child_val("scheme").map(str::to_string);
-                        s.font = Font { name, size, ..f };
+                        // A differential font's scheme is in `dxf_stated`.
+                        s.font = Font { name, size, minor: s.font.minor, ..f };
                     }
                     if let Some(n) = x.child("numFmt") {
                         if let Some(code) = n.attr("formatCode") {
@@ -753,8 +762,9 @@ pub fn write_styles(wb: &Workbook, dxfs: &[Style], extra_fmts: &[String]) -> (St
     }
     let _ = write!(s, "<fonts count=\"{}\">", fonts.len());
     for (i, f) in fonts.iter().enumerate() {
-        // A Normal font in the theme's body face follows the theme, as Excel writes it (#805).
-        s.push_str(&font_xml(f, None, i == 0 && f.name == wb.theme.minor_font));
+        // 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));
     }
     let _ = write!(s, "</fonts><fills count=\"{}\">", fills.len());
     for f in &fills {

crates/xlsx/src/tests/roundtrip.rs

@@ -1247,6 +1247,32 @@ fn default_font_survives_a_round_trip() {
     assert_eq!(font0(&saved), (Some("12".into()), Some("Calibri".into()), Some("2".into()), Some("minor".into())));
 }
 
+/// Every font that follows the theme's body font keeps `<scheme val="minor"/>`, not only font 0,
+/// and a body-face font without the scheme does not gain one (#2517).
+#[test]
+fn minor_scheme_survives_on_every_font() {
+    let styles = r#"<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><fonts count="3"><font><sz val="11"/><color theme="1"/><name val="Calibri"/><family val="2"/><scheme val="minor"/></font><font><b/><sz val="11"/><color theme="1"/><name val="Calibri"/><family val="2"/><scheme val="minor"/></font><font><i/><sz val="11"/><color theme="1"/><name val="Calibri"/><family val="2"/></font></fonts>
+        <fills count="2"><fill><patternFill patternType="none"/></fill><fill><patternFill patternType="gray125"/></fill></fills><borders count="1"><border><left/><right/><top/><bottom/><diagonal/></border></borders>
+        <cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs>
+        <cellXfs count="3"><xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/><xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/><xf numFmtId="0" fontId="2" fillId="0" borderId="0" xfId="0" applyFont="1"/></cellXfs>
+        <cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles></styleSheet>"#;
+    let rels = r#"<Relationship Id="rId2" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/styles" Target="styles.xml"/>"#;
+    let body = r#"<sheetData><row r="1"><c r="A1" s="1"><v>1</v></c><c r="B1" s="2"><v>2</v></c></row></sheetData>"#;
+    let file = super::minimal(body, &[("xl/styles.xml", styles)], rels, "");
+    let saved = write_xlsx(&read_xlsx(&file).unwrap().0).unwrap();
+    let mut pkg = Package::open(&saved).unwrap();
+    let st = pkg.read_xml("xl/styles.xml").unwrap().unwrap();
+    let fonts: Vec<(bool, bool, Option<String>)> = st
+        .child("fonts")
+        .unwrap()
+        .kids("font")
+        .map(|f| (f.child("b").is_some(), f.child("i").is_some(), f.child("scheme").and_then(|e| e.attr("val")).map(str::to_string)))
+        .collect();
+    let scheme = |b: bool, i: bool| fonts.iter().find(|f| f.0 == b && f.1 == i).map(|f| f.2.clone());
+    assert_eq!(scheme(true, false), Some(Some("minor".into())), "{fonts:?}");
+    assert_eq!(scheme(false, true), Some(None), "{fonts:?}");
+}
+
 fn one_font_workbook(font: &str) -> Vec<u8> {
     let styles = format!(
         r#"<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><fonts count="1"><font>{font}</font></fonts><fills count="1"><fill><patternFill patternType="none"/></fill></fills><borders count="1"><border><left/><right/><top/><bottom/><diagonal/></border></borders><cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs><cellXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/></cellXfs><cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles></styleSheet>"#