From 974c32ddc693b4c5e71838e8a02efece9942c032 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 8 Nov 2026 13:09:17 +1100 Subject: [PATCH] Patch: History names as the tool's (#12117) select.lasso takes an optional historyName; the Patch tool sends "Patch Tool" so its lasso is that step (the Lasso tool keeps "Lasso"). paint.patch records "Patch Selection" in every mode; the content-aware job keeps its progress text. Tests: patch_history_names_tests (engine) and patch_tool_history_names_tests (ui-egui). --- crates/engine/src/cmd/retouch_cmds.rs | 3 + crates/engine/src/cmd/retouch_cmds/patch.rs | 10 ++-- .../retouch_cmds/patch_history_names_tests.rs | 58 +++++++++++++++++++ crates/engine/src/cmd/retouch_cmds/tests.rs | 2 +- crates/engine/src/cmd/selection_cmds.rs | 14 ++++- crates/ui-egui/src/canvas/gesture.rs | 7 ++- crates/ui-egui/src/lib.rs | 2 + .../src/patch_tool_history_names_tests.rs | 33 +++++++++++ 8 files changed, 121 insertions(+), 8 deletions(-) create mode 100644 crates/engine/src/cmd/retouch_cmds/patch_history_names_tests.rs create mode 100644 crates/ui-egui/src/patch_tool_history_names_tests.rs diff --git a/crates/engine/src/cmd/retouch_cmds.rs b/crates/engine/src/cmd/retouch_cmds.rs index 0746ca7440..3cba7c4318 100644 --- a/crates/engine/src/cmd/retouch_cmds.rs +++ b/crates/engine/src/cmd/retouch_cmds.rs @@ -397,3 +397,6 @@ pub fn specs() -> Vec { #[cfg(test)] mod tests; + +#[cfg(test)] +mod patch_history_names_tests; diff --git a/crates/engine/src/cmd/retouch_cmds/patch.rs b/crates/engine/src/cmd/retouch_cmds/patch.rs index 1cfcd3bfca..38655075fd 100644 --- a/crates/engine/src/cmd/retouch_cmds/patch.rs +++ b/crates/engine/src/cmd/retouch_cmds/patch.rs @@ -23,6 +23,8 @@ use super::{ }; const CMD: &str = "paint.patch"; +/// The History step of applying a patch, in every mode. +const HISTORY_NAME: &str = "Patch Selection"; /// Largest patch, in pixels of the selection's bounding box. The solve holds the two regions, the /// result and per-channel float planes of it (on the order of 150 bytes per pixel with four @@ -151,7 +153,6 @@ struct Plan { /// Shift of the repaired area from the selection, and of the texture from the repaired area. dst_shift: (i32, i32), src_shift: (i32, i32), - destination: bool, offset: (i32, i32), } @@ -185,7 +186,7 @@ fn plan(s: &Session, p: &Value) -> Result { PatchMode::Source => ((0, 0), (dx, dy)), PatchMode::Destination => ((dx, dy), (-dx, -dy)), }; - Ok(Plan { id, canvas, area, dst_shift, src_shift, destination: mode == PatchMode::Destination, offset: (dx, dy) }) + Ok(Plan { id, canvas, area, dst_shift, src_shift, offset: (dx, dy) }) } /// Heal `surf` at the selection (shifted by `dst_shift`) with texture read `src_shift` away, the @@ -216,7 +217,7 @@ pub(super) fn patch(s: &mut Session, p: &Value) -> Result { return patch_content_aware(s, p); } let plan = plan(s, p)?; - let label = if plan.destination { "Patch (Destination)" } else { "Patch" }; + let label = HISTORY_NAME; let dmg = run_stroke(s, label, plan.id, p, |pre, surf, _, lock| { let sel = pre.selection.as_ref().ok_or_else(|| bad(CMD, "select the area to patch first"))?; Ok(patch_surface(surf, sel, &plan, lock, 1)) @@ -263,6 +264,7 @@ fn level(p: &Value, k: &str, default: u8, lo: u8, hi: u8) -> Result { /// docs), as a background job. fn patch_content_aware(s: &mut Session, p: &Value) -> Result { use super::content_aware_move::{Target, land, mask, read, window}; + // The job's progress text; its History step is `HISTORY_NAME`. const LABEL: &str = "Patch (Content-Aware)"; let plan = plan(s, p)?; let structure = level(p, "structure", 4, 1, 7)?; @@ -294,7 +296,7 @@ fn patch_content_aware(s: &mut Session, p: &Value) -> Result { } }, move |s, (surf, damage)| { - s.edit(LABEL, |doc, _| { + s.edit(HISTORY_NAME, |doc, _| { *crate::channel_cmds::target_surface(doc, target.id, &p)?.0 = surf; Ok(()) })?; diff --git a/crates/engine/src/cmd/retouch_cmds/patch_history_names_tests.rs b/crates/engine/src/cmd/retouch_cmds/patch_history_names_tests.rs new file mode 100644 index 0000000000..fae53cc0e5 --- /dev/null +++ b/crates/engine/src/cmd/retouch_cmds/patch_history_names_tests.rs @@ -0,0 +1,58 @@ +//! History step names of the Patch Tool (#12117): "Patch Tool" for the lasso it draws with and +//! "Patch Selection" for applying a patch in every mode; the plain Lasso tool keeps "Lasso". + +use super::*; + +fn session() -> Session { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 128, "height": 64, "depth": 8, "mode": "rgb"})).unwrap(); + s +} + +fn last_step(s: &Session) -> Option { + s.active().unwrap().history.undo_label().map(str::to_string) +} + +#[test] +fn applying_a_patch_is_one_step_named_patch_selection_in_every_mode() { + let modes = [ + json!({"offset": [30, 0]}), + json!({"offset": [30, 0], "mode": "source"}), + json!({"offset": [30, 0], "mode": "destination"}), + json!({"offset": [30, 0], "contentAware": true}), + json!({"offset": [30, 0], "mode": "destination", "contentAware": true}), + ]; + for p in modes { + let mut s = session(); + s.execute("select.rect", json!({"x": 16, "y": 22, "width": 20, "height": 20})).unwrap(); + let steps = s.active().unwrap().history.past_len(); + s.execute("paint.patch", p.clone()).unwrap(); + assert_eq!(s.active().unwrap().history.past_len(), steps + 1, "{p}: one step"); + assert_eq!(last_step(&s).as_deref(), Some("Patch Selection"), "{p}"); + } +} + +const TRIANGLE: [[i32; 2]; 3] = [[16, 22], [36, 22], [16, 42]]; + +#[test] +fn the_lasso_is_named_for_the_tool_that_drew_it() { + let mut s = session(); + s.execute("select.lasso", json!({"points": TRIANGLE})).unwrap(); + assert_eq!(last_step(&s).as_deref(), Some("Lasso"), "the Lasso tool sends no name"); + for mode in ["replace", "add", "subtract", "intersect"] { + s.execute("select.lasso", json!({"points": TRIANGLE, "mode": mode, "historyName": "Patch Tool"})).unwrap(); + assert_eq!(last_step(&s).as_deref(), Some("Patch Tool"), "{mode}"); + } + s.execute("select.lasso", json!({"points": TRIANGLE, "polygonal": true, "historyName": "Patch Tool"})).unwrap(); + assert_eq!(last_step(&s).as_deref(), Some("Polygonal Lasso"), "the Polygonal Lasso keeps its own name"); +} + +#[test] +fn a_hostile_history_name_is_refused_without_a_step() { + let mut s = session(); + let steps = s.active().unwrap().history.past_len(); + for name in [json!(""), json!(" "), json!("x".repeat(65)), json!("a\nb"), json!(7), json!(["Patch Tool"])] { + assert!(s.execute("select.lasso", json!({"points": TRIANGLE, "historyName": name})).is_err(), "{name}"); + } + assert_eq!(s.active().unwrap().history.past_len(), steps); +} diff --git a/crates/engine/src/cmd/retouch_cmds/tests.rs b/crates/engine/src/cmd/retouch_cmds/tests.rs index 25ebcc8a97..636d3c1ce7 100644 --- a/crates/engine/src/cmd/retouch_cmds/tests.rs +++ b/crates/engine/src/cmd/retouch_cmds/tests.rs @@ -1022,7 +1022,7 @@ fn patch_content_aware_copies_the_dragged_to_content_into_the_selection() { let (got, want) = (rgba(&s, x, y), texture(x, y)); assert!((got[0] - want[0]).abs() <= tol(depth), "depth {depth}: untouched {x},{y}: {got:?}"); } - assert_eq!(s.active().unwrap().history.undo_label(), Some("Patch (Content-Aware)")); + assert_eq!(s.active().unwrap().history.undo_label(), Some("Patch Selection")); s.execute("edit.undo", json!({})).unwrap(); assert_eq!(rgba(&s, 44, 24)[1], 0.0, "depth {depth}: undo brings the blemish back"); } diff --git a/crates/engine/src/cmd/selection_cmds.rs b/crates/engine/src/cmd/selection_cmds.rs index 13737e70bd..b74a9d7f9f 100644 --- a/crates/engine/src/cmd/selection_cmds.rs +++ b/crates/engine/src/cmd/selection_cmds.rs @@ -622,7 +622,17 @@ fn lasso(s: &mut Session, p: &Value) -> Result { return Err(EngineError::BadParams { cmd: "select.lasso".into(), msg: "needs at least 3 points".into() }); } // The History step is named after the tool: the Polygonal Lasso sends `polygonal` (Photoshop). - let step = if b(p, "polygonal", false) { "Polygonal Lasso" } else { "Lasso" }; + // The Patch and Content-Aware Move tools draw with this command and send their own name as + // `historyName`, so the plain Lasso tool keeps "Lasso". + let step = if b(p, "polygonal", false) { + "Polygonal Lasso" + } else { + match p.get("historyName") { + None | Some(Value::Null) => "Lasso", + Some(Value::String(n)) if !n.trim().is_empty() && n.chars().count() <= 64 && !n.chars().any(char::is_control) => n.trim(), + Some(_) => return Err(EngineError::BadParams { cmd: "select.lasso".into(), msg: "historyName must be a short text".into() }), + } + }; polygon_selection(s, "select.lasso", step, &pts, p) } @@ -756,7 +766,7 @@ pub fn specs() -> Vec { "select.lasso", "Lasso", [], - r##"{"points":[[x,y],…],"mode":"replace|add|subtract|intersect"="replace","antiAlias":bool=true,"feather":px=0,"polygonal":bool=false}"##, + r##"{"points":[[x,y],…],"mode":"replace|add|subtract|intersect"="replace","antiAlias":bool=true,"feather":px=0,"polygonal":bool=false,"historyName"?:text (1..64 characters, no control characters; the History step name when a tool other than the Lasso draws it, e.g. "Patch Tool"; default "Lasso")}"##, has_doc, lasso ), diff --git a/crates/ui-egui/src/canvas/gesture.rs b/crates/ui-egui/src/canvas/gesture.rs index b67aec4beb..60af838375 100644 --- a/crates/ui-egui/src/canvas/gesture.rs +++ b/crates/ui-egui/src/canvas/gesture.rs @@ -144,7 +144,12 @@ fn finish(app: &mut PhotocraftApp, mut d: Drag, click: Option) { let o = &app.ui.tool_options; // The Patch and Content-Aware Move tools have no Feather in their options bar. let feather = if d.tool == Tool::Lasso { o.feather } else { 0.0 }; - let made = app.run("select.lasso", json!({"points": pts, "mode": mode, "antiAlias": o.anti_alias, "feather": feather})); + let mut params = json!({"points": pts, "mode": mode, "antiAlias": o.anti_alias, "feather": feather}); + // The Patch tool's lasso is a step of that tool in History; the Lasso tool keeps "Lasso". + if d.tool == Tool::Patch { + params["historyName"] = json!("Patch Tool"); + } + let made = app.run("select.lasso", params); // The lasso only outlines the patch; say that it is dragged next (#1715). if made.is_ok() && d.tool != Tool::Lasso { crate::retouch_ui::patch_hint(app); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 7d5d2a90e8..3ee1d27bbe 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -979,6 +979,8 @@ mod status_bar_tests; #[cfg(test)] mod stroke_timing_tests; +#[cfg(test)] +mod patch_tool_history_names_tests; #[cfg(test)] mod polygon_lasso_step_snap_tests; #[cfg(test)] diff --git a/crates/ui-egui/src/patch_tool_history_names_tests.rs b/crates/ui-egui/src/patch_tool_history_names_tests.rs new file mode 100644 index 0000000000..de9054fef1 --- /dev/null +++ b/crates/ui-egui/src/patch_tool_history_names_tests.rs @@ -0,0 +1,33 @@ +//! The Patch tool's lasso is a History step named "Patch Tool" (#12117); the Lasso tool keeps "Lasso". + +use egui::Modifiers; +use serde_json::json; + +use crate::PhotocraftApp; +use crate::canvas::{ToolEvent, tool_event}; +use crate::state::Tool; + +fn app_with(tool: Tool) -> PhotocraftApp { + let mut app = PhotocraftApp::new(photocraft_engine::Session::new(), crate::Services::default()); + app.run("file.new", json!({"width": 800, "height": 600, "background": "transparent"})).unwrap(); + app.sync_views(); + app.ui.tool = tool; + app +} + +/// Drag a triangle with the active tool and return the last History step. +fn drag_lasso(app: &mut PhotocraftApp) -> Option { + tool_event(app, ToolEvent::Down { x: 100.0, y: 100.0, pressure: 1.0 }, Modifiers::NONE); + for (x, y) in [(200.0, 100.0), (300.0, 100.0), (300.0, 300.0)] { + tool_event(app, ToolEvent::Move { x, y, pressure: 1.0 }, Modifiers::NONE); + } + tool_event(app, ToolEvent::Up { x: 200.0, y: 300.0 }, Modifiers::NONE); + assert!(app.session.active().unwrap().doc.selection.is_some()); + app.session.active().unwrap().history.entries().last().cloned() +} + +#[test] +fn the_patch_tool_lasso_is_named_for_the_tool() { + assert_eq!(drag_lasso(&mut app_with(Tool::Patch)).as_deref(), Some("Patch Tool")); + assert_eq!(drag_lasso(&mut app_with(Tool::Lasso)).as_deref(), Some("Lasso")); +}