Merge #343 update: Tabs commands return the stored list; tab lists from files are sanitized

Conflict: crates/idml/src/tests.rs: both tests (next's CJK settings and ruby round trips, the branch's hostile tab list import)

crates/doc/src/attrs.rs

@@ -563,6 +563,9 @@ pub enum TabAlign {
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct TabStop {
+    /// A file that held a non-finite position (written as `null`) reads it back as NaN, which
+    /// [`TabStop::sanitized_list`] drops.
+    #[serde(deserialize_with = "position_or_nan")]
     pub position: f64,
     #[serde(default)]
     pub align: TabAlign,
@@ -609,26 +612,93 @@ impl TabStop {
     /// A tab-stop list as the Tabs commands keep it: at most [`MAX_TAB_STOPS`] stops, positions
     /// clamped, leaders and align-on characters checked, a char stop aligning on `.` unless it
     /// names a character, sorted by position, and one stop per position (the later one wins).
-    pub fn checked_list(tabs: Vec<TabStop>) -> Result<Vec<TabStop>, String> {
+    pub fn checked_list(mut tabs: Vec<TabStop>) -> Result<Vec<TabStop>, String> {
         if tabs.len() > MAX_TAB_STOPS {
             return Err(format!("a paragraph holds at most {MAX_TAB_STOPS} tab stops"));
         }
-        let mut out: Vec<TabStop> = Vec::with_capacity(tabs.len());
-        for mut t in tabs {
+        for t in &mut tabs {
             t.position = Self::clamp_position(t.position)?;
             Self::check_leader(&t.leader)?;
             Self::check_align_on(&t.align_on)?;
+        }
+        Ok(Self::normalized(tabs))
+    }
+
+    /// A tab-stop list read from a file, repaired to what [`checked_list`](Self::checked_list)
+    /// accepts: stops with a non-finite position dropped, positions clamped, control characters
+    /// removed from leaders and align-on characters and both cut to their limits, sorted, one stop
+    /// per position (the later one wins), and the first [`MAX_TAB_STOPS`] by position kept.
+    pub fn sanitized_list(tabs: Vec<TabStop>) -> Vec<TabStop> {
+        let tabs = tabs
+            .into_iter()
+            .filter_map(|mut t| {
+                t.position = Self::clamp_position(t.position).ok()?;
+                if Self::check_leader(&t.leader).is_err() {
+                    t.leader = t.leader.chars().filter(|c| !c.is_control()).take(MAX_TAB_LEADER).collect();
+                }
+                if Self::check_align_on(&t.align_on).is_err() {
+                    t.align_on = t.align_on.chars().filter(|c| !c.is_control()).take(1).collect();
+                }
+                Some(t)
+            })
+            .collect();
+        let mut out = Self::normalized(tabs);
+        out.truncate(MAX_TAB_STOPS);
+        out
+    }
+
+    /// Finite, clamped stops sorted by position, one per position: of stops closer than
+    /// [`SAME_TAB_POSITION`], the later one in `tabs` wins. A char stop with no character aligns
+    /// on `.`.
+    fn normalized(tabs: Vec<TabStop>) -> Vec<TabStop> {
+        let mut stops: Vec<(usize, TabStop)> = tabs.into_iter().enumerate().collect();
+        stops.sort_by(|a, b| a.1.position.total_cmp(&b.1.position));
+        let mut out: Vec<(usize, TabStop)> = Vec::with_capacity(stops.len());
+        for (i, mut t) in stops {
             if t.align == TabAlign::Char && t.align_on.is_empty() {
                 t.align_on = ".".into();
             }
-            out.retain(|o| (o.position - t.position).abs() >= SAME_TAB_POSITION);
-            out.push(t);
+            match out.last_mut() {
+                Some(last) if t.position - last.1.position < SAME_TAB_POSITION => {
+                    if i > last.0 {
+                        *last = (i, t);
+                    }
+                }
+                _ => out.push((i, t)),
+            }
         }
-        out.sort_by(|a, b| a.position.total_cmp(&b.position));
-        Ok(out)
+        out.into_iter().map(|(_, t)| t).collect()
     }
 }
 
+/// A stop's position; `null` (how JSON writes a non-finite number) reads as NaN.
+fn position_or_nan<'de, D: serde::Deserializer<'de>>(d: D) -> Result<f64, D::Error> {
+    Ok(Option::<f64>::deserialize(d)?.unwrap_or(f64::NAN))
+}
+
+/// A paragraph's tab-stop list in either attribute form, as read from a file.
+trait FileTabs: Sized {
+    fn sanitized(self) -> Self;
+}
+
+impl FileTabs for Vec<TabStop> {
+    fn sanitized(self) -> Self {
+        TabStop::sanitized_list(self)
+    }
+}
+
+impl FileTabs for Option<Vec<TabStop>> {
+    fn sanitized(self) -> Self {
+        self.map(TabStop::sanitized_list)
+    }
+}
+
+/// Deserializes a `tabs` field through [`TabStop::sanitized_list`]: a saved document's tab lists
+/// get the limits the Tabs commands keep.
+fn sanitized_tabs<'de, D: serde::Deserializer<'de>, T: Deserialize<'de> + FileTabs>(d: D) -> Result<T, D::Error> {
+    T::deserialize(d).map(FileTabs::sanitized)
+}
+
 /// A paragraph rule (Rule Above / Rule Below).
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 #[serde(default, rename_all = "camelCase")]
@@ -1110,6 +1180,7 @@ attr_set! {
         split_outside_gutter: f64 = 0.0,
         rule_above: Rule = Rule::default(),
         rule_below: Rule = Rule::default(),
+        #[serde(deserialize_with = "sanitized_tabs")]
         tabs: Vec<TabStop> = Vec::new(),
         list_type: ListType = ListType::None,
         /// Named list (Define Lists) the numbers belong to ("" = the story's own list).
@@ -1479,6 +1550,36 @@ mod tests {
         assert_eq!(list.iter().map(|t| t.position).collect::<Vec<_>>(), [10.0, 50.001], "sorted, one stop per position");
     }
 
+    /// A file's tab list is repaired, never refused; a list the commands accept comes through
+    /// unchanged.
+    #[test]
+    fn tab_lists_from_files_are_sanitized() {
+        let stop = |position: f64| TabStop { position, align: TabAlign::Left, leader: String::new(), align_on: String::new() };
+        let mut tabs: Vec<TabStop> = (0..500).rev().map(|i| stop(i as f64 * 10.0)).collect();
+        tabs.push(stop(f64::NAN));
+        tabs.push(stop(f64::NEG_INFINITY));
+        tabs.push(TabStop { leader: "\u{7}0123456789".into(), align: TabAlign::Char, align_on: "\n,;".into(), ..stop(-3.0) });
+        let list = TabStop::sanitized_list(tabs);
+        assert_eq!(list.len(), MAX_TAB_STOPS);
+        assert!(list.windows(2).all(|w| w[1].position - w[0].position >= SAME_TAB_POSITION), "sorted, one stop per position");
+        let first = &list[0];
+        assert_eq!((first.position, first.leader.as_str(), first.align_on.as_str()), (0.0, "01234567", ","), "the later stop at 0 wins");
+        assert_eq!(list.last().map(|t| t.position), Some(990.0), "the first stops by position are kept");
+
+        let ok = TabStop::checked_list(vec![stop(72.0), TabStop { align: TabAlign::Char, ..stop(10.0) }]).unwrap();
+        assert_eq!(TabStop::sanitized_list(ok.clone()), ok);
+
+        // `null` is how JSON writes a non-finite position; that stop is dropped, the others kept.
+        let json = r#"{"tabs":[{"position":null},{"position":20000,"leader":"0123456789"},{"position":5}]}"#;
+        let a: ParaAttrs = serde_json::from_str(json).unwrap();
+        let tabs = a.tabs.unwrap();
+        assert_eq!(tabs.iter().map(|t| t.position).collect::<Vec<_>>(), [5.0, MAX_TAB_POSITION]);
+        assert_eq!(tabs[1].leader, "01234567");
+        let p: ParaProps = serde_json::from_str(json).unwrap();
+        assert_eq!(p.tabs, tabs);
+        assert!(serde_json::from_str::<TabStop>("{}").is_err(), "a stop names its position");
+    }
+
     #[test]
     fn set_json_by_name() {
         let mut a = ParaAttrs::default();

crates/engine/src/cmd/tabs.rs

@@ -245,7 +245,8 @@ struct Measure {
     width: f64,
 }
 
-/// The stops and indents of the first selected paragraph, or of style `style`.
+/// The stops ([`TabStop::sanitized_list`]) and indents of the first selected paragraph, or of
+/// style `style`.
 fn current(s: &Session, style: Option<&str>, c: &str) -> Result<(Vec<TabStop>, Measure)> {
     let st = s.doc()?;
     let props = match style {
@@ -263,8 +264,8 @@ fn current(s: &Session, style: Option<&str>, c: &str) -> Result<(Vec<TabStop>, M
         }
     };
     let width = if style.is_none() { column_of(s).map(|(_, r)| r.width()) } else { None }.unwrap_or_else(|| page_column_width(s));
-    let mut tabs = props.tabs;
-    sort(&mut tabs);
+    // A list that never went through the checks (one set by code) reads as a file's would.
+    let tabs = TabStop::sanitized_list(props.tabs);
     Ok((tabs, Measure { left_indent: props.left_indent, first_line_indent: props.first_line_indent, right_indent: props.right_indent, width }))
 }
 
@@ -312,6 +313,7 @@ fn get(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 /// Apply `op` to the current stops and write the list to the selected paragraphs or the style.
+/// Returns the list as stored ([`TabStop::checked_list`]) and the index of `op`'s stop in it.
 fn edit(
     s: &mut Session,
     p: &Value,
@@ -321,6 +323,10 @@ fn edit(
     let style = str_param(p, "style").map(str::to_string);
     let (mut tabs, m) = current(s, style.as_deref(), c)?;
     let index = op(&mut tabs, m).map_err(|e| bad(c, e))?;
+    let at = index.and_then(|i| tabs.get(i)).map(|t| t.position);
+    let tabs = TabStop::checked_list(tabs).map_err(|e| bad(c, e))?;
+    // Checking can drop stops that share a position, which moves the indices after them.
+    let index = at.and_then(|x| tabs.iter().position(|t| (t.position - x).abs() < SAME));
     let list = serde_json::to_value(&tabs).map_err(|e| bad(c, e.to_string()))?;
     match style {
         Some(name) => s.edit(|d, _| {
@@ -447,6 +453,26 @@ mod tests {
         assert_eq!(at, [(50.0, TabAlign::Char, "."), (72.001, TabAlign::Right, ""), (120.0, TabAlign::Left, "")]);
     }
 
+    /// A command returns the list as it stores it, though the list it started from was never
+    /// checked.
+    #[test]
+    fn tab_commands_return_the_stored_list() {
+        let (mut s, _) = session("x");
+        s.execute("style.paragraph.create", &json!({"name": "Old"})).unwrap();
+        let stop = |position, align| TabStop { position, align, leader: String::new(), align_on: String::new() };
+        s.edit(|d, _| {
+            let st = d.styles_mut().para_mut("Old").unwrap();
+            st.para.tabs = Some(vec![stop(50.0, TabAlign::Char), stop(72.0, TabAlign::Left), stop(72.001, TabAlign::Right)]);
+            Ok(())
+        })
+        .unwrap();
+        let out = s.execute("type.tabs.add", &json!({"style": "Old", "position": 120})).unwrap();
+        let d = s.doc().unwrap().doc.clone();
+        let stored = d.styles.para("Old").unwrap().para.tabs.clone().unwrap();
+        assert_eq!(out["tabs"], serde_json::to_value(&stored).unwrap());
+        assert_eq!(out["index"], 2, "the added stop, in the stored list");
+    }
+
     /// `type.para` and the paragraph style commands take a whole `tabs` list; it gets the checks
     /// the Tabs commands apply.
     #[test]

crates/format/src/lib.rs

@@ -452,6 +452,27 @@ mod tests {
         assert!(load(&serde_json::to_vec(&v).unwrap()).unwrap().settings.glyph_fallback);
     }
 
+    /// A saved tab list over the limits loads repaired, as an imported one does: a non-finite
+    /// position (saved as `null`) dropped, positions clamped, leaders cut, the count capped.
+    #[test]
+    fn saved_tab_lists_load_sanitized() {
+        use designcraft_doc::{MAX_TAB_LEADER, MAX_TAB_POSITION, MAX_TAB_STOPS, TabAlign, TabStop};
+        let mut d = Document::new(&NewDocument::default());
+        let lid = d.default_layer();
+        let (_, sid) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(72.0, 72.0, 300.0, 300.0), lid, "a\tb", ParaFormat::default()).unwrap();
+        let stop = |position: f64| TabStop { position, align: TabAlign::Right, leader: "0123456789".into(), align_on: String::new() };
+        let mut tabs: Vec<TabStop> = (0..500).rev().map(|i| stop(i as f64 * 40.0)).collect();
+        tabs.push(stop(f64::NAN));
+        d.story_mut(sid).unwrap().paras[0].para.tabs = Some(tabs);
+        let bytes = save(&d).unwrap();
+        let back = load(&bytes).unwrap();
+        let tabs = back.stories[&sid].paras[0].para.tabs.clone().unwrap();
+        assert_eq!(tabs.len(), MAX_TAB_STOPS);
+        assert!(tabs.windows(2).all(|w| w[0].position < w[1].position), "sorted");
+        assert!(tabs.iter().all(|t| t.position.is_finite() && t.position <= MAX_TAB_POSITION && t.leader.chars().count() <= MAX_TAB_LEADER));
+        assert_eq!(tabs.last().map(|t| t.position), Some(3960.0), "the first stops by position");
+    }
+
     #[test]
     fn rejects_garbage_and_newer_versions() {
         assert!(load(b"nope").is_err());

crates/idml/src/import.rs

@@ -1593,7 +1593,7 @@ impl<'r> Importer<'r> {
                     align_on: get("AlignmentCharacter").filter(|c| c != ".").unwrap_or_default(),
                 });
             }
-            a.tabs = Some(tabs);
+            a.tabs = Some(TabStop::sanitized_list(tabs));
         }
         a.list_type = e.prop("BulletsAndNumberingListType").map(|v| match v.trim() {
             "BulletList" => ListType::Bullets,

crates/idml/src/tests.rs

@@ -2606,3 +2606,30 @@ fn ruby_settings_import_from_idml_and_round_trip() {
         ruby_overprint_stroke
     );
 }
+
+/// A file's tab list is sanitized as the Tabs commands check theirs: a hostile one imports
+/// within the limits instead of failing the import.
+#[test]
+fn hostile_tab_lists_import_within_the_limits() {
+    let item = |position: &str, leader: &str| {
+        format!(
+            r#"<ListItem type="record"><Alignment type="enumeration">LeftAlign</Alignment><AlignmentCharacter type="string">.</AlignmentCharacter><Leader type="string">{leader}</Leader><Position type="unit">{position}</Position></ListItem>"#
+        )
+    };
+    let mut items: String = (0..500).rev().map(|i| item(&(i * 3).to_string(), "")).collect();
+    items.push_str(&item("NaN", ""));
+    items.push_str(&item("1e300", "0123456789"));
+    let story = format!(
+        r#"<idPkg:Story xmlns:idPkg="http://ns.adobe.com/AdobeInDesign/idml/1.0/packaging"><Story Self="s1">
+      <ParagraphStyleRange AppliedParagraphStyle="ParagraphStyle/$ID/[No paragraph style]">
+        <Properties><TabList type="list">{items}</TabList></Properties>
+        <CharacterStyleRange AppliedCharacterStyle="CharacterStyle/$ID/[No character style]"><Content>Many tabs</Content></CharacterStyleRange>
+      </ParagraphStyleRange></Story></idPkg:Story>"#
+    );
+    let doc = import_idml(&fixture_with_story(&story)).unwrap();
+    let tabs = doc.stories.values().find(|s| s.text.contains("Many tabs")).unwrap().paras[0].para.tabs.clone().unwrap();
+    assert_eq!(tabs.len(), designcraft_doc::MAX_TAB_STOPS);
+    assert!(tabs.iter().all(|t| t.position.is_finite() && t.leader.chars().count() <= designcraft_doc::MAX_TAB_LEADER));
+    assert!(tabs.windows(2).all(|w| w[0].position < w[1].position), "sorted");
+    assert_eq!((tabs[0].position, tabs[99].position), (0.0, 297.0), "the first stops by position");
+}