From 5a33441af3bf5d9b93550055fe540a4ed3b448b8 Mon Sep 17 00:00:00 2001 From: Gert Pellin - GPE Date: Sat, 17 Oct 2026 02:34:40 +0200 Subject: [PATCH] fix(engine): an unknown layer id or name lists the document's layers, and a name matches regardless of case as a fallback (#5058) (#5133) A command that fails on a layer id or name the document doesn't have now says which layers there are, top-down as id and name (at most 20, names cut at 64 characters), so an agent can recover from the error alone: ids are not positions, and a freshly opened PNG's Background is layer 2. NoLayer becomes UnknownLayer { id, known } at both command entry points, keeping the 'no such layer' prefix. A layer name that matches no layer exactly matches regardless of case; an exact match always wins. Fixes #5058. --- crates/engine/src/commands.rs | 35 +++++++++-- crates/engine/src/jobs.rs | 16 ++++- crates/engine/src/lib.rs | 4 ++ crates/engine/src/type_caret_cmds.rs | 2 +- crates/engine/tests/missing_layer_error.rs | 68 ++++++++++++++++++++++ 5 files changed, 116 insertions(+), 9 deletions(-) create mode 100644 crates/engine/tests/missing_layer_error.rs diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index 48b576dcb4..ced8462c9e 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -295,7 +295,8 @@ fn set_clipped(s: &mut Session, p: &Value, clip: bool) -> Result { /// The layer a command's `"layer"` param names: an id, or a name (the topmost layer of that /// name, as Photoshop's "select layer by name" action step finds it, so a loaded action replays -/// in another document). `None` when the param is absent. +/// in another document; a name that matches no layer exactly matches regardless of case, so +/// `"background"` finds the Background). `None` when the param is absent. pub(crate) fn explicit_layer(s: &Session, p: &Value) -> Result> { match p.get("layer") { Some(Value::Number(n)) => { @@ -303,19 +304,41 @@ pub(crate) fn explicit_layer(s: &Session, p: &Value) -> Result> } Some(Value::String(name)) => { let d = s.active().ok_or(EngineError::NoDocument)?; - d.doc - .walk() - .into_iter() - .rev() + let layers = d.doc.walk(); + let mut top_down = layers.iter().rev(); + top_down + .clone() .find(|(_, _, l)| l.name == *name) + .or_else(|| top_down.find(|(_, _, l)| l.name.to_lowercase() == name.to_lowercase())) .map(|(_, _, l)| Some(l.id)) - .ok_or_else(|| EngineError::Other(format!("no layer named \"{name}\""))) + .ok_or_else(|| EngineError::Other(format!("no layer named \"{name}\"; {}", layer_listing(&d.doc)))) } Some(Value::Null) | None => Ok(None), Some(other) => Err(EngineError::Other(format!("`layer` must be an id or a name, not {other}"))), } } +/// The layers of `doc` as a caller can name them, for an error about a layer that isn't there: +/// `the document's layers are 2 "Background", 3 "Layer 1"` (top-down, at most 20). +pub(crate) fn layer_listing(doc: &Document) -> String { + const MAX: usize = 20; + let layers = doc.walk(); + if layers.is_empty() { + return "the document has no layers".into(); + } + // Names are unbounded input: each one is cut at NAME characters so the error stays short. + const NAME: usize = 64; + let name = |n: &str| match n.char_indices().nth(NAME) { + Some((cut, _)) => format!("{}…", &n[..cut]), + None => n.to_string(), + }; + let mut list: Vec = layers.iter().rev().take(MAX).map(|(_, _, l)| format!("{} {:?}", l.id.0, name(&l.name))).collect(); + if layers.len() > MAX { + list.push(format!("… {} more (see `document.inspect`)", layers.len() - MAX)); + } + format!("the document's layers are {} (a `layer` param takes an id or a name)", list.join(", ")) +} + /// The layer a command targets: [`explicit_layer`], else the active layer. pub(crate) fn layer_param(s: &Session, p: &Value) -> Result { match explicit_layer(s, p)? { diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index de2afd1556..17d8288f8b 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -543,7 +543,7 @@ impl Session { job.ctx.cancel(); Err(e) } - (Err(e), None) => Err(e), + (Err(e), None) => Err(self.explain_missing_layer(e)), (Ok(_), Some(mut job)) => { job.command = command.to_string(); job.params = params; @@ -740,7 +740,7 @@ impl Session { job.ctx.cancel(); Err(e) } - (Err(e), None) => Err(e), + (Err(e), None) => Err(self.explain_missing_layer(e)), (Ok(_), Some(mut job)) => { job.command = id.to_string(); job.params = params; @@ -756,6 +756,18 @@ impl Session { } } + /// A command that failed on a layer id the document doesn't have says which ids it does have + /// (#5058): ids are not positions (an opened PNG's Background is not layer 1), and an agent + /// should not need `document.inspect` to recover. + fn explain_missing_layer(&self, e: EngineError) -> EngineError { + match (e, self.active()) { + (EngineError::NoLayer(id), Some(st)) if st.doc.layer(id).is_none() => { + EngineError::UnknownLayer { id, known: crate::commands::layer_listing(&st.doc) } + } + (e, _) => e, + } + } + /// Bookkeeping after a command (or a job's apply) succeeded. /// /// `nested_from` is the journal length when the command started. A replayable command stands diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 0bf4d1b40f..22b1e6d728 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -156,6 +156,10 @@ pub enum EngineError { NoDocument, #[error("no such layer {0:?}")] NoLayer(LayerId), + /// [`Self::NoLayer`] as a command reports it: with the layers the document does have, as + /// ids are not positions (#5058). + #[error("no such layer {}; {known}", id.0)] + UnknownLayer { id: LayerId, known: String }, #[error("{0}")] Other(String), /// A background job was cancelled (see [`jobs`]); nothing changed. diff --git a/crates/engine/src/type_caret_cmds.rs b/crates/engine/src/type_caret_cmds.rs index 419d305ee6..3b9b244f70 100644 --- a/crates/engine/src/type_caret_cmds.rs +++ b/crates/engine/src/type_caret_cmds.rs @@ -482,7 +482,7 @@ mod tests { bad(&mut s, "type.navigate", json!({"layer": id, "index": 9, "move": "end"})); bad(&mut s, "type.navigate", json!({"layer": id, "index": 0, "move": "lineNext", "x": "no"})); bad(&mut s, "type.navigate", json!({"layer": pixel, "index": 0, "move": "start"})); - assert!(matches!(s.execute("type.caret", json!({"layer": u64::MAX, "index": 0})), Err(EngineError::NoLayer(_)))); + assert!(matches!(s.execute("type.caret", json!({"layer": u64::MAX, "index": 0})), Err(EngineError::UnknownLayer { .. }))); // A point on the named layer but outside its text still returns a caret, marked outside. let far = s.execute("type.hitTest", json!({"layer": id, "x": 390, "y": 290})).unwrap(); assert_eq!(far["inside"], false); diff --git a/crates/engine/tests/missing_layer_error.rs b/crates/engine/tests/missing_layer_error.rs new file mode 100644 index 0000000000..5764f5d85c --- /dev/null +++ b/crates/engine/tests/missing_layer_error.rs @@ -0,0 +1,68 @@ +//! A `"layer"` param that names no layer says which layers there are (#5058): ids are not +//! positions (a freshly opened PNG's Background is layer 2), and an agent should be able to +//! recover from the error alone. A name matches regardless of case as a fallback. + +use photocraft_engine::{EngineError, Session}; +use serde_json::json; + +fn session() -> (Session, u64) { + let mut s = Session::new(); + s.execute("file.new", json!({"width": 8, "height": 6})).unwrap(); + let bg = s.active().unwrap().active_layer.unwrap().0; + (s, bg) +} + +fn unused_id(s: &Session) -> u64 { + let doc = &s.active().unwrap().doc; + (1..).find(|id| doc.layer(photocraft_doc::LayerId(*id)).is_none()).unwrap() +} + +#[test] +fn an_unknown_layer_id_lists_the_documents_layers() { + let (mut s, bg) = session(); + let layer = s.execute("layer.new.layer", json!({})).unwrap()["layer"].as_u64().unwrap(); + let missing = unused_id(&s); + let err = s.execute("layer.setProps", json!({"layer": missing, "visible": false})).unwrap_err(); + assert!(matches!(err, EngineError::UnknownLayer { id, .. } if id.0 == missing), "{err}"); + let err = err.to_string(); + assert!(err.contains(&format!("no such layer {missing};")), "{err}"); + assert!(err.contains(&format!("{layer} \"Layer 1\", {bg} \"Background\"")), "top-down ids and names: {err}"); +} + +#[test] +fn an_unknown_layer_name_lists_the_documents_layers() { + let (mut s, bg) = session(); + let err = s.execute("layer.setProps", json!({"layer": "Nope", "visible": false})).unwrap_err().to_string(); + assert!(err.contains("no layer named \"Nope\""), "{err}"); + assert!(err.contains(&format!("{bg} \"Background\"")), "{err}"); +} + +#[test] +fn a_layer_name_matches_regardless_of_case() { + let (mut s, bg) = session(); + s.execute("layer.new.layer", json!({})).unwrap(); + s.execute("layer.setProps", json!({"layer": "background", "name": "Base"})).unwrap(); + assert_eq!(s.active().unwrap().doc.layer(photocraft_doc::LayerId(bg)).unwrap().name, "Base"); +} + +#[test] +fn an_exact_name_wins_over_a_case_insensitive_one() { + let (mut s, _) = session(); + let lower = s.execute("layer.new.layer", json!({"name": "shadow"})).unwrap()["layer"].as_u64().unwrap(); + let upper = s.execute("layer.new.layer", json!({"name": "Shadow"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("layer.setProps", json!({"layer": "shadow", "visible": false})).unwrap(); + let doc = &s.active().unwrap().doc; + assert!(!doc.layer(photocraft_doc::LayerId(lower)).unwrap().visible); + assert!(doc.layer(photocraft_doc::LayerId(upper)).unwrap().visible); +} + +#[test] +fn a_long_layer_name_is_shortened_in_the_listing() { + let (mut s, _) = session(); + let long = "é".repeat(500); + let layer = s.execute("layer.new.layer", json!({"name": long})).unwrap()["layer"].as_u64().unwrap(); + let err = s.execute("layer.setProps", json!({"layer": "Nope", "visible": false})).unwrap_err().to_string(); + let shown = format!("{layer} \"{}…\"", "é".repeat(64)); + assert!(err.contains(&shown), "the name is cut at 64 characters: {err}"); + assert!(err.chars().count() < 300, "{err}"); +}