diff --git a/crates/engine/src/commands.rs b/crates/engine/src/commands.rs index e5cde54e..474e72c5 100644 --- a/crates/engine/src/commands.rs +++ b/crates/engine/src/commands.rs @@ -187,18 +187,32 @@ pub(crate) fn clips_p(s: &Session, p: &Value) -> Vec { None => clip_p(p, "clip").map(|c| vec![c]).unwrap_or_else(|| s.state.selection.clone()), } } -pub(crate) fn track_p(s: &Session, p: &Value, k: &str) -> Option { - let v = p.get(k)?; - if let Some(id) = v.as_u64() { - return Some(TrackId(id)); - } - // "V1", "A2" - let name = v.as_str()?; - let seq = s.active_sequence()?; - let (kind, idx) = name.split_at(1); - let i: usize = idx.parse().ok()?; - let tracks = if kind.eq_ignore_ascii_case("v") { &seq.video_tracks } else { &seq.audio_tracks }; - tracks.get(i.checked_sub(1)?).map(|t| t.id) +/// The track parameter `k` names: a track id, or "V1" / "A2" (kind and 1-based number). +/// `Ok(None)` when the parameter is absent. Naming a track the active sequence does not have is +/// an error (bad parameters of `cmd`): callers fall back to a default track only when no track +/// was asked for. +pub(crate) fn track_p(s: &Session, p: &Value, k: &str, cmd: &str) -> Result> { + let v = match p.get(k) { + None | Some(Value::Null) => return Ok(None), + Some(v) => v, + }; + let seq = s.active_sequence().ok_or(EngineError::NoSequence)?; + let by_name = |name: &str| { + let mut rest = name.chars(); + let tracks = match rest.next()? { + 'v' | 'V' => &seq.video_tracks, + 'a' | 'A' => &seq.audio_tracks, + _ => return None, + }; + let i: usize = rest.as_str().parse().ok()?; + tracks.get(i.checked_sub(1)?).map(|t| t.id) + }; + let found = match v.as_u64() { + Some(id) => seq.track(TrackId(id)).map(|t| t.id), + None => v.as_str().and_then(by_name), + }; + let named = v.as_str().map(str::to_string).unwrap_or_else(|| v.to_string()); + found.map(Some).ok_or_else(|| bad(cmd, format!("no track {named} in this sequence (`{k}`)"))) } pub(crate) fn item_p(p: &Value, k: &str) -> Option { u64_p(p, k).map(ItemId) @@ -1208,7 +1222,7 @@ fn build() -> Vec { set_transition ), cmd!("sequence.closeGap", "Close Gap", ["Sequence"], None, r#"{"track":"V1"|id,"time":ticks}"#, has_seq, |s, p| { - let tr = track_p(s, p, "track").ok_or_else(|| bad("sequence.closeGap", "need `track`"))?; + let tr = track_p(s, p, "track", "sequence.closeGap")?.ok_or_else(|| bad("sequence.closeGap", "need `track`"))?; let t = time_p(s, p, "").unwrap_or(s.playhead()); s.edit_sequence("Ripple Delete", |q, _, st| { let before: Vec<(ClipId, Tick)> = q.track(tr).map(|x| x.items.iter().map(|i| (i.id, i.start)).collect()).unwrap_or_default(); @@ -1252,7 +1266,7 @@ fn build() -> Vec { Ok(Value::Null) }), cmd!("sequence.deleteTrack", "Delete Track", [], None, r#"{"track":"V3"|id}"#, has_seq, |s, p| { - let tr = track_p(s, p, "track").ok_or_else(|| bad("sequence.deleteTrack", "need `track`"))?; + let tr = track_p(s, p, "track", "sequence.deleteTrack")?.ok_or_else(|| bad("sequence.deleteTrack", "need `track`"))?; s.edit_sequence("Delete Track", |q, _, _| { q.video_tracks.retain(|t| t.id != tr); q.audio_tracks.retain(|t| t.id != tr); @@ -1565,8 +1579,8 @@ fn build() -> Vec { let item = item_p(p, "item").ok_or_else(|| bad("timeline.place", "need `item`"))?; let at = time_p(s, p, "").unwrap_or(s.playhead()); let tg = s.targeting(); - let v = track_p(s, p, "track").or(tg.video_dest); - let a = track_p(s, p, "audioTrack").or(tg.audio_dest); + let v = track_p(s, p, "track", "timeline.place")?.or(tg.video_dest); + let a = track_p(s, p, "audioTrack", "timeline.place")?.or(tg.audio_dest); let pi = s.project.item(item).ok_or_else(|| bad("timeline.place", "no such item"))?; let full = match &pi.kind { // Settings ▸ Timeline ▸ Still Image Default Duration @@ -1617,11 +1631,13 @@ fn build() -> Vec { Ok(json!({"selection": s.state.selection.iter().map(|c| c.0).collect::>()})) }), cmd!("timeline.move", "Move Clips", [], None, r#"{"moves":[{"clip":id,"track":id|"V2","time":ticks}],"insert":bool}"#, has_seq, |s, p| { - let moves: Vec<(ClipId, TrackId, Tick)> = p - .get("moves") - .and_then(Value::as_array) - .map(|a| a.iter().filter_map(|m| Some((clip_p(m, "clip")?, track_p(s, m, "track")?, Tick(m.get("time")?.as_i64()?)))).collect()) - .unwrap_or_default(); + let mut moves: Vec<(ClipId, TrackId, Tick)> = Vec::new(); + for m in p.get("moves").and_then(Value::as_array).into_iter().flatten() { + let track = track_p(s, m, "track", "timeline.move")?; + if let (Some(clip), Some(track), Some(time)) = (clip_p(m, "clip"), track, m.get("time").and_then(Value::as_i64)) { + moves.push((clip, track, Tick(time))); + } + } if moves.is_empty() { return Err(bad("timeline.move", "need `moves`")); } @@ -1830,7 +1846,7 @@ fn build() -> Vec { }), cmd!("timeline.razor", "Razor", [], None, r#"{"time":ticks,"track":"V1"|id?,"clip":id?}"#, has_seq, |s, p| { let t = time_p(s, p, "").unwrap_or(s.playhead()); - let tr = track_p(s, p, "track"); + let tr = track_p(s, p, "track", "timeline.razor")?; let clip = clip_p(p, "clip"); let n = if let Some(c) = clip { let clips = with_links(s, &[c]); @@ -1849,7 +1865,7 @@ fn build() -> Vec { r#"{"track":"V1"|id,"locked":bool?,"syncLock":bool?,"enabled":bool?,"muted":bool?,"solo":bool?,"name":str?,"volumeDb":f64?,"pan":f64?}"#, has_seq, |s, p| { - let tr = track_p(s, p, "track").ok_or_else(|| bad("timeline.setTrack", "need `track`"))?; + let tr = track_p(s, p, "track", "timeline.setTrack")?.ok_or_else(|| bad("timeline.setTrack", "need `track`"))?; let p = p.clone(); s.edit_sequence("Track Settings", |q, _, _| { let t = q.track_mut(tr).ok_or(filmcraft_edit::EditError::NoTrack(tr))?; @@ -1883,7 +1899,7 @@ fn build() -> Vec { } ), cmd!("timeline.setTargeting", "Track Targeting", [], None, r#"{"track":"V1"|id,"targeted":bool?,"sourcePatch":bool?}"#, has_seq, |s, p| { - let tr = track_p(s, p, "track").ok_or_else(|| bad("timeline.setTargeting", "need `track`"))?; + let tr = track_p(s, p, "track", "timeline.setTargeting")?.ok_or_else(|| bad("timeline.setTargeting", "need `track`"))?; let seq_id = s.state.active_sequence.ok_or(EngineError::NoSequence)?; let mut tg = s.targeting(); let is_video = s.active_sequence().is_some_and(|q| q.video_tracks.iter().any(|t| t.id == tr)); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index a5f34e61..1d0b3888 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -924,6 +924,8 @@ mod shortcuts_tests; #[cfg(test)] mod tests; #[cfg(test)] +mod track_params_tests; +#[cfg(test)] mod transcript_tests; #[cfg(test)] mod trim_tests; diff --git a/crates/engine/src/multicam.rs b/crates/engine/src/multicam.rs index 7e0d7c0e..f43a736a 100644 --- a/crates/engine/src/multicam.rs +++ b/crates/engine/src/multicam.rs @@ -382,7 +382,7 @@ fn synchronize(s: &mut Session, p: &Value) -> Result { // reference unit let reference = if let Some(c) = u64_p(p, "reference").map(ClipId) { units.iter().position(|u| u.members.contains(&c)).ok_or_else(|| bad(cmd, "`reference` is not among the clips"))? - } else if let Some(t) = track_p(s, p, "track") { + } else if let Some(t) = track_p(s, p, "track", cmd)? { units .iter() .position(|u| u.members.iter().any(|m| q.track(t).is_some_and(|tr| tr.item(*m).is_some()))) diff --git a/crates/engine/src/sequence_extras.rs b/crates/engine/src/sequence_extras.rs index f861405f..8477fffc 100644 --- a/crates/engine/src/sequence_extras.rs +++ b/crates/engine/src/sequence_extras.rs @@ -225,7 +225,7 @@ fn transcribe(s: &mut Session, p: &Value) -> Result { let track = match p.get("track") { None => None, Some(Value::String(t)) if t.eq_ignore_ascii_case("mix") => None, - Some(_) => Some(crate::commands::track_p(s, p, "track").ok_or_else(|| bad("sequence.transcribe", "unknown `track`"))?), + Some(_) => Some(crate::commands::track_p(s, p, "track", "sequence.transcribe")?.ok_or_else(|| bad("sequence.transcribe", "unknown `track`"))?), }; let mut items = Vec::new(); for t in q.audio_tracks.iter().filter(|t| track.is_none_or(|x| x == t.id)) { diff --git a/crates/engine/src/sequence_tools.rs b/crates/engine/src/sequence_tools.rs index d3add542..86059131 100644 --- a/crates/engine/src/sequence_tools.rs +++ b/crates/engine/src/sequence_tools.rs @@ -304,7 +304,7 @@ fn go_to_gap(s: &mut Session, p: &Value, next: bool, in_track: bool) -> Result = if in_track { - let tracks: Vec = match track_p(s, p, "track") { + let tracks: Vec = match track_p(s, p, "track", if next { "sequence.goToNextGapInTrack" } else { "sequence.goToPrevGapInTrack" })? { Some(id) => vec![id], None => s.targeting().targeted, }; @@ -460,7 +460,7 @@ fn delete_tracks(s: &mut Session, p: &Value) -> Result { tracks.iter().filter(|t| t.items.is_empty()).map(|t| t.id).collect() } Some(_) => { - let id = track_p(s, p, key).ok_or_else(|| bad("sequence.deleteTracks", format!("unknown {key} track")))?; + let id = track_p(s, p, key, "sequence.deleteTracks")?.ok_or_else(|| bad("sequence.deleteTracks", format!("unknown {key} track")))?; if !tracks.iter().any(|t| t.id == id) { return Err(bad("sequence.deleteTracks", format!("`{key}` must name a {key} track"))); } diff --git a/crates/engine/src/track_params_tests.rs b/crates/engine/src/track_params_tests.rs new file mode 100644 index 00000000..c6b7b4c3 --- /dev/null +++ b/crates/engine/src/track_params_tests.rs @@ -0,0 +1,63 @@ +//! Track parameters (`track`, `audioTrack`: an id, or "V1" / "A2"): a track the active sequence +//! does not have is a parameter error, never a silent default. + +use serde_json::json; + +use crate::{EngineError, Session}; + +fn demo() -> Session { + let mut s = Session::default(); + s.execute("file.openDemoProject", json!({})).unwrap(); + s +} + +/// `timeline.place {audioTrack: "A2"}` on a sequence with only A1 used to fall back to the +/// source-patched track and overwrite what was on A1. +#[test] +fn naming_a_missing_track_is_an_error_not_a_default() { + let mut s = demo(); + let seq = s.execute("file.newSequence", json!({"name": "One of each", "video": 1, "audio": 1})).unwrap(); + let q = s.active_sequence().unwrap(); + assert_eq!((q.video_tracks.len(), q.audio_tracks.len()), (1, 1), "{seq}"); + let (v1, a1) = (q.video_tracks[0].id, q.audio_tracks[0].id); + let item = s.project.items.values().find(|i| i.has_video() && i.has_audio() && i.as_media().is_some()).map(|i| i.id).unwrap(); + // omitted tracks keep their default: the source-patched V1 / A1 + s.execute("timeline.place", json!({"item": item.0, "time": 0})).unwrap(); + let before = s.project.clone(); + let undo = s.history.undo.len(); + for (key, name) in [("audioTrack", json!("A2")), ("track", json!("V2")), ("audioTrack", json!(999_999)), ("track", json!("V0")), ("track", json!("X1"))] { + let mut p = json!({"item": item.0, "time": 0}); + p[key] = name.clone(); + let e = s.execute("timeline.place", p).unwrap_err(); + assert!(matches!(&e, EngineError::BadParams { cmd, .. } if cmd == "timeline.place"), "a parameter error of the command: {e:?}"); + let e = e.to_string(); + let shown = name.as_str().map(str::to_string).unwrap_or_else(|| name.to_string()); + assert!(e.contains(&format!("no track {shown} in this sequence")), "{e}"); + } + assert!(s.execute("timeline.place", json!({"item": item.0, "time": 0, "audioTrack": "A2"})).unwrap_err().to_string().contains("audioTrack")); + // hostile names are errors too, never a panic + for name in ["", "V", "é1", "1", "V-1", "A1.5", "V99999999999999999999"] { + assert!(s.execute("timeline.place", json!({"item": item.0, "time": 0, "track": name})).is_err(), "{name:?}"); + } + // every command that takes a track by name refuses one that is not there + let clip = s.active_sequence().unwrap().video_tracks[0].items[0].id; + let e = s.execute("timeline.move", json!({"moves": [{"clip": clip.0, "track": "V2", "time": 0}]})).unwrap_err(); + assert!(matches!(&e, EngineError::BadParams { cmd, .. } if cmd == "timeline.move"), "{e:?}"); + assert!(s.execute("sequence.closeGap", json!({"track": "A2", "time": 0})).is_err()); + assert!(s.execute("sequence.deleteTrack", json!({"track": "V2"})).is_err()); + assert!(s.execute("sequence.deleteTrack", json!({"track": 999_999})).is_err()); + assert!(s.execute("timeline.setTrack", json!({"track": "A2", "muted": true})).is_err()); + assert!(s.execute("timeline.setTargeting", json!({"track": "V2", "targeted": true})).is_err()); + assert!(s.execute("timeline.razor", json!({"track": "A2", "time": 10})).is_err()); + assert!(s.execute("sequence.goToNextGapInTrack", json!({"track": "V2"})).is_err()); + assert_eq!(*s.project, *before, "nothing was edited"); + assert_eq!(s.history.undo.len(), undo, "and nothing was added to the undo history"); + // tracks that exist still resolve by name (either case) and by id + s.execute("timeline.place", json!({"item": item.0, "seconds": 60.0, "track": "v1", "audioTrack": a1.0})).unwrap(); + s.execute("timeline.setTrack", json!({"track": v1.0, "locked": true})).unwrap(); + s.execute("timeline.setTrack", json!({"track": "a1", "muted": true})).unwrap(); + let q = s.active_sequence().unwrap(); + assert!(q.video_tracks[0].locked && q.audio_tracks[0].muted); + s.execute("edit.undo", json!({})).unwrap(); + assert!(!s.active_sequence().unwrap().audio_tracks[0].muted); +} diff --git a/docs/agents.md b/docs/agents.md index 50ce3bcf..0d3b7856 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -89,6 +89,9 @@ Notes: - Time is in ticks (254 016 000 000 per second). Commands also accept `seconds`, `frame` or `timecode`. +- A track parameter (`track`, `audioTrack`) is a track id or a name such as `"V1"` / `"A2"`. + Leaving it out picks the command's default track; naming a track the sequence does not have is + an "invalid parameters" error, never a different track. - `engine.execute` also runs UI-only commands (`tool.razor`, `playback.toggle`, `window.workspace.color`, `window.panel.`). - Element ids come from the previous frame. If an element is missing right after a layout change,