Batch r2-1 review: SVG ids unique as written; a filled open curve closes for its fill

crates/svg/src/import/mod.rs

@@ -446,8 +446,8 @@ fn paint_path(cx: &mut Ctx, p: &usvg::Path, mut n: Node, transform: Affine, worl
 const CLOSE_TOLERANCE: f64 = 1e-3;
 
 /// `path` with the subpaths that end within `tol` of their start closed onto it and, with `all`
-/// (SVG fills every subpath as if closed), every other subpath of two or more segments closed
-/// with a straight segment.
+/// (SVG fills every subpath as if closed), every other subpath that can enclose an area (two or
+/// more segments, or one curve) closed with a straight segment.
 fn closed_for_fill(path: &BezPath, tol: f64, all: bool) -> BezPath {
     use figcraft_geom::kurbo::PathEl;
     let mut out = BezPath::new();
@@ -462,7 +462,8 @@ fn closed_for_fill(path: &BezPath, tol: f64, all: bool) -> BezPath {
         };
         let open = !matches!(sub.last(), Some(PathEl::ClosePath));
         let segments = sub.len() - 1;
-        if open && segments >= 2 {
+        let curve = matches!(sub.last(), Some(PathEl::QuadTo(..) | PathEl::CurveTo(..)));
+        if open && (segments >= 2 || curve) {
             let near = sub.last().and_then(|e| e.end_point()).is_some_and(|end| (end - start).hypot() <= tol);
             if near && let Some(last) = sub.last_mut() {
                 match last {

crates/svg/src/lib.rs

@@ -155,13 +155,12 @@ fn blend_style(b: figcraft_color::BlendMode) -> String {
 /// `s` escaped for XML text and attribute values; characters XML 1.0 forbids (control characters
 /// other than tab, newline and carriage return, U+FFFE and U+FFFF) are left out.
 fn esc(s: &str) -> String {
-    s.chars()
-        .filter(|&c| !matches!(c, '\u{0}'..='\u{8}' | '\u{b}' | '\u{c}' | '\u{e}'..='\u{1f}' | '\u{fffe}' | '\u{ffff}'))
-        .collect::<String>()
-        .replace('&', "&amp;")
-        .replace('<', "&lt;")
-        .replace('>', "&gt;")
-        .replace('"', "&quot;")
+    s.chars().filter(|&c| xml_char(c)).collect::<String>().replace('&', "&amp;").replace('<', "&lt;").replace('>', "&gt;").replace('"', "&quot;")
+}
+
+/// Whether XML 1.0 allows `c` (see [`esc`]).
+fn xml_char(c: char) -> bool {
+    !matches!(c, '\u{0}'..='\u{8}' | '\u{b}' | '\u{c}' | '\u{e}'..='\u{1f}' | '\u{fffe}' | '\u{ffff}')
 }
 
 fn hex(c: Color) -> String {
@@ -562,7 +561,9 @@ impl W<'_> {
 
     /// `name` as an `id` not used yet in this file (spaces as underscores; repeats get `_2`…).
     fn unique_id(&mut self, name: &str) -> String {
-        let base: String = name.chars().take(200).map(|c| if c.is_whitespace() || c.is_control() { '_' } else { c }).collect();
+        // As written: what `esc` leaves out isn't part of it.
+        let base: String =
+            name.chars().take(200).map(|c| if c.is_whitespace() || c.is_control() { '_' } else { c }).filter(|&c| xml_char(c)).collect();
         let base = if base.is_empty() { "layer".to_string() } else { base };
         let next = self.next_suffix.entry(base.clone()).or_insert(1);
         let id = loop {

crates/svg/src/tests.rs

@@ -229,6 +229,9 @@ fn unique_id_is_linear_for_repeated_names() {
     assert_eq!(w.unique_id("a_3"), "a_3");
     assert_eq!(w.unique_id("a"), "a_2");
     assert_eq!(w.unique_id("a"), "a_4");
+    // Unique as written, without what XML leaves out.
+    assert_eq!(w.unique_id("b\u{fffe}"), "b");
+    assert_eq!(w.unique_id("b\u{ffff}"), "b_2");
     let start = std::time::Instant::now();
     for _ in 0..30_000 {
         w.unique_id("Rectangle");
@@ -247,3 +250,17 @@ fn layer_ids_never_clash_with_generated_ids() {
     let svg = export_svg(&d, &[f], &SvgOptions { include_ids: true, ..SvgOptions::default() }).unwrap();
     assert_eq!(svg.matches("id=\"clip1\"").count(), 1, "{svg}");
 }
+
+#[test]
+fn a_filled_open_curve_is_closed_for_its_fill() {
+    // SVG fills an open subpath as if closed: one curve and the line back enclose an area.
+    let mut d = Document::new("t");
+    let p = d.first_page().unwrap();
+    let svg = r##"<svg xmlns="http://www.w3.org/2000/svg" width="100" height="50"><path fill="#000000" d="M0 0C0 50 100 50 100 0"/></svg>"##;
+    import_svg(&mut d, svg.as_bytes(), p, 0.0, 0.0, "x").unwrap();
+    let loops = |n: &figcraft_doc::Node| match &n.kind {
+        NodeKind::Vector { network } => network.regions.iter().map(|r| r.loops.len()).sum::<usize>(),
+        _ => 0,
+    };
+    assert_eq!(d.nodes().map(loops).sum::<usize>(), 1);
+}