Merge pull request #12811 from rmsff/fix/delete-last-anchor-12764

Delete Anchor Point: deleting a saved path's last anchor leaves it empty

crates/engine/src/cmd/path_delete_last_anchor_tests.rs

@@ -0,0 +1,140 @@
+//! `path.deleteAnchor` on a path's last anchor (#12764): a saved path is left empty and stays
+//! listed; the Work Path and a vector mask keep refusing it.
+
+use photocraft_doc::text::TextShape;
+use photocraft_doc::{Document, LayerContent, LayerId, Path, Subpath};
+use serde_json::{Value, json};
+
+use crate::Session;
+
+fn session() -> Session {
+    let mut s = Session::new();
+    s.execute("file.new", json!({"width": 400, "height": 400, "background": "white"})).unwrap();
+    s
+}
+
+fn lone() -> Value {
+    json!({"subpaths": [{"closed": false, "knots": [[100, 300]]}]})
+}
+
+fn doc(s: &Session) -> &Document {
+    &s.active().unwrap().doc
+}
+
+/// "Path 1" with one subpath of one anchor, emptied by deleting that anchor.
+fn emptied() -> Session {
+    let mut s = session();
+    s.execute("path.set", json!({"name": "Path 1", "path": lone()})).unwrap();
+    s.execute("path.deleteAnchor", json!({"name": "Path 1", "subpath": 0, "knot": 0})).unwrap();
+    s
+}
+
+#[test]
+fn deleting_a_saved_paths_last_anchor_leaves_it_empty_in_one_undoable_step() {
+    let mut s = session();
+    s.execute("path.set", json!({"name": "Path 1", "path": lone()})).unwrap();
+    let before = doc(&s).paths[0].path.clone();
+    let steps = s.active().unwrap().history.entries().len();
+    let r = s.execute("path.deleteAnchor", json!({"name": "Path 1", "subpath": 0, "knot": 0})).unwrap();
+    assert_eq!(r["subpathRemoved"], json!(true));
+    assert_eq!(s.active().unwrap().history.entries().len(), steps + 1);
+    assert_eq!(s.active().unwrap().history.undo_label(), Some("Delete Anchor Point"));
+    let list = s.execute("path.list", json!({})).unwrap();
+    assert_eq!(list["paths"], json!([{"name": "Path 1", "knots": 0, "subpaths": 0}]), "Path 1 stays in the Paths panel");
+    let info = s.execute("path.info", json!({"name": "Path 1"})).unwrap();
+    assert_eq!(info["path"]["subpaths"], json!([]));
+    assert!(s.undo());
+    assert_eq!(doc(&s).paths[0].path, before, "Undo brings the anchor back");
+    assert!(s.redo());
+    assert!(doc(&s).paths[0].path.subpaths.is_empty());
+    assert_eq!(s.execute("path.list", json!({})).unwrap()["paths"], json!([{"name": "Path 1", "knots": 0, "subpaths": 0}]));
+}
+
+#[test]
+fn the_work_paths_and_a_vector_masks_last_anchor_are_still_refused() {
+    let mut s = session();
+    s.execute("path.set", json!({"path": lone()})).unwrap();
+    let steps = s.active().unwrap().history.entries().len();
+    let e = s.execute("path.deleteAnchor", json!({"subpath": 0, "knot": 0})).unwrap_err();
+    assert!(e.to_string().contains("only anchor"), "{e}");
+    assert_eq!(s.active().unwrap().history.entries().len(), steps);
+    assert_eq!(doc(&s).work_path.as_ref().map(|p| p.subpaths.len()), Some(1));
+    s.execute("layer.new.layer", json!({})).unwrap();
+    let id = s.active().unwrap().active_layer.unwrap().0;
+    s.execute("layer.vectorMask.add", json!({"layer": id, "path": lone()})).unwrap();
+    let steps_mask = s.active().unwrap().history.entries().len();
+    let e = s.execute("path.deleteAnchor", json!({"name": "layer", "layer": id, "subpath": 0, "knot": 0})).unwrap_err();
+    assert!(e.to_string().contains("only anchor"), "{e}");
+    assert_eq!(doc(&s).layer(LayerId(id)).unwrap().vector_mask.as_ref().unwrap().path.subpaths.len(), 1);
+    assert_eq!(s.active().unwrap().history.entries().len(), steps_mask);
+}
+
+#[test]
+fn an_empty_saved_path_selects_fills_and_strokes_nothing() {
+    let mut s = emptied();
+    s.execute("layer.new.layer", json!({})).unwrap();
+    let id = s.active().unwrap().active_layer.unwrap();
+    let pixels = |s: &Session| doc(s).layer(id).unwrap().surface().unwrap().clone();
+    let before = pixels(&s);
+    assert_eq!(s.execute("path.toSelection", json!({"name": "Path 1"})).unwrap(), json!({"selected": false}));
+    assert!(doc(&s).selection.is_none());
+    s.execute("path.fill", json!({"name": "Path 1", "color": "#ff0000"})).unwrap();
+    let r = s.execute("path.stroke", json!({"name": "Path 1", "tool": "pencil", "size": 4, "color": "#0000ff"})).unwrap();
+    assert_eq!(r["damage"], json!([0, 0, 0, 0]));
+    assert!(pixels(&s) == before, "an empty path paints nothing");
+}
+
+#[test]
+fn an_empty_saved_path_survives_pcraft_and_psd_round_trips() {
+    let s = emptied();
+    let bytes = photocraft_format::save_to_bytes(doc(&s), &Default::default()).unwrap();
+    let back = photocraft_format::load_from_bytes(&bytes).unwrap();
+    assert_eq!(back.paths.iter().map(|p| (p.name.as_str(), p.path.subpaths.len())).collect::<Vec<_>>(), [("Path 1", 0)]);
+    let psd = photocraft_io::export(doc(&s), "x.psd", &Default::default()).unwrap().bytes;
+    let back = photocraft_io::import("x.psd", &psd).unwrap().document;
+    assert_eq!(back.paths.iter().map(|p| (p.name.as_str(), p.path.subpaths.len())).collect::<Vec<_>>(), [("Path 1", 0)]);
+}
+
+#[test]
+fn an_empty_clipping_path_survives_a_psd_round_trip() {
+    let mut s = emptied();
+    s.execute("path.clippingPath.set", json!({"name": "Path 1"})).unwrap();
+    let psd = photocraft_io::export(doc(&s), "x.psd", &Default::default()).unwrap().bytes;
+    let back = photocraft_io::import("x.psd", &psd).unwrap().document;
+    assert_eq!(back.paths.iter().map(|p| (p.name.as_str(), p.path.subpaths.len())).collect::<Vec<_>>(), [("Path 1", 0)]);
+    assert_eq!(back.clipping_path.map(|c| c.name).as_deref(), Some("Path 1"));
+}
+
+/// A Type Path is not a saved path: its only anchor stays refused. No command makes a one-anchor
+/// Type Path (attaching and editing need two points), so the document is trimmed directly.
+#[test]
+fn a_type_paths_last_anchor_is_still_refused() {
+    let mut s = session();
+    let id = s.execute("type.create", json!({"x": 20, "y": 70, "text": "Hello", "size": 36, "color": "#000000"})).unwrap()["layer"].as_u64().unwrap();
+    s.edit("path", |doc, _| {
+        doc.work_path = Some(Path::new(vec![Subpath::polyline(&[(150.0, 10.0), (150.0, 110.0)])]));
+        Ok(())
+    })
+    .unwrap();
+    s.execute("type.attachToPath", json!({"layer": id})).unwrap();
+    s.edit("trim", |doc, _| {
+        if let Some(LayerContent::Text(t)) = doc.layer_mut(LayerId(id)).map(|l| &mut l.content)
+            && let TextShape::Path { path, .. } = &mut t.shape
+        {
+            path.knots.truncate(1);
+        }
+        Ok(())
+    })
+    .unwrap();
+    let type_path = |s: &Session| match &doc(s).layer(LayerId(id)).unwrap().content {
+        LayerContent::Text(t) => crate::type_extra_cmds::type_path(t),
+        _ => None,
+    };
+    let before = type_path(&s).unwrap();
+    assert_eq!(before.subpaths.iter().map(|sp| sp.knots.len()).collect::<Vec<_>>(), [1]);
+    let steps = s.active().unwrap().history.entries().len();
+    let e = s.execute("path.deleteAnchor", json!({"name": "layer", "layer": id, "typePath": true, "subpath": 0, "knot": 0})).unwrap_err();
+    assert!(e.to_string().contains("only anchor"), "{e}");
+    assert_eq!(type_path(&s), Some(before));
+    assert_eq!(s.active().unwrap().history.entries().len(), steps);
+}

crates/engine/src/cmd/path_edit_cmds.rs

@@ -175,9 +175,12 @@ fn add_anchor(s: &mut Session, p: &Value) -> Result<Value> {
 fn delete_anchor(s: &mut Session, p: &Value) -> Result<Value> {
     const CMD: &str = "path.deleteAnchor";
     let k = knot(CMD, p)?;
+    // A saved path may lose its last anchor and stay listed, empty; the Work Path, a shape's path,
+    // a vector mask and a Type Path keep refusing it.
+    let saved = p.get("name").and_then(Value::as_str).is_some_and(|n| n != "layer" && !crate::vector_cmds::is_work(Some(n)));
     let mut removed = false;
     let mut r = edit_path(s, CMD, p, "Delete Anchor Point", |path| {
-        removed = edit::delete_anchor(path, k)?;
+        removed = if saved { edit::delete_anchor_allow_empty(path, k)? } else { edit::delete_anchor(path, k)? };
         Ok(())
     })?;
     if let Some(o) = r.as_object_mut() {
@@ -291,7 +294,7 @@ pub fn specs() -> Vec<CommandSpec> {
         spec(
             "path.deleteAnchor",
             "Delete Anchor Point",
-            r#""subpath":i,"knot":j (its neighbours are joined, keeping their handles; a subpath's last anchor removes the subpath; the path's only anchor is refused) (the result also has subpathRemoved:bool)"#.into(),
+            r#""subpath":i,"knot":j (its neighbours are joined, keeping their handles; a subpath's last anchor removes the subpath; the path's only anchor empties a saved path and is refused elsewhere) (the result also has subpathRemoved:bool)"#.into(),
             delete_anchor,
         ),
         spec(
@@ -756,3 +759,7 @@ mod tests {
         assert_eq!(s.active().unwrap().history.entries().len(), steps);
     }
 }
+
+#[cfg(test)]
+#[path = "path_delete_last_anchor_tests.rs"]
+mod path_delete_last_anchor_tests;

crates/vector/src/edit.rs

@@ -244,12 +244,19 @@ pub fn add_anchor(path: &mut Path, knot: [usize; 2], t: f64) -> Result<[usize; 2
 /// previous anchor's out handle and the next anchor's in handle; a closed subpath stays closed.
 /// Removing a subpath's last anchor removes the subpath. Refused (unchanged) when it would leave
 /// the path with no anchors at all. Returns whether the subpath was removed.
-pub fn delete_anchor(path: &mut Path, [s, k]: [usize; 2]) -> Result<bool, String> {
-    knot_mut(path, [s, k])?;
+pub fn delete_anchor(path: &mut Path, k: [usize; 2]) -> Result<bool, String> {
+    knot_mut(path, k)?;
     let total: usize = path.subpaths.iter().map(|sp| sp.knots.len()).fold(0, usize::saturating_add);
     if total <= 1 {
         return Err("can't delete the path's only anchor point; delete the path instead".into());
     }
+    delete_anchor_allow_empty(path, k)
+}
+
+/// [`delete_anchor`] that also takes the path's only anchor, leaving a path with no subpaths: a
+/// saved path, which Photoshop keeps in the Paths panel empty (#12764).
+pub fn delete_anchor_allow_empty(path: &mut Path, [s, k]: [usize; 2]) -> Result<bool, String> {
+    knot_mut(path, [s, k])?;
     let sp = path.subpaths.get_mut(s).ok_or_else(|| format!("no subpath {s}"))?;
     if sp.knots.len() <= 1 {
         path.subpaths.remove(s);
@@ -626,3 +633,7 @@ mod tests {
         assert!(subpath_anchors(&p, 4).is_empty());
     }
 }
+
+#[cfg(test)]
+#[path = "edit_delete_last_anchor_tests.rs"]
+mod edit_delete_last_anchor_tests;

crates/vector/src/edit_delete_last_anchor_tests.rs

@@ -0,0 +1,53 @@
+//! Deleting the last anchor of a path (#12764): a saved path may end up empty, everything else
+//! keeps refusing it.
+
+use photocraft_doc::{Path, Subpath};
+use photocraft_geom::Rect;
+
+use super::*;
+
+fn lone() -> Path {
+    Path::new(vec![Subpath::polyline(&[(100.0, 300.0)])])
+}
+
+#[test]
+fn allowing_empty_removes_the_paths_only_anchor_and_its_subpath() {
+    let mut p = lone();
+    assert_eq!(delete_anchor_allow_empty(&mut p, [0, 0]), Ok(true));
+    assert!(p.subpaths.is_empty());
+}
+
+#[test]
+fn without_it_the_only_anchor_is_still_refused_and_kept() {
+    let mut p = lone();
+    assert!(delete_anchor(&mut p, [0, 0]).is_err());
+    assert_eq!(p, lone());
+}
+
+#[test]
+fn allowing_empty_changes_nothing_else() {
+    let mut a = Path::new(vec![Subpath::polygon(&[(0.0, 0.0), (10.0, 0.0), (10.0, 10.0)]), Subpath::polyline(&[(5.0, 5.0)])]);
+    let mut b = a.clone();
+    assert_eq!(delete_anchor_allow_empty(&mut a, [1, 0]), delete_anchor(&mut b, [1, 0]));
+    assert_eq!(delete_anchor_allow_empty(&mut a, [0, 1]), delete_anchor(&mut b, [0, 1]));
+    assert_eq!(a, b);
+}
+
+#[test]
+fn allowing_empty_still_rejects_missing_anchors() {
+    let mut p = lone();
+    for k in [[0, 1], [1, 0], [usize::MAX, 0], [0, usize::MAX]] {
+        assert!(delete_anchor_allow_empty(&mut p, k).is_err(), "{k:?}");
+    }
+    assert_eq!(p, lone());
+    let mut empty = Path::new(Vec::new());
+    assert!(delete_anchor_allow_empty(&mut empty, [0, 0]).is_err());
+}
+
+#[test]
+fn an_empty_path_draws_nothing() {
+    let mut p = lone();
+    delete_anchor_allow_empty(&mut p, [0, 0]).unwrap();
+    let cov = crate::fill_rasterizer(&p, crate::DEFAULT_TOLERANCE).render(Rect::from_xywh(0, 0, 200, 400));
+    assert!(cov.iter().all(|c| *c == 0.0));
+}