From 71a669635c88471b38bda0eeb63f08ffffdd411c Mon Sep 17 00:00:00 2001 From: el-j Date: Sat, 24 Oct 2026 00:55:51 +0200 Subject: [PATCH] M2.1: prevent click-through on open path fills and support segment toggle (#3405) --- crates/engine/src/cmd/select.rs | 97 +++++++++-- crates/engine/tests/selection_commands.rs | 29 ++++ crates/tools/src/direct.rs | 200 +++++++++++++++++++++- 3 files changed, 303 insertions(+), 23 deletions(-) diff --git a/crates/engine/src/cmd/select.rs b/crates/engine/src/cmd/select.rs index 0a7ec31d3..8a7d220e3 100644 --- a/crates/engine/src/cmd/select.rs +++ b/crates/engine/src/cmd/select.rs @@ -76,7 +76,7 @@ pub fn specs() -> Vec { "Select Segments", [], None, - "{id, segments: [[subpath, segment]…]} select path segments; their endpoints move with them, Delete removes only their connections", + "{id, segments: [[subpath, segment]…], mode?: \"set\"|\"add\"|\"toggle\"} select path segments; their endpoints move with them, Delete removes only their connections", has_doc, segments ), @@ -436,22 +436,85 @@ pub(crate) fn checked_refs(v: Option<&Value>, cmd: &str) -> Result, cmd: &str) -> Result> { + let path = doc.node(id).and_then(|n| n.path_data()).ok_or_else(|| bad(cmd, "object is not a path"))?; + let mut endpoints = BTreeSet::new(); + for &(si, seg) in refs { + let sp = path.subpaths.get(si).filter(|sp| seg < sp.segment_count()).ok_or_else(|| bad(cmd, "segment does not exist"))?; + let n = sp.anchors.len().max(1); + endpoints.extend([(si, seg), (si, (seg + 1) % n)]); + } + Ok(endpoints) +} + fn segments(s: &mut Session, p: &Value) -> Result { const C: &str = "select.segments"; let id = checked_id(s, p.get("id").ok_or_else(|| bad(C, "missing id"))?, C)?; let refs: BTreeSet<_> = checked_refs(p.get("segments"), C)?.into_iter().collect(); - let path = s.doc()?.doc.node(id).and_then(|n| n.path_data()).ok_or_else(|| bad(C, "object is not a path"))?; - let mut endpoints = BTreeSet::new(); - for &(si, seg) in &refs { - let sp = path.subpaths.get(si).filter(|sp| seg < sp.segment_count()).ok_or_else(|| bad(C, "segment does not exist"))?; - endpoints.extend([(si, seg), (si, (seg + 1) % sp.anchors.len())]); - } - s.select(|_, sel| { - sel.clear(); - if !refs.is_empty() { - sel.add(id); - sel.anchors.insert(id, endpoints); - sel.segments.insert(id, refs); + let mode = match str_param(p, "mode") { + None | Some("set") => "set", + Some("add") => "add", + Some("toggle") => "toggle", + Some(m) => return Err(bad(C, format!("unknown mode {m:?}"))), + }; + let endpoints = segment_endpoints(&s.doc()?.doc, id, &refs, C)?; + s.select(|d, sel| { + if mode == "set" { + sel.clear(); + if !refs.is_empty() { + sel.add(id); + sel.anchors.insert(id, endpoints); + sel.segments.insert(id, refs); + } + } else if mode == "add" { + if !refs.is_empty() { + sel.add(id); + sel.anchors.entry(id).or_default().extend(endpoints); + sel.segments.entry(id).or_default().extend(refs); + } + } else if mode == "toggle" { + let seg_entry = sel.segments.entry(id).or_default(); + let mut removed_endpoints = BTreeSet::new(); + let mut added_endpoints = BTreeSet::new(); + let path = d.node(id).and_then(|n| n.path_data()); + for &(si, seg) in &refs { + let Some(sp) = path.as_ref().and_then(|p| p.subpaths.get(si)) else { continue }; + let n = sp.anchors.len().max(1); + let end1 = (si, seg); + let end2 = (si, (seg + 1) % n); + if seg_entry.contains(&(si, seg)) { + seg_entry.remove(&(si, seg)); + removed_endpoints.insert(end1); + removed_endpoints.insert(end2); + } else { + seg_entry.insert((si, seg)); + added_endpoints.insert(end1); + added_endpoints.insert(end2); + } + } + let mut surviving_seg_endpoints = BTreeSet::new(); + for &(si, seg) in seg_entry.iter() { + if let Some(sp) = path.as_ref().and_then(|p| p.subpaths.get(si)) { + let n = sp.anchors.len().max(1); + surviving_seg_endpoints.insert((si, seg)); + surviving_seg_endpoints.insert((si, (seg + 1) % n)); + } + } + let anchor_entry = sel.anchors.entry(id).or_default(); + for pt in removed_endpoints { + if !surviving_seg_endpoints.contains(&pt) { + anchor_entry.remove(&pt); + } + } + anchor_entry.extend(added_endpoints); + if seg_entry.is_empty() { + sel.segments.remove(&id); + } + if anchor_entry.is_empty() { + sel.remove(id); + } else { + sel.add(id); + } } })?; ok() @@ -471,8 +534,9 @@ fn anchors(s: &mut Session, p: &Value) -> Result { if mode == "set" { sel.clear(); } - // A path selected as a whole has every anchor selected: toggle and add start from them all. - let whole = sel.contains(id) && sel.partial(id).is_none(); + // A path selected as a whole has every anchor selected: toggle and add start from them all, + // but only when no partial anchor selection is active and only on a single isolated object. + let whole = sel.contains(id) && sel.partial(id).is_none() && sel.anchors.is_empty() && sel.objects.len() <= 1; sel.add(id); // A segment pick's endpoints aren't selected anchors: start from none of them. if sel.segments.remove(&id).is_some() { @@ -489,6 +553,9 @@ fn anchors(s: &mut Session, p: &Value) -> Result { e.insert(r); } } + if e.is_empty() { + sel.remove(id); + } })?; ok() } diff --git a/crates/engine/tests/selection_commands.rs b/crates/engine/tests/selection_commands.rs index 94db94a7b..7d05e68ce 100644 --- a/crates/engine/tests/selection_commands.rs +++ b/crates/engine/tests/selection_commands.rs @@ -133,3 +133,32 @@ fn select_key_and_document_inspect_report_the_key_object() { assert_eq!(s.execute("select.key", &json!({"id": b.0})).unwrap(), json!({"key": null})); assert_eq!(inspected(&mut s), Some(Value::Null)); } + +#[test] +fn select_segments_mode_toggle_and_add() { + let (mut s, a, _) = session(); + // Select segment 0 with mode set + s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 0]], "mode": "set"})).unwrap(); + let sel = &s.active().unwrap().selection; + assert_eq!(sel.segments.get(&a), Some(&[(0, 0)].into())); + assert_eq!(sel.anchors.get(&a), Some(&[(0, 0), (0, 1)].into())); + + // Toggle segment 1 on (add) + s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 1]], "mode": "toggle"})).unwrap(); + let sel = &s.active().unwrap().selection; + assert_eq!(sel.segments.get(&a), Some(&[(0, 0), (0, 1)].into())); + assert_eq!(sel.anchors.get(&a), Some(&[(0, 0), (0, 1), (0, 2)].into())); + + // Toggle segment 0 off + s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 0]], "mode": "toggle"})).unwrap(); + let sel = &s.active().unwrap().selection; + assert_eq!(sel.segments.get(&a), Some(&[(0, 1)].into())); + assert_eq!(sel.anchors.get(&a), Some(&[(0, 1), (0, 2)].into())); + + // Toggle segment 1 off -> clears selection for this path + s.execute("select.segments", &json!({"id": a.0, "segments": [[0, 1]], "mode": "toggle"})).unwrap(); + let sel = &s.active().unwrap().selection; + assert!(!sel.contains(a)); + assert!(sel.segments.is_empty()); + assert!(sel.anchors.is_empty()); +} diff --git a/crates/tools/src/direct.rs b/crates/tools/src/direct.rs index 71fc26e28..ca39112f6 100644 --- a/crates/tools/src/direct.rs +++ b/crates/tools/src/direct.rs @@ -300,11 +300,51 @@ fn anchor_owners(cx: &ToolContext, f: fn(&Node) -> bool) -> Vec { /// The anchor within `tol` of `p` nearest to it, of the topmost selected path or area type frame /// that has one, else of the topmost visible one: (id, si, ai, where it is). Selected objects come /// first so an object above can't take the press from the one being edited (#1508), as -/// [`hovered_anchor`] marks it. +/// [`hovered_anchor`] marks it. In normal viewmode, an opaque fill in front occludes unselected +/// objects behind it (#3405). fn hit_anchor(cx: &ToolContext, p: Point, tol: f64) -> Option<(NodeId, usize, usize, Point)> { let mut owners = anchor_owners(cx, |n| matches!(n.kind, NodeKind::Path { .. }) || is_area_type(n)); owners.sort_by_key(|id| !cx.selection.contains(*id)); - owners.into_iter().find_map(|id| { + + // 1. Check selected objects first. Selected objects are NEVER occluded (#1508). + let mut unselected_idx = owners.len(); + for (i, &id) in owners.iter().enumerate() { + if !cx.selection.contains(id) { + unselected_idx = i; + break; + } + if let Some(pd) = cx.doc.node(id).and_then(editable_path) + && let Some((si, ai, a)) = nearest_anchor(&pd, p, tol) + { + return Some((id, si, ai, a)); + } + } + + // 2. For unselected objects: in normal viewmode, an opaque fill in front occludes + // unselected objects behind it (#3405). + let unselected = &owners[unselected_idx..]; + if unselected.is_empty() { + return None; + } + + let filter: Option bool>> = if !cx.outline + && let Some(h) = hit_test(cx.doc, p, cx.selection_hit_options()) + && matches!(h.kind, HitKind::Fill) + { + let leaf = h.leaf; + let ancestry = h.ancestry.clone(); + let part = cx.doc.node(leaf).and_then(|n| compound_part_at(n, p, tol)); + Some(Box::new(move |id| id == leaf || ancestry.contains(&id) || part == Some(id))) + } else { + None + }; + + unselected.iter().copied().find_map(|id| { + if let Some(f) = &filter + && !f(id) + { + return None; + } let pd = cx.doc.node(id).and_then(editable_path)?; nearest_anchor(&pd, p, tol).map(|(si, ai, a)| (id, si, ai, a)) }) @@ -579,27 +619,36 @@ impl Tool for DirectSelectionTool { self.state = State::Idle; return out; } - if let Some(h) = hit_test(cx.doc, p, cx.selection_hit_options()) { + if let Some(mut h) = hit_test(cx.doc, p, cx.selection_hit_options()) { + if let Some(part) = cx.doc.node(h.leaf).and_then(|n| compound_part_at(n, p, tol)) { + h.ancestry.push(part); + h.leaf = part; + } // A segment pick keeps its endpoints available for movement without selecting // the anchors themselves. Dragging reshapes a curve, else moves it. Alt picks // the whole path (and Alt-drag copies it), as with Group Selection. let whole = cx.selection.contains(h.leaf) && cx.selection.partial(h.leaf).is_none(); - if !ev.mods.shift - && !ev.mods.alt + if !ev.mods.alt && matches!(h.kind, HitKind::Stroke | HitKind::Outline) && let Some(s) = segment_at(cx, h.leaf, p) { - if whole { + if whole && !ev.mods.shift { self.click_segment = Some((h.leaf, s.si, s.seg)); } else { let selected = cx.selection.segments.get(&h.leaf).is_some_and(|sel| sel.contains(&(s.si, s.seg))); - self.state = if s.curved { + self.state = if s.curved && !ev.mods.shift { State::Segment { id: h.leaf, si: s.si, seg: s.seg, t: s.t, start: p, began: false } } else { // Held by the segment itself, so the segment is what lands on a // ruler guide or lines up, not the pressed spot beside it (#3701). State::MoveAnchors { start: p, grab: hold_point(cx, &h, p), began: false } }; + if ev.mods.shift { + return vec![Action::Exec( + "select.segments".into(), + json!({"id": h.leaf.0, "segments": [[s.si, s.seg]], "mode": "toggle"}), + )]; + } if selected { return vec![]; } @@ -962,8 +1011,9 @@ impl DirectSelectionTool { /// The anchor within a press's reach of `p` ([`ToolContext::point_tol`]), of a selected path or the /// path under `p`: what Highlight anchors on mouse over marks (a press there picks it). fn hovered_anchor(cx: &ToolContext, p: Point) -> Option<(NodeId, Point)> { - let under = hit_test(cx.doc, p, cx.selection_hit_options()).map(|h| h.leaf); let tol = cx.point_tol(); + let under = + hit_test(cx.doc, p, cx.selection_hit_options()).map(|h| cx.doc.node(h.leaf).and_then(|n| compound_part_at(n, p, tol)).unwrap_or(h.leaf)); cx.selection.objects.iter().copied().filter(|id| in_scope(cx.doc, cx.isolation, *id)).chain(under).find_map(|id| { let path = cx.doc.node(id).and_then(editable_path)?; nearest_anchor(&path, p, tol).map(|(.., a)| (id, a)) @@ -2113,4 +2163,138 @@ mod tests { let acts_group_text = t.pointer(&c2, &PointerEvent::new(PointerKind::DoubleClick, in_group_b.center().x, in_group_b.center().y)); assert_eq!(acts_group_text, vec![Action::SwitchTool("type".into())]); } + + #[test] + fn direct_selection_click_and_drag_member_of_compound_path() { + let mut d = vectorcraft_doc::Document::new(500.0, 500.0); + let l = d.layers[0].id; + let (outer, hole, compound) = (d.alloc_id(), d.alloc_id(), d.alloc_id()); + let rect = |id, a: f64, b: f64| { + std::sync::Arc::new(vectorcraft_doc::Node::path(id, vectorcraft_geom::shapes::rectangle(Rect::new(a, a, b, b)), Default::default())) + }; + let mut c = vectorcraft_doc::Node::new( + compound, + vectorcraft_doc::NodeKind::Compound { + children: vec![rect(outer, 0.0, 200.0), rect(hole, 50.0, 150.0)], + rule: vectorcraft_geom::FillRule::EvenOdd, + }, + ); + c.appearance = vectorcraft_doc::Appearance::default_art().into(); + d.insert(Some(l), 0, c).unwrap(); + let p = paint(); + let none = Selection::default(); + let mut t = DirectSelectionTool::new(false); + let ctx = cx(&d, &none, &p); + let down = t.pointer(&ctx, &PointerEvent::new(PointerKind::Down, 25.0, 100.0)); + assert_eq!( + down, + vec![Action::Exec("select.set".into(), json!({"ids": [outer.0]}))], + "Direct Selection selects child member, not compound parent" + ); + let mut s = Selection::default(); + s.set([outer]); + let ctx_sel = cx(&d, &s, &p); + let drag = t.pointer(&ctx_sel, &PointerEvent::new(PointerKind::Drag, 35.0, 110.0)); + assert!(drag.iter().any(|a| matches!(a, Action::Preview(cmd, _) if cmd == "object.transform")), "drags the selected child member: {drag:?}"); + let up = t.pointer(&ctx_sel, &PointerEvent::new(PointerKind::Up, 35.0, 110.0)); + assert_eq!(up, vec![Action::Commit]); + } + + #[test] + fn shift_clicking_anchor_after_segment_pick_toggles_or_adds() { + let (d, id) = doc_with_rect(); + let p = paint(); + let mut s = Selection::default(); + s.set([id]); + s.segments.insert(id, [(0, 0)].into()); + s.anchors.insert(id, [(0, 0), (0, 1)].into()); + let cx = cx(&d, &s, &p); + let mut t = DirectSelectionTool::new(false); + let shift = Mods { shift: true, ..Default::default() }; + // Shift-click on anchor (0, 0) (top-left at 100, 100) toggles it off + let a = t.pointer(&cx, &PointerEvent::new(PointerKind::Down, 100.0, 100.0).with_mods(shift)); + assert_eq!(a, vec![Action::Exec("select.anchors".into(), json!({"id": id.0, "anchors": [[0, 0]], "mode": "toggle"}))]); + // Shift-click on anchor (0, 2) (bottom-right at 200, 200) toggles it on (adds it) + let a2 = t.pointer(&cx, &PointerEvent::new(PointerKind::Down, 200.0, 200.0).with_mods(shift)); + assert_eq!(a2, vec![Action::Exec("select.anchors".into(), json!({"id": id.0, "anchors": [[0, 2]], "mode": "toggle"}))]); + } + + #[test] + fn shift_clicking_segment_toggles_segment() { + let (d, id) = doc_with_rect(); + let p = paint(); + let s = Selection::default(); + let cx = cx(&d, &s, &p); + let mut t = DirectSelectionTool::new(false); + let shift = Mods { shift: true, ..Default::default() }; + // The rectangle is at 100..200. Segment 0 is the top edge (y=100, x between 100 and 200). + // Shift-click at (150, 100) selects segment 0 with mode "toggle". + let a = t.pointer(&cx, &PointerEvent::new(PointerKind::Down, 150.0, 100.0).with_mods(shift)); + assert_eq!(a, vec![Action::Exec("select.segments".into(), json!({"id": id.0, "segments": [[0, 0]], "mode": "toggle"}))]); + } + + /// In normal viewmode, clicking the fill of an open path in front selects the open path's shape + /// and anchors, and NEVER clicks through to select or grab a path behind it (even if the path + /// behind has an anchor directly under the click). + #[test] + fn clicking_fill_of_open_path_in_front_never_selects_path_behind() { + use vectorcraft_geom::{Anchor, SubPath}; + let (mut d, _behind) = doc_with_rect(); + // `behind` has an anchor at (100.0, 100.0). + // Create an open path in front (id `front`), with a fill, covering (100.0, 100.0). + let front = d.alloc_id(); + let l = d.layers[0].id; + // Triangle from (50, 50) to (150, 50) to (150, 150), open (implicit closing edge from (150, 150) to (50, 50)). + // (100.0, 100.0) is inside this filled triangle, but `front` has NO anchor at (100.0, 100.0). + let anchors = vec![Anchor::corner(Point::new(50.0, 50.0)), Anchor::corner(Point::new(150.0, 50.0)), Anchor::corner(Point::new(150.0, 150.0))]; + let open_path = PathData::single(SubPath::new(anchors, false)); + let ap = vectorcraft_doc::Appearance::default_art(); // has solid white fill + d.insert(Some(l), 1, vectorcraft_doc::Node::path(front, open_path, ap)).unwrap(); + + let (s, p) = (Selection::default(), paint()); + let c = cx(&d, &s, &p); + + // Hovering over (100.0, 100.0) does NOT hover the hidden anchor of `behind`. + assert_eq!(hovered_anchor(&c, Point::new(100.0, 100.0)), None); + + // Clicking at (100.0, 100.0) selects `front` (the open path in front), NOT `behind`. + let mut t = DirectSelectionTool::new(false); + let acts = t.pointer(&c, &PointerEvent::new(PointerKind::Down, 100.0, 100.0)); + assert_eq!(acts, vec![Action::Exec("select.set".into(), json!({"ids": [front.0]}))]); + } + + /// A selected path's Bézier direction handle positioned over a background filled rectangle + /// can be hit and dragged without being occluded by the background fill. + #[test] + fn selected_path_handle_over_background_rect_is_draggable() { + use vectorcraft_geom::{Anchor, SubPath}; + let mut d = vectorcraft_doc::Document::new(500.0, 500.0); + let l = d.layers[0].id; + // Background rectangle covering 0..300 with default white fill + let bg = d.alloc_id(); + let bg_rect = vectorcraft_geom::shapes::rectangle(Rect::new(0.0, 0.0, 300.0, 300.0)); + d.insert(Some(l), 0, vectorcraft_doc::Node::path(bg, bg_rect, vectorcraft_doc::Appearance::default_art())).unwrap(); + + // Selected path with a Bézier curve whose out-handle is at (150, 150) + let fg = d.alloc_id(); + let mut a0 = Anchor::corner(Point::new(100.0, 150.0)); + a0.h_out = Point::new(150.0, 150.0); // handle over the background rect + let a1 = Anchor::corner(Point::new(200.0, 150.0)); + let curve = PathData::single(SubPath::new(vec![a0, a1], false)); + d.insert(Some(l), 1, vectorcraft_doc::Node::path(fg, curve, vectorcraft_doc::Appearance::default_art())).unwrap(); + + let mut s = Selection::default(); + s.set([fg]); + s.anchors.insert(fg, [(0, 0)].into()); + let p = paint(); + let c = cx(&d, &s, &p); + + // hit_handle on the handle at (150, 150) must find the handle of `fg` + assert_eq!(hit_handle(&c, Point::new(150.0, 150.0), c.point_tol()), Some((fg, 0, 0, true))); + + // Direct Selection tool clicking on the handle initiates Reshape + let mut t = DirectSelectionTool::new(false); + let acts = t.pointer(&c, &PointerEvent::new(PointerKind::Down, 150.0, 150.0)); + assert_eq!(acts, vec![Action::Begin("Reshape".into())]); + } }