Merge #951: fig import: a note says how much of the file it covers (#467) (@Hamza0101)

crates/format/src/fig/map.rs

@@ -58,7 +58,7 @@ pub(super) fn to_rest(msg: &Kv, name: &str, max_nodes: usize) -> (Value, Vec<Str
     if !styles.is_empty() {
         out["styleDefinitions"] = Value::Array(styles);
     }
-    (out, m.warnings, m.networks)
+    (out, m.warnings.finish(), m.networks)
 }
 
 #[cfg(test)]
@@ -387,7 +387,7 @@ struct Mapper<'a> {
     library_contexts: Vec<Value>,
     /// Layers visited while scanning internal canvases, across all of them.
     library_scan_count: usize,
-    warnings: Vec<String>,
+    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<String>) {
-        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<String> {

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:?}"
+    );
 }

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<String, usize>,
 }
 
 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<String> {
+    /// 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<Item = &str> {
+        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<String> {
         self.list.into_iter().map(|(m, n)| if n > 1 { format!("{m} (×{n})") } else { m }).collect()
     }
 }