From a6bafb4453832d95b0c8f019c7e2a1ca72afa840 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 14:46:28 +0800 Subject: [PATCH] fix(shell): boolean ops nested-source removal + drop dead export rect helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex stop-gate BLOCK #1: `boolean_ops::apply_boolean_op` looked up source paths recursively (via `active_page().find()`) but only removed them from the top-level `page.children` list. When the sources lived inside a Group or Frame, the originals stayed in their parent's children while the result was appended at the canvas root — duplication + orphans. Fix: replace the top-level `retain` call with a recursive `remove_nodes_recursively` walker that drops any node whose id is in the source set from every children Vec depth-first. New regression test `boolean_op_removes_nested_paths_not_just_top_level` wraps two paths in a Group, runs Union, and asserts: - the Group still exists but is empty - one result Path lives at top level (total page.children = 2) Codex stop-gate BLOCK #2: `property_panel_sections::export_section_rect` was added in commit `5bde95b9` (PropertyPanel preview pills) but never wired into `hit_test_action`, leaving the visible Export section unable to open the new ExportDialog. The helper is dead code in the meantime. Drop it; replace with a comment marking the follow-up. Existing UX (File menu → Export image / Cmd+Shift+P) still opens the dialog. Tracking via task #52. Codex stop-gate finding #3 (`main.rs` 828 / `input.rs` 886 over the 800-line cap) is real but stylistic — already tracked as task #55 (sibling-module split). No functional impact; deferred so this commit stays scoped to the data-corruption + dead-code fixes. Tests: 184 shell-core + 19 shell-native (+1 nested boolean ops regression). Wasm32 build clean. --- .../src/widgets/property_panel_sections.rs | 16 ++--- .../src/boolean_ops.rs | 61 ++++++++++++++++++- 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/crates/openpencil-shell-core/src/widgets/property_panel_sections.rs b/crates/openpencil-shell-core/src/widgets/property_panel_sections.rs index 043258875..4aac8af10 100644 --- a/crates/openpencil-shell-core/src/widgets/property_panel_sections.rs +++ b/crates/openpencil-shell-core/src/widgets/property_panel_sections.rs @@ -745,18 +745,10 @@ pub fn paint_export_section( y } -/// Y-extent of the export section, used by the hit-test layout -/// walker so a click anywhere inside emits OpenExportDialog. -pub fn export_section_rect(x: f32, y_top: f32, width: f32) -> Rect { - // section_label + 1 row of pills + 12px gap. Mirrors the paint - // layout above; if you change one, change both. - let label_h = 28.0; - let total_h = label_h + INPUT_HEIGHT + 12.0; - Rect { - origin: Point2D::new(x, y_top), - size: Point2D::new(width, total_h), - } -} +// NOTE: a future `export_section_rect` walker will live here once +// `hit_test_action` is extended to emit `OpenExportDialog` for +// clicks inside this section. Until then the section is preview- +// only — open the dialog via File menu / Cmd+Shift+P. // All shared paint primitives + layout constants are imported // from `property_panel_inputs` via the `pub use` block earlier in diff --git a/crates/openpencil-shell-native/src/boolean_ops.rs b/crates/openpencil-shell-native/src/boolean_ops.rs index 16123b69a..01e24d218 100644 --- a/crates/openpencil-shell-native/src/boolean_ops.rs +++ b/crates/openpencil-shell-native/src/boolean_ops.rs @@ -85,7 +85,12 @@ pub fn apply_boolean_op(doc: &mut Document, op: BooleanOp, next_id: &mut u64) -> new_node.bounds = openpencil_shell_core::Rect { origin, size }; let id_set: std::collections::HashSet = path_ids.iter().copied().collect(); if let Some(page) = doc.pages.get_mut(active) { - page.children.retain(|n| !id_set.contains(&n.id)); + // Recursive removal — sources may be nested inside groups / + // frames (codex CONCERN: `retain` only on top-level left + // originals behind + duplicated the result). Walk every + // children Vec depth-first and drop any node whose id is in + // the source set. + remove_nodes_recursively(&mut page.children, &id_set); page.children.push(new_node); } doc.selected_set.clear(); @@ -131,6 +136,16 @@ fn extract_points(path: &SkPath) -> Vec { out } +fn remove_nodes_recursively( + children: &mut Vec, + targets: &std::collections::HashSet, +) { + children.retain(|n| !targets.contains(&n.id)); + for child in children.iter_mut() { + remove_nodes_recursively(&mut child.children, targets); + } +} + fn bbox_of(points: &[Point2D]) -> (Point2D, Point2D) { let mut min_x = f32::INFINITY; let mut min_y = f32::INFINITY; @@ -226,6 +241,50 @@ mod tests { assert_eq!(doc.history.past.len(), 0); } + #[test] + fn boolean_op_removes_nested_paths_not_just_top_level() { + // Codex BLOCK: when source paths live inside a group/frame, + // the previous top-level-only `retain` left them in the + // group + appended the result at top level — duplication. + // Fix removes from anywhere in the children tree. + let mut doc = Document::empty(); + let page = doc.pages.get_mut(0).unwrap(); + page.children.clear(); + let mut a = Node::leaf(10, NodeKind::Path, "a"); + a.points = vec![ + Point2D::new(0.0, 0.0), + Point2D::new(20.0, 0.0), + Point2D::new(20.0, 20.0), + Point2D::new(0.0, 20.0), + ]; + a.bounds = Rect::xywh(0.0, 0.0, 20.0, 20.0); + let mut b = Node::leaf(11, NodeKind::Path, "b"); + b.points = vec![ + Point2D::new(10.0, 10.0), + Point2D::new(30.0, 10.0), + Point2D::new(30.0, 30.0), + Point2D::new(10.0, 30.0), + ]; + b.bounds = Rect::xywh(10.0, 10.0, 20.0, 20.0); + // Wrap both paths inside a Group. + let group = Node::with_children(99, NodeKind::Group, "g", vec![a, b]); + page.children.push(group); + doc.selected_set = vec![NodeId::new(10), NodeId::new(11)]; + doc.selected = NodeId::new(11); + let mut next = 100u64; + assert!(apply_boolean_op(&mut doc, BooleanOp::Union, &mut next)); + // Group still exists but is now empty of paths; result Path + // lives at top level — total page.children = 2 (empty group + result). + let page = doc.active_page().unwrap(); + assert_eq!(page.children.len(), 2); + let group_after = &page.children[0]; + assert!(matches!(group_after.kind, NodeKind::Group)); + assert!( + group_after.children.is_empty(), + "source paths must be removed from their group, not duplicated" + ); + } + #[test] fn boolean_op_skips_non_path_nodes_in_selection() { let mut doc = doc_with_two_squares();