Make Shape: use the draft's own target; never copy a layer's path

- A path being drawn becomes the shape worked out with the target and
  settings captured when drawing began, as the commit does.
- With nothing selected, the drawing replaces an old Work Path.
- A draft begun in Shape mode is committed, so a resumed subpath keeps
  its layer's other subpaths.
- Without a draft, only the Work Path or a saved path is used.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZbgszHyuEuWAzxaamteuU

crates/ui-egui/src/tools/vector_ui/options.rs

@@ -109,7 +109,8 @@ pub fn options_bar(app: &mut PhotocraftApp, ui: &mut egui::Ui, tool: Tool) -> bo
                 app.ui.status_error = true;
             }
             // Make: Shape turns the path into a shape layer with the shape fill and stroke (#10934).
-            if ui.add_enabled(has_path, egui::Button::new(tl!("Make Shape"))).clicked()
+            let has_shape_path = super::path_ops::shape_source_path(app).is_some() || app.ui.pen.as_ref().is_some_and(|p| p.knots.len() >= 2);
+            if ui.add_enabled(has_shape_path, egui::Button::new(tl!("Make Shape"))).clicked()
                 && let Err(e) = super::make_shape(app)
             {
                 app.ui.status = e;

crates/ui-egui/src/tools/vector_ui/path_ops.rs

@@ -74,35 +74,28 @@ pub fn path_to_selection(app: &mut PhotocraftApp, params: Value) -> Result<Value
 }
 
 /// The Pen options bar's Make Shape (Path mode, #10934): a new shape layer from the path being
-/// drawn or the active path, with the shape tools' fill and stroke, as one History step. A path
-/// being drawn is not committed: the shape takes what the commit would have made (with a vector
-/// mask targeted, only the drawn path, and the mask stays). The Work Path becomes the shape's
-/// path, so it is no longer selected in the Paths panel.
+/// drawn, or else from the Work Path or saved path that is the target path, with the shape tools'
+/// fill and stroke, as one History step. A path being drawn is not committed: the shape takes what
+/// the commit would have made (with a vector mask targeted, only the drawn path, and the mask
+/// stays), see [`super::pen::make_shape_from_draft`]. A shape layer's own path or vector mask
+/// isn't copied. The Work Path becomes the shape's path, so it is no longer selected in the Paths
+/// panel.
 pub fn make_shape(app: &mut PhotocraftApp) -> Result<Value, String> {
     super::cancel_stale_pen(app);
     if let Some(mut pen) = app.ui.pen.clone().filter(|p| p.knots.len() >= 2) {
-        // A path still being drawn becomes the shape in one step, without being committed first:
-        // into a targeted vector mask that would change the mask.
         if pen.curvature {
             super::curvature::curvature_refresh(&mut pen, false);
         }
-        let (path, consume) = super::pen::draft_shape_path(app, &pen).ok_or_else(|| tl!("No document").to_string())?;
-        let r = app.run(
-            "path.toShape",
-            json!({"path": path, "consumeWorkPath": consume, "fill": super::shapes::fill_param(app), "stroke": super::shapes::stroke_param(app)}),
-        )?;
+        let r = super::pen::make_shape_from_draft(app, &pen)?;
         app.ui.pen = None;
         app.ui.pen_doc = None;
         if app.ui.direct_selection.target == Some(crate::direct_select::PathRef::Pen) {
             app.ui.direct_selection = Default::default();
         }
-        if consume && app.ui.selected_path.as_deref() == Some("work") {
-            app.ui.selected_path = None;
-        }
         return Ok(r);
     }
     let name =
-        active_path_name(app).ok_or_else(|| tl!("No path to make a shape from: draw one with the Pen tool or select one in the Paths panel.").to_string())?;
+        shape_source_path(app).ok_or_else(|| tl!("No path to make a shape from: draw one with the Pen tool or select one in the Paths panel.").to_string())?;
     let r = app.run("path.toShape", json!({"name": name, "fill": super::shapes::fill_param(app), "stroke": super::shapes::stroke_param(app)}))?;
     if name == "work" && app.ui.selected_path.as_deref() == Some("work") {
         app.ui.selected_path = None;
@@ -110,6 +103,12 @@ pub fn make_shape(app: &mut PhotocraftApp) -> Result<Value, String> {
     Ok(r)
 }
 
+/// The path Make Shape makes a shape from without a draft: [`active_path_name`] when it is the
+/// Work Path or a saved path, never a layer's own path (that would duplicate the shape or mask).
+pub fn shape_source_path(app: &PhotocraftApp) -> Option<String> {
+    active_path_name(app).filter(|n| n != "layer")
+}
+
 /// After the Pen or Path Selection tool made a selection from the path `name` (⌘↩, the canvas
 /// menu's Make Selection…), Photoshop deselects it in the Paths panel and stops drawing it; the
 /// path itself stays (#6079). The Work Path is hidden too when a saved path was loaded, so no

crates/ui-egui/src/tools/vector_ui/pen.rs

@@ -471,9 +471,9 @@ fn draft_target(app: &PhotocraftApp, pen: &PenPath) -> Option<String> {
 }
 
 /// What Make Shape turns a Path-mode draft into, without committing it: the path the commit would
-/// leave (the draft joined to the path it goes into, with the Path Operation), and whether that is
-/// the Work Path, which the shape then takes. With a vector mask targeted, the draft alone: the
-/// mask stays as it was. `None` with no document.
+/// leave (the draft joined to the path it goes into, with the Path Operation, or alone as a new
+/// Work Path), and whether that is the Work Path, which the shape then takes. With a vector mask
+/// targeted, the draft alone: the mask stays as it was. `None` with no document.
 pub(super) fn draft_shape_path(app: &PhotocraftApp, pen: &PenPath) -> Option<(Value, bool)> {
     let drawn = pen_to_json(pen, false);
     let doc = &app.session.active()?.doc;
@@ -485,7 +485,8 @@ pub(super) fn draft_shape_path(app: &PhotocraftApp, pen: &PenPath) -> Option<(Va
     {
         return Some((p, r.path == "work"));
     }
-    let Some(name) = draft_target(app, pen) else { return Some((drawn, false)) };
+    // With nothing to join, the commit's new Work Path would replace the old one.
+    let Some(name) = draft_target(app, pen) else { return Some((drawn, true)) };
     let existing = doc_path(doc, &name)?;
     let mut p = photocraft_engine::vector_cmds::path_json(existing);
     let op = pen_path_op(&app.ui.tool_options).unwrap_or("combine");
@@ -814,7 +815,21 @@ fn fit_with_straight_segments(trace: &[[f64; 2]], straight: &[usize], fit: f64,
 use super::{PenOwner, cancel_stale_pen, capture_pen_owner};
 
 fn commit_path(app: &mut PhotocraftApp, pen: &PenPath, closed: bool) -> Result<(), String> {
-    let Some(owner) = pen.owner.as_ref().filter(|o| o.valid(app)) else { return Err("Pen draft target is stale; no path was changed".into()) };
+    let Some(owner) = pen.owner.clone().filter(|o| o.valid(app)) else { return Err("Pen draft target is stale; no path was changed".into()) };
+    let result = as_owner(app, &owner, |app| {
+        let result = commit_path_current(app, pen, closed);
+        if result && matches!(owner.target, super::PenTarget::Shape { .. } | super::PenTarget::VectorMask(_)) {
+            app.ui.selected_path = Some("layer".into());
+        }
+        result
+    });
+    if result { Ok(()) } else { Err(app.ui.status.clone()) }
+}
+
+/// Run `f` with the options, colours, active layer, Paths panel selection and vector mask target
+/// the draft's `owner` captured when drawing began, then put the current ones back (the Paths
+/// panel selection stays as `f` left it).
+fn as_owner<R>(app: &mut PhotocraftApp, owner: &PenOwner, f: impl FnOnce(&mut PhotocraftApp) -> R) -> R {
     let options = std::mem::replace(&mut app.ui.tool_options, owner.options.clone());
     let vector_mask = std::mem::replace(&mut app.ui.vector_mask_target, owner.vector_mask);
     app.ui.selected_path = match &owner.target {
@@ -834,10 +849,7 @@ fn commit_path(app: &mut PhotocraftApp, pen: &PenPath, closed: bool) -> Result<(
             photocraft_engine::channel_cmds::ChannelTarget::Composite
         };
     }
-    let result = commit_path_current(app, pen, closed);
-    if result && matches!(owner.target, super::PenTarget::Shape { .. } | super::PenTarget::VectorMask(_)) {
-        app.ui.selected_path = Some("layer".into());
-    }
+    let result = f(app);
     if let (Some(st), Some(channel)) = (app.session.active_mut(), channel) {
         st.channel_view.target = channel;
     }
@@ -845,5 +857,36 @@ fn commit_path(app: &mut PhotocraftApp, pen: &PenPath, closed: bool) -> Result<(
     app.session.tools.background = background;
     app.ui.tool_options = options;
     app.ui.vector_mask_target = vector_mask;
-    if result { Ok(()) } else { Err(app.ui.status.clone()) }
+    result
+}
+
+/// Make Shape on a Pen draft (#10934), as one History step: a draft begun in Shape mode is
+/// committed (it is a shape already, and a resumed subpath keeps its layer's other subpaths);
+/// otherwise a new shape layer, with the shape tools' fill and stroke, of what the commit would
+/// have made, worked out as the commit does, with the settings drawing began with.
+pub(super) fn make_shape_from_draft(app: &mut PhotocraftApp, pen: &PenPath) -> Result<Value, String> {
+    let owner = pen.owner.clone();
+    if owner.as_ref().is_some_and(|o| !o.valid(app)) {
+        return Err("Pen draft target is stale; no path was changed".into());
+    }
+    if owner.as_ref().is_some_and(|o| matches!(o.target, super::PenTarget::Shape { .. })) {
+        return try_pen_commit(app, false).map(|()| Value::Null);
+    }
+    let selected = app.ui.selected_path.clone();
+    let params = |app: &mut PhotocraftApp| {
+        let (path, consume) = draft_shape_path(app, pen).ok_or_else(|| tl!("No document").to_string())?;
+        let params = json!({"path": path, "consumeWorkPath": consume, "fill": fill_param(app), "stroke": stroke_param(app)});
+        Ok::<_, String>((params, consume))
+    };
+    let made = match &owner {
+        Some(o) => as_owner(app, o, params),
+        None => params(app),
+    };
+    app.ui.selected_path = selected;
+    let (params, consume) = made?;
+    let r = app.run("path.toShape", params);
+    if r.is_ok() && consume && app.ui.selected_path.as_deref() == Some("work") {
+        app.ui.selected_path = None;
+    }
+    r
 }

crates/ui-egui/src/tools/vector_ui/pen_path_ops_tests.rs

@@ -452,3 +452,93 @@ fn make_shape_keeps_a_saved_path() {
     assert_eq!(st.doc.paths.first().map(|p| p.name.as_str()), Some("Box"));
     assert_eq!(shape_path(&app).1, st.doc.paths[0].path);
 }
+
+/// Clicks an open path with the Pen through `points`, without finishing it.
+fn draw(app: &mut PhotocraftApp, points: &[(f64, f64)]) {
+    for &(x, y) in points {
+        pen_down(app, x, y, false);
+        pen_up(app);
+    }
+}
+
+/// Make Shape while drawing into a saved path uses the path drawing began with, as the commit
+/// does, not one selected in the Paths panel since.
+#[test]
+fn make_shape_while_drawing_uses_the_path_drawing_began_with() {
+    let mut app = app();
+    square(&mut app, 20.0, 20.0, 50.0);
+    app.run("path.rename", json!({"name": "work", "to": "Box"})).unwrap();
+    app.run("path.set", json!({"name": "Other", "path": {"subpaths": [{"closed": true, "knots": [[100, 100], [150, 100], [150, 150]]}]}})).unwrap();
+    app.ui.selected_path = Some("Box".into());
+    let doc_paths = app.session.active().unwrap().doc.paths.clone();
+    draw(&mut app, &[(10.0, 150.0), (60.0, 150.0), (60.0, 190.0)]);
+    app.ui.selected_path = Some("Other".into());
+    super::make_shape(&mut app).unwrap();
+    let box_path = &doc_paths.iter().find(|p| p.name == "Box").unwrap().path;
+    let shape = shape_path(&app).1;
+    assert_eq!(shape.subpaths.len(), box_path.subpaths.len() + 1, "Box and the drawing");
+    assert_eq!(shape.subpaths[0], box_path.subpaths[0]);
+    assert_eq!(app.session.active().unwrap().doc.paths, doc_paths, "the saved paths stay as they were");
+    assert_eq!(app.ui.selected_path.as_deref(), Some("Other"), "the Paths panel selection stays");
+}
+
+/// With no Work Path and no saved path selected, Make Shape has nothing to make a shape from: the
+/// selected shape layer's own path isn't copied into a second shape layer.
+#[test]
+fn make_shape_doesnt_copy_a_shape_layers_own_path() {
+    let mut app = app();
+    app.ui.tool_options.vector_mode = "shape".into();
+    square(&mut app, 20.0, 20.0, 50.0);
+    app.ui.tool_options.vector_mode = "path".into();
+    assert_eq!(super::active_path_name(&app).as_deref(), Some("layer"));
+    let count = layers(&app);
+    let steps = app.session.active().unwrap().history.past_len();
+    assert!(super::make_shape(&mut app).is_err());
+    assert_eq!(layers(&app), count);
+    assert_eq!(app.session.active().unwrap().history.past_len(), steps);
+}
+
+/// Make Shape while drawing a new path with an unselected Work Path: the commit would have
+/// replaced the Work Path with the drawing, so the shape takes the drawing and no Work Path is
+/// left behind.
+#[test]
+fn make_shape_while_drawing_a_new_work_path_replaces_the_old_one() {
+    let mut app = app();
+    square(&mut app, 20.0, 20.0, 50.0);
+    app.ui.selected_path = None;
+    draw(&mut app, &[(10.0, 150.0), (60.0, 150.0), (60.0, 190.0)]);
+    super::make_shape(&mut app).unwrap();
+    let st = app.session.active().unwrap();
+    assert!(st.doc.work_path.is_none(), "the old Work Path is gone, as after a commit and Make Shape");
+    let shape = shape_path(&app).1;
+    assert_eq!(shape.subpaths.len(), 1);
+    assert_eq!(shape.subpaths[0].knots[0].anchor, photocraft_geom::Point::new(10.0, 150.0));
+}
+
+/// A draft begun in Shape mode that carries on an open subpath of a shape layer, with the mode
+/// switched to Path before Make Shape: the layer keeps its other subpaths, extended in one step,
+/// as the commit does, and no second layer copies part of it.
+#[test]
+fn make_shape_on_a_resumed_shape_layer_draft_keeps_its_other_subpaths() {
+    let mut app = app();
+    app.ui.tool_options.vector_mode = "shape".into();
+    draw(&mut app, &[(20.0, 20.0), (80.0, 20.0)]);
+    assert!(super::pen_commit(&mut app, false));
+    app.ui.tool_options.pen_path_op = "combine".into();
+    square(&mut app, 100.0, 100.0, 40.0);
+    let (id, before) = shape_path(&app);
+    assert_eq!(before.subpaths.len(), 2);
+    let count = layers(&app);
+    let steps = app.session.active().unwrap().history.past_len();
+    draw(&mut app, &[(80.0, 20.0), (80.0, 80.0)]);
+    assert!(app.ui.pen.as_ref().is_some_and(|p| p.resumes.is_some()), "drawing carries on the open subpath");
+    app.ui.tool_options.vector_mode = "path".into();
+    super::make_shape(&mut app).unwrap();
+    assert_eq!(layers(&app), count, "no second layer");
+    let (now, after) = shape_path(&app);
+    assert_eq!(now, id);
+    assert_eq!(after.subpaths.len(), 2, "the square stays");
+    assert_eq!(after.subpaths[0].knots.len(), 3, "the open subpath, extended");
+    assert_eq!(after.subpaths[1], before.subpaths[1]);
+    assert_eq!(app.session.active().unwrap().history.past_len(), steps + 1);
+}