From 42e92cb5123b8071ffe50dd3b68385b2cec48db2 Mon Sep 17 00:00:00 2001 From: David Mann Date: Thu, 5 Nov 2026 13:20:01 +0100 Subject: [PATCH] Address review: Batch stops, rename targets, Shape Blur memory, sampler releases - A direct Batch in a script or a CLI command list stops it when it is cancelled, cannot write its error log, or ran out of script steps; per-file errors alone still continue (CodeRabbit, outside-diff findings on automate_cmds.rs and photocraft-cli document.rs). - Recorded Save steps count against the script's step budget, and a Batch whose files failed with the budget spent carries scriptLimitError, so Browse reports the failure instead of ok: true (yoriswind). - actions.rename rewrites a named Batch, nested Play or Conditional call only if it resolved to the renamed action before the rename; a Batch with explicit steps is left alone (yoriswind: two same-named actions in different sets). - Shape Blur's run admission counts each worker's f64 prefix sums and, for a custom shape, the kernel mask kept for the run plus its worst-case coverage runs (yoriswind). - Every sampler that takes the primary release (Color Picker, Curves/Levels and Properties eyedroppers, Curves Cmd-click, Color Range) ends a transform handle drag and a Move drag begun before it, not only the Color Picker (#11210 review). Co-Authored-By: Claude Opus 5.5 --- apps/photocraft-cli/src/document.rs | 12 ++- apps/photocraft-cli/tests/it/cli.rs | 26 ++++++ crates/algo/src/lib.rs | 19 +++- crates/algo/src/tests.rs | 25 ++++++ crates/engine/src/cmd/actions_cmds.rs | 32 +++++-- crates/engine/src/cmd/actions_cmds/tests.rs | 44 ++++++++++ crates/engine/src/cmd/automate_cmds.rs | 5 +- crates/engine/src/cmd/file_cmds/batch_run.rs | 20 ++++- crates/engine/src/cmd/file_cmds/tests.rs | 88 +++++++++++++++++++ crates/ui-egui/src/control/dispatch.rs | 26 ++++-- .../src/transform_controls_pointer_tests.rs | 31 +++++++ 11 files changed, 301 insertions(+), 27 deletions(-) diff --git a/apps/photocraft-cli/src/document.rs b/apps/photocraft-cli/src/document.rs index 2b6f9fe344..c322e7d307 100644 --- a/apps/photocraft-cli/src/document.rs +++ b/apps/photocraft-cli/src/document.rs @@ -117,9 +117,15 @@ pub(crate) fn run_cmds(a: &RunArgs, out: &mut dyn Write, err: &mut dyn Write) -> continue; } let r = h.command_run(&id, params).map_err(|source| CliError::Command { id: id.clone(), source })?; - // A direct Batch reports per-file errors and continues, as in Scripts; failed - // action or script calls still stop the command list before export. - if let Some(message) = photocraft_engine::actions_cmds::nested_failure(&id, &r).filter(|_| id != "file.automate.batch") { + // A direct Batch reports per-file errors and continues, as in Scripts, unless it was + // cancelled, ran out of script steps or couldn't write its error log; failed action or + // script calls still stop the command list before export. + let failure = if id == "file.automate.batch" { + photocraft_engine::actions_cmds::batch_stop(&r) + } else { + photocraft_engine::actions_cmds::nested_failure(&id, &r) + }; + if let Some(message) = failure { return Err(CliError::NestedFailure { id, message }); } let new_warnings: Vec = diff --git a/apps/photocraft-cli/tests/it/cli.rs b/apps/photocraft-cli/tests/it/cli.rs index bb9d4b654b..8d751735d9 100644 --- a/apps/photocraft-cli/tests/it/cli.rs +++ b/apps/photocraft-cli/tests/it/cli.rs @@ -404,6 +404,32 @@ fn run_continues_after_batch_file_errors_and_keeps_partial_results() { std::fs::remove_dir_all(d).unwrap(); } +/// A direct Batch whose error log can't be written stops the command list before export, also +/// when a file failed as well (per-file errors alone would let it go on). +#[test] +fn run_stops_when_a_direct_batch_cannot_write_its_error_log() { + let d = tmp("run-batch-log-fails"); + let bad = d.join("bad.png"); + let out = d.join("result.png"); + std::fs::write(&bad, b"not a PNG").unwrap(); + // The folder itself as the log path: the log write fails. + let params = json!({"input":[bad],"output":d.join("batch"),"format":"png","steps":[],"errorLog":d}).to_string(); + let result = bin() + .args(["run", "--new", r#"{"width":8,"height":8}"#]) + .args(["--cmd", "file.automate.batch", "--params", ¶ms]) + .args(["--cmd", "document.inspect", "--out"]) + .arg(&out) + .output() + .unwrap(); + let error = String::from_utf8_lossy(&result.stderr); + assert_eq!(result.status.code(), Some(1), "stdout: {}\nstderr: {error}", String::from_utf8_lossy(&result.stdout)); + assert!(error.contains("error log"), "{error}"); + let rows: Vec = String::from_utf8_lossy(&result.stdout).lines().map(|line| serde_json::from_str(line).unwrap()).collect(); + assert!(rows.iter().all(|row| row["command"] != "document.inspect"), "later commands must not run"); + assert!(!out.exists(), "a stopped command list must not export"); + std::fs::remove_dir_all(d).unwrap(); +} + #[test] fn run_continues_after_successful_action_and_exports() { let d = tmp("run-successful-action"); diff --git a/crates/algo/src/lib.rs b/crates/algo/src/lib.rs index 1a4dd942d8..7ef07732ae 100644 --- a/crates/algo/src/lib.rs +++ b/crates/algo/src/lib.rs @@ -1049,7 +1049,7 @@ pub fn working_bytes(params: &FilterParams, area: Rect, bounds: Rect, channels: let floats = (channels as u64).saturating_mul(4); // Result groups hold at most RESULT_BUDGET bytes of floats, and never more than the area. let base = area.size().area().saturating_mul(floats).min(RESULT_BUDGET as u64).saturating_mul(3); - match params.halo_for(bounds) { + let windows = match params.halo_for(bounds) { Halo::Bounds => base.saturating_add(bounds.union(&area).size().area().saturating_mul(floats)), Halo::Radius(r) => { let side = tile.saturating_add((r.max(0) as u64).saturating_mul(2)); @@ -1057,7 +1057,22 @@ pub fn working_bytes(params: &FilterParams, area: Rect, bounds: Rect, channels: side.saturating_mul(side).saturating_add(tile.saturating_mul(tile).saturating_mul(3)).saturating_mul(floats).saturating_mul(threads), ) } - } + }; + windows.saturating_add(shape_blur_bytes(params, bounds, tile, channels as u64, threads)) +} + +/// What Shape Blur holds beyond the float windows: each worker's f64 prefix sums over its +/// halo-expanded tile, and for a custom shape the kernel mask kept for the whole run plus its +/// prepared coverage runs (at most [`shape_blur::max_spans`], 20 bytes each). +fn shape_blur_bytes(params: &FilterParams, bounds: Rect, tile: u64, channels: u64, threads: u64) -> u64 { + let (FilterParams::ShapeBlur { .. } | FilterParams::CustomShapeBlur { .. }) = params else { return 0 }; + let Halo::Radius(r) = params.halo_for(bounds) else { return 0 }; + let side = tile.min(2048).saturating_add((r.max(0) as u64).saturating_mul(2)).saturating_add(1); + let prefix = side.saturating_mul(side).saturating_mul(channels).saturating_mul(8).saturating_mul(threads); + let FilterParams::CustomShapeBlur { kernel: Some(mask), .. } = params else { return prefix }; + let size = (mask.rect.width() as usize).max(mask.rect.height() as usize); + let spans = (shape_blur::max_spans(size) as u64).saturating_mul(20); + prefix.saturating_add((mask.data.len() as u64).saturating_mul(4)).saturating_add(spans) } /// [`apply`] / [`apply_in`] with an explicit tile size (tests use it to check tile independence). diff --git a/crates/algo/src/tests.rs b/crates/algo/src/tests.rs index 8a2e6a272d..bad9b3bff7 100644 --- a/crates/algo/src/tests.rs +++ b/crates/algo/src/tests.rs @@ -16,6 +16,31 @@ fn relight_admission_tracks_the_document_dependent_working_set() { assert!(working_bytes(¶ms, tiny, tiny, 4) < 64 << 20, "{}", working_bytes(¶ms, tiny, tiny, 4)); } +/// Shape Blur admits each worker's f64 prefix sums over its halo-expanded tile, and a custom +/// shape also the kernel mask it keeps for the whole run and its worst-case coverage runs. +#[test] +fn shape_blur_admission_counts_prefix_sums_and_the_retained_kernel() { + let area = Rect::new(0, 0, 4096, 4096); + let radius = 100.0f32; + let plain = FilterParams::ShapeBlur { radius, shape: BlurShape::Circle }; + let Halo::Radius(reach) = plain.halo_for(area) else { panic!("Shape Blur has a radius halo") }; + let threads = rayon::current_num_threads().max(1) as u64; + let tile = auto_tile(&plain, area) as u64; + let window = tile + 2 * reach as u64; + // The float windows every radius filter admits (4 channels of f32), then the f64 prefix sums. + let windows = (area.size().area() * 16).min(RESULT_BUDGET as u64) * 3 + (window * window + tile * tile * 3) * 16 * threads; + let prefix = (window + 1) * (window + 1) * 4 * 8 * threads; + assert_eq!(working_bytes(&plain, area, area, 4), windows + prefix); + + let size = (2 * shape_blur::radius(radius).ceil() as usize + 1) as i32; + let mask = Image { rect: Rect::new(0, 0, size, size), ch: 1, data: vec![1.0; (size * size) as usize] }; + let custom = FilterParams::CustomShapeBlur { radius, kernel: Some(std::sync::Arc::new(mask)) }; + let kernel = (size * size) as u64 * 4 + shape_blur::max_spans(size as usize) as u64 * 20; + assert_eq!(working_bytes(&custom, area, area, 4), working_bytes(&plain, area, area, 4) + kernel); + // Unresolved, it holds no kernel yet. + assert_eq!(working_bytes(&FilterParams::CustomShapeBlur { radius, kernel: None }, area, area, 4), working_bytes(&plain, area, area, 4)); +} + fn fmt(s: SampleType) -> PixelFormat { PixelFormat::new(ColorMode::Rgb, s, true) } diff --git a/crates/engine/src/cmd/actions_cmds.rs b/crates/engine/src/cmd/actions_cmds.rs index 6b95648705..69b1156726 100644 --- a/crates/engine/src/cmd/actions_cmds.rs +++ b/crates/engine/src/cmd/actions_cmds.rs @@ -704,6 +704,19 @@ pub fn restore_recording(s: &mut Session, idx: usize, action: &str, from: usize) s.journal.push(("actions.play".into(), params)); } +/// What stops the caller of a Batch even where per-file errors don't (a direct Batch in a script +/// or a CLI command list): the script ran out of steps inside it, it was cancelled, or its +/// requested error log could not be written. +pub fn batch_stop(value: &Value) -> Option { + if let Some(error) = value.get("scriptLimitError").and_then(Value::as_str) { + return Some(error.to_string()); + } + if value.get("cancelled") == Some(&Value::Bool(true)) { + return Some("Batch was cancelled".into()); + } + value.get("errorLogError").and_then(Value::as_str).map(|error| format!("Batch error log failed: {error}")) +} + /// Nested playback and Batch return structured failures even though the command succeeded. /// Promote it to the caller's failed step so no later parent steps run after a child fails. /// Batch text summarizes counts and the first failure; its full result retains every file outcome. @@ -721,10 +734,7 @@ pub fn nested_failure(id: &str, value: &Value) -> Option { let Some(first) = errors.first() else { // Every input succeeded, yet the run was stopped or its requested log is missing: // later parent steps must not run as if the Batch had finished. - if value.get("cancelled") == Some(&Value::Bool(true)) { - return Some("Batch was cancelled".into()); - } - return value.get("errorLogError").and_then(Value::as_str).map(|error| format!("Batch error log failed: {error}")); + return batch_stop(value); }; let error = first.get("error")?.as_str()?; let file = first.get("file").or_else(|| first.get("document")).and_then(Value::as_str).unwrap_or("unknown input"); @@ -917,14 +927,18 @@ fn rename(s: &mut Session, p: &Value) -> Result { return Err(bad("actions.rename", "an action with this name already exists")); } let pending = s.actions.recording.map_or(s.journal.len(), |(start, _)| start); - let old_set = s.actions.list.get(idx).and_then(|a| a.set.clone()); + // A loaded library can hold two actions of one name (in different sets): a call follows the + // rename only if it resolved to this action before it, so one reaching the other stays put. + let before = s.actions.list.clone(); + let by_name = resolve(&before, &json!({"action": old}), "actions.rename").ok() == Some(idx); let calls = s.actions.list.iter_mut().flat_map(|a| a.steps.iter_mut()).chain(s.journal.iter_mut().skip(pending)); for (id, params) in calls { let keys: &[&str] = match id.as_str() { - "actions.play" => &["action"], - "actions.conditional" => &["then", "else"], - // A named Batch selects from the caller's list, by name alone or within its set. - "file.automate.batch" if params.get("set").is_none_or(|set| set.is_null() || set.as_str() == old_set.as_deref()) => &["action"], + "actions.play" if by_name => &["action"], + "actions.conditional" if by_name => &["then", "else"], + // A named Batch selects from the caller's list, by name alone or within its set; + // explicit `steps` run instead of any named action, so those are left alone. + "file.automate.batch" if params.get("steps").is_none() && crate::file_cmds::batch_action(&before, params).ok() == Some(idx) => &["action"], _ => &[], }; for key in keys { diff --git a/crates/engine/src/cmd/actions_cmds/tests.rs b/crates/engine/src/cmd/actions_cmds/tests.rs index bf56ba7178..0e5cfce8e3 100644 --- a/crates/engine/src/cmd/actions_cmds/tests.rs +++ b/crates/engine/src/cmd/actions_cmds/tests.rs @@ -1401,6 +1401,50 @@ fn record_keeps_an_empty_loaded_sets_exact_name() { assert_eq!(s.execute("actions.save", json!({"set": " Web "})).unwrap()["actions"], json!(["First"])); } +/// Two actions of one name in different sets (a loaded library): renaming the second by index +/// leaves the unqualified calls, which reach the first, and a Batch with explicit steps alone; +/// only the Batch qualified with the second's set follows. Renaming the first moves them all. +#[test] +fn rename_leaves_calls_that_reach_a_same_named_action_in_another_set() { + let calls = || { + vec![ + ("file.automate.batch".into(), json!({"action": "Target", "input": [], "output": "/out"})), + ("file.automate.batch".into(), json!({"action": "Target", "set": "B", "input": [], "output": "/out"})), + ("file.automate.batch".into(), json!({"action": "Target", "set": "B", "steps": [["image.adjustments.invert", {}]], "input": [], "output": "/out"})), + ("actions.play".into(), json!({"action": "Target"})), + ("actions.conditional".into(), json!({"if": "documentIsOpen", "then": "Target", "else": "Target"})), + ] + }; + let session = || { + let mut s = Session::new(); + for set in ["A", "B"] { + let mut target = Action::new("Target", vec![("image.adjustments.invert".into(), json!({}))]); + target.set = Some(set.into()); + s.actions.list.push(target); + } + s.actions.list.push(Action::new("Caller", calls())); + s + }; + let mut s = session(); + s.execute("actions.rename", json!({"action": 1, "name": "Renamed"})).unwrap(); + let caller = &s.actions.list[2].steps; + assert_eq!(caller[0].1["action"], "Target", "an unqualified Batch reaches set A's action"); + assert_eq!(caller[1].1["action"], "Renamed", "the Batch qualified with set B follows"); + assert_eq!(caller[2].1["action"], "Target", "explicit steps are left alone"); + assert_eq!(caller[3].1["action"], "Target", "a nested Play reaches set A's action"); + assert_eq!((&caller[4].1["then"], &caller[4].1["else"]), (&json!("Target"), &json!("Target"))); + assert_eq!(crate::file_cmds::batch_action(&s.actions.list, &caller[0].1).unwrap(), 0); + assert_eq!(crate::file_cmds::batch_action(&s.actions.list, &caller[1].1).unwrap(), 1); + + let mut s = session(); + s.execute("actions.rename", json!({"action": 0, "name": "Renamed"})).unwrap(); + let caller = &s.actions.list[2].steps; + assert_eq!(caller[0].1["action"], "Renamed"); + assert_eq!(caller[1].1["action"], "Target", "the Batch qualified with set B still names B's action"); + assert_eq!(caller[3].1["action"], "Renamed"); + assert_eq!((&caller[4].1["then"], &caller[4].1["else"]), (&json!("Renamed"), &json!("Renamed"))); +} + /// A saved named Batch step follows a renamed action, by name alone or within its own set; /// a Batch naming the same action in another set and an inline Batch stay as they were. #[test] diff --git a/crates/engine/src/cmd/automate_cmds.rs b/crates/engine/src/cmd/automate_cmds.rs index 3ad1b6a744..1a92114c96 100644 --- a/crates/engine/src/cmd/automate_cmds.rs +++ b/crates/engine/src/cmd/automate_cmds.rs @@ -135,8 +135,9 @@ fn run_steps(s: &mut Session, steps: &[(String, Value)]) -> Vec { } match s.execute(id, params.clone()) { Ok(r) => { - // Direct Batch calls report file errors successfully to scripts. Called actions still fail the script. - let failure = if id == "file.automate.batch" { None } else { crate::actions_cmds::nested_failure(id, &r) }; + // Direct Batch calls report file errors successfully to scripts, but running out of + // script steps, a cancel or a failed error log stops it. Called actions still fail it. + let failure = if id == "file.automate.batch" { crate::actions_cmds::batch_stop(&r) } else { crate::actions_cmds::nested_failure(id, &r) }; match failure { Some(error) => { results.push(json!({"command": id, "result": r, "error": error})); diff --git a/crates/engine/src/cmd/file_cmds/batch_run.rs b/crates/engine/src/cmd/file_cmds/batch_run.rs index aaccf4b4d5..3f65384e16 100644 --- a/crates/engine/src/cmd/file_cmds/batch_run.rs +++ b/crates/engine/src/cmd/file_cmds/batch_run.rs @@ -967,6 +967,8 @@ pub(super) fn batch(s: &mut Session, p: &Value) -> Result { if ctx.batch_interrupt_requested() { return Err(EngineError::Cancelled.into()); } + // A recorded Save is a step too: Batch writes the output for it. + charge_script_step(scratch)?; if batch_save_point(id) { let st = scratch.active_mut().ok_or(EngineError::NoDocument)?; saved = Some(st.doc.clone()); @@ -980,7 +982,6 @@ pub(super) fn batch(s: &mut Session, p: &Value) -> Result { continue; } let params = crate::actions_cmds::playback_params(scratch, id, params)?; - charge_script_step(scratch)?; let r = scratch.execute(id, params)?; ctx.publish_batch_preview(&source_path, scratch.active().map(|state| state.doc.clone())); // A failed step inside a called action makes this file an error, not a saved result. @@ -1032,6 +1033,8 @@ pub(super) fn batch(s: &mut Session, p: &Value) -> Result { s.actions.playback_steps = carried; } s.actions.script_steps = script_steps; + let mut result = result; + mark_script_limit(s, &mut result); if let (Some(notices), Some(imports)) = (&mut s.batch_open_notices, result.get("notices").and_then(Value::as_array)) { notices.extend(imports.iter().cloned()); } @@ -1115,6 +1118,8 @@ fn batch_open_documents( s.set_active(position); let mut saved: Option> = None; for (command, params) in steps { + // A recorded Save is a step too: Batch writes the output for it. + charge_script_step(s)?; if batch_save_point(command) { let state = s.active_mut().ok_or(EngineError::NoDocument)?; saved = Some(state.doc.clone()); @@ -1128,7 +1133,6 @@ fn batch_open_documents( continue; } let params = crate::actions_cmds::playback_params(s, command, params)?; - charge_script_step(s)?; let response = s.execute(command, params)?; if let Some(error) = crate::actions_cmds::nested_failure(command, &response) { return Err(FileFailure { error: EngineError::Other(error), result: Some(response) }); @@ -1177,6 +1181,7 @@ fn batch_open_documents( result["errorLogError"] = json!(error); } result["documents"] = json!(originals.len()); + mark_script_limit(s, &mut result); Ok(result) } @@ -1194,6 +1199,17 @@ fn charge_script_step(s: &mut Session) -> Result<()> { Ok(()) } +/// A file that failed once the script's step budget is spent failed because of it, and every +/// later script step would fail the same way: the result says so, so a script running this +/// Batch directly stops with that error (`batch_stop`) instead of reporting success. +fn mark_script_limit(s: &Session, result: &mut Value) { + let max = crate::automate_cmds::MAX_SCRIPT_STEPS; + let failed = result.get("errors").and_then(Value::as_array).is_some_and(|errors| !errors.is_empty()); + if s.actions.script_depth > 0 && s.actions.script_steps >= max && failed { + result["scriptLimitError"] = json!(format!("the script ran more than {max} steps in all (scripts calling scripts)")); + } +} + /// Check source structure once at the common pre-write seam, after Open authorization and ownership. fn preserve_source(path: &str) -> Result<()> { let bytes = read_file(path)?; diff --git a/crates/engine/src/cmd/file_cmds/tests.rs b/crates/engine/src/cmd/file_cmds/tests.rs index 750562a026..c2d5a21817 100644 --- a/crates/engine/src/cmd/file_cmds/tests.rs +++ b/crates/engine/src/cmd/file_cmds/tests.rs @@ -4145,6 +4145,94 @@ fn batch_steps_inside_a_script_count_against_its_step_limit() { } } +/// A recorded Save is a step too: with the script's budget spent, a Save-only Batch writes +/// nothing, in both sources, and its result says the script ran out of steps. +#[test] +fn a_save_only_batch_writes_nothing_once_the_script_steps_are_spent() { + let dir = tmp("batch-script-save-only"); + let input = png(&dir, "in.png", 4, 4, "#ff0000"); + for source in ["open", "folder"] { + let mut s = session(4, 4, 8); + s.actions.script_depth = 1; + s.actions.script_steps = crate::automate_cmds::MAX_SCRIPT_STEPS; + let out = join(&dir, &format!("out-{source}")); + let mut p = json!({"source": source, "output": out, "format": "png", "steps": [["file.save", {}]]}); + if source == "folder" { + p["input"] = json!([input]); + } + let result = s.execute("file.automate.batch", p).unwrap(); + assert_eq!(result["files"], json!([]), "{source}: {result}"); + assert!(result["errors"][0]["error"].as_str().unwrap_or_default().contains("steps in all"), "{source}: {result}"); + assert!(result["scriptLimitError"].is_string(), "{source}: {result}"); + let written = std::fs::read_dir(&out).map(|d| d.count()).unwrap_or(0); + assert_eq!(written, 0, "{source}: nothing may be written past the budget"); + } +} + +/// A direct Batch in a script that spends the script's last step makes the script fail there +/// instead of reporting `ok: true` with the exhaustion hidden as a per-file error. +#[test] +fn a_direct_batch_that_spends_the_script_budget_fails_the_script() { + let dir = tmp("batch-script-budget-browse"); + let input = png(&dir, "in.png", 4, 4, "#ff0000"); + for source in ["open", "folder"] { + let mut s = session(4, 4, 8); + // A nested script: the outer one has already spent all but two steps. + s.actions.script_depth = 1; + s.actions.script_steps = crate::automate_cmds::MAX_SCRIPT_STEPS - 2; + let mut batch = json!({"source": source, "output": join(&dir, &format!("out-{source}")), "format": "png", + "steps": [["image.adjustments.invert", {}], ["image.adjustments.invert", {}]]}); + if source == "folder" { + batch["input"] = json!([input]); + } + let r = s.execute("file.scripts.browse", json!({"steps": [["file.automate.batch", batch]]})).unwrap(); + assert_eq!(r["ok"], false, "{source}: {r}"); + assert!(r["results"][0]["error"].as_str().unwrap_or_default().contains("steps in all"), "{source}: {r}"); + assert!(r["results"][0]["result"]["errors"].as_array().is_some_and(|e| !e.is_empty()), "{source}: {r}"); + } +} + +/// Per-file errors from a direct Batch stay script results, but a cancel or an error log that +/// could not be written stops the script, also when files failed as well. +#[test] +fn a_direct_batch_in_a_script_stops_it_when_cancelled_or_its_log_fails() { + let dir = tmp("batch-script-stop"); + let input = png(&dir, "in.png", 4, 4, "#ff0000"); + let missing = join(&dir, "missing.png"); + let next = json!(["layer.new.layer", {}]); + + // Ordinary per-file errors: the script goes on. + let mut s = session(4, 4, 8); + let layers = doc(&s).layers.len(); + let batch = json!({"input": [missing.clone()], "output": join(&dir, "out-plain"), "steps": []}); + let r = s.execute("file.scripts.browse", json!({"steps": [["file.automate.batch", batch], next.clone()]})).unwrap(); + assert_eq!(r["ok"], true, "{r}"); + assert_eq!(doc(&s).layers.len(), layers + 1, "{r}"); + + // A failed error log next to a failed file: the script stops at the Batch. + let mut s = session(4, 4, 8); + let layers = doc(&s).layers.len(); + let batch = json!({"input": [missing], "output": join(&dir, "out-log"), "steps": [], "errorLog": dir.clone()}); + let r = s.execute("file.scripts.browse", json!({"steps": [["file.automate.batch", batch], next.clone()]})).unwrap(); + assert_eq!(r["ok"], false, "{r}"); + assert!(r["results"][0]["error"].as_str().unwrap_or_default().contains("error log"), "{r}"); + assert_eq!(doc(&s).layers.len(), layers, "the step after the Batch must not run: {r}"); + + // A cancel: the script stops at the Batch. + let mut s = session(4, 4, 8); + let ctx = crate::jobs::JobCtx::new(); + s.set_inline_job_ctx(Some(ctx.clone())); + REVIEW_CANCEL.with(|slot| *slot.borrow_mut() = Some(ctx.clone())); + s.authorize = Some(cancel_at_marker); + let layers = doc(&s).layers.len(); + let batch = json!({"input": [input], "output": join(&dir, "out-cancel"), "steps": [["layer.renameLayer", {"name": "Cancel here"}]]}); + let r = s.execute("file.scripts.browse", json!({"steps": [["file.automate.batch", batch], next]})).unwrap(); + REVIEW_CANCEL.with(|slot| *slot.borrow_mut() = None); + assert_eq!(r["ok"], false, "{r}"); + assert_eq!(r["results"][0]["error"], "Batch was cancelled", "{r}"); + assert_eq!(doc(&s).layers.len(), layers, "the step after the Batch must not run: {r}"); +} + /// Two spellings of one destination through a symlinked folder are one output claim. #[cfg(not(target_arch = "wasm32"))] #[test] diff --git a/crates/ui-egui/src/control/dispatch.rs b/crates/ui-egui/src/control/dispatch.rs index ba3ad21452..2d1b3d8486 100644 --- a/crates/ui-egui/src/control/dispatch.rs +++ b/crates/ui-egui/src/control/dispatch.rs @@ -1320,7 +1320,22 @@ fn parse_pointer_event(i: usize, e: &Value) -> std::result::Result bool { - pointer_eyedropper_took(app, ctx, g, s) || color_range_took(app, g, s) || (g.secondary && secondary_menu_took(app, ctx, g, s)) + let took = pointer_eyedropper_took(app, ctx, g, s) || color_range_took(app, g, s) || (g.secondary && secondary_menu_took(app, ctx, g, s)); + if took && matches!(s.ev, ToolEvent::Up { .. }) && !g.secondary && !g.middle { + release_taken_drag(app, g); + } + took +} + +/// A sampler (Color Picker, Curves/Levels or Properties eyedroppers, Curves ⌘-click, Color +/// Range) took the primary release: a transform handle drag or Move drag begun before it +/// ends, so later moves can't resume either without a new press. The Move drag ends where it +/// last was, since the release's coordinates belong to the sampler. +fn release_taken_drag(app: &mut PhotocraftApp, g: &PointerGesture) { + crate::tools::transform_tool::release_handle_drag(app); + if let Some([x, y]) = app.drag.as_ref().map(|d| d.points.last().map_or(d.start, |p| [p[0], p[1]])) { + tool_event(app, ToolEvent::Up { x, y }, g.mods); + } } fn pointer_eyedropper_took(app: &mut PhotocraftApp, ctx: &egui::Context, g: &PointerGesture, s: &PointerSample) -> bool { @@ -1328,14 +1343,7 @@ fn pointer_eyedropper_took(app: &mut PhotocraftApp, ctx: &egui::Context, g: &Poi let down = matches!(s.ev, ToolEvent::Down { .. }); // With the Color Picker on top the image is its eyedropper, as for the mouse. if crate::color_picker_ui::top(app).is_some() { - if matches!(s.ev, ToolEvent::Up { .. }) { - crate::tools::transform_tool::release_handle_drag(app); - // A drag begun before the picker opened ends where it last was (the release's - // coordinates are the picker's), so later moves can't resume it without a press. - if let Some([x, y]) = app.drag.as_ref().map(|d| d.points.last().map_or(d.start, |p| [p[0], p[1]])) { - tool_event(app, ToolEvent::Up { x, y }, g.mods); - } - } else { + if !matches!(s.ev, ToolEvent::Up { .. }) { crate::color_picker_ui::sample_at(app, x, y); } return true; diff --git a/crates/ui-egui/src/transform_controls_pointer_tests.rs b/crates/ui-egui/src/transform_controls_pointer_tests.rs index 3dea67706d..c7a1a8ac71 100644 --- a/crates/ui-egui/src/transform_controls_pointer_tests.rs +++ b/crates/ui-egui/src/transform_controls_pointer_tests.rs @@ -533,6 +533,37 @@ fn color_picker_release_clears_the_handle_drags_snap_lines() { assert!(app.prefs_rt.snap.is_none()); } +/// Every sampler that takes the primary release ends a transform handle drag begun before it, +/// not only the Color Picker: the Curves eyedroppers and Color Range too. A move after the +/// sampler closes needs a new press. +#[test] +fn other_samplers_releases_end_an_existing_transform_handle_drag() { + for sampler in ["curves", "colorRange"] { + let (mut app, ctx, _) = fixture(); + pointer(&mut app, &ctx, "down", [800.0, 500.0]); + pointer(&mut app, &ctx, "move", [700.0, 440.0]); + let preview = app.ui.transform.as_ref().unwrap().quad; + let dialog = match sampler { + "curves" => { + let id = crate::adjust_dialog::open(&mut app, "image.adjustments.curves").unwrap(); + let d = app.ui.dialog_mut(id).unwrap(); + let base = crate::filter_dialog::params_of(&d.fields); + d.fields.insert("__curvePicker".into(), json!("black")); + d.fields.insert("__curvePickerBase".into(), base); + assert!(crate::adjust_dialog::picker_armed(&app), "{sampler}"); + id + } + _ => crate::color_range_ui::open(&mut app), + }; + pointer(&mut app, &ctx, "down", [200.0, 150.0]); + pointer(&mut app, &ctx, "up", [999.0, 690.0]); + assert_eq!(app.ui.transform.as_ref().unwrap().quad, preview, "{sampler}: the sampler's coordinates do not move the box"); + app.ui.dialogs.retain(|d| d.id != dialog); + pointer(&mut app, &ctx, "move", [650.0, 410.0]); + assert_eq!(app.ui.transform.as_ref().unwrap().quad, preview, "{sampler}: later moves need a new press"); + } +} + /// The Color Picker takes the release of an ordinary Move drag: the drag ends where it last was, and /// moving after the picker closes needs a new press. #[test]