M4: font review fixes: new styles of loaded families, ids checks, repeat loads

- text.rescanFonts offers a loaded family the files it hasn't read (styles
  installed since) and refits every family that gained faces; `added` still
  lists only the families that were unknown
- text.refit refuses an `ids` that isn't a list instead of refitting the
  whole document
- text.loadFont with a file already loaded returns its families without
  keeping its bytes or adding its faces again
- doc comments on the functions this change touches

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/engine/src/cmd/create.rs

@@ -362,11 +362,13 @@ fn free_spot(s: &Session, w: f64, h: f64) -> Result<(f64, f64)> {
     })
 }
 
+/// The bytes of the file at `path`, within the session's allowed file roots.
 #[cfg(not(target_arch = "wasm32"))]
 pub(super) fn read_path(path: &str) -> Result<Vec<u8>> {
     crate::file_access::read(path)
 }
 
+/// The browser has no files to read: always an error.
 #[cfg(target_arch = "wasm32")]
 pub(super) fn read_path(path: &str) -> Result<Vec<u8>> {
     Err(crate::EngineError::Other(format!("{path}: files can't be read in the browser")))

crates/engine/src/cmd/text.rs

@@ -477,12 +477,12 @@ pub fn specs() -> Vec<CommandSpec> {
         spec!(
             "text.rescanFonts",
             "Rescan installed fonts",
-            "{}  walks the system font folders again, for fonts installed while the app runs; the open document's auto-sizing text in those families fits them (text.refit) → {added: [families that were unknown], refitted: [ids]}",
+            "{}  walks the system font folders again, for fonts installed while the app runs; the open document's auto-sizing text in the families that gained faces (new ones, or styles installed since) fits them (text.refit) → {added: [families that were unknown], refitted: [ids]}",
             true,
             |s, _| {
-                let added = figcraft_text::rescan_system_fonts();
-                let refitted = crate::fonts::refit_open(s, None, Some(&added));
-                Ok(json!({"added": added, "refitted": refitted}))
+                let rescan = figcraft_text::rescan_system_fonts();
+                let refitted = crate::fonts::refit_open(s, None, Some(&rescan.changed));
+                Ok(json!({"added": rescan.added, "refitted": refitted}))
             }
         ),
         spec!(

crates/engine/src/fonts.rs

@@ -118,10 +118,12 @@ pub(crate) fn refit_open(s: &mut Session, under: Option<&[NodeId]>, families: Op
     }
 }
 
+/// `text.refit`: the layers `ids` names (an `ids` that isn't a list of layers is refused), else the
+/// whole document.
 fn refit_text(s: &mut Session, p: &Value) -> Result<Value> {
     let under = match get(p, "ids") {
-        Some(Value::Array(_)) => Some(req_targets("text.refit", s, p)?),
-        _ => None,
+        None | Some(Value::Null) => None,
+        Some(_) => Some(req_targets("text.refit", s, p)?),
     };
     s.doc()?;
     Ok(json!({"refitted": refit_open(s, under.as_deref(), None)}))
@@ -133,6 +135,7 @@ fn missing_fonts(s: &mut Session, _: &Value) -> Result<Value> {
     Ok(json!({"families": families}))
 }
 
+/// The font commands: `text.missingFonts` and `text.refit`.
 pub fn specs() -> Vec<CommandSpec> {
     vec![
         spec!(

crates/engine/src/fonts_tests.rs

@@ -135,14 +135,17 @@ fn stale(s: &mut Session, id: figcraft_doc::NodeId, dw: f64) -> f64 {
     want
 }
 
+/// The width of layer `id`.
 fn width(s: &Session, id: figcraft_doc::NodeId) -> f64 {
     s.doc().unwrap().node(id).unwrap().width
 }
 
+/// The ids a refit reports.
 fn refitted(r: &Value) -> Vec<u64> {
     r["refitted"].as_array().unwrap().iter().map(|v| v.as_u64().unwrap()).collect()
 }
 
+/// Stale boxes fit their text again, by family, by layer or everywhere; fixed boxes keep theirs.
 #[test]
 fn refit_measures_stale_text_boxes_again() {
     let mut s = Session::new();
@@ -172,9 +175,14 @@ fn refit_measures_stale_text_boxes_again() {
     // Nothing stale: nothing changes.
     assert!(refitted(&s.execute("text.refit", &json!({})).unwrap()).is_empty());
     assert!(s.execute("text.refit", &json!({"ids": ["nope"]})).is_err());
+    // An `ids` that isn't a list is refused, not taken as the whole document.
+    stale(&mut s, a, 9.0);
+    assert!(s.execute("text.refit", &json!({"ids": a.0.to_string()})).is_err());
+    assert_eq!(width(&s, a), want_a + 9.0);
     assert!(Session::empty().execute("text.refit", &json!({})).is_err());
 }
 
+/// Text inside an instance is refitted too.
 #[test]
 fn refit_reaches_text_in_instances() {
     let mut s = Session::new();

crates/engine/src/tests.rs

@@ -376,6 +376,7 @@ fn pointer_draw_move_resize() {
     assert_eq!(s.selection(), vec![r]);
 }
 
+/// No command panics on junk parameters.
 #[test]
 fn every_command_survives_junk_params() {
     // A junk-parameter sweep: no command may panic (guard turns panics into Internal errors,
@@ -627,6 +628,7 @@ fn find_replace_changing_only_the_case_replaces_every_occurrence() {
     assert_eq!(s.doc().unwrap().node(f).unwrap().name, "FigCraft / FigCraft");
 }
 
+/// `text.loadFont` refuses what isn't a font file; `text.rescanFonts` reports what it added.
 #[test]
 fn fonts_load_from_files_and_rescans_report_new_families() {
     let mut s = sess();

crates/engine/src/textedit.rs

@@ -782,6 +782,7 @@ mod tests {
         assert!(r.is_null() || r["axes"].is_array());
     }
 
+    /// No text command panics on junk parameters while a text is being edited.
     #[test]
     fn text_commands_survive_junk_while_editing() {
         let junk = [

crates/text/src/fontdb.rs

@@ -444,9 +444,22 @@ pub struct FontDb {
     families: BTreeMap<String, (String, Vec<usize>)>,
     /// System faces known by name but not loaded (lower-case family → entries).
     pending: BTreeMap<String, (String, Vec<Pending>)>,
+    /// System faces read from disk (file, face index), so a rescan only offers new ones.
+    loaded_files: std::collections::BTreeSet<(std::path::PathBuf, u32)>,
+    /// Font files added as bytes (hash and length → families), so loading one again adds nothing.
+    loaded_bytes: std::collections::HashMap<(u64, usize), Vec<String>>,
     system_scanned: bool,
 }
 
+/// What a later walk of the font folders found ([`rescan_system_fonts`]).
+#[derive(Clone, Debug, Default, PartialEq, Eq)]
+pub struct Rescan {
+    /// Families that were unknown before.
+    pub added: Vec<String>,
+    /// Every family that gained faces: the new ones and those with styles installed since.
+    pub changed: Vec<String>,
+}
+
 static DB: OnceLock<RwLock<FontDb>> = OnceLock::new();
 /// Set by [`scan_system_fonts_on_demand`].
 static ON_DEMAND: AtomicBool = AtomicBool::new(false);
@@ -580,6 +593,7 @@ impl FontDb {
             };
             if let Some(face) = Face::from_bytes(FontBytes::Shared(bytes), e.index, None, Some(&e.style)) {
                 self.add(face);
+                self.loaded_files.insert((e.path, e.index));
             }
         }
     }
@@ -670,21 +684,33 @@ impl FontDb {
         self.faces.len()
     }
 
-    /// Offer the families of a later walk of the font folders: new families become pending, and a
-    /// family not loaded yet takes the walk's files (styles installed since). Loaded families keep
-    /// their faces. Returns the display names of the families that were unknown.
-    fn merge_rescan(&mut self, found: BTreeMap<String, (String, Vec<Pending>)>) -> Vec<String> {
-        let mut added = Vec::new();
-        for (k, v) in found {
+    /// Offer the families of a later walk of the font folders: new families become pending, a
+    /// family not loaded yet takes the walk's files, and a loaded family keeps its faces and gets
+    /// the files it hasn't read (styles installed since) as pending, read when it is next used.
+    fn merge_rescan(&mut self, found: BTreeMap<String, (String, Vec<Pending>)>) -> Rescan {
+        let mut out = Rescan::default();
+        for (k, (display, entries)) in found {
             if self.families.contains_key(&k) {
+                let waiting = self.pending.get(&k).map(|(_, v)| v.clone()).unwrap_or_default();
+                let new: Vec<Pending> = entries
+                    .into_iter()
+                    .filter(|e| {
+                        !self.loaded_files.contains(&(e.path.clone(), e.index)) && !waiting.iter().any(|w| w.path == e.path && w.index == e.index)
+                    })
+                    .collect();
+                if !new.is_empty() {
+                    out.changed.push(display.clone());
+                    self.pending.entry(k).or_insert_with(|| (display, Vec::new())).1.extend(new);
+                }
                 continue;
             }
             if !self.pending.contains_key(&k) {
-                added.push(v.0.clone());
+                out.added.push(display.clone());
             }
-            self.pending.insert(k, v);
+            out.changed.push(display.clone());
+            self.pending.insert(k, (display, entries));
         }
-        added
+        out
     }
 
     /// Index the system font directories (desktop only): family names are read now, faces loaded
@@ -905,39 +931,51 @@ pub fn scan_system_fonts() {
 }
 
 /// Walk the system font folders again, for fonts installed while the app runs (the first scan is
-/// once per process). Returns the families that were unknown before. Before the first scan this
-/// is that scan; a no-op on the web or with `FIGCRAFT_NO_SYSTEM_FONTS` set.
-pub fn rescan_system_fonts() -> Vec<String> {
+/// once per process): the families that were unknown before, and every family that gained faces.
+/// Before the first scan this is that scan; a no-op on the web or with `FIGCRAFT_NO_SYSTEM_FONTS`.
+pub fn rescan_system_fonts() -> Rescan {
     #[cfg(not(target_arch = "wasm32"))]
     if !system_fonts_opted_out() {
         if SCANNED.get().is_none() {
             let before: std::collections::BTreeSet<String> =
                 raw_db().read().unwrap_or_else(std::sync::PoisonError::into_inner).families().into_iter().collect();
             scan_system_fonts();
-            return family_names().into_iter().filter(|f| !before.contains(f)).collect();
+            let added: Vec<String> = family_names().into_iter().filter(|f| !before.contains(f)).collect();
+            return Rescan { changed: added.clone(), added };
         }
         let mut found: BTreeMap<String, (String, Vec<Pending>)> = BTreeMap::new();
         for dir in system_font_dirs() {
             scan_dir(&dir, 0, &mut found);
         }
-        let added = raw_db().write().unwrap_or_else(std::sync::PoisonError::into_inner).merge_rescan(found);
-        if !added.is_empty() {
+        let rescan = raw_db().write().unwrap_or_else(std::sync::PoisonError::into_inner).merge_rescan(found);
+        if !rescan.changed.is_empty() {
             crate::clear_layout_cache();
         }
-        return added;
+        return rescan;
     }
-    Vec::new()
+    Rescan::default()
 }
 
 /// Load font file bytes (every face of a collection) into the global db, for a font that isn't
-/// installed. Returns the families of the faces added (none: not a font file).
+/// installed. Returns the families of its faces (none: not a font file); the same file again
+/// returns them without adding its faces twice.
 pub fn load_font_bytes(bytes: Vec<u8>) -> Vec<String> {
-    let faces = db().write().unwrap_or_else(std::sync::PoisonError::into_inner).add_bytes(bytes);
-    if !faces.is_empty() {
-        crate::clear_layout_cache();
+    use std::hash::{Hash, Hasher};
+    let mut h = std::collections::hash_map::DefaultHasher::new();
+    bytes.hash(&mut h);
+    let key = (h.finish(), bytes.len());
+    let mut db = db().write().unwrap_or_else(std::sync::PoisonError::into_inner);
+    if let Some(families) = db.loaded_bytes.get(&key) {
+        return families.clone();
     }
+    let faces = db.add_bytes(bytes);
     let mut families: Vec<String> = faces.iter().map(|f| f.family.clone()).collect();
     families.dedup();
+    if !families.is_empty() {
+        db.loaded_bytes.insert(key, families.clone());
+        drop(db);
+        crate::clear_layout_cache();
+    }
     families
 }
 

crates/text/src/fontdb_tests.rs

@@ -218,6 +218,7 @@ fn user_font_dirs_preserve_non_utf8_paths() {
     assert_eq!(dirs[1].as_os_str().as_bytes(), b"/data/\xfe/fonts");
 }
 
+/// A rescan offers new families and the styles installed since, and never a file twice.
 #[test]
 fn a_rescan_offers_fonts_installed_since() {
     let entry = |s: &str| Pending { path: format!("no-such-dir/{s}.ttf").into(), index: 0, style: s.into() };
@@ -225,26 +226,40 @@ fn a_rescan_offers_fonts_installed_since() {
     let found = |fams: &[(&str, &[&str])]| -> BTreeMap<String, (String, Vec<Pending>)> {
         fams.iter().map(|(f, styles)| (f.to_lowercase(), ((*f).to_string(), styles.iter().map(|s| entry(s)).collect()))).collect()
     };
-    // A new family is reported and offered; a pending one takes the walk's files (a new style).
-    let added = db.merge_rescan(found(&[("CJK UI", &["Regular", "Bold"]), ("Brand Sans", &["Regular"])]));
-    assert_eq!(added, ["Brand Sans"]);
+    // A new family is added; a pending one takes the walk's files (a new style); both changed.
+    let r = db.merge_rescan(found(&[("CJK UI", &["Regular", "Bold"]), ("Brand Sans", &["Regular"])]));
+    assert_eq!(r.added, ["Brand Sans"]);
+    assert_eq!(r.changed, ["Brand Sans", "CJK UI"]);
     assert!(db.has_family("brand sans"));
     assert_eq!(left(&db), ["Regular", "Bold"]);
-    // A loaded family keeps its faces and isn't reported again.
+    // A loaded family keeps its faces and gets only the files it hasn't read, once.
     db.families.insert("brand sans".into(), ("Brand Sans".into(), Vec::new()));
     db.pending.remove("brand sans");
-    assert!(db.merge_rescan(found(&[("Brand Sans", &["Regular"])])).is_empty());
+    db.loaded_files.insert(("no-such-dir/Regular.ttf".into(), 0));
+    let same = db.merge_rescan(found(&[("Brand Sans", &["Regular"])]));
+    assert_eq!(same, Rescan::default());
     assert!(!db.pending.contains_key("brand sans"));
+    let bold = db.merge_rescan(found(&[("Brand Sans", &["Regular", "Black"])]));
+    assert_eq!((bold.added.len(), bold.changed), (0, vec!["Brand Sans".to_string()]));
+    let waiting = |db: &FontDb| db.pending.get("brand sans").map(|(_, v)| v.iter().map(|e| e.style.clone()).collect::<Vec<_>>()).unwrap_or_default();
+    assert_eq!(waiting(&db), ["Black"]);
+    assert_eq!(db.merge_rescan(found(&[("Brand Sans", &["Regular", "Black"])])), Rescan::default());
+    assert_eq!(waiting(&db), ["Black"]);
 }
 
+/// Font file bytes add their faces once; junk adds nothing.
 #[test]
 fn font_bytes_load_into_the_db() {
     assert!(load_font_bytes(b"not a font".to_vec()).is_empty());
     assert!(load_font_bytes(Vec::new()).is_empty());
-    // With the embedded fonts (absent on CI builds without craft-fonts): a file's family is added.
+    // With the embedded fonts (absent on CI builds without craft-fonts): a file's family is added,
+    // and loading the same file again returns it without adding its faces twice.
     if let Some(f) = CRAFT_FONTS.first() {
         let families = load_font_bytes(f.bytes.to_vec());
         assert_eq!(families.len(), 1);
         assert!(families.iter().all(|f| has_family(f)));
+        let faces = db().read().unwrap().face_count();
+        assert_eq!(load_font_bytes(f.bytes.to_vec()), families);
+        assert_eq!(db().read().unwrap().face_count(), faces);
     }
 }

crates/text/src/lib.rs

@@ -16,8 +16,9 @@ mod onpath;
 pub use breaks::grapheme_step;
 pub use color::{ColorBitmap, ColorInk, ColorLayer};
 pub use fontdb::{
-    Axis, CRAFT_FONTS, Coords, Face, FontDb, NamedInstance, axis_label, db, face_with_char, family_names, family_styles, find_face, has_family,
-    installed_fonts_known, load_font_bytes, no_coords, rescan_system_fonts, scan_system_fonts, scan_system_fonts_on_demand, system_fonts_scanned,
+    Axis, CRAFT_FONTS, Coords, Face, FontDb, NamedInstance, Rescan, axis_label, db, face_with_char, family_names, family_styles, find_face,
+    has_family, installed_fonts_known, load_font_bytes, no_coords, rescan_system_fonts, scan_system_fonts, scan_system_fonts_on_demand,
+    system_fonts_scanned,
 };
 pub use layout::{Decoration, Glyph, Line, TextLayout, layout};
 pub use onpath::OnPath;