diff --git a/crates/audio-io/src/aiff.rs b/crates/audio-io/src/aiff.rs index d8462c0e..77ed6f80 100644 --- a/crates/audio-io/src/aiff.rs +++ b/crates/audio-io/src/aiff.rs @@ -185,6 +185,9 @@ pub(crate) struct Writer { quantizers: Vec, frames: u64, bytes: Vec, + /// Set when a block write has failed: the sink may hold a prefix of that block while + /// `frames` does not count it, so the writer refuses every later call. + failed: bool, } impl Writer { @@ -193,7 +196,7 @@ impl Writer { let start = out.stream_position()?; out.write_all(&header(channels, sample_rate, depth, 0)?)?; let quantizers = Quantizer::per_channel(u32::from(sample_format(depth).0), dither, channels); - Ok(Writer { out, start, channels, sample_rate, depth, quantizers, frames: 0, bytes: Vec::new() }) + Ok(Writer { out, start, channels, sample_rate, depth, quantizers, frames: 0, bytes: Vec::new(), failed: false }) } /// The frame count after `frames` more, or an error when AIFF can't hold that many (4 GiB). @@ -206,13 +209,22 @@ impl Writer { /// Appends `frames` frames of planar audio (missing samples are silence), unless they don't /// fit (see [`Writer::check_room`]). pub(crate) fn write(&mut self, block: &[Vec], frames: usize) -> Result<()> { + if self.failed { + return Err(crate::failed_write_error()); + } let total = self.check_room(frames as u64)?; - write_interleaved(&mut self.out, &mut self.bytes, block, frames, &mut self.quantizers, sample_format(self.depth), true)?; + if let Err(e) = write_interleaved(&mut self.out, &mut self.bytes, block, frames, &mut self.quantizers, sample_format(self.depth), true) { + self.failed = true; + return Err(AudioError::from(e)); + } self.frames = total; Ok(()) } pub(crate) fn finish(mut self) -> Result { + if self.failed { + return Err(crate::failed_write_error()); + } let data_len = u64::from(sample_format(self.depth).0 / 8).saturating_mul(self.channels as u64).saturating_mul(self.frames); if data_len % 2 == 1 { self.out.write_all(&[0])?; diff --git a/crates/audio-io/src/flac.rs b/crates/audio-io/src/flac.rs index 8a8c0ce1..b119f09d 100644 --- a/crates/audio-io/src/flac.rs +++ b/crates/audio-io/src/flac.rs @@ -318,6 +318,10 @@ pub(crate) struct Writer { pending: Vec>, frames: u64, frame_no: u64, + /// Set when a frame write has failed: `write_frame` clears the pending samples before + /// writing, so a failed frame is lost while `frames` still counts it; the writer refuses + /// every later call. + failed: bool, } impl Writer { @@ -336,6 +340,7 @@ impl Writer { pending: (0..channels).map(|_| Vec::with_capacity(BLOCK_SIZE)).collect(), frames: 0, frame_no: 0, + failed: false, }) } @@ -351,6 +356,9 @@ impl Writer { /// Appends `frames` frames of planar audio (missing samples are silence), unless they don't /// fit (see [`Writer::check_room`]). pub(crate) fn write(&mut self, block: &[Vec], frames: usize) -> Result<()> { + if self.failed { + return Err(crate::failed_write_error()); + } let total = self.check_room(frames as u64)?; let mut done = 0; while done < frames { @@ -361,8 +369,11 @@ impl Writer { p.extend((0..n).map(|i| i64::from(q.quantize(src.get(i).copied().unwrap_or(0.0))))); } done += n; - if held + n == BLOCK_SIZE { - self.write_frame()?; + if held + n == BLOCK_SIZE + && let Err(e) = self.write_frame() + { + self.failed = true; + return Err(e); } } self.frames = total; @@ -408,6 +419,9 @@ impl Writer { } pub(crate) fn finish(mut self) -> Result { + if self.failed { + return Err(crate::failed_write_error()); + } self.write_frame()?; let info = stream_info(self.channels, self.sample_rate, self.bps, self.frames); self.out.seek(SeekFrom::Start(self.start.saturating_add(4)))?; diff --git a/crates/audio-io/src/lib.rs b/crates/audio-io/src/lib.rs index 1b2b43f5..4a871946 100644 --- a/crates/audio-io/src/lib.rs +++ b/crates/audio-io/src/lib.rs @@ -294,6 +294,13 @@ pub fn encode_with_channel_mask(buf: &AudioBuffer, opts: &EncodeOptions, mask: u } } +/// The error a stream encoder returns for every call after one of its writes has failed: the +/// sink may already hold a prefix of the failed block, so the encoder's counters can no longer +/// describe the bytes on the sink, and later writes or `finish` must not claim success. +pub(crate) fn failed_write_error() -> AudioError { + AudioError::Encode("the stream encoder already failed on an earlier write; its output is incomplete".into()) +} + /// Writes WAV/BWF (RF64 above 4 GiB), AIFF / AIFF-C `fl32` or FLAC block by block, producing /// the same bytes [`encode_with_channel_mask`] would for the whole buffer. pub struct FileEncoder(Writer); @@ -329,7 +336,9 @@ impl FileEncoder { } /// Appends the first `frames` frames of `block`, one `Vec` per channel (missing samples are - /// silence), unless they don't fit (see [`FileEncoder::check_room`]). + /// silence), unless they don't fit (see [`FileEncoder::check_room`]). Once a write has + /// failed, the encoder is unusable: the sink may hold a prefix of the failed block, so + /// every later `write` and `finish` fails instead of completing an inconsistent file. pub fn write(&mut self, block: &[Vec], frames: usize) -> Result<()> { match &mut self.0 { Writer::Wav(w) => w.write(block, frames), diff --git a/crates/audio-io/src/wav.rs b/crates/audio-io/src/wav.rs index 1aa847d4..e941eb9f 100644 --- a/crates/audio-io/src/wav.rs +++ b/crates/audio-io/src/wav.rs @@ -321,6 +321,9 @@ pub(crate) struct Writer { quantizers: Vec, frames: u64, bytes: Vec, + /// Set when a block write has failed: the sink may hold a prefix of that block while + /// `frames` does not count it, so the writer refuses every later call. + failed: bool, } impl Writer { @@ -339,7 +342,20 @@ impl Writer { let start = out.stream_position()?; out.write_all(&header(channels, sample_rate, format, bwf, mask, 0, force_rf64)?)?; let quantizers = Quantizer::per_channel(u32::from(format.0), dither, channels); - Ok(Writer { out, start, channels, sample_rate, format, bwf: bwf.cloned(), mask, force_rf64, quantizers, frames: 0, bytes: Vec::new() }) + Ok(Writer { + out, + start, + channels, + sample_rate, + format, + bwf: bwf.cloned(), + mask, + force_rf64, + quantizers, + frames: 0, + bytes: Vec::new(), + failed: false, + }) } fn header(&self, frames: u64) -> Result> { @@ -355,13 +371,22 @@ impl Writer { /// Appends `frames` frames of planar audio (missing samples are silence). pub(crate) fn write(&mut self, block: &[Vec], frames: usize) -> Result<()> { + if self.failed { + return Err(crate::failed_write_error()); + } let total = self.check_room(frames as u64)?; - write_interleaved(&mut self.out, &mut self.bytes, block, frames, &mut self.quantizers, self.format, false)?; + if let Err(e) = write_interleaved(&mut self.out, &mut self.bytes, block, frames, &mut self.quantizers, self.format, false) { + self.failed = true; + return Err(AudioError::from(e)); + } self.frames = total; Ok(()) } pub(crate) fn finish(mut self) -> Result { + if self.failed { + return Err(crate::failed_write_error()); + } let data_len = u64::from(self.format.0 / 8).saturating_mul(self.channels as u64).saturating_mul(self.frames); if data_len % 2 == 1 { self.out.write_all(&[0])?; @@ -442,7 +467,10 @@ pub(crate) fn header( if is_float { push_chunk(&mut meta, b"fact", &u32::try_from(frames).unwrap_or(u32::MAX).to_le_bytes()); } - let total = (4 + (8 + rf64_body_len) + meta.len() + 8) as u64 + data_len + (data_len & 1); + let total = ((4 + (8 + rf64_body_len) + meta.len() + 8) as u64) + .checked_add(data_len) + .and_then(|t| t.checked_add(data_len & 1)) + .ok_or_else(|| AudioError::TooLarge("WAV size overflow".into()))?; let rf64 = force_rf64 || u32::try_from(total).is_err(); if rf64 { diff --git a/crates/audio-io/tests/roundtrip.rs b/crates/audio-io/tests/roundtrip.rs index a53c272a..10ba2ecf 100644 --- a/crates/audio-io/tests/roundtrip.rs +++ b/crates/audio-io/tests/roundtrip.rs @@ -284,3 +284,10 @@ fn float_wav_header_is_rewritten_in_place_as_the_file_grows() { assert_eq!((&big[..4], big.len()), (&b"RF64"[..], len)); assert!(wav_float_header(0, 48_000, 0).is_err()); } + +#[test] +fn float_wav_header_rejects_a_frame_count_whose_total_size_overflows() { + // 4 bytes per frame: the data length fits in u64, but adding the header to it does not. + let frames = u64::MAX / 4; + assert!(matches!(wav_float_header(1, 48_000, frames), Err(soundcraft_audio_io::AudioError::TooLarge(_)))); +} diff --git a/crates/audio-io/tests/stream.rs b/crates/audio-io/tests/stream.rs index 61a28946..0baae09d 100644 --- a/crates/audio-io/tests/stream.rs +++ b/crates/audio-io/tests/stream.rs @@ -1,5 +1,5 @@ use soundcraft_audio_io::{AudioBuffer, AudioError, BitDepth, BwfInfo, EncodeOptions, FileEncoder, FileFormat, encode, encode_with_channel_mask}; -use std::io::Cursor; +use std::io::{Cursor, Seek, SeekFrom, Write}; fn tone(channels: usize, frames: usize) -> AudioBuffer { let channels = (0..channels).map(|c| (0..frames).map(|i| ((i * (c + 3)) as f32 * 0.013).sin() * 1.2).collect()).collect(); @@ -49,16 +49,62 @@ fn streamed_files_match_whole_buffer_encodes() { } } +/// A `Write + Seek` sink that accepts `budget` bytes in total and then fails every write, so +/// a `write_all` on it appends a prefix of the block and only then reports the error. +struct FailAfter { + inner: Cursor>, + budget: usize, +} + +impl Write for FailAfter { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + if self.budget == 0 { + return Err(std::io::Error::other("sink is full")); + } + let n = buf.len().min(self.budget); + self.budget -= n; + self.inner.write(&buf[..n]) + } + + fn flush(&mut self) -> std::io::Result<()> { + self.inner.flush() + } +} + +impl Seek for FailAfter { + fn seek(&mut self, pos: SeekFrom) -> std::io::Result { + self.inner.seek(pos) + } +} + +#[test] +fn a_failed_write_poisons_the_encoder() { + let buf = tone(2, 20_001); + for format in [FileFormat::Wav, FileFormat::Aiff, FileFormat::Flac] { + let opts = EncodeOptions { format, bit_depth: BitDepth::Int16, dither: false, bwf: None }; + let sink = FailAfter { inner: Cursor::new(Vec::new()), budget: 256 }; + let mut enc = FileEncoder::new(sink, &opts, 2, buf.sample_rate, None).unwrap(); + // The sink accepts a prefix of the first block, then fails. + assert!(enc.write(&buf.channels, buf.frames()).is_err(), "{format:?}"); + // Later calls must not claim success: the bytes on the sink no longer match the + // encoder's counters (and for FLAC the failed frame's samples are already gone). + assert!(matches!(enc.write(&buf.channels, 16), Err(AudioError::Encode(_))), "{format:?}"); + assert!(matches!(enc.finish(), Err(AudioError::Encode(_))), "{format:?}"); + } +} + #[test] fn blocks_past_a_format_limit_are_refused_before_anything_is_written() { let buf = tone(2, 100); - for (format, limit) in [(FileFormat::Aiff, 1usize << 30), (FileFormat::Flac, 1 << 36)] { + for (format, limit) in [(FileFormat::Aiff, 1u64 << 30), (FileFormat::Flac, 1u64 << 36)] { let opts = EncodeOptions { format, bit_depth: BitDepth::Int16, dither: false, bwf: None }; let mut enc = FileEncoder::new(Cursor::new(Vec::new()), &opts, 2, buf.sample_rate, None).unwrap(); enc.write(&buf.channels, 100).unwrap(); - assert!(enc.check_room(limit as u64 / 2).is_ok(), "{format:?}"); - assert!(matches!(enc.check_room(limit as u64), Err(AudioError::TooLarge(_))), "{format:?}"); - assert!(matches!(enc.write(&[], limit), Err(AudioError::TooLarge(_))), "{format:?}"); + assert!(enc.check_room(limit / 2).is_ok(), "{format:?}"); + assert!(matches!(enc.check_room(limit), Err(AudioError::TooLarge(_))), "{format:?}"); + if let Ok(frames) = usize::try_from(limit) { + assert!(matches!(enc.write(&[], frames), Err(AudioError::TooLarge(_))), "{format:?}"); + } assert_eq!(enc.finish().unwrap().into_inner(), encode(&buf, &opts).unwrap()); } } diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 17c3a5bc..1172f7be 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -106,37 +106,48 @@ impl Remote { Ok(()) } - fn roundtrip(&mut self, method: &str, params: &Value) -> Result { - self.connect()?; + /// One request/reply exchange. On failure the flag says whether the request line was already + /// written to the socket (so the app may have executed it). + fn roundtrip(&mut self, method: &str, params: &Value) -> Result { + self.connect().map_err(|e| (e, false))?; let id = self.next_id; self.next_id += 1; let line = json!({"id": id, "method": method, "params": params}).to_string(); - let (w, r) = self.conn.as_mut().ok_or("not connected")?; - writeln!(w, "{line}").map_err(|e| e.to_string())?; + let (w, r) = self.conn.as_mut().ok_or_else(|| ("not connected".to_string(), false))?; + writeln!(w, "{line}").map_err(|e| (e.to_string(), false))?; let mut buf = String::new(); - r.read_line(&mut buf).map_err(|e| e.to_string())?; + r.read_line(&mut buf).map_err(|e| (e.to_string(), true))?; if buf.is_empty() { - return Err("connection closed".into()); + return Err(("connection closed".into(), true)); } - let v: Value = serde_json::from_str(&buf).map_err(|e| e.to_string())?; + let v: Value = serde_json::from_str(&buf).map_err(|e| (e.to_string(), true))?; if v.get("ok").and_then(Value::as_bool).unwrap_or(false) { Ok(v.get("result").cloned().unwrap_or(Value::Null)) } else { - Err(v.get("error").and_then(Value::as_str).unwrap_or("error").to_string()) + Err((v.get("error").and_then(Value::as_str).unwrap_or("error").to_string(), true)) } } } +/// Methods that only read state, so repeating them after a lost reply is harmless. +fn is_read_only(method: &str) -> bool { + matches!(method, "session.inspect" | "document.inspect" | "engine.commands" | "engine.parity" | "ui.inspect" | "ui.menu.list") +} + impl Backend for Remote { fn call(&mut self, method: &str, params: Value) -> Result { match self.roundtrip(method, ¶ms) { Ok(v) => Ok(v), - Err(e) if e.contains("closed") || e.contains("Broken pipe") || e.contains("reset") => { - // Retry once on a fresh connection. + Err((e, sent)) if e.contains("closed") || e.contains("Broken pipe") || e.contains("reset") => { self.conn = None; - self.roundtrip(method, ¶ms) + if sent && !is_read_only(method) { + // The app may already have executed the request; repeating it could apply it twice. + return Err(format!("{e}; `{method}` may or may not have been executed, check the session before repeating it")); + } + // Retry once on a fresh connection. + self.roundtrip(method, ¶ms).map_err(|(e, _)| e) } - Err(e) => Err(e), + Err((e, _)) => Err(e), } } fn has_ui(&self) -> bool { @@ -167,4 +178,43 @@ mod tests { let mut r = Remote::new("127.0.0.1:1"); assert!(r.call("session.inspect", json!({})).is_err()); } + + /// Fake app: accepts one connection per entry in `replies`, counts the request line it reads, + /// then either drops the connection (`false`) or answers ok (`true`). + fn fake_app(replies: Vec) -> (String, Arc) { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let seen = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let counter = seen.clone(); + std::thread::spawn(move || { + for answer in replies { + let Ok((mut sock, _)) = listener.accept() else { return }; + let mut line = String::new(); + if BufReader::new(sock.try_clone().unwrap()).read_line(&mut line).unwrap_or(0) > 0 { + counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + if answer { + let _ = writeln!(sock, "{}", json!({"id": 1, "ok": true, "result": {"done": true}})); + } + } + } + }); + (addr, seen) + } + + #[test] + fn remote_does_not_repeat_a_mutation_after_a_lost_reply() { + let (addr, seen) = fake_app(vec![false, true]); + let mut r = Remote::new(&addr); + let err = r.call("engine.execute", json!({"command": "track.new"})).unwrap_err(); + assert!(err.contains("may or may not have been executed"), "{err}"); + assert_eq!(seen.load(std::sync::atomic::Ordering::SeqCst), 1); + } + + #[test] + fn remote_retries_a_read_after_a_lost_reply() { + let (addr, seen) = fake_app(vec![false, true]); + let mut r = Remote::new(&addr); + assert!(r.call("session.inspect", json!({})).is_ok()); + assert_eq!(seen.load(std::sync::atomic::Ordering::SeqCst), 2); + } } diff --git a/crates/engine/src/clip_group_file.rs b/crates/engine/src/clip_group_file.rs index e20caf0c..ee1f3764 100644 --- a/crates/engine/src/clip_group_file.rs +++ b/crates/engine/src/clip_group_file.rs @@ -160,6 +160,10 @@ pub fn import(e: &mut Engine, path: &str, first: Option, at: Samples) - c.id = s.new_clip_id(); c.start = at.saturating_add(scale(c.start.max(0))); c.length = scale(c.length); + c.fade_in.len = scale(c.fade_in.len); + c.fade_out.len = scale(c.fade_out.len); + c.sync_point = scale(c.sync_point); + c.gain_env.iter_mut().for_each(|(at, _)| *at = scale(*at)); c.group = Some(group_id); c.sanitize(); placed.push(c.id); @@ -229,6 +233,28 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn fades_sync_point_and_envelope_follow_the_sample_rate() { + let dir = std::env::temp_dir().join(format!("sc-grp-rate-{}", std::process::id())); + let _ = std::fs::create_dir_all(&dir); + let p = dir.join("rate.scgrp"); + let mut e = crate::demo::demo_engine(); + // The file was saved at half the session rate, so every position doubles. + let src_rate = e.session().sample_rate.hz() / 2; + let fade = |len: i64| json!({"len": len, "shape": "Linear"}); + let clip = json!({ + "id": 1, "name": "m", "start": 0, "length": 10_000, "content": {"Midi": {"sequence": {"notes": [], "ctrls": []}}}, + "fade_in": fade(1000), "fade_out": fade(2000), "sync_point": 500, "gain_env": [[100, -6.0], [4000, 0.0]] + }); + let file = json!({"format": "soundcraft.clipgroup", "version": 1, "sample_rate": src_rate, "length": 10_000, "lanes": [[clip]], "audio": {}}); + std::fs::write(&p, file.to_string()).unwrap(); + let r = e.execute("file.import_clip_groups", &json!({"path": p.to_string_lossy()})).unwrap(); + let c = e.session().find_clip(soundcraft_model::ClipId(r["clips"][0].as_u64().unwrap())).unwrap().1; + assert_eq!((c.length, c.fade_in.len, c.fade_out.len, c.sync_point), (20_000, 2000, 4000, 1000)); + assert_eq!(c.gain_env.iter().map(|&(at, _)| at).collect::>(), vec![200, 8000]); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn a_file_named_by_many_ids_becomes_one_source() { let dir = std::env::temp_dir().join(format!("sc-grp-shared-{}", std::process::id())); diff --git a/crates/engine/src/cmd/audiosuite.rs b/crates/engine/src/cmd/audiosuite.rs index 585f2bec..911ccca5 100644 --- a/crates/engine/src/cmd/audiosuite.rs +++ b/crates/engine/src/cmd/audiosuite.rs @@ -117,6 +117,8 @@ fn process(e: &mut Engine, p: &Value) -> Result { nc.name = new_name; nc.content = ClipContent::Audio { source: src, offset: 0 }; nc.length = i64::try_from(frames).unwrap_or(nc.length); + // The render already has the clip's stretch applied; keeping it would stretch the new file again. + nc.stretch = 1.0; // Invert and Duplicate are phase/copy operations. The clip's gain stays a clip gain, // the same way its fades stay editable. Other processes bake a new file at unity gain. if !matches!(name.as_str(), "invert" | "duplicate") { @@ -216,6 +218,21 @@ mod tests { assert!((sample - 0.5).abs() < 1e-3, "duplicate keeps the file level and the clip gain, got {sample}"); } + #[test] + fn processed_clip_does_not_keep_the_stretch_it_baked() { + for stretch in [0.5, 2.0] { + for process in ["invert", "duplicate"] { + let (mut e, id) = gained_clip(); + e.session_mut().find_clip_mut(id).unwrap().stretch = stretch; + e.execute("audiosuite.process", &json!({"process": process, "clips": [id.0]})).unwrap(); + let c = e.session().tracks[0].clips()[0].clone(); + let frames = e.session().pool.get(c.source().unwrap()).unwrap().buffer.channels[0].len(); + assert_eq!(c.stretch, 1.0, "{process} at {stretch}"); + assert_eq!(c.length, i64::try_from(frames).unwrap(), "{process} at {stretch}"); + } + } + } + #[test] fn signal_generator_fills_the_selection_with_a_clip() { let (mut e, _) = gained_clip(); diff --git a/crates/engine/src/cmd/clip.rs b/crates/engine/src/cmd/clip.rs index 9d68dc25..aff6d8cc 100644 --- a/crates/engine/src/cmd/clip.rs +++ b/crates/engine/src/cmd/clip.rs @@ -102,6 +102,7 @@ pub fn specs() -> Vec { } Ok(json!({})) }), + cmd!("clip.remove_sync_point", "Remove Sync Point", ["Clip"], None, "{clips?}", has_sync_point, remove_sync_point), cmd!("clip.quantize_to_grid", "Quantize to Grid", ["Clip"], Some("Cmd+0"), "{clips?}", has_selection, quantize_to_grid), cmd!("clip.color", "Clip Color", [], None, "{clips?, color: [r,g,b] | null}", has_selection, |e, p| { let ids = clip_ids_param(e, p); @@ -348,6 +349,31 @@ fn nudge_gain(e: &mut Engine, p: &Value, db: f64) -> Result { gain(e, &json!({"clips": clips, "delta_db": db})) } +/// Sync point 0 is "none" (edit_window only draws one when > 0). +fn has_sync_point(e: &Engine) -> std::result::Result<(), String> { + let s = e.session(); + if clip_ids_param(e, &json!({})).iter().any(|id| s.find_clip(*id).is_some_and(|(_, c)| c.sync_point != 0)) { + Ok(()) + } else { + Err("no selected clip has a sync point".into()) + } +} + +fn remove_sync_point(e: &mut Engine, p: &Value) -> Result { + let s = e.session(); + // Only clips that have a point are touched, so a call that changes nothing leaves no undo step. + let ids: Vec = clip_ids_param(e, p).into_iter().filter(|id| s.find_clip(*id).is_some_and(|(_, c)| c.sync_point != 0)).collect(); + if !ids.is_empty() { + let s = e.session_mut(); + for id in &ids { + if let Some(c) = s.find_clip_mut(*id) { + c.sync_point = 0 + } + } + } + Ok(json!({"clips": ids.len()})) +} + fn quantize_to_grid(e: &mut Engine, p: &Value) -> Result { let ids = clip_ids_param(e, p); let s = e.session(); @@ -452,4 +478,72 @@ mod tests { e.execute("clip.gain_nudge_up", &json!({})).unwrap(); assert_eq!(gain(&e), 36.0); } + + /// Three 10 000-sample clips side by side on one track; sync points are set by each test. + fn three_clips() -> (crate::Engine, [ClipId; 3]) { + use soundcraft_model::{ChannelFormat, Clip, Session, TrackKind}; + let mut s = Session::default(); + let t = s.add_track(TrackKind::Audio, ChannelFormat::Mono, Some("Dialog")); + let buf = soundcraft_audio_io::AudioBuffer { sample_rate: 48_000, channels: vec![vec![0.1; 10_000]] }; + let src = crate::io::add_source(&mut s, "Line", buf, None, soundcraft_audio_io::FileFormat::Wav); + let ids = [0, 1, 2].map(|k| { + let cid = s.new_clip_id(); + crate::edit::place_clip(&mut s, t, Clip::audio(cid, "Line", src, 0, k * 12_000, 10_000)); + cid + }); + (crate::Engine::new(s), ids) + } + + fn sync_of(e: &crate::Engine, id: ClipId) -> i64 { + e.session().find_clip(id).unwrap().1.sync_point + } + + #[test] + fn remove_sync_point_clears_the_point_identify_set() { + let (mut e, [a, ..]) = three_clips(); + e.execute("edit.select", &json!({"clips": [a.0]})).unwrap(); + e.execute("clip.identify_sync_point", &json!({"at": 4_000})).unwrap(); + assert_eq!(sync_of(&e, a), 4_000); + e.execute("clip.remove_sync_point", &json!({})).unwrap(); + assert_eq!(sync_of(&e, a), 0); + } + + #[test] + fn remove_sync_point_clears_every_selected_clip_and_only_those() { + let (mut e, [a, b, c]) = three_clips(); + for (id, at) in [(a, 1_000), (b, 2_000), (c, 3_000)] { + e.session_mut().find_clip_mut(id).unwrap().sync_point = at; + } + e.execute("edit.select", &json!({"clips": [a.0, b.0]})).unwrap(); + let r = e.execute("clip.remove_sync_point", &json!({})).unwrap(); + assert_eq!(r["clips"], 2); + assert_eq!([sync_of(&e, a), sync_of(&e, b), sync_of(&e, c)], [0, 0, 3_000]); + } + + #[test] + fn remove_sync_point_is_disabled_unless_a_selected_clip_has_one() { + let (mut e, [a, b, c]) = three_clips(); + e.session_mut().find_clip_mut(c).unwrap().sync_point = 3_000; + let enabled = |e: &crate::Engine| crate::find_command("clip.remove_sync_point").unwrap().info(e).enabled; + e.execute("edit.select", &json!({"clips": [a.0, b.0]})).unwrap(); + assert!(!enabled(&e), "neither selected clip has a sync point (the unselected one does)"); + e.execute("edit.select", &json!({"clips": [b.0, c.0]})).unwrap(); + assert!(enabled(&e)); + } + + #[test] + fn remove_sync_point_is_one_undo_step_that_puts_the_points_back() { + let (mut e, [a, b, _]) = three_clips(); + for (id, at) in [(a, 1_000), (b, 2_000)] { + e.session_mut().find_clip_mut(id).unwrap().sync_point = at; + } + e.execute("edit.select", &json!({"clips": [a.0, b.0]})).unwrap(); + let steps = e.undo_history().len(); + e.execute("clip.remove_sync_point", &json!({})).unwrap(); + assert_eq!(e.undo_history().len(), steps + 1); + assert_eq!(e.undo_label(), Some("Remove Sync Point")); + assert_eq!([sync_of(&e, a), sync_of(&e, b)], [0, 0]); + assert!(e.undo()); + assert_eq!([sync_of(&e, a), sync_of(&e, b)], [1_000, 2_000]); + } } diff --git a/crates/engine/src/cmd/mix.rs b/crates/engine/src/cmd/mix.rs index c0569429..d6e9ffdb 100644 --- a/crates/engine/src/cmd/mix.rs +++ b/crates/engine/src/cmd/mix.rs @@ -73,7 +73,8 @@ pub fn specs() -> Vec { )), cmd!("mix.trim", "Input Trim", [], None, "{tracks?, db}", has_selection, |e, p| { let tracks = tracks_required(e, "mix.trim", p)?; - let db = f32_or(p, "db", 0.0).clamp(-144.0, 24.0); + let db = (p.get("db").and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad("mix.trim", "`db` (number) required"))? as f32) + .clamp(-144.0, 24.0); let s = e.session_mut(); for t in tracks { if let Some(tr) = s.track_mut(t) { @@ -234,7 +235,8 @@ fn volume(e: &mut Engine, p: &Value) -> Result { fn pan(e: &mut Engine, p: &Value) -> Result { let tracks = tracks_required(e, "mix.pan", p)?; - let v = f32_or(p, "pan", 0.0).clamp(-1.0, 1.0); + let v = (p.get("pan").and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad("mix.pan", "`pan` (number) required"))? as f32) + .clamp(-1.0, 1.0); let idx = i64_or(p, "index", 0).clamp(0, 1) as usize; let s = e.session_mut(); for t in &tracks { @@ -287,17 +289,30 @@ fn slot_of(p: &Value, key: &str, max: usize) -> Option { (i < max).then_some(i) } +fn slot_error(cmd: &str, max: usize) -> EngineError { + bad(cmd, format!("`slot` must be 0..{} or a..{}", max - 1, (b'a' + max as u8 - 1) as char)) +} + fn track_slot(e: &Engine, p: &Value, cmd: &str, max: usize) -> Result<(TrackId, usize)> { let t = tracks_required(e, cmd, p)?.first().copied().ok_or_else(|| bad(cmd, "no track"))?; - let slot = slot_of(p, "slot", max).ok_or_else(|| bad(cmd, format!("`slot` must be 0..{} or a..{}", max - 1, (b'a' + max as u8 - 1) as char)))?; + let slot = slot_of(p, "slot", max).ok_or_else(|| slot_error(cmd, max))?; Ok((t, slot)) } +/// An optional `slot`: absent (or null) means "first free"; a present but invalid value is +/// an error, never silently treated as absent. +fn optional_slot(p: &Value, cmd: &str, max: usize) -> Result> { + match p.get("slot") { + None | Some(Value::Null) => Ok(None), + Some(_) => slot_of(p, "slot", max).map(Some).ok_or_else(|| slot_error(cmd, max)), + } +} + fn insert(e: &mut Engine, p: &Value) -> Result { let plugin = str_param(p, "plugin").ok_or_else(|| bad("mix.insert", "`plugin` required"))?.to_string(); let info = super::clap::plugin_info(&plugin).ok_or_else(|| bad("mix.insert", format!("unknown plugin `{plugin}`")))?; let t = tracks_required(e, "mix.insert", p)?.first().copied().ok_or_else(|| bad("mix.insert", "no track"))?; - let explicit = slot_of(p, "slot", INSERT_SLOTS); + let explicit = optional_slot(p, "mix.insert", INSERT_SLOTS)?; let s = e.session_mut(); let tr = s.track_mut(t).ok_or_else(|| bad("mix.insert", "no track"))?; let slot = @@ -416,7 +431,7 @@ fn insert_move(e: &mut Engine, p: &Value) -> Result { fn send(e: &mut Engine, p: &Value) -> Result { let bus = str_param(p, "bus").ok_or_else(|| bad("mix.send", "`bus` required"))?.to_string(); let t = tracks_required(e, "mix.send", p)?.first().copied().ok_or_else(|| bad("mix.send", "no track"))?; - let explicit = slot_of(p, "slot", SEND_SLOTS); + let explicit = optional_slot(p, "mix.send", SEND_SLOTS)?; let s = e.session_mut(); let bid = s.add_bus(&bus, soundcraft_model::ChannelFormat::Stereo); let tr = s.track_mut(t).ok_or_else(|| bad("mix.send", "no track"))?; @@ -694,7 +709,13 @@ fn clip_gain_to_vol(e: &mut Engine, p: &Value) -> Result { fn copy_to_send(e: &mut Engine, p: &Value) -> Result { let tracks = tracks_required(e, "automation.copy_to_send", p)?; - let slot = slot_of(p, "send", SEND_SLOTS).unwrap_or(0) as u8; + // No `send` means Send A (the menu item passes none); a given one must be valid. + let slot = match p.get("send") { + None | Some(Value::Null) => 0, + Some(_) => slot_of(p, "send", SEND_SLOTS).ok_or_else(|| { + bad("automation.copy_to_send", format!("`send` must be 0..{} or a..{}", SEND_SLOTS - 1, (b'a' + SEND_SLOTS as u8 - 1) as char)) + })?, + } as u8; let s = e.session_mut(); for t in &tracks { if let Some(tr) = s.track_mut(*t) { @@ -874,6 +895,26 @@ mod tests { assert_eq!(e.session().tracks[0].mixer.inserts[0].as_ref().unwrap().preset, "Saved"); } + #[test] + fn copy_to_send_rejects_an_invalid_send_slot() { + let mut e = crate::demo::demo_engine(); + let history = e.undo_history().len(); + let lanes = |e: &Engine| e.session().tracks.iter().filter(|t| t.lane(&AutoParam::SendLevel(0)).is_some_and(|l| !l.points.is_empty())).count(); + let before = lanes(&e); + for bad_params in [json!({"track": "Kick", "send": 99}), json!({"track": "Kick", "send": "zz"})] { + let r = e.execute("automation.copy_to_send", &bad_params); + assert!(r.is_err(), "{bad_params} accepted: {r:?}"); + assert_eq!(e.undo_history().len(), history, "{bad_params} pushed an undo step"); + assert_eq!(lanes(&e), before, "{bad_params} edited Send A"); + } + // No `send` (the menu item's call) still means Send A. + e.execute("automation.copy_to_send", &json!({"track": "Kick"})).unwrap(); + assert_eq!(lanes(&e), before + 1); + // A valid slot still copies. + e.execute("automation.copy_to_send", &json!({"track": "Kick", "send": "a"})).unwrap(); + assert_eq!(lanes(&e), before + 1); + } + #[test] fn retargeting_a_send_keeps_its_settings() { let mut e = Engine::default(); @@ -886,4 +927,27 @@ mod tests { assert_eq!(sl.target, soundcraft_model::Route::Bus(delay)); assert_eq!((sl.level_db, sl.pan, sl.mute, sl.pre_fader), (-9.0, 0.5, true, true)); } + + #[test] + fn insert_and_send_reject_invalid_slot() { + let mut e = Engine::default(); + e.execute("track.new", &json!({"count": 1, "name": "Gtr", "format": "mono"})).unwrap(); + for slot in [json!(12), json!("k"), json!("x1")] { + let r = e.execute("mix.insert", &json!({"track": "Gtr", "slot": slot, "plugin": "compressor"})); + assert!(r.is_err(), "insert slot {slot} accepted: {r:?}"); + } + for slot in [json!(12), json!("k")] { + let r = e.execute("mix.send", &json!({"track": "Gtr", "slot": slot, "bus": "Verb"})); + assert!(r.is_err(), "send slot {slot} accepted: {r:?}"); + } + let mixer = &e.session().tracks[0].mixer; + assert!(mixer.inserts.iter().all(Option::is_none)); + assert!(mixer.sends.iter().all(Option::is_none)); + + // Leaving `slot` out still picks the first free slot. + let r = e.execute("mix.insert", &json!({"track": "Gtr", "plugin": "compressor"})).unwrap(); + assert_eq!(r["slot"], 0); + let r = e.execute("mix.send", &json!({"track": "Gtr", "bus": "Verb"})).unwrap(); + assert_eq!(r["slot"], 0); + } } diff --git a/crates/engine/src/cmd/track.rs b/crates/engine/src/cmd/track.rs index cc496448..4929b468 100644 --- a/crates/engine/src/cmd/track.rs +++ b/crates/engine/src/cmd/track.rs @@ -543,6 +543,15 @@ fn folder_bus(s: &Session, folder: TrackId) -> Option { s.track(folder).map(|folder| folder.mixer.input.clone()).filter(|route| matches!(route, Route::Bus(_))) } +/// The summing bus owned by a routing folder. +fn owned_folder_bus(s: &Session, folder: TrackId) -> Option { + let folder = s.track(folder).filter(|track| track.kind == TrackKind::Folder)?; + match folder.mixer.input { + Route::Bus(id) => Some(id), + _ => None, + } +} + /// Keep only the selected roots so selecting a folder and one of its children preserves the subtree. fn root_tracks(s: &Session, tracks: Vec) -> Vec { tracks.iter().copied().filter(|track| !tracks.iter().any(|root| root != track && in_folder(s, *track, *root))).collect() @@ -627,6 +636,20 @@ fn change_width(e: &mut Engine, p: &Value) -> Result { fn delete(e: &mut Engine, p: &Value) -> Result { let tracks = tracks_required(e, "track.delete", p)?; let s = e.session_mut(); + let removed_busses: Vec<_> = tracks + .iter() + .filter_map(|folder| { + let bus = owned_folder_bus(s, *folder)?; + let mut parent = s.track(*folder).and_then(|folder| folder.folder); + for _ in 0..64 { + match parent { + Some(candidate) if tracks.contains(&candidate) => parent = s.track(candidate).and_then(|folder| folder.folder), + _ => break, + } + } + Some((bus, parent.and_then(|parent| folder_bus(s, parent)).unwrap_or(Route::Main))) + }) + .collect(); // Promote children of deleted folders and repair only routes that used the deleted // folder's own bus. Custom outputs remain untouched. let mut promoted = Vec::new(); @@ -657,6 +680,22 @@ fn delete(e: &mut Engine, p: &Value) -> Result { for g in &mut s.groups { g.members.retain(|t| !tracks.contains(t)); } + for (bus, replacement) in removed_busses { + s.busses.retain(|candidate| candidate.id != bus); + for track in &mut s.tracks { + if track.mixer.input == Route::Bus(bus) { + track.mixer.input = Route::None; + } + if track.mixer.output == Route::Bus(bus) { + track.mixer.output = replacement.clone(); + } + for send in &mut track.mixer.sends { + if send.as_ref().is_some_and(|send| send.target == Route::Bus(bus)) { + *send = None; + } + } + } + } Ok(json!({"deleted": tracks.len()})) } @@ -999,6 +1038,14 @@ mod tests { e.execute("track.move_to_folder", &json!({"tracks": ["Kick"], "folder": inner.0})).unwrap(); e.execute("track.move_to_folder", &json!({"tracks": [inner.0], "folder": outer.0})).unwrap(); + let inner_bus = match e.session().track(inner).unwrap().mixer.input { + Route::Bus(bus) => bus, + _ => panic!("folder did not receive a bus"), + }; + let outer_bus = match e.session().track(outer).unwrap().mixer.input { + Route::Bus(bus) => bus, + _ => panic!("folder did not receive a bus"), + }; assert!(e.execute("track.move_to_folder", &json!({"tracks": [outer.0], "folder": inner.0})).is_err()); let kick = e.session().track_by_name("Kick").unwrap(); @@ -1025,6 +1072,8 @@ mod tests { let kick = e.session().track_by_name("Kick").unwrap(); assert_eq!(kick.folder, None); assert_eq!(kick.mixer.output, Route::Main); + assert!(e.session().bus(inner_bus).is_none(), "the deleted folder's owned bus must be removed"); + assert!(e.session().bus(outer_bus).is_some(), "the surviving parent folder's bus must remain"); } #[test] diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index e04cfbc9..eff105ec 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -309,17 +309,19 @@ pub fn import_midi_bytes(e: &mut Engine, bytes: &[u8], at: Samples, tempo_map: b let base_tick = s.tempo.samples_to_ticks(at, s.sample_rate); let mut out = Vec::new(); for (i, tr) in smf.tracks.iter().enumerate() { - if tr.sequence.notes.is_empty() { + if tr.sequence.notes.is_empty() && tr.sequence.ctrls.is_empty() { continue; } let mut name = if tr.name.trim().is_empty() { format!("MIDI {}", i + 1) } else { tr.name.clone() }; soundcraft_model::truncate_chars(&mut name, soundcraft_model::MAX_NAME_CHARS); let id = s.add_track(TrackKind::Instrument, ChannelFormat::Stereo, Some(&name)); - let drums = tr.sequence.notes.iter().all(|n| n.channel == 9); + let drums = !tr.sequence.notes.is_empty() && tr.sequence.notes.iter().all(|n| n.channel == 9); if let Some(t) = s.track_mut(id) { t.instrument = Some(soundcraft_model::Insert::new(if drums { "drum_synth" } else { "subtractive_synth" })); } - let end_tick = base_tick.saturating_add(tr.sequence.end_tick().max(1)); + // A controller on the clip's last tick would not play, so the clip runs one tick past the last one. + let past_ctrls = tr.sequence.ctrls.iter().map(|c| c.tick.saturating_add(1)).max().unwrap_or(0); + let end_tick = base_tick.saturating_add(tr.sequence.end_tick().max(past_ctrls).max(1)); let len = s.tempo.tick_to_samples(end_tick, s.sample_rate).saturating_sub(at); if len > s.sample_rate.samples(86_400.0) { return Err(EngineError::Io(format!("MIDI track {}: longer than 24 hours", i + 1))); @@ -1041,6 +1043,18 @@ fn write_synced(path: &Path, bytes: &[u8]) -> std::io::Result<()> { f.sync_all() } +/// Copies `from` to `to`, which must not exist yet; an existing entry, even a link, is never followed. +fn copy_new(from: &Path, to: &Path) -> std::io::Result<()> { + let mut src = std::fs::File::open(from)?; + let mut dst = std::fs::OpenOptions::new().write(true).create_new(true).open(to)?; + if let Err(err) = std::io::copy(&mut src, &mut dst) { + drop(dst); + let _ = std::fs::remove_file(to); + return Err(err); + } + Ok(()) +} + /// Copy the session file about to be replaced to `Backups/ backup .`, keeping the /// newest ten. fn back_up(file: &Path) { @@ -1061,9 +1075,16 @@ fn back_up(file: &Path) { .flatten() .filter_map(|f| f.file_name().to_str()?.strip_prefix(&prefix)?.strip_suffix(&suffix)?.parse().ok()) .collect(); - let next = numbers.iter().max().map_or(1, |n| n.saturating_add(1)); - let copied = std::fs::create_dir_all(&backups).and_then(|()| std::fs::copy(file, backups.join(format!("{prefix}{next}{suffix}")))); - if let Err(err) = copied { + let Some(next) = numbers.iter().max().map_or(Some(1), |n| n.checked_add(1)) else { + log::warn!("no backup of {}: backup numbers are exhausted", file.display()); + return; + }; + let dest = backups.join(format!("{prefix}{next}{suffix}")); + if let Err(err) = crate::file_access::check_write(&dest) { + log::warn!("no backup of {}: {err}", file.display()); + return; + } + if let Err(err) = std::fs::create_dir_all(&backups).and_then(|()| copy_new(file, &dest)) { log::warn!("no backup of {}: {err}", file.display()); return; } @@ -1286,14 +1307,32 @@ pub fn open_session_bytes(e: &mut Engine, name: &str, bytes: &[u8]) -> Result>> = std::collections::HashMap::new(); for src in &s.sources { - match files.remove(&src.path) { - Some(audio) => match decode_source(&audio, Path::new(&src.path).extension().and_then(|x| x.to_str()), s.sample_rate.hz()) { - Ok(audio) => s.pool.insert(src.id, audio), - Err(err) => missing.push(format!("{}: {err}", src.path)), - }, - None => missing.push(src.path.clone()), + if let Some(audio) = decoded.get(&src.path) { + if let Some(audio) = audio { + s.pool.insert(src.id, Arc::clone(audio)); + } + continue; } + let audio = match files.remove(&src.path) { + Some(bytes) => match decode_source(&bytes, Path::new(&src.path).extension().and_then(|x| x.to_str()), s.sample_rate.hz()) { + Ok(audio) => Some(audio), + Err(err) => { + missing.push(format!("{}: {err}", src.path)); + None + } + }, + None => { + missing.push(src.path.clone()); + None + } + }; + if let Some(audio) = &audio { + s.pool.insert(src.id, Arc::clone(audio)); + } + decoded.insert(src.path.clone(), audio); } // Movies are referenced by path, never copied into a session: none can be found here. missing.extend(s.videos.iter().map(|v| v.path.clone())); @@ -1724,6 +1763,43 @@ mod save_tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn backup_never_overwrites_an_existing_entry_when_numbers_run_out() { + let dir = std::env::temp_dir().join(format!("soundcraft-backups-full-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join("Backups")).unwrap(); + let path = dir.join("Song.scraft"); + std::fs::write(&path, b"session").unwrap(); + let last = dir.join(format!("Backups/Song backup {}.scraft", u32::MAX)); + std::fs::write(&last, b"sentinel").unwrap(); + back_up(&path); + assert_eq!(std::fs::read(&last).unwrap(), b"sentinel"); + let dest = dir.join("Backups/Song backup 1.scraft"); + std::fs::write(&dest, b"taken").unwrap(); + assert!(copy_new(&path, &dest).is_err()); + assert_eq!(std::fs::read(&dest).unwrap(), b"taken"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[cfg(unix)] + #[test] + fn backup_never_writes_through_a_link_to_an_outside_file() { + let dir = std::env::temp_dir().join(format!("soundcraft-backups-link-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(dir.join("Project/Backups")).unwrap(); + let outside = dir.join("outside.txt"); + std::fs::write(&outside, b"sentinel").unwrap(); + let path = dir.join("Project/Song.scraft"); + std::fs::write(&path, b"session").unwrap(); + std::os::unix::fs::symlink(&outside, dir.join(format!("Project/Backups/Song backup {}.scraft", u32::MAX))).unwrap(); + back_up(&path); + let link = dir.join("Project/Backups/Song backup 1.scraft"); + std::os::unix::fs::symlink(&outside, &link).unwrap(); + assert!(copy_new(&path, &link).is_err()); + assert_eq!(std::fs::read(&outside).unwrap(), b"sentinel"); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn save_gives_sources_that_share_a_path_their_own_files() { let mut s = Session::default(); @@ -1878,6 +1954,28 @@ mod name_tests { assert_eq!((track.name.len(), track.clips()[0].name.len()), (soundcraft_model::MAX_NAME_CHARS, soundcraft_model::MAX_NAME_CHARS)); } + #[test] + fn a_controller_only_midi_track_is_imported_with_its_events() { + let ctrls = vec![ + soundcraft_midi::CtrlEvent { tick: 0, channel: 0, kind: soundcraft_midi::CtrlKind::Cc(7), value: 100 }, + soundcraft_midi::CtrlEvent { tick: 480, channel: 0, kind: soundcraft_midi::CtrlKind::Cc(7), value: 64 }, + soundcraft_midi::CtrlEvent { tick: 960, channel: 0, kind: soundcraft_midi::CtrlKind::Cc(7), value: 20 }, + ]; + let sequence = soundcraft_midi::Sequence { ctrls: ctrls.clone(), ..Default::default() }; + let smf = soundcraft_midi::Smf { tracks: vec![soundcraft_midi::SmfTrack { name: "Vol".into(), sequence }], ..Default::default() }; + let mut e = Engine::default(); + let ids = import_midi_bytes(&mut e, &soundcraft_midi::write_smf(&smf).unwrap(), 0, false).unwrap(); + assert_eq!(ids.len(), 1); + let track = e.session().track(ids[0]).unwrap(); + assert_eq!(track.instrument.as_ref().map(|i| i.plugin.as_str()), Some("subtractive_synth")); + // The clip must reach past the last event, or playback skips it. + assert!(track.clips()[0].end() > e.session().tempo.tick_to_samples(960, e.session().sample_rate)); + match &track.clips()[0].content { + ClipContent::Midi { sequence } => assert_eq!(sequence.ctrls, ctrls), + _ => panic!("expected a MIDI clip"), + } + } + #[test] fn midi_longer_than_a_day_is_refused() { let held = soundcraft_midi::Note { pitch: 60, velocity: 100, release_velocity: 64, channel: 0, start: 0, length: (1 << 28) - 1 }; @@ -2016,6 +2114,31 @@ mod bundle_tests { assert_eq!(r.path.as_deref(), Some("Midnight Groove.scraftz")); } + #[test] + fn sources_sharing_one_file_all_load_from_a_bundle() { + use std::io::Write as _; + let e = crate::demo::demo_engine(); + let (bytes, _) = session_bundle(e.session()).unwrap(); + let (text, files) = read_bundle(&bytes).unwrap(); + let mut s = Session::from_json(&text).unwrap(); + s.sources[1].path = s.sources[0].path.clone(); + let mut w = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); + for path in s.sources.iter().map(|x| &x.path).collect::>() { + w.start_file(path.as_str(), opts).unwrap(); + w.write_all(&files[path]).unwrap(); + } + w.start_file("Song.scraft", opts).unwrap(); + w.write_all(s.to_json().unwrap().as_bytes()).unwrap(); + let bundle = w.finish().unwrap().into_inner(); + let mut r = Engine::default(); + let missing = open_session_bytes(&mut r, "Song.scraftz", &bundle).unwrap(); + assert!(missing.is_empty(), "{missing:?}"); + for src in &r.session().sources { + assert!(r.session().pool.get(src.id).is_some(), "{} has no audio", src.name); + } + } + #[test] fn same_named_sources_get_distinct_files() { let mut e = crate::demo::demo_engine(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 6cc1c90a..03d173dd 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -369,7 +369,15 @@ impl Engine { let key = self.merge_hold.clone().unwrap_or_else(|| key.to_string()); let continuing = self.merge_key.as_ref() == Some(&key); let before_len = self.undo.len(); - let r = self.execute(id, params)?; + // The step pushed for a continuing tick is dropped again below, so it must not trim the + // oldest entry first when the history is full. + let limit = self.history_limit; + if continuing && before_len > 0 { + self.history_limit = limit.max(before_len.saturating_add(1)); + } + let r = self.execute(id, params); + self.history_limit = limit; + let r = r?; if continuing && self.undo.len() == before_len + 1 && before_len > 0 { // Drop the step just pushed; the earlier one still holds the pre-gesture document. self.undo.pop(); diff --git a/crates/engine/tests/fader_merge_full_history.rs b/crates/engine/tests/fader_merge_full_history.rs new file mode 100644 index 00000000..c4656535 --- /dev/null +++ b/crates/engine/tests/fader_merge_full_history.rs @@ -0,0 +1,42 @@ +use serde_json::json; +use soundcraft_engine::Engine; +use soundcraft_model::{ChannelFormat, TrackKind}; + +fn engine_with_full_history(limit: usize) -> (Engine, u64) { + let mut e = Engine::default(); + let t = e.session_mut().add_track(TrackKind::Audio, ChannelFormat::Mono, None).0; + e.history_limit = limit; + for i in 0..limit + 1 { + e.execute("mix.pan", &json!({"track": t, "pan": 0.1 * (i as f64 + 1.0)})).unwrap(); + } + assert_eq!(e.undo_history().len(), limit); + (e, t) +} + +#[test] +fn a_fader_gesture_is_one_undo_step_when_the_history_is_full() { + let (mut e, t) = engine_with_full_history(3); + for db in [-1.0, -2.0, -3.0, -4.0, -5.0] { + e.execute_merged("mix.volume", &json!({"track": t, "db": db}), "fader").unwrap(); + } + e.end_merge(); + let volume = |e: &Engine| e.session().tracks.iter().find(|x| x.id.0 == t).map(|x| x.mixer.volume_db); + assert_eq!(volume(&e), Some(-5.0)); + // The gesture costs one step; it must not push further real edits out of the history. + assert_eq!(e.undo_history().len(), 3, "history: {:?}", e.undo_history()); + assert_eq!(e.undo_history().iter().filter(|l| l.as_str() == "Set Volume").count(), 1, "history: {:?}", e.undo_history()); + assert!(e.undo()); + assert_eq!(volume(&e), Some(0.0), "one undo should undo the whole fader gesture; history: {:?}", e.undo_history()); +} + +#[test] +fn a_zero_history_limit_keeps_no_undo_entry_for_a_gesture() { + let mut e = Engine::default(); + let t = e.session_mut().add_track(TrackKind::Audio, ChannelFormat::Mono, None).0; + e.history_limit = 0; + for db in [-1.0, -2.0, -3.0] { + e.execute_merged("mix.volume", &json!({"track": t, "db": db}), "fader").unwrap(); + } + e.end_merge(); + assert!(e.undo_history().is_empty(), "history: {:?}", e.undo_history()); +} diff --git a/crates/engine/tests/mix_required_params.rs b/crates/engine/tests/mix_required_params.rs new file mode 100644 index 00000000..f11e2f31 --- /dev/null +++ b/crates/engine/tests/mix_required_params.rs @@ -0,0 +1,42 @@ +use serde_json::json; +use soundcraft_engine::{Engine, EngineError}; + +fn engine_with_kick() -> Engine { + let mut e = Engine::default(); + e.execute("track.new", &json!({"name": "Kick", "format": "stereo"})).unwrap(); + e +} + +fn kick_pan(e: &Engine) -> Vec { + e.session().tracks.iter().find(|t| t.name == "Kick").unwrap().mixer.pan.to_vec() +} + +fn kick_trim(e: &Engine) -> f32 { + e.session().tracks.iter().find(|t| t.name == "Kick").unwrap().mixer.trim_db +} + +#[test] +fn mix_pan_rejects_a_missing_or_non_numeric_pan_and_keeps_the_pan() { + let mut e = engine_with_kick(); + e.execute("mix.pan", &json!({"tracks": ["Kick"], "pan": -1.0})).unwrap(); + assert_eq!(kick_pan(&e)[0], -1.0); + + for params in [json!({"tracks": ["Kick"]}), json!({"tracks": ["Kick"], "pan": "left"}), json!({"tracks": ["Kick"], "pan": null})] { + let r = e.execute("mix.pan", ¶ms); + assert!(matches!(r, Err(EngineError::BadParams(..))), "{params}: expected BadParams, got {r:?}"); + assert_eq!(kick_pan(&e)[0], -1.0, "{params}: the pan must be untouched"); + } +} + +#[test] +fn mix_trim_rejects_a_missing_or_non_numeric_db_and_keeps_the_trim() { + let mut e = engine_with_kick(); + e.execute("mix.trim", &json!({"tracks": ["Kick"], "db": -6.0})).unwrap(); + assert_eq!(kick_trim(&e), -6.0); + + for params in [json!({"tracks": ["Kick"]}), json!({"tracks": ["Kick"], "db": "loud"}), json!({"tracks": ["Kick"], "db": null})] { + let r = e.execute("mix.trim", ¶ms); + assert!(matches!(r, Err(EngineError::BadParams(..))), "{params}: expected BadParams, got {r:?}"); + assert_eq!(kick_trim(&e), -6.0, "{params}: the trim must be untouched"); + } +} diff --git a/crates/midi/src/smf.rs b/crates/midi/src/smf.rs index 36c18a0b..7747a061 100644 --- a/crates/midi/src/smf.rs +++ b/crates/midi/src/smf.rs @@ -485,20 +485,20 @@ pub fn write_smf(smf: &Smf) -> Result, MidiError> { } for n in &track.sequence.notes { let ch = n.channel & 0x0F; - let pitch = n.pitch & 0x7F; + let pitch = n.pitch.min(127); let start = clamp_tick(n.start); let end = clamp_tick(n.end()).max(start.saturating_add(1)); ev.push(WEvent { tick: start, order: 3, bytes: vec![0x90 | ch, pitch, n.velocity.clamp(1, 127)] }); - ev.push(WEvent { tick: end, order: 1, bytes: vec![0x80 | ch, pitch, n.release_velocity & 0x7F] }); + ev.push(WEvent { tick: end, order: 1, bytes: vec![0x80 | ch, pitch, n.release_velocity.min(127)] }); } for c in &track.sequence.ctrls { let ch = c.channel & 0x0F; let v7 = c.value.clamp(0, 127) as u8; let bytes = match c.kind { - CtrlKind::Cc(num) => vec![0xB0 | ch, num & 0x7F, v7], + CtrlKind::Cc(num) => vec![0xB0 | ch, num.min(127), v7], CtrlKind::Program => vec![0xC0 | ch, v7], CtrlKind::ChannelPressure => vec![0xD0 | ch, v7], - CtrlKind::PolyPressure(p) => vec![0xA0 | ch, p & 0x7F, v7], + CtrlKind::PolyPressure(p) => vec![0xA0 | ch, p.min(127), v7], CtrlKind::PitchBend => { let v = (c.value.saturating_add(8192)).clamp(0, 16383); vec![0xE0 | ch, (v & 0x7F) as u8, ((v >> 7) & 0x7F) as u8] diff --git a/crates/midi/tests/smf.rs b/crates/midi/tests/smf.rs index f1861d83..f069be39 100644 --- a/crates/midi/tests/smf.rs +++ b/crates/midi/tests/smf.rs @@ -332,3 +332,21 @@ proptest! { prop_assert_eq!(a, b); } } + +/// #472: out-of-range pitches, release velocities and controller numbers clamp to 127 instead of +/// wrapping into other notes and controllers (128 & 0x7F would be 0). +#[test] +fn writer_clamps_out_of_range_data_bytes() { + for raw in [127u8, 128, 255] { + let mut seq = Sequence::default(); + seq.notes.push(Note { pitch: raw, velocity: 100, release_velocity: raw, channel: 0, start: 0, length: 960 }); + seq.ctrls.push(CtrlEvent { tick: 0, channel: 0, kind: CtrlKind::Cc(raw), value: 64 }); + seq.ctrls.push(CtrlEvent { tick: 0, channel: 0, kind: CtrlKind::PolyPressure(raw), value: 64 }); + let smf = Smf { tracks: vec![SmfTrack { name: String::new(), sequence: seq }], ..Smf::default() }; + let back = read_smf(&write_smf(&smf).unwrap()).unwrap(); + let s = &back.tracks.last().unwrap().sequence; + assert_eq!((s.notes[0].pitch, s.notes[0].release_velocity), (127, 127), "input {raw}"); + let kinds: Vec = s.ctrls.iter().map(|c| c.kind).collect(); + assert!(kinds.contains(&CtrlKind::Cc(127)) && kinds.contains(&CtrlKind::PolyPressure(127)), "input {raw}: {kinds:?}"); + } +} diff --git a/crates/mix/src/lib.rs b/crates/mix/src/lib.rs index e8c4fbad..3d78bde5 100644 --- a/crates/mix/src/lib.rs +++ b/crates/mix/src/lib.rs @@ -515,6 +515,9 @@ impl MixEngine { s.align.clear(); s.align_pre.clear(); s.out_extra.clear(); + for fx in s.clip_fx.values_mut() { + fx.reset(); + } } self.meters.clear(); self.main_meter = StripMeter::default(); @@ -1149,6 +1152,12 @@ impl ClipFx { fn new(sr: f32, ch: usize) -> ClipFx { ClipFx { eq: None, comp: None, gain_db: 0.0, sr, ch } } + /// Clear filter, detector and lookahead history (transport stop / locate). + fn reset(&mut self) { + for p in self.eq.iter_mut().chain(self.comp.iter_mut()) { + p.reset(); + } + } fn make(&self, id: &str) -> Option> { soundcraft_dsp::create(id).map(|mut p| { p.prepare(self.sr, 16_384, self.ch.max(1)); @@ -2902,6 +2911,29 @@ mod tests { assert!(out.iter().flatten().all(|x| *x == 0.0), "old aligned audio after a reset"); } + /// A reset (stop, locate) must clear the cached clip EQ/compressor history too. + #[test] + fn reset_clears_clip_effect_history() { + // Compressor lookahead delay, then EQ high-pass filter state, each primed with DC. + for (param, value) in [("comp.lookahead", 5.0), ("eq.hpf_on", 1.0)] { + let (mut s, t) = session_with_dc(0.5, 9600); + s.edit.click = false; + let cid = s.track(t).unwrap().clips()[0].id.0; + s.edit.values.insert(format!("clip_fx.{cid}.{param}"), value); + let mut out = vec![vec![0.0f32; 512]; 2]; + let mut eng = MixEngine::new(48_000.0, 512); + for b in 0..4 { + eng.render(&s, b * 512, 512, &mut out); + } + eng.reset(); + eng.render(&s, 4096, 512, &mut out); + let mut fresh = MixEngine::new(48_000.0, 512); + let mut expect = vec![vec![0.0f32; 512]; 2]; + fresh.render(&s, 4096, 512, &mut expect); + assert_eq!(out, expect, "{param}: clip effect state survived the reset"); + } + } + fn impulse_at(ch: &[f32]) -> Vec { ch.iter().enumerate().filter(|(_, v)| v.abs() > 1e-4).map(|(i, _)| i).collect() } diff --git a/crates/model/src/automation.rs b/crates/model/src/automation.rs index 3a68b041..a6d3963b 100644 --- a/crates/model/src/automation.rs +++ b/crates/model/src/automation.rs @@ -204,7 +204,8 @@ impl AutomationLane { /// Thin: remove points whose removal changes the curve by less than `tolerance`. pub fn thin(&mut self, tolerance: f32) -> usize { - if self.points.len() < 3 || tolerance.is_nan() { + // Linear thinning would move the transitions of step lanes. + if matches!(self.param, AutoParam::Mute | AutoParam::SendMute(_)) || self.points.len() < 3 || tolerance.is_nan() { return 0; } let tolerance = f64::from(tolerance); @@ -303,6 +304,19 @@ mod tests { } } + #[test] + fn thin_keeps_mute_changes_in_place() { + for param in [AutoParam::Mute, AutoParam::SendMute(0)] { + let mut lane = AutomationLane::new(param); + for (at, value) in [(0, 0.0), (9000, 1.0), (10000, 1.0)] { + lane.set_point(at, value); + } + lane.thin(0.1); + assert_eq!(lane.value_at(8999, 0.0), 0.0); + assert_eq!(lane.value_at(9000, 0.0), 1.0); + } + } + #[test] fn param_ids() { assert_eq!(AutoParam::parse("send_b_level"), Some(AutoParam::SendLevel(1))); diff --git a/crates/model/src/b64.rs b/crates/model/src/b64.rs index 8288c466..1435d313 100644 --- a/crates/model/src/b64.rs +++ b/crates/model/src/b64.rs @@ -37,15 +37,19 @@ fn value(c: u8) -> Option { /// Decodes padded (or unpadded) standard base64. `None` for malformed input or a result larger /// than [`MAX_DECODED_BYTES`]. pub fn decode(text: &str) -> Option> { + if text.bytes().filter(|c| !c.is_ascii_whitespace()).count() > MAX_DECODED_BYTES.div_ceil(3).saturating_mul(4) { + return None; + } let syms: Vec = text.bytes().filter(|c| !c.is_ascii_whitespace()).collect(); let body = syms.strip_suffix(b"==").or_else(|| syms.strip_suffix(b"=")).unwrap_or(&syms); if body.contains(&b'=') || (syms.len() != body.len() && !syms.len().is_multiple_of(4)) || body.len() % 4 == 1 { return None; } - if body.len() / 4 * 3 > MAX_DECODED_BYTES { + let decoded_len = body.len() / 4 * 3 + body.len() % 4 * 3 / 4; + if decoded_len > MAX_DECODED_BYTES { return None; } - let mut out = Vec::with_capacity(body.len() / 4 * 3 + 2); + let mut out = Vec::with_capacity(decoded_len); for chunk in body.chunks(4) { let mut n = 0u32; for (i, &c) in chunk.iter().enumerate() { @@ -81,4 +85,17 @@ mod tests { assert!(decode(bad).is_none(), "{bad}"); } } + + #[test] + fn size_cap_counts_the_partial_quartet() { + for (extra, ok) in [(-1, true), (0, true), (1, false), (2, false)] { + let data = vec![7u8; MAX_DECODED_BYTES.saturating_add_signed(extra)]; + let padded = encode(&data); + let unpadded = padded.trim_end_matches('='); + assert_eq!(decode(&padded).is_some(), ok, "padded, {extra}"); + assert_eq!(decode(unpadded).is_some(), ok, "unpadded, {extra}"); + let spaced = format!("{}\n {}", &padded[..8], &padded[8..]); + assert_eq!(decode(&spaced).is_some(), ok, "whitespace, {extra}"); + } + } } diff --git a/crates/playback/src/lib.rs b/crates/playback/src/lib.rs index e290727d..d940c255 100644 --- a/crates/playback/src/lib.rs +++ b/crates/playback/src/lib.rs @@ -519,54 +519,58 @@ impl Player { if config.buffer_size != cpal::BufferSize::Default { attempts.push(cpal::StreamConfig { buffer_size: cpal::BufferSize::Default, ..config }); } - for config in attempts { - let rate = config.sample_rate.0; - let channels = usize::from(config.channels); - let (tx, rx) = channel(); - let (gtx, grx) = channel(); - let state = AudioState::new(rx, Arc::clone(&shared), Arc::clone(&session), f64::from(rate), gtx); - let fault = Arc::new(Fault::default()); - use cpal::traits::StreamTrait; - let built = match format { - cpal::SampleFormat::I32 => build_output::(&device, &config, state, channels, &fault, &shared), - cpal::SampleFormat::I16 => build_output::(&device, &config, state, channels, &fault, &shared), - _ => build_output::(&device, &config, state, channels, &fault, &shared), - }; - match built { - Ok(stream) => { - if let Err(e) = stream.play() { - log::warn!("audio stream failed to start: {e}"); - } - let buffer = match config.buffer_size { - cpal::BufferSize::Fixed(n) => Some(n), - cpal::BufferSize::Default => None, - }; - log::info!( - "audio device: {name} @ {rate} Hz, {channels} ch, {format}, buffer {}", - buffer.map_or("default".to_string(), |n| n.to_string()) - ); - return Player { - shared, - fault, - #[cfg(not(target_arch = "wasm32"))] - watch: Mutex::new(None), - tx, - side: new_side(grx), - _stream: Some(stream), - device_name: name, - device_rate: rate, - device_channels: config.channels, - device_buffer: buffer, - silent: false, - }; + let opened = first_started( + attempts, + |config: &cpal::StreamConfig| -> Result { + let rate = config.sample_rate.0; + let channels = usize::from(config.channels); + let (tx, rx) = channel(); + let (gtx, grx) = channel(); + let state = AudioState::new(rx, Arc::clone(&shared), Arc::clone(&session), f64::from(rate), gtx); + let fault = Arc::new(Fault::default()); + let stream = match format { + cpal::SampleFormat::I32 => build_output::(&device, config, state, channels, &fault, &shared), + cpal::SampleFormat::I16 => build_output::(&device, config, state, channels, &fault, &shared), + _ => build_output::(&device, config, state, channels, &fault, &shared), + }?; + Ok(Opened { stream, tx, grx, fault }) + }, + |opened| start_stream(&opened.stream), + ); + match opened { + Some((config, Opened { stream, tx, grx, fault })) => { + let rate = config.sample_rate.0; + let channels = usize::from(config.channels); + let buffer = match config.buffer_size { + cpal::BufferSize::Fixed(n) => Some(n), + cpal::BufferSize::Default => None, + }; + log::info!( + "audio device: {name} @ {rate} Hz, {channels} ch, {format}, buffer {}", + buffer.map_or("default".to_string(), |n| n.to_string()) + ); + Player { + shared, + fault, + #[cfg(not(target_arch = "wasm32"))] + watch: Mutex::new(None), + tx, + side: new_side(grx), + _stream: Some(stream), + device_name: name, + device_rate: rate, + device_channels: config.channels, + device_buffer: buffer, + silent: false, } - Err(e) => log::warn!("cannot open audio stream ({:?}): {e}", config.buffer_size), + } + None => { + log::warn!("no usable audio stream; using a silent clock"); + let (tx, rx) = channel(); + let (gtx, grx) = channel(); + Player::silent_clock(shared, tx, rx, want, new_side(grx), gtx) } } - log::warn!("no usable audio stream; using a silent clock"); - let (tx, rx) = channel(); - let (gtx, grx) = channel(); - Player::silent_clock(shared, tx, rx, want, new_side(grx), gtx) } Err(e) => { log::warn!("no audio output: {e}; using a silent clock"); @@ -868,6 +872,44 @@ fn fold_gains(s: &Session) -> [(f32, f32); MAX_CHANNELS] { g } +/// Start a freshly built output stream. `false` when the device refuses to start it, so the caller +/// treats it like a stream that failed to build and tries the next configuration. +fn start_stream(stream: &impl cpal::traits::StreamTrait) -> bool { + match stream.play() { + Ok(()) => true, + Err(e) => { + log::warn!("audio stream failed to start: {e}"); + false + } + } +} + +/// A built output stream and the channels and fault flag its callback shares with the player. +struct Opened { + stream: cpal::Stream, + tx: Sender, + grx: Receiver>>, + fault: Arc, +} + +/// Build and start the attempts in order; the first that builds and starts wins. A build error or +/// a stream that will not start is logged, and the next attempt is tried. `None` when none works. +fn first_started( + attempts: impl IntoIterator, + mut build: impl FnMut(&A) -> Result, + start: impl Fn(&S) -> bool, +) -> Option<(A, S)> { + for attempt in attempts { + match build(&attempt) { + Ok(built) if start(&built) => return Some((attempt, built)), + // `start` has already logged why the stream would not start. + Ok(_) => {} + Err(e) => log::warn!("cannot open audio stream ({attempt:?}): {e}"), + } + } + None +} + /// The output stream: the mix renders f32 into a scratch buffer allocated here (so the callback /// never allocates), converted to the device's sample type. fn build_output( @@ -973,6 +1015,64 @@ fn open_device(want_rate: u32, want_ch: u16, cfg: &AudioConfig) -> Result<(cpal: mod tests { use super::*; + /// A stream that builds; `starts` says whether the device lets it play (it can refuse after a + /// reconnect). + struct Fake { + starts: bool, + } + + impl cpal::traits::StreamTrait for Fake { + fn play(&self) -> Result<(), cpal::PlayStreamError> { + if self.starts { Ok(()) } else { Err(cpal::PlayStreamError::DeviceNotAvailable) } + } + fn pause(&self) -> Result<(), cpal::PauseStreamError> { + Ok(()) + } + } + + #[test] + fn a_stream_that_will_not_start_is_reported_as_failed() { + assert!(!start_stream(&Fake { starts: false })); + assert!(start_stream(&Fake { starts: true })); + } + + #[test] + fn the_first_config_that_starts_wins_and_later_ones_are_not_tried() { + let mut tried = Vec::new(); + let won = first_started( + [1u32, 2, 3], + |&n| { + tried.push(n); + Ok::<_, String>(Fake { starts: n != 1 }) + }, + start_stream, + ); + assert_eq!(won.map(|(n, _)| n), Some(2)); + assert_eq!(tried, vec![1, 2]); + } + + #[test] + fn a_config_that_fails_to_build_falls_through_to_the_next() { + let won = + first_started([1u32, 2], |&n| if n == 1 { Err("no such buffer size".to_string()) } else { Ok(Fake { starts: true }) }, start_stream); + assert_eq!(won.map(|(n, _)| n), Some(2)); + } + + #[test] + fn no_config_that_starts_yields_none_so_the_caller_falls_back_to_a_silent_clock() { + let mut tried = 0; + let won = first_started( + [1u32, 2], + |_| { + tried += 1; + Ok::<_, String>(Fake { starts: false }) + }, + start_stream, + ); + assert!(won.is_none()); + assert_eq!(tried, 2); + } + #[test] fn reusable_meter_snapshot_updates_and_drops_stale_tracks() { let keep = TrackId(1); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 44428206..99e4b503 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -202,6 +202,11 @@ pub fn tree() -> Vec { insert_path(&mut roots, line); } insert_path(&mut roots, "Window > Reset Panel Layout"); + // Not in the observed catalog either; Pro Tools lists it right under Identify Sync Point. + if let Some(clip) = roots.iter_mut().find(|n| n.label == "Clip") { + let at = clip.children.iter().position(|n| n.label == "Identify Sync Point").map_or(clip.children.len(), |i| i + 1); + clip.children.insert(at, MenuNode { label: "Remove Sync Point".into(), path: "Clip > Remove Sync Point".into(), children: Vec::new() }); + } // Not in the incumbent catalog, but the only UI path to an empty MIDI clip (#282). // It creates a clip rather than acting on the selected ones, so it leads the Clip menu. insert_path(&mut roots, "Clip > New MIDI Clip"); diff --git a/crates/ui-egui/tests/command_reachability.rs b/crates/ui-egui/tests/command_reachability.rs index d6907618..797f1d86 100644 --- a/crates/ui-egui/tests/command_reachability.rs +++ b/crates/ui-egui/tests/command_reachability.rs @@ -102,3 +102,21 @@ fn new_midi_clip_is_in_the_clip_menu() { let clip = tree.iter().find(|n| n.label == "Clip").unwrap(); assert_eq!(clip.children[0].label, "New MIDI Clip", "creates rather than edits, so it comes first"); } + +#[test] +fn remove_sync_point_follows_identify_and_is_greyed_out_without_a_point() { + let tree = menus::tree(); + let clip = tree.iter().find(|n| n.label == "Clip").unwrap(); + let labels: Vec<&str> = clip.children.iter().map(|n| n.label.as_str()).collect(); + let at = labels.iter().position(|l| *l == "Identify Sync Point").unwrap(); + assert_eq!(labels.get(at + 1), Some(&"Remove Sync Point")); + let path = &clip.children[at + 1].path; + let id = menus::command_for_path(path, &menus::ui_aliases()); + assert_eq!(id.as_deref(), Some("clip.remove_sync_point")); + + let mut app = soundcraft_ui_egui::SoundApp::new(soundcraft_engine::demo::demo_engine(), None, soundcraft_ui_egui::Services::default()); + app.engine.execute("edit.select", &serde_json::json!({"tracks": ["Kick"], "start": {"seconds": 12.0}})).unwrap(); + assert!(!menus::item_state(&app, path, id.as_deref()).0, "the selected Kick clip has no sync point yet"); + app.engine.execute("clip.identify_sync_point", &serde_json::json!({})).unwrap(); + assert!(menus::item_state(&app, path, id.as_deref()).0); +} diff --git a/xtask/src/version.rs b/xtask/src/version.rs index d9bb7aa5..d469c2e9 100644 --- a/xtask/src/version.rs +++ b/xtask/src/version.rs @@ -46,11 +46,20 @@ pub fn validate(v: &str) -> Result<(), String> { None => (v, None), }; let nums: Vec<&str> = core.split('.').collect(); - if nums.len() != 3 || nums.iter().any(|n| n.is_empty() || !n.bytes().all(|b| b.is_ascii_digit()) || (n.len() > 1 && n.starts_with('0'))) { + // Numeric parts must fit in u64, like the semver crate Cargo uses. + if nums.len() != 3 + || nums.iter().any(|n| !n.bytes().all(|b| b.is_ascii_digit()) || (n.len() > 1 && n.starts_with('0')) || n.parse::().is_err()) + { return bad(); } + // A numeric prerelease identifier has no leading zero (any length is fine); alphanumeric ones may start with `0`. + let bad_id = |id: &str| { + id.is_empty() + || !id.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'-') + || (id.len() > 1 && id.starts_with('0') && id.bytes().all(|b| b.is_ascii_digit())) + }; if let Some(p) = pre - && (p.is_empty() || p.split('.').any(|id| id.is_empty() || !id.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'-'))) + && (p.is_empty() || p.split('.').any(bad_id)) { return bad(); } @@ -91,6 +100,31 @@ fn cargo_update(root: &Path, offline: bool) -> Result<(), String> { if st.success() { Ok(()) } else { Err(format!("cargo update --workspace{} failed", if offline { " --offline" } else { "" })) } } +/// Write atomically: a half-written root manifest breaks every build in the workspace. +fn write_manifest(root: &Path, path: &Path, content: &str) -> Result<(), String> { + let dir = root.join("target"); + std::fs::create_dir_all(&dir).map_err(|e| format!("create {}: {e}", dir.display()))?; + let tmp = dir.join("Cargo.toml.xtask-version"); + std::fs::write(&tmp, content).map_err(|e| format!("write {}: {e}", tmp.display()))?; + std::fs::rename(&tmp, path).map_err(|e| format!("replace {}: {e}", path.display())) +} + +/// Writes version `new` into the manifest at `path` (current text `text`), then runs `update`. +/// If `update` fails the old manifest is put back. Returns the old version. +fn set(root: &Path, path: &Path, text: &str, new: &str, update: impl FnOnce() -> Result<(), String>) -> Result { + let old = read(text)?; + let updated = replace(text, new)?; + write_manifest(root, path, &updated)?; + if let Err(e) = update() { + // Put the old manifest back so a failed update does not leave the workspace changed. + return Err(match write_manifest(root, path, text) { + Ok(()) => format!("{e}; Cargo.toml restored to {old}"), + Err(r) => format!("{e}; could not restore Cargo.toml: {r}"), + }); + } + Ok(old) +} + /// `cargo xtask version [set X.Y.Z[-pre]]`; `args` are the words after `version`. pub fn run(args: &[String]) -> Result<(), String> { let root = root(); @@ -103,18 +137,8 @@ pub fn run(args: &[String]) -> Result<(), String> { Ok(()) } ["set", new] => { - let old = read(&text)?; - let updated = replace(&text, new)?; - // Write atomically: a half-written root manifest breaks every build in the workspace. - let dir = root.join("target"); - std::fs::create_dir_all(&dir).map_err(|e| format!("create {}: {e}", dir.display()))?; - let tmp = dir.join("Cargo.toml.xtask-version"); - std::fs::write(&tmp, updated).map_err(|e| format!("write {}: {e}", tmp.display()))?; - std::fs::rename(&tmp, &path).map_err(|e| format!("replace {}: {e}", path.display()))?; // Refresh the workspace members' entries in Cargo.lock (no dependency upgrades). - if cargo_update(&root, true).is_err() { - cargo_update(&root, false)?; - } + let old = set(&root, &path, &text, new, || cargo_update(&root, true).or_else(|_| cargo_update(&root, false)))?; println!("version: {old} -> {new}"); Ok(()) } @@ -160,6 +184,31 @@ mod tests { assert!(replace(MANIFEST, "nope").is_err()); } + #[test] + fn rejects_what_cargo_rejects() { + for bad in ["1.0.0-01", "1.0.0-rc.01", "1.0.0-0.00", "1.0.0-rc.1.007", "99999999999999999999.0.0", "1.18446744073709551616.0"] { + assert!(validate(bad).is_err(), "{bad}"); + assert!(replace(MANIFEST, bad).is_err(), "{bad}"); + } + for ok in ["1.0.0-0", "1.0.0-rc.0", "1.0.0-0a", "1.0.0-01a", "1.0.0-99999999999999999999", "18446744073709551615.0.0"] { + assert!(validate(ok).is_ok(), "{ok}"); + } + } + + #[test] + fn a_failed_update_restores_the_manifest() { + let root = std::env::temp_dir().join(format!("xtask-version-restore-{}", std::process::id())); + std::fs::create_dir_all(&root).unwrap(); + let path = root.join("Cargo.toml"); + std::fs::write(&path, MANIFEST).unwrap(); + let err = set(&root, &path, MANIFEST, "2.0.0", || Err("cargo update failed".into())).unwrap_err(); + assert!(err.contains("restored to 0.1.0"), "{err}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), MANIFEST); + assert_eq!(set(&root, &path, MANIFEST, "2.0.0", || Ok(())).unwrap(), "0.1.0"); + assert_eq!(read(&std::fs::read_to_string(&path).unwrap()).unwrap(), "2.0.0"); + std::fs::remove_dir_all(&root).unwrap(); + } + #[test] fn the_real_manifest_has_a_valid_version() { let text = std::fs::read_to_string(root().join("Cargo.toml")).unwrap();