Merge pull request #12815 from TrevorBrown-dev/fix/anchor-tools-targeted-path-12762

Add/Delete Anchor Point: edit only the targeted path (#12762, #12705)

crates/ui-egui/src/tools/anchor_tools.rs

@@ -1,6 +1,7 @@
 //! Add Anchor Point and Delete Anchor Point tools (P group with the Pen, #1044), as in Photoshop.
-//! They edit the paths Direct Selection edits, front first: the work or saved path selected in the
-//! Paths panel, the targeted vector mask or the active shape layer's path, then the work path.
+//! They edit the targeted path only (#12762, #12705): the saved path selected in the Paths panel,
+//! the targeted vector mask or the active shape layer's path (not while that layer is hidden), or the work
+//! path. A top-level shape layer that is not active is left alone, unlike Direct Selection; one inside a group is still edited as before.
 //!
 //! * Add Anchor Point: hovering a segment highlights it and shows where the anchor would go; a
 //!   click inserts it there (`path.addAnchor`). A curve is split without changing its shape.
@@ -38,7 +39,7 @@ pub fn effective(tool: Tool, mods: Modifiers) -> Option<Tool> {
 /// segment (Add) or anchor (Delete) hit on it. An anchor hit with Add, or a segment with Delete,
 /// edits nothing.
 fn target(app: &PhotocraftApp, tool: Tool, p: [f64; 2]) -> Option<(PathRef, Path, Hit)> {
-    let (r, path, hit) = crate::direct_select::hit_target(app, p, false)?;
+    let (r, path, hit) = crate::direct_select::hit_active(app, p)?;
     match (tool, hit) {
         (Tool::AddAnchorPoint, Hit::Segment(..)) | (Tool::DeleteAnchorPoint, Hit::Anchor(_)) => Some((r, path, hit)),
         _ => None,
@@ -185,6 +186,10 @@ fn draw_hit(painter: &egui::Painter, to_scr: &dyn Fn([f64; 2]) -> Pos2, accent:
     }
 }
 
+#[cfg(test)]
+mod add_anchor_targeted_path_tests;
+#[cfg(test)]
+mod delete_anchor_targeted_path_tests;
 #[cfg(test)]
 mod tests;
 

crates/ui-egui/src/tools/anchor_tools/add_anchor_targeted_path_tests.rs

@@ -0,0 +1,72 @@
+//! #12705: Add Anchor Point edits only the targeted path: the path selected in the Paths panel
+//! or the active shape layer's. A shape layer that is not active, or is hidden, is left alone.
+
+use serde_json::json;
+
+use crate::state::Tool;
+
+// The setup and steps shared with #12762's tests.
+use super::delete_anchor_targeted_path_tests::{click, group_shape, select_layer, select_saved_path, setup, shape_path, steps};
+
+#[test]
+fn a_click_on_the_top_edge_of_a_shape_that_is_not_active_changes_nothing() {
+    let (mut app, shape) = setup(Tool::AddAnchorPoint);
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [620.0, 100.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn a_click_on_the_top_edge_of_the_active_shape_adds_an_anchor() {
+    let (mut app, shape) = setup(Tool::AddAnchorPoint);
+    select_layer(&mut app, shape);
+    let n = steps(&app);
+    click(&mut app, [550.0, 100.0]);
+    assert_eq!(shape_path(&mut app, shape)["subpaths"][0]["knots"].as_array().unwrap().len(), 5);
+    assert_eq!(steps(&app), n + 1);
+    assert_eq!(app.session.active().unwrap().history.undo_label(), Some("Add Anchor Point"));
+}
+
+#[test]
+fn with_a_saved_path_selected_a_click_on_the_shape_edge_does_nothing() {
+    let (mut app, shape) = setup(Tool::AddAnchorPoint);
+    select_saved_path(&mut app);
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [550.0, 100.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn a_hidden_active_shape_is_not_edited() {
+    let (mut app, shape) = setup(Tool::AddAnchorPoint);
+    select_layer(&mut app, shape);
+    app.run("layer.setProps", json!({"layer": shape, "visible": false})).unwrap();
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [550.0, 100.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn the_pen_draft_is_still_edited_by_the_anchor_tools() {
+    let (mut app, _shape) = setup(Tool::Pen);
+    click(&mut app, [100.0, 500.0]);
+    click(&mut app, [300.0, 500.0]);
+    assert_eq!(app.ui.pen.as_ref().map(|p| p.knots.len()), Some(2));
+    app.ui.tool = Tool::AddAnchorPoint;
+    click(&mut app, [200.0, 500.0]);
+    assert_eq!(app.ui.pen.as_ref().map(|p| p.knots.len()), Some(3));
+}
+
+#[test]
+fn a_shape_inside_a_group_that_is_not_active_keeps_todays_behaviour() {
+    let (mut app, shape) = setup(Tool::AddAnchorPoint);
+    group_shape(&mut app, shape);
+    let n = steps(&app);
+    click(&mut app, [620.0, 100.0]);
+    assert_eq!(shape_path(&mut app, shape)["subpaths"][0]["knots"].as_array().unwrap().len(), 5);
+    assert_eq!(steps(&app), n + 1);
+    assert_eq!(app.session.active().unwrap().history.undo_label(), Some("Add Anchor Point"));
+}

crates/ui-egui/src/tools/anchor_tools/delete_anchor_targeted_path_tests.rs

@@ -0,0 +1,138 @@
+//! #12762: Delete Anchor Point edits only the targeted path: the path selected in the Paths panel
+//! or the active shape layer's. A shape layer that is not active is left alone.
+
+use egui::Modifiers;
+use serde_json::{Value, json};
+
+use crate::PhotocraftApp;
+use crate::canvas::{ToolEvent, tool_event};
+use crate::direct_select::PathRef;
+use crate::state::Tool;
+
+/// 800 x 600, white Background, pixel layer "Red" active, and a shape layer from (450,100) to
+/// (650,250) below it in the stack's activity (Red stays the active layer).
+pub(super) fn setup(tool: Tool) -> (PhotocraftApp, u64) {
+    let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default());
+    app.session.execute("file.new", json!({"width": 800, "height": 600})).unwrap();
+    app.sync_views();
+    app.ui.tool = tool;
+    let shape = app.run("shape.create", json!({"kind": "rect", "rect": [450, 100, 200, 150], "fill": "#3070c0"})).unwrap()["layer"].as_u64().unwrap();
+    app.run("layer.new.layer", json!({})).unwrap();
+    let red = app.session.active().unwrap().active_layer.unwrap();
+    app.run("layer.renameLayer", json!({"layer": red.0, "name": "Red"})).unwrap();
+    (app, shape)
+}
+
+pub(super) fn shape_path(app: &mut PhotocraftApp, id: u64) -> Value {
+    app.run("shape.info", json!({"layer": id})).unwrap()["path"].clone()
+}
+
+pub(super) fn steps(app: &PhotocraftApp) -> usize {
+    app.session.active().unwrap().history.entries().len()
+}
+
+pub(super) fn click(app: &mut PhotocraftApp, at: [f64; 2]) {
+    tool_event(app, ToolEvent::Down { x: at[0], y: at[1], pressure: 1.0 }, Modifiers::NONE);
+    tool_event(app, ToolEvent::Up { x: at[0], y: at[1] }, Modifiers::NONE);
+}
+
+pub(super) fn select_saved_path(app: &mut PhotocraftApp) {
+    let path = json!({"subpaths": [{"closed": true, "knots": [[50, 450], [150, 450], [150, 550], [50, 550]]}]});
+    app.run("path.set", json!({"name": "Path 1", "path": path})).unwrap();
+    app.ui.selected_path = Some("Path 1".into());
+}
+
+pub(super) fn select_layer(app: &mut PhotocraftApp, id: u64) {
+    app.run("layer.select", json!({"layer": id})).unwrap();
+}
+
+/// Moves `shape` into a new group and makes Red the active layer again; returns the group's id.
+pub(super) fn group_shape(app: &mut PhotocraftApp, shape: u64) -> u64 {
+    let red = app.session.active().unwrap().active_layer.unwrap();
+    let group = app.run("layer.groupLayers", json!({"layer": shape})).unwrap()["layer"].as_u64().unwrap();
+    select_layer(app, red.0);
+    group
+}
+
+#[test]
+fn a_click_on_the_corner_of_a_shape_that_is_not_active_changes_nothing() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn a_click_on_the_corner_of_the_active_shape_deletes_the_anchor() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    select_layer(&mut app, shape);
+    let n = steps(&app);
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape)["subpaths"][0]["knots"].as_array().unwrap().len(), 3);
+    assert_eq!(steps(&app), n + 1);
+    assert_eq!(app.session.active().unwrap().history.undo_label(), Some("Delete Anchor Point"));
+}
+
+#[test]
+fn with_a_saved_path_selected_a_click_on_the_shape_corner_does_nothing() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    select_saved_path(&mut app);
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn direct_selection_still_edits_a_shape_layer_that_is_not_active() {
+    let (mut app, shape) = setup(Tool::DirectSelection);
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(app.ui.direct_selection.target, Some(PathRef::Layer(shape)));
+}
+
+#[test]
+fn with_a_saved_path_selected_a_click_on_the_active_shape_does_nothing() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    select_layer(&mut app, shape);
+    select_saved_path(&mut app);
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn with_the_work_path_selected_a_click_on_the_active_shape_does_nothing() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    select_layer(&mut app, shape);
+    let path = json!({"subpaths": [{"closed": true, "knots": [[50, 450], [150, 450], [150, 550], [50, 550]]}]});
+    app.run("path.set", json!({"path": path})).unwrap();
+    app.ui.selected_path = Some("work".into());
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}
+
+#[test]
+fn a_shape_inside_a_group_that_is_not_active_keeps_todays_behaviour() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    group_shape(&mut app, shape);
+    let n = steps(&app);
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape)["subpaths"][0]["knots"].as_array().unwrap().len(), 3);
+    assert_eq!(steps(&app), n + 1);
+    assert_eq!(app.session.active().unwrap().history.undo_label(), Some("Delete Anchor Point"));
+}
+
+#[test]
+fn a_shape_inside_a_hidden_group_is_not_edited() {
+    let (mut app, shape) = setup(Tool::DeleteAnchorPoint);
+    let group = group_shape(&mut app, shape);
+    app.run("layer.setProps", json!({"layer": group, "visible": false})).unwrap();
+    let (before, n) = (shape_path(&mut app, shape), steps(&app));
+    click(&mut app, [650.0, 250.0]);
+    assert_eq!(shape_path(&mut app, shape), before);
+    assert_eq!(steps(&app), n);
+}

crates/ui-egui/src/tools/direct_select.rs

@@ -347,6 +347,51 @@ pub(crate) fn hit_target(app: &PhotocraftApp, p: [f64; 2], handles: bool) -> Opt
     })
 }
 
+/// What the anchor tools edit at `p`: a hit on the targeted paths only (`active_paths`), never on
+/// a top-level shape layer that is not active. A hidden active layer shows no path, so it is skipped
+/// (#12762, #12705). Not measured, so as today: a layer inside a hidden group, and a shown shape layer
+/// inside a group, which stays a target without being active.
+pub(crate) fn hit_active(app: &PhotocraftApp, p: [f64; 2]) -> Option<(PathRef, Path, Hit)> {
+    let tol = 6.0 / f64::from(app.point_zoom().max(0.01));
+    let st = app.session.active()?;
+    // A path selected in the Paths panel (saved or Work Path) is the only target; otherwise the Pen draft first (as
+    // `candidates` builds it), then the active paths. The draft's destination is private to
+    // `vector_ui`, so it is not filtered here.
+    let targets: Vec<(PathRef, Path)> = match crate::vector_ui::selected_saved_path_index(app) {
+        Some((i, path)) => Some((PathRef::Saved(i), path)).filter(|(r, _)| writable(app, r)).into_iter().collect(),
+        // The Work Path selected in the Paths panel is the only target too, as for Path Selection.
+        None if app.ui.selected_path.as_deref() == Some("work") => active_paths(app).into_iter().filter(|(r, _)| *r == PathRef::Work).collect(),
+        None => {
+            let pen = app
+                .ui
+                .pen
+                .as_ref()
+                .filter(|_| app.ui.pen_doc.is_none_or(|owner| Some(owner) == Some(st.doc.id)))
+                .filter(|p| !p.dragging && !p.knots.is_empty())
+                .map(|p| (PathRef::Pen, crate::vector_ui::pen_path(p)));
+            let mut out: Vec<(PathRef, Path)> = pen.into_iter().chain(active_paths(app)).collect();
+            // Not measured (#12762, #12705): a shape layer inside a group keeps today's behaviour. Every shown,
+            // unlocked one stays a target after the targeted paths, front first, as `candidates` lists it.
+            // A top-level shape layer (depth 0) is a target only as the active layer.
+            let layers = st.doc.walk();
+            let grouped = layers.iter().rev().filter(|(at, depth, _)| *depth > 0 && st.doc.shown_at(at)).filter_map(|(_, _, l)| match &l.content {
+                photocraft_doc::LayerContent::Shape(sh) => Some((PathRef::Layer(l.id.0), sh.path.clone())),
+                _ => None,
+            });
+            for (r, path) in grouped {
+                if writable(app, &r) && !out.iter().any(|(have, _)| *have == r) {
+                    out.push((r, path));
+                }
+            }
+            out
+        }
+    };
+    targets
+        .into_iter()
+        .filter(|(r, _)| !matches!(r, PathRef::Layer(id) if !st.doc.layer(photocraft_doc::LayerId(*id)).is_some_and(|l| l.visible)))
+        .find_map(|(r, path)| edit::hit(&path, Point::new(p[0], p[1]), tol, &[]).map(|h| (r, path, h)))
+}
+
 fn retracted(k: &photocraft_doc::Knot, h: Handle) -> bool {
     let c = if h == Handle::In { k.in_ctrl } else { k.out_ctrl };
     (c.x - k.anchor.x).abs() < 1e-9 && (c.y - k.anchor.y).abs() < 1e-9