From 0650c24a09fc261e78b51ed44deac2d4debe2605 Mon Sep 17 00:00:00 2001 From: nrq6993-a11y Date: Sun, 18 Oct 2026 22:21:38 +0300 Subject: [PATCH 1/2] Fix #2178: Pathfinder Unite groups disconnected islands for Ungroup After Unite, split the result with draw2::components so each island (outer contour plus its holes) becomes one group child instead of one compound path. Co-authored-by: Cursor --- crates/engine/src/cmd/pathops.rs | 24 ++++++++++++++--- crates/engine/src/tests_pathops.rs | 43 ++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/pathops.rs b/crates/engine/src/cmd/pathops.rs index cbed03515..a67fbad44 100644 --- a/crates/engine/src/cmd/pathops.rs +++ b/crates/engine/src/cmd/pathops.rs @@ -300,6 +300,7 @@ fn shape_node_styled(d: &mut Document, path: PathData, style: &Style) -> Node { n } + /// Whether `ids` are separate objects, as [`Document::remove_many`] and [`Document::insert_beside`] /// take them: each one is in the tree and listed once, and none lies inside another listed one. pub(crate) fn separate(d: &Document, ids: &[NodeId]) -> bool { @@ -464,14 +465,29 @@ fn run_pf(s: &mut Session, op: PathfinderOp) -> Result { let r = results.into_iter().find(|r| !r.path.is_empty()).ok_or_else(|| EngineError::Other(format!("{label} produced an empty result")))?; let style = styles.get(r.key as usize).ok_or_else(|| EngineError::Other(format!("{label}: lost track of a shape's style")))?; - let mut n = shape_node_styled(d, r.path, style); - n.name = match op { - // A subtraction keeps its surviving source's identity, even when unnamed. + let name = match op { PathfinderOp::MinusFront => names.first().cloned().flatten(), PathfinderOp::MinusBack => names.last().cloned().flatten(), _ => names.into_iter().flatten().next(), }; - n + let path = r.path; + if op == PathfinderOp::Unite { + let pieces = super::draw2::components(&path); + if pieces.len() > 1 { + let children = pieces.into_iter().map(|p| Arc::new(shape_node_styled(d, p, style))).collect(); + let mut g = Node::group(d.alloc_id(), children); + g.name = name; + g + } else { + let mut n = shape_node_styled(d, path, style); + n.name = name; + n + } + } else { + let mut n = shape_node_styled(d, path, style); + n.name = name; + n + } } else { let mut children = vec![]; for r in results { diff --git a/crates/engine/src/tests_pathops.rs b/crates/engine/src/tests_pathops.rs index 384fa6593..f774801f6 100644 --- a/crates/engine/src/tests_pathops.rs +++ b/crates/engine/src/tests_pathops.rs @@ -205,6 +205,49 @@ fn a_group_selected_alone_is_combined_member_by_member() { assert_eq!((bounds.x0, bounds.x1), (0.0, 10.0), "the back member less the front one"); } +/// #2178: Unite on separated shapes should group them so Object › Ungroup releases each piece. +#[test] +fn pathfinder_unite_non_overlapping_grouped_shapes_stay_ungroupable() { + let mut s = session(); + let a = rect(&mut s, 0.0, 0.0, 20.0, 20.0); + let b = rect(&mut s, 30.0, 0.0, 20.0, 20.0); + s.execute("select.set", &json!({"ids": [a.0, b.0]})).unwrap(); + let g = NodeId(s.execute("object.group", &json!({})).unwrap()["id"].as_u64().unwrap()); + let layer = s.doc().unwrap().doc.parent_of(g).unwrap(); + s.execute("object.pathfinder.unite", &json!({})).unwrap(); + let result = s.doc().unwrap().selection.objects[0]; + assert!(matches!(node(&s, result).kind, NodeKind::Group { .. })); + assert_eq!(node(&s, result).children().unwrap().len(), 2); + s.execute("object.ungroup", &json!({})).unwrap(); + let d = &s.doc().unwrap().doc; + assert!(d.node(g).is_none()); + assert_eq!(s.doc().unwrap().selection.len(), 2); + for id in &s.doc().unwrap().selection.objects { + assert_eq!(d.parent_of(*id), Some(layer)); + } +} + +/// #2178: each connected island stays one object; holes stay with their outer contour, not separate group children. +#[test] +fn pathfinder_unite_groups_disjoint_islands_not_subpaths_of_one_island() { + let mut s = session(); + let outer = rect(&mut s, 0.0, 0.0, 40.0, 40.0); + let hole = rect(&mut s, 10.0, 10.0, 10.0, 10.0); + s.execute("select.set", &json!({"ids": [outer.0, hole.0]})).unwrap(); + let ring = NodeId(s.execute("object.compoundPath.make", &json!({})).unwrap()["id"].as_u64().unwrap()); + let separate = rect(&mut s, 60.0, 0.0, 20.0, 20.0); + s.execute("select.set", &json!({"ids": [ring.0, separate.0]})).unwrap(); + s.execute("object.group", &json!({})).unwrap(); + s.execute("object.pathfinder.unite", &json!({})).unwrap(); + let result = s.doc().unwrap().selection.objects[0]; + let g = node(&s, result); + assert!(matches!(g.kind, NodeKind::Group { .. })); + let kids = g.children().unwrap(); + assert_eq!(kids.len(), 2); + let subpath_counts: Vec = kids.iter().map(|c| path_of(&s, c.id).0.subpaths.len()).collect(); + assert_eq!(subpath_counts, [2, 1]); +} + #[test] fn unite_with_group() { let mut s = session(); From 7ceaf900d67d2eba691d7fcdefac51c83c0174f2 Mon Sep 17 00:00:00 2001 From: nrq6993-a11y Date: Mon, 19 Oct 2026 22:07:46 +0300 Subject: [PATCH 2/2] Pathfinder Unite: compound path, not group (#2178) Illustrator documents Unite as a single compound path; use shape_node_styled and compoundPath.release in tests instead of wrapping islands in groups. Co-authored-by: Cursor --- crates/engine/src/cmd/pathops.rs | 24 ++++-------------------- crates/engine/src/tests_pathops.rs | 29 ++++++++++++----------------- 2 files changed, 16 insertions(+), 37 deletions(-) diff --git a/crates/engine/src/cmd/pathops.rs b/crates/engine/src/cmd/pathops.rs index a67fbad44..cbed03515 100644 --- a/crates/engine/src/cmd/pathops.rs +++ b/crates/engine/src/cmd/pathops.rs @@ -300,7 +300,6 @@ fn shape_node_styled(d: &mut Document, path: PathData, style: &Style) -> Node { n } - /// Whether `ids` are separate objects, as [`Document::remove_many`] and [`Document::insert_beside`] /// take them: each one is in the tree and listed once, and none lies inside another listed one. pub(crate) fn separate(d: &Document, ids: &[NodeId]) -> bool { @@ -465,29 +464,14 @@ fn run_pf(s: &mut Session, op: PathfinderOp) -> Result { let r = results.into_iter().find(|r| !r.path.is_empty()).ok_or_else(|| EngineError::Other(format!("{label} produced an empty result")))?; let style = styles.get(r.key as usize).ok_or_else(|| EngineError::Other(format!("{label}: lost track of a shape's style")))?; - let name = match op { + let mut n = shape_node_styled(d, r.path, style); + n.name = match op { + // A subtraction keeps its surviving source's identity, even when unnamed. PathfinderOp::MinusFront => names.first().cloned().flatten(), PathfinderOp::MinusBack => names.last().cloned().flatten(), _ => names.into_iter().flatten().next(), }; - let path = r.path; - if op == PathfinderOp::Unite { - let pieces = super::draw2::components(&path); - if pieces.len() > 1 { - let children = pieces.into_iter().map(|p| Arc::new(shape_node_styled(d, p, style))).collect(); - let mut g = Node::group(d.alloc_id(), children); - g.name = name; - g - } else { - let mut n = shape_node_styled(d, path, style); - n.name = name; - n - } - } else { - let mut n = shape_node_styled(d, path, style); - n.name = name; - n - } + n } else { let mut children = vec![]; for r in results { diff --git a/crates/engine/src/tests_pathops.rs b/crates/engine/src/tests_pathops.rs index f774801f6..e4cc2218f 100644 --- a/crates/engine/src/tests_pathops.rs +++ b/crates/engine/src/tests_pathops.rs @@ -205,31 +205,31 @@ fn a_group_selected_alone_is_combined_member_by_member() { assert_eq!((bounds.x0, bounds.x1), (0.0, 10.0), "the back member less the front one"); } -/// #2178: Unite on separated shapes should group them so Object › Ungroup releases each piece. +/// #2178: Unite on separated shapes is one compound path; Release releases each subpath (Illustrator parity). #[test] -fn pathfinder_unite_non_overlapping_grouped_shapes_stay_ungroupable() { +fn pathfinder_unite_non_overlapping_shapes_become_compound_path_releasable() { let mut s = session(); let a = rect(&mut s, 0.0, 0.0, 20.0, 20.0); let b = rect(&mut s, 30.0, 0.0, 20.0, 20.0); + let layer = s.doc().unwrap().doc.parent_of(a).unwrap(); s.execute("select.set", &json!({"ids": [a.0, b.0]})).unwrap(); - let g = NodeId(s.execute("object.group", &json!({})).unwrap()["id"].as_u64().unwrap()); - let layer = s.doc().unwrap().doc.parent_of(g).unwrap(); s.execute("object.pathfinder.unite", &json!({})).unwrap(); let result = s.doc().unwrap().selection.objects[0]; - assert!(matches!(node(&s, result).kind, NodeKind::Group { .. })); - assert_eq!(node(&s, result).children().unwrap().len(), 2); - s.execute("object.ungroup", &json!({})).unwrap(); + let united = node(&s, result); + assert!(matches!(united.kind, NodeKind::Compound { .. })); + assert_eq!(united.children().unwrap().len(), 2); + s.execute("object.compoundPath.release", &json!({})).unwrap(); let d = &s.doc().unwrap().doc; - assert!(d.node(g).is_none()); + assert!(d.node(result).is_none()); assert_eq!(s.doc().unwrap().selection.len(), 2); for id in &s.doc().unwrap().selection.objects { assert_eq!(d.parent_of(*id), Some(layer)); } } -/// #2178: each connected island stays one object; holes stay with their outer contour, not separate group children. +/// #2178: Unite keeps disjoint geometry in one compound path (not a group of separate path objects). #[test] -fn pathfinder_unite_groups_disjoint_islands_not_subpaths_of_one_island() { +fn pathfinder_unite_disjoint_islands_stay_one_compound_path() { let mut s = session(); let outer = rect(&mut s, 0.0, 0.0, 40.0, 40.0); let hole = rect(&mut s, 10.0, 10.0, 10.0, 10.0); @@ -237,15 +237,10 @@ fn pathfinder_unite_groups_disjoint_islands_not_subpaths_of_one_island() { let ring = NodeId(s.execute("object.compoundPath.make", &json!({})).unwrap()["id"].as_u64().unwrap()); let separate = rect(&mut s, 60.0, 0.0, 20.0, 20.0); s.execute("select.set", &json!({"ids": [ring.0, separate.0]})).unwrap(); - s.execute("object.group", &json!({})).unwrap(); s.execute("object.pathfinder.unite", &json!({})).unwrap(); let result = s.doc().unwrap().selection.objects[0]; - let g = node(&s, result); - assert!(matches!(g.kind, NodeKind::Group { .. })); - let kids = g.children().unwrap(); - assert_eq!(kids.len(), 2); - let subpath_counts: Vec = kids.iter().map(|c| path_of(&s, c.id).0.subpaths.len()).collect(); - assert_eq!(subpath_counts, [2, 1]); + assert!(matches!(node(&s, result).kind, NodeKind::Compound { .. })); + assert_eq!(path_of(&s, result).0.subpaths.len(), 3); } #[test]