From f3e5d2abc4c5db3289dce999bc1c1ed47303975d Mon Sep 17 00:00:00 2001 From: Hamza Date: Wed, 21 Oct 2026 00:06:57 +0500 Subject: [PATCH] .fig import: a note says how much of the file it covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A .fig import's notes were deduplicated and left uncounted, so each kind was named once with no sign of how much of the file it applied to: a file whose every text layer lost a setting read exactly like one where a single layer did. That is what made the loss in #467 invisible -- the reporter saw one line while three kinds of data went, one of them on all 83 text layers. The mapper now collects its notes through the same `Warnings` the REST importer already uses, which keeps the first-seen order and the cap and appends `(×n)` where a note was met more than once. Nothing else changes: the note text stays the catalog key it was, and the import report already reads a `(×n)` suffix off before translating the base (`ui-egui`'s `note_text`), so a counted note is still translated. `figcraft-cli info` prints these notes, so it now names each kind with its count. With the count dropped again, the test reports the two notes plain: ["connector layers import as static frame/vector snapshots ...", "connector label was skipped: depth limit prevents nested ..."] Part of #467. --- crates/format/src/fig/map.rs | 18 +++++---- .../map_connector_snapshot_budget_tests.rs | 40 +++++++++++++++++-- crates/format/src/rest/mod.rs | 19 +++++++-- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/crates/format/src/fig/map.rs b/crates/format/src/fig/map.rs index 8e467151..f36cbb83 100644 --- a/crates/format/src/fig/map.rs +++ b/crates/format/src/fig/map.rs @@ -58,7 +58,7 @@ pub(super) fn to_rest(msg: &Kv, name: &str, max_nodes: usize) -> (Value, Vec { library_contexts: Vec, /// Layers visited while scanning internal canvases, across all of them. library_scan_count: usize, - warnings: Vec, + warnings: crate::rest::Warnings, count: usize, /// Most layers produced or scanned, instance expansion included; the one place the cap comes /// from (see [`super::layer_budget`]). @@ -464,7 +464,7 @@ impl<'a> Mapper<'a> { override_scope_entries: 0, library_contexts: Vec::new(), library_scan_count: 0, - warnings: Vec::new(), + warnings: crate::rest::Warnings::default(), count: 0, max_nodes, networks: Vec::new(), @@ -493,12 +493,14 @@ impl<'a> Mapper<'a> { .cloned() } + /// A note about what this file's mapping left out or changed. Counted, as the REST importer + /// counts its own: these notes are met once per layer, so the kinds were named once each with + /// no sign of how much of the file they covered -- a file whose every text layer lost a + /// setting read the same as one where a single layer did (#467). The collector keeps the + /// first-seen order and the same cap, since some notes name fields of the file (image + /// filters). fn warn(&mut self, w: impl Into) { - let w = w.into(); - // Capped like the REST importer's list: some warnings name fields of the file (image filters). - if self.warnings.len() < crate::rest::MAX_WARNINGS && !self.warnings.contains(&w) { - self.warnings.push(w); - } + self.warnings.add(w.into()); } fn children(&self, id: &str) -> Vec { diff --git a/crates/format/src/fig/map_connector_snapshot_budget_tests.rs b/crates/format/src/fig/map_connector_snapshot_budget_tests.rs index ca47578d..8727c31b 100644 --- a/crates/format/src/fig/map_connector_snapshot_budget_tests.rs +++ b/crates/format/src/fig/map_connector_snapshot_budget_tests.rs @@ -115,14 +115,14 @@ fn connector_snapshot_respects_last_and_zero_child_slots() { assert!(!children[0]["fillGeometry"].as_array().unwrap().is_empty(), "saved outline survives the budget limit"); assert_eq!(children[0]["fillGeometry"][0]["path"], "M 0 0 L 20 10"); assert_eq!(mapper.count, MAX_NODES); - assert!(mapper.warnings.iter().any(|warning| warning == &format!("the file has more than {MAX_NODES} layers; connector label was skipped"))); + assert!(mapper.warnings.seen().any(|warning| warning == format!("the file has more than {MAX_NODES} layers; connector label was skipped"))); let mut mapper = Mapper::new(&message, MAX_NODES); mapper.count = MAX_NODES; let children = mapper.connector_snapshot_children(&view, "1:123"); assert!(children.is_empty()); assert_eq!(mapper.count, MAX_NODES, "the node budget is never exceeded"); - assert!(mapper.warnings.iter().any(|warning| warning.contains("connector snapshot children were skipped"))); + assert!(mapper.warnings.seen().any(|warning| warning.contains("connector snapshot children were skipped"))); } #[test] @@ -133,7 +133,7 @@ fn connector_snapshot_flattens_vector_at_depth_and_node_limits() { assert_eq!(depth_limited["type"], "VECTOR"); assert!(!depth_limited["fillGeometry"].as_array().unwrap().is_empty()); assert!(depth_limited.get("children").is_none()); - assert!(mapper.warnings.iter().any(|warning| warning == "connector label was skipped: depth limit prevents nested snapshot children")); + assert!(mapper.warnings.seen().any(|warning| warning == "connector label was skipped: depth limit prevents nested snapshot children")); let mut mapper = Mapper::new(&message, MAX_NODES); mapper.count = MAX_NODES - 1; @@ -142,5 +142,37 @@ fn connector_snapshot_flattens_vector_at_depth_and_node_limits() { assert!(!budget_limited["fillGeometry"].as_array().unwrap().is_empty()); assert!(budget_limited.get("children").is_none()); assert_eq!(mapper.count, MAX_NODES); - assert!(mapper.warnings.iter().any(|warning| warning == "connector label was skipped: node budget prevents nested snapshot children")); + assert!(mapper.warnings.seen().any(|warning| warning == "connector label was skipped: node budget prevents nested snapshot children")); +} + +/// A mapper note is met once per layer, so the kinds were named once each with no sign of how much +/// of the file they covered: a file whose every layer lost a setting read the same as one where a +/// single layer did (#467). They are counted now, with the `(×n)` the REST importer already uses +/// and the import report already reads back off before translating a note. +#[test] +fn a_note_met_more_than_once_carries_its_count() { + let message = message(); + + let mut mapper = Mapper::new(&message, MAX_NODES); + for _ in 0..2 { + mapper.layer("1:123", None, Affine::IDENTITY, None, None, MAX_DEPTH).expect("terminal connector"); + } + let notes = std::mem::take(&mut mapper.warnings).finish(); + let counted: Vec<&String> = notes.iter().filter(|n| n.contains("(×")).collect(); + assert!( + counted.iter().any(|n| n.ends_with("(×2)") && n.contains("connector label was skipped: depth limit")), + "the note met twice says so: {notes:?}" + ); + assert!(notes.iter().all(|n| !n.ends_with("(×1)")), "a count of one is left off: {notes:?}"); + + // One layer, and the same notes have no count at all. + let mut mapper = Mapper::new(&message, MAX_NODES); + mapper.layer("1:123", None, Affine::IDENTITY, None, None, MAX_DEPTH).expect("terminal connector"); + let once = std::mem::take(&mut mapper.warnings).finish(); + assert!(once.iter().all(|n| !n.contains("(×")), "{once:?}"); + assert_eq!( + once.len(), + counted.len() + notes.iter().filter(|n| !n.contains("(×")).count(), + "the same kinds, counted or not: {once:?} vs {notes:?}" + ); } diff --git a/crates/format/src/rest/mod.rs b/crates/format/src/rest/mod.rs index 78279e91..b86b0b91 100644 --- a/crates/format/src/rest/mod.rs +++ b/crates/format/src/rest/mod.rs @@ -329,15 +329,16 @@ fn children(v: &Value) -> &[Value] { v.get("children").and_then(Value::as_array).map(Vec::as_slice).unwrap_or_default() } -/// Distinct warnings in first-seen order, with repeat counts. +/// Distinct warnings in first-seen order, with repeat counts. Shared with the `.fig` mapper +/// (`crate::fig::map`), whose notes repeat per layer in the same way. #[derive(Default)] -struct Warnings { +pub(crate) struct Warnings { list: Vec<(String, usize)>, index: BTreeMap, } impl Warnings { - fn add(&mut self, msg: String) { + pub(crate) fn add(&mut self, msg: String) { if let Some(i) = self.index.get(&msg) { if let Some(e) = self.list.get_mut(*i) { e.1 += 1; @@ -348,7 +349,17 @@ impl Warnings { } } - fn finish(self) -> Vec { + /// The notes met so far, without their counts, for the tests that ask whether one was met + /// (the product reads them through [`Self::finish`], with the counts). + #[cfg(test)] + pub(crate) fn seen(&self) -> impl Iterator { + self.list.iter().map(|(msg, _)| msg.as_str()) + } + + /// The notes, each with `(×n)` after it where it was met more than once. The import report + /// reads that suffix off before it looks the note up in the catalogue, so a counted note is + /// still translated (`ui-egui`'s `note_text`). + pub(crate) fn finish(self) -> Vec { self.list.into_iter().map(|(m, n)| if n > 1 { format!("{m} (×{n})") } else { m }).collect() } }