From 33c0f94cc8b5475d38c866a38fcbdc8eb774340f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20Valentin=20N=C3=A9meth?= <61153253+attisvalentin@users.noreply.github.com> Date: Sun, 18 Oct 2026 18:46:20 +0200 Subject: [PATCH] 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 --- crates/engine/src/cmd/create.rs | 2 + crates/engine/src/cmd/text.rs | 8 ++-- crates/engine/src/fonts.rs | 7 ++- crates/engine/src/fonts_tests.rs | 8 ++++ crates/engine/src/tests.rs | 2 + crates/engine/src/textedit.rs | 1 + crates/text/src/fontdb.rs | 80 +++++++++++++++++++++++--------- crates/text/src/fontdb_tests.rs | 27 ++++++++--- crates/text/src/lib.rs | 5 +- 9 files changed, 105 insertions(+), 35 deletions(-) diff --git a/crates/engine/src/cmd/create.rs b/crates/engine/src/cmd/create.rs index 65c526e4..6ec7d4e5 100644 --- a/crates/engine/src/cmd/create.rs +++ b/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> { 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> { Err(crate::EngineError::Other(format!("{path}: files can't be read in the browser"))) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index fbc41334..0f0f003d 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -477,12 +477,12 @@ pub fn specs() -> Vec { 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!( diff --git a/crates/engine/src/fonts.rs b/crates/engine/src/fonts.rs index 42cc8a56..672d4f2c 100644 --- a/crates/engine/src/fonts.rs +++ b/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 { 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 { Ok(json!({"families": families})) } +/// The font commands: `text.missingFonts` and `text.refit`. pub fn specs() -> Vec { vec![ spec!( diff --git a/crates/engine/src/fonts_tests.rs b/crates/engine/src/fonts_tests.rs index c947773d..463fb904 100644 --- a/crates/engine/src/fonts_tests.rs +++ b/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 { 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(); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 7cfa320b..ad3b6aa3 100644 --- a/crates/engine/src/tests.rs +++ b/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(); diff --git a/crates/engine/src/textedit.rs b/crates/engine/src/textedit.rs index 38d6a199..902ba149 100644 --- a/crates/engine/src/textedit.rs +++ b/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 = [ diff --git a/crates/text/src/fontdb.rs b/crates/text/src/fontdb.rs index aa5777b5..814d3a2f 100644 --- a/crates/text/src/fontdb.rs +++ b/crates/text/src/fontdb.rs @@ -444,9 +444,22 @@ pub struct FontDb { families: BTreeMap)>, /// System faces known by name but not loaded (lower-case family → entries). pending: BTreeMap)>, + /// 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>, 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, + /// Every family that gained faces: the new ones and those with styles installed since. + pub changed: Vec, +} + static DB: OnceLock> = 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)>) -> Vec { - 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)>) -> 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 = 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 { +/// 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 = 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 = family_names().into_iter().filter(|f| !before.contains(f)).collect(); + return Rescan { changed: added.clone(), added }; } let mut found: BTreeMap)> = 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) -> Vec { - 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 = 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 } diff --git a/crates/text/src/fontdb_tests.rs b/crates/text/src/fontdb_tests.rs index 3574fdfb..f2456d28 100644 --- a/crates/text/src/fontdb_tests.rs +++ b/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)> { 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::>()).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); } } diff --git a/crates/text/src/lib.rs b/crates/text/src/lib.rs index e107593e..667ab731 100644 --- a/crates/text/src/lib.rs +++ b/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;