From 4e4cb7d8e3521ae482544ff047e4e4882bd9a542 Mon Sep 17 00:00:00 2001 From: DingusMcRinkyDinky <127614779+DingusMcRinkyDinky@users.noreply.github.com> Date: Wed, 4 Nov 2026 21:18:50 +0100 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01EZbgszHyuEuWAzxaamteuU --- crates/ui-egui/src/tools/vector_ui/options.rs | 3 +- .../ui-egui/src/tools/vector_ui/path_ops.rs | 29 +++--- crates/ui-egui/src/tools/vector_ui/pen.rs | 63 ++++++++++--- .../src/tools/vector_ui/pen_path_ops_tests.rs | 90 +++++++++++++++++++ 4 files changed, 159 insertions(+), 26 deletions(-) diff --git a/crates/ui-egui/src/tools/vector_ui/options.rs b/crates/ui-egui/src/tools/vector_ui/options.rs index fb2528a65f..d5941af320 100644 --- a/crates/ui-egui/src/tools/vector_ui/options.rs +++ b/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; diff --git a/crates/ui-egui/src/tools/vector_ui/path_ops.rs b/crates/ui-egui/src/tools/vector_ui/path_ops.rs index c24f64a279..3edef2ad86 100644 --- a/crates/ui-egui/src/tools/vector_ui/path_ops.rs +++ b/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 Result { 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 { 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 { + 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 diff --git a/crates/ui-egui/src/tools/vector_ui/pen.rs b/crates/ui-egui/src/tools/vector_ui/pen.rs index ce1e90cc12..b162619bba 100644 --- a/crates/ui-egui/src/tools/vector_ui/pen.rs +++ b/crates/ui-egui/src/tools/vector_ui/pen.rs @@ -471,9 +471,9 @@ fn draft_target(app: &PhotocraftApp, pen: &PenPath) -> Option { } /// 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(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 { + 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 } diff --git a/crates/ui-egui/src/tools/vector_ui/pen_path_ops_tests.rs b/crates/ui-egui/src/tools/vector_ui/pen_path_ops_tests.rs index e09cc9e82a..f20b688cfc 100644 --- a/crates/ui-egui/src/tools/vector_ui/pen_path_ops_tests.rs +++ b/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); +}