SemianiakaVY/openpencil
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(shell): boolean ops nested-source removal + drop dead export rect helper

Browse source 
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.
This commit is contained in:
Kayshen-X 2026-05-14 14:46:28 +08:00
parent 57931d6a82
commit a6bafb4453
2 changed files with 64 additions and 13 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
crates/openpencil-shell-core/src/widgets/property_panel_sections.rs
View file

@ -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

61
crates/openpencil-shell-native/src/boolean_ops.rs
View file

@ -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<NodeId> = 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<Point2D> {
out
}
fn remove_nodes_recursively(
children: &mut Vec<openpencil_shell_core::document::Node>,
targets: &std::collections::HashSet<NodeId>,
) {
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();