diff --git a/src/app.rs b/src/app.rs index bd10512..82cb14d 100644 --- a/src/app.rs +++ b/src/app.rs @@ -195,6 +195,11 @@ pub struct HonkHonk { /// Frame-interpolated playhead position fed to the now-playing view and the /// waveform cache-sync. Distinct from `progress` (the raw 10 Hz anchor). display_progress: f32, + /// Monotonic counter bumped on every play dispatch. Stamped onto the `Play` + /// command and echoed back on `PlaybackFinished`, so the app can tell the + /// genuine end of the current voice from the stale `Finished` emitted for a + /// voice that was superseded by re-pressing the same sound (#149). + play_generation: u64, /// Per-sound peak-amplitude envelopes for the now-playing waveform, computed /// once at decode and reused across frames (#138, PR-B). Session-lifetime. waveform_cache: std::collections::HashMap>, @@ -362,6 +367,7 @@ impl HonkHonk { now_playing: crate::ui::now_playing::NowPlaying::default(), playhead: None, display_progress: 0.0, + play_generation: 0, waveform_cache: std::collections::HashMap::new(), } } @@ -410,6 +416,7 @@ impl HonkHonk { now_playing: crate::ui::now_playing::NowPlaying::default(), playhead: None, display_progress: 0.0, + play_generation: 0, waveform_cache: std::collections::HashMap::new(), } } @@ -579,11 +586,21 @@ impl HonkHonk { self.playing = Some(sound_id); } } - AudioEvent::PlaybackFinished { sound_id } => { - // Only clear the highlight if this event refers to the - // sound we are showing — a Finished for an already- - // replaced sound must not blank a newer press (#111). - if self.playing.as_deref() == Some(sound_id.as_str()) { + AudioEvent::PlaybackFinished { + sound_id, + generation, + } => { + // Clear only when this Finished is for the sound we are + // showing AND belongs to the current play. The sound_id + // check keeps a Finished for an already-replaced sound + // from blanking a newer press (#111); the generation + // check additionally ignores the stale Finished emitted + // for a same-sound voice that was superseded by an + // immediate re-press, so its fresh playhead survives + // (#149). + if self.playing.as_deref() == Some(sound_id.as_str()) + && generation == self.play_generation + { self.clear_playback_state(); } } @@ -1127,6 +1144,12 @@ impl HonkHonk { return; } }; + // Bump the play generation so this dispatch is distinguishable from any + // voice it supersedes. The matching `PlaybackFinished` echoes it back; a + // stale `Finished` for a re-pressed (superseded) voice carries an older + // generation and must not tear down this fresh playhead (#149). + self.play_generation = self.play_generation.wrapping_add(1); + let generation = self.play_generation; // Start the smooth-playhead clock; duration is exact from the decoded // PCM (avoids depending on the lazy duration scan). PR-B inserts the // waveform envelope here too, from `decoded.samples` before per-vol. @@ -1163,6 +1186,7 @@ impl HonkHonk { samples, sample_rate: decoded.sample_rate, channels: decoded.channels, + generation, }); // Highlight the tile immediately rather than waiting for the // engine's PlaybackStarted to round-trip through the event @@ -1646,6 +1670,7 @@ mod tests { })); let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { sound_id: "abc123".into(), + generation: 0, })); assert!(app.playing().is_none()); } @@ -1721,6 +1746,7 @@ mod tests { // highlight of the sound that superseded it (issue #111). let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { sound_id: "older".into(), + generation: 0, })); assert_eq!(app.playing(), Some("newer")); } @@ -1765,6 +1791,7 @@ mod tests { }, AudioEvent::PlaybackFinished { sound_id: "a".into(), + generation: 0, }, AudioEvent::PlaybackStarted { sound_id: "b".into(), @@ -1772,6 +1799,7 @@ mod tests { AudioEvent::Progress(0.25), AudioEvent::PlaybackFinished { sound_id: "b".into(), + generation: 0, }, AudioEvent::PlaybackStarted { sound_id: "c".into(), @@ -1894,11 +1922,65 @@ mod tests { let _ = app.update(Message::AudioEvent(AudioEvent::Progress(0.8))); let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { sound_id: "test".into(), + generation: 0, })); assert!(app.playhead.is_none()); assert_eq!(app.display_progress, 0.0); } + #[test] + fn re_pressing_same_sound_keeps_playhead_alive() { + // Re-pressing the SAME tile while it is still playing must re-trigger the + // playhead. The engine replaces the active voice and emits a + // `PlaybackFinished` for the *displaced* voice carrying the SAME + // `sound_id`; the app must not mistake that stale event for a genuine end + // and tear down the freshly-created playhead, freezing it at 0 (#149). + let mut app = HonkHonk::new_for_test(); + let (handle, _evt_tx) = crate::audio::test_handle(); + app.audio = Some(handle); + + let dir = tempfile::tempdir().expect("tempdir"); + let wav_path = dir.path().join("honk.wav"); + write_test_wav(&wav_path); + app.sounds = vec![SoundEntry { + id: "wav1".into(), + name: "Honk".into(), + path: wav_path, + format: crate::state::AudioFormat::Wav, + duration_ms: Some(100), + category: "Test".into(), + }]; + + // First press → playhead created for the first voice (generation 1). + let _ = app.update(Message::PlaySound("wav1".into())); + // Second press re-triggers the same sound → a fresh playhead (generation 2). + let _ = app.update(Message::PlaySound("wav1".into())); + assert!( + app.playhead.is_some(), + "re-press should create a fresh playhead" + ); + + // The displaced first voice's Finished (older generation) arrives on the + // next drain — it must NOT clear the re-triggered playhead. + let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { + sound_id: "wav1".into(), + generation: 1, + })); + assert!( + app.playhead.is_some(), + "stale displaced Finished must not clear the re-triggered playhead" + ); + assert_eq!(app.playing(), Some("wav1")); + + // The genuine end of the current voice (matching generation) still clears. + let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { + sound_id: "wav1".into(), + generation: 2, + })); + assert!(app.playhead.is_none(), "genuine end clears the playhead"); + assert_eq!(app.playing(), None); + } + #[test] fn progress_event_updates_progress() { let mut app = HonkHonk::new_for_test(); @@ -1915,6 +1997,7 @@ mod tests { let _ = app.update(Message::AudioEvent(AudioEvent::Progress(0.8))); let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { sound_id: "test".into(), + generation: 0, })); assert!((app.progress() - 0.0).abs() < f32::EPSILON); } @@ -1995,6 +2078,7 @@ mod tests { let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished { sound_id: "old".into(), + generation: 0, })); assert!( diff --git a/src/audio/engine.rs b/src/audio/engine.rs index 7d3645e..f586cb7 100644 --- a/src/audio/engine.rs +++ b/src/audio/engine.rs @@ -22,6 +22,11 @@ pub enum AudioCommand { samples: Arc>, sample_rate: u32, channels: u16, + /// Monotonic token identifying this specific play. Echoed back on the + /// matching `PlaybackFinished` so the app can tell a genuine end from the + /// stale `Finished` emitted for a voice that was immediately superseded by + /// a re-press of the same sound (#149). + generation: u64, }, Stop, SetVolume(f32), @@ -58,6 +63,10 @@ pub enum AudioEvent { }, PlaybackFinished { sound_id: String, + /// Echoes the `generation` of the `Play` this voice came from, so a stale + /// `Finished` for a superseded voice can be distinguished from a genuine + /// end (#149). + generation: u64, }, Progress(f32), Error(EngineErrorEvent), @@ -156,6 +165,9 @@ fn query_default_source_name() -> Option { struct ActivePlayback { sound_id: String, + /// The `generation` of the `Play` command that started this voice; echoed on + /// its `PlaybackFinished` so the app can ignore a stale end (#149). + generation: u64, sink_state: Rc>, monitor_state: Rc>, _sink_stream: playback::PlaybackStream, @@ -301,6 +313,7 @@ fn setup_completion_timer( if let Some(ap) = active_timer.borrow_mut().take() { let _ = evt_tx_timer.send(AudioEvent::PlaybackFinished { sound_id: ap.sound_id, + generation: ap.generation, }); } } @@ -490,14 +503,25 @@ fn run_engine( samples, sample_rate, channels, + generation, } => { - handle_play(&ctx, sound_id, samples, sample_rate, channels); + handle_play( + &ctx, + PlayRequest { + sound_id, + samples, + sample_rate, + channels, + generation, + }, + ); } AudioCommand::Stop => { let prev = ctx.active.borrow_mut().take(); if let Some(ap) = prev { let _ = ctx.evt_tx.send(AudioEvent::PlaybackFinished { sound_id: ap.sound_id, + generation: ap.generation, }); } } @@ -615,24 +639,47 @@ fn rebuild_monitor_stream(ctx: &EngineCtx) { } } -fn handle_play( - ctx: &EngineCtx, +/// Decoded PCM plus identity for a single play, bundled so `handle_play` stays +/// within the argument-count lint as fields accrete (e.g. `generation`, #149). +struct PlayRequest { sound_id: String, samples: Arc>, sample_rate: u32, channels: u16, -) { + generation: u64, +} + +fn handle_play(ctx: &EngineCtx, req: PlayRequest) { + let PlayRequest { + sound_id, + samples, + sample_rate, + channels, + generation, + } = req; if ctx.registry_sink_id.get().is_none() { let _ = ctx.evt_tx.send(AudioEvent::Error( EngineErrorEvent::VirtualSinkNotRegistered, )); + // Uphold the invariant that every Play yields one matching-generation + // Finished. Without it the app's optimistic playing/playhead state for + // this generation would stick forever, since the generation guard + // ignores any other Finished (#149). + let _ = ctx.evt_tx.send(AudioEvent::PlaybackFinished { + sound_id, + generation, + }); return; } let prev = ctx.active.borrow_mut().take(); if let Some(ap) = prev { + // The displaced voice carries its OWN (older) generation, so the app can + // tell this "superseded" Finished from the genuine end of the new voice + // when a sound is re-pressed while still playing (#149). let _ = ctx.evt_tx.send(AudioEvent::PlaybackFinished { sound_id: ap.sound_id, + generation: ap.generation, }); } @@ -671,6 +718,14 @@ fn handle_play( .send(AudioEvent::Error(EngineErrorEvent::SinkStreamCreation { detail: e.to_string(), })); + // Same invariant as the sink-not-registered path: signal that this + // generation produced no playback so the app clears it rather than + // getting stuck on a phantom playing state (#149). The displaced + // voice (if any) was already taken above and dropped. + let _ = ctx.evt_tx.send(AudioEvent::PlaybackFinished { + sound_id, + generation, + }); return; } }; @@ -688,6 +743,7 @@ fn handle_play( }; *ctx.active.borrow_mut() = Some(ActivePlayback { sound_id: sound_id.clone(), + generation, sink_state, monitor_state: mon_state, _sink_stream: sink_s, diff --git a/tests/pipewire_integration.rs b/tests/pipewire_integration.rs index 254f15e..a99ca93 100644 --- a/tests/pipewire_integration.rs +++ b/tests/pipewire_integration.rs @@ -131,7 +131,7 @@ fn expect_finished(handle: &honkhonk::audio::AudioHandle, expected: &str, timeou handle, timeout, &label, - |event| matches!(event, honkhonk::audio::AudioEvent::PlaybackFinished { sound_id } if sound_id == expected), + |event| matches!(event, honkhonk::audio::AudioEvent::PlaybackFinished { sound_id, .. } if sound_id == expected), ); } @@ -249,6 +249,7 @@ fn sink_stream_reaches_virtual_sink() { samples, sample_rate: 48000, channels: 2, + generation: 1, }); expect_started(&handle, "routing-test"); @@ -280,6 +281,7 @@ fn audio_pipeline_end_to_end() { samples, sample_rate: 48000, channels: 2, + generation: 1, }); expect_started(&handle, "e2e-test"); @@ -344,6 +346,7 @@ fn play_sound_emits_started_and_finished_events() { samples, sample_rate: decoded.sample_rate, channels: decoded.channels, + generation: 1, }); expect_started(&handle, "test-sine"); @@ -369,6 +372,7 @@ fn stop_command_halts_playback() { samples, sample_rate: 48000, channels: 2, + generation: 1, }); expect_started(&handle, "long-sound"); @@ -378,6 +382,7 @@ fn stop_command_halts_playback() { samples: replacement, sample_rate: 48000, channels: 2, + generation: 2, }); expect_finished(&handle, "long-sound", Duration::from_secs(5));