Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 89 additions & 5 deletions src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, std::sync::Arc<crate::audio::Envelope>>,
Expand Down Expand Up @@ -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(),
}
}
Expand Down Expand Up @@ -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(),
}
}
Expand Down Expand Up @@ -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();
}
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1646,6 +1670,7 @@ mod tests {
}));
let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished {
sound_id: "abc123".into(),
generation: 0,
}));
assert!(app.playing().is_none());
}
Expand Down Expand Up @@ -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"));
}
Expand Down Expand Up @@ -1765,13 +1791,15 @@ mod tests {
},
AudioEvent::PlaybackFinished {
sound_id: "a".into(),
generation: 0,
},
AudioEvent::PlaybackStarted {
sound_id: "b".into(),
},
AudioEvent::Progress(0.25),
AudioEvent::PlaybackFinished {
sound_id: "b".into(),
generation: 0,
},
AudioEvent::PlaybackStarted {
sound_id: "c".into(),
Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
Expand Down Expand Up @@ -1995,6 +2078,7 @@ mod tests {

let _ = app.update(Message::AudioEvent(AudioEvent::PlaybackFinished {
sound_id: "old".into(),
generation: 0,
}));

assert!(
Expand Down
64 changes: 60 additions & 4 deletions src/audio/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ pub enum AudioCommand {
samples: Arc<Vec<f32>>,
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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -156,6 +165,9 @@ fn query_default_source_name() -> Option<String> {

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<RefCell<PlaybackState>>,
monitor_state: Rc<RefCell<PlaybackState>>,
_sink_stream: playback::PlaybackStream,
Expand Down Expand Up @@ -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,
});
}
}
Expand Down Expand Up @@ -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,
});
}
}
Expand Down Expand Up @@ -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<Vec<f32>>,
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,
});
}

Expand Down Expand Up @@ -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;
}
};
Expand All @@ -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,
Expand Down
7 changes: 6 additions & 1 deletion tests/pipewire_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
}

Expand Down Expand Up @@ -249,6 +249,7 @@ fn sink_stream_reaches_virtual_sink() {
samples,
sample_rate: 48000,
channels: 2,
generation: 1,
});

expect_started(&handle, "routing-test");
Expand Down Expand Up @@ -280,6 +281,7 @@ fn audio_pipeline_end_to_end() {
samples,
sample_rate: 48000,
channels: 2,
generation: 1,
});

expect_started(&handle, "e2e-test");
Expand Down Expand Up @@ -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");
Expand All @@ -369,6 +372,7 @@ fn stop_command_halts_playback() {
samples,
sample_rate: 48000,
channels: 2,
generation: 1,
});

expect_started(&handle, "long-sound");
Expand All @@ -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));
Expand Down