engine: naming a track that does not exist is an error
Problem: `timeline.place {"audioTrack": "A2"}` on a sequence that has
only A1 placed the clip on A1, overwrote what was there and returned ok.
`track_p` returned `None` both for "no track asked for" and for "that
track does not exist", so callers fell back to the source-patched track.
It also took any first letter other than V as an audio track, accepted
track ids that are not in the sequence, and `split_at(1)` panicked on an
empty or non-ASCII name.
Solution: `track_p` returns `Result<Option<TrackId>>` and takes the
command id, like the other fallible parameter helpers. An absent (or
null) parameter is `Ok(None)` and keeps every default as it was. A name
or id the active sequence does not have is a parameter error
(`EngineError::BadParams`): "invalid parameters for `timeline.place`: no
track A2 in this sequence (`audioTrack`)". All callers propagate it:
timeline.place (`track`, `audioTrack`), timeline.move, timeline.razor,
timeline.setTrack, timeline.setTargeting, sequence.closeGap,
sequence.deleteTrack, sequence.deleteTracks, the go-to-gap commands,
sequence.transcribe and the multicam sync reference track.
Not changed: a `timeline.move` entry without a `clip`, `track` or `time`
is still skipped, as before; only a track that was named and is missing
is an error.
Test: `naming_a_missing_track_is_an_error_not_a_default` in
track_params_tests.rs (missing and hostile names, the error kind, nothing
edited, no undo step, existing tracks by name and id still resolve).
docs/agents.md notes the rule.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
ada55eb625
commit
9d3975a697
|
|
@ -187,18 +187,32 @@ pub(crate) fn clips_p(s: &Session, p: &Value) -> Vec<ClipId> {
|
|||
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<TrackId> {
|
||||
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<Option<TrackId>> {
|
||||
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<ItemId> {
|
||||
u64_p(p, k).map(ItemId)
|
||||
|
|
@ -1208,7 +1222,7 @@ fn build() -> Vec<CommandSpec> {
|
|||
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<CommandSpec> {
|
|||
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<CommandSpec> {
|
|||
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<CommandSpec> {
|
|||
Ok(json!({"selection": s.state.selection.iter().map(|c| c.0).collect::<Vec<_>>()}))
|
||||
}),
|
||||
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<CommandSpec> {
|
|||
}),
|
||||
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<CommandSpec> {
|
|||
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<CommandSpec> {
|
|||
}
|
||||
),
|
||||
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));
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -382,7 +382,7 @@ fn synchronize(s: &mut Session, p: &Value) -> Result<Value> {
|
|||
// 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())))
|
||||
|
|
|
|||
|
|
@ -225,7 +225,7 @@ fn transcribe(s: &mut Session, p: &Value) -> Result<Value> {
|
|||
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)) {
|
||||
|
|
|
|||
|
|
@ -304,7 +304,7 @@ fn go_to_gap(s: &mut Session, p: &Value, next: bool, in_track: bool) -> Result<V
|
|||
let t = s.playhead();
|
||||
let q = s.active_sequence().ok_or(EngineError::NoSequence)?;
|
||||
let gaps: Vec<TimeRange> = if in_track {
|
||||
let tracks: Vec<TrackId> = match track_p(s, p, "track") {
|
||||
let tracks: Vec<TrackId> = 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<Value> {
|
|||
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")));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
@ -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.<name>`).
|
||||
- Element ids come from the previous frame. If an element is missing right after a layout change,
|
||||
|
|
|
|||
Loading…
Reference in New Issue