Repository navigation
feat: play one phrase at a time, repeatedly, with room to write (M11, 3/4) - #37
Conversation
M11, third of four. The transport half of listening practice. A phrase stops at its own end, set through `forwardPlaybackEndTime` rather than watched for. The time observer reports four times a second, so stopping by hand overshoots by up to 250 ms — audible, and enough to leak the first syllable of the next phrase to a learner who is about to be asked to transcribe it. The player stops itself exactly and reports it as the end of the item, which is the path a finished track already took. The drill plays a phrase a given number of times with a gap after each. The gap is the point rather than a detail: it is where the learner says the phrase back or writes it down, and it is what separates a drill from a loop. Advancing to the next phrase is deliberately not done here — which phrase comes next depends on the text and on what the reader has already passed, and neither belongs in a player. Writing the tests found a real one. `pause()` guarded on `isPlaying`, and during the gap between repeats nothing is playing, so the guard returned before cancelling the pending repeat: press pause, hear silence, hear the phrase start again. Cancelling now happens before the guard, because pausing has to mean stop whether or not sound is coming out at that instant. **A correction.** `audioTimePitchAlgorithm` was written up in the plan as a latent M7 defect — that a rate change would deepen the voice. Measured here rather than assumed: `.timeDomain` is already the default on macOS 26, so M7 never shipped a chipmunk voice and there was nothing to repair. It is set explicitly anyway, and the comment now says why that is pinning a behaviour rather than fixing one: the documented default has been `.spectral` in the past, and this app also runs on macOS 14 and iOS 17. One test was flaky as first written — a 500 ms sleep for a 300 ms gap, which passes here and fails under parallel execution or on a slower machine. It polls to a generous deadline now: the claim is that the repeat happens, not that it happens within half a second. Five mutants planted and killed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoAdd phrase playback with timed repetition gaps
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
CI failed the gap test again, this time despite polling to a five-second deadline — so it was never slack to be absorbed. A repeat is scheduled on a task that suspends twice before it plays, and "wait a bit and look" is a guess whether the waiting is a sleep or a poll. The gap is now injected. `PlaybackModel` takes a `wait`, defaulting to `Task.sleep`, so a test can let it return at once and assert what was asked for and what followed it rather than how long it took. And a repeat currently waiting one out can be awaited directly instead of yielded at. The suite went from 0.85 seconds with two fixed sleeps in it to 0.001 seconds with no sleeping at all, and gained a test that nothing waits when no gap was asked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven review findings, all the same root cause: `cancelDrill()` was treated as something called from three places rather than as an invariant. A repeat waiting out its gap is a command still pending, and the reader can change their mind in between — so every transport event that changes what should be playing now cancels it. The worst was the headphones. `handle(.outputGone)` returned early because nothing was playing during the gap, so the repeat it should have stopped started a moment later through whatever speaker was left. The same early return let an interruption do it. Both cancel before their guard now, as `pause()` already did. Loading another recording left the old track's drill scheduled, so it would wake against the new item, seek it to the previous phrase's bounds and start it there — and the finish that followed would be handled as a phrase pass of a phrase from another text. Pressing play during the gap started the engine and left the repeat to restart the phrase a moment later. Seeking during the gap was worse: the repeat dragged the playhead back and played the old phrase over the reader. Seeking outside the phrase now leaves phrase playback altogether, rather than keeping a stop time that would halt them somewhere unrelated. The gap is now taken after the last pass too. It is the interval the learner writes in, and skipping it on the final pass meant a caller advancing from `onSegmentFinished` moved on the instant the audio stopped. And the repeat is scheduled before the state callback runs, not after, so a `pause()` issued from inside that callback finds a task to cancel. Writing the test for that last one found a hang of my own making: the early return in `pause()` announced a state change, so a listener that pauses in response to one called its own pause for ever. It announces nothing now — nothing observable had changed. One guard is kept although mutation testing shows it unreachable: a completion is reported only if the phrase is still the current one. Every path that changes the segment also cancels the task, so cancellation covers it today — but `segment` has an internal setter, and the cost of being wrong is a listener told "finished" about a phrase the reader has already left. The comment says exactly that rather than implying it is load-bearing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All seven addressed in 1 — headphones, the worst of them. 2 — loading another recording left the old drill scheduled: it would wake against the new item, seek it to the previous phrase's bounds and start it there, and the finish would be handled as a phrase pass from another text. 4, 5 — play and seek during the gap. Play started the engine and left the repeat to restart the phrase; seek was worse, the repeat dragging the playhead back and playing the old phrase over the reader. Seeking outside the phrase now leaves phrase playback entirely, rather than keeping a stop time that would halt them somewhere unrelated. 3 — the gap now follows the last pass too. It is the interval the learner writes in; skipping it on the final pass meant a caller advancing from 6 — the repeat is scheduled before the state callback, not after, so a Two things writing the tests foundA hang of my own making. The early return in Two mutants survived and both were informative. The seek test seeked outside the phrase, which clears the segment, so the pending repeat was harmlessly skipped whether or not it was cancelled — the test proved nothing about cancellation. Seeking inside is the case that needs it, and that is what it does now. The other survivor is finding 7's guard: completion is reported only if the phrase is still current. Every path that changes the segment also cancels the task, so cancellation already covers it and the guard is unreachable today. I have kept it, because 465 core and 68 playback tests. 🤖 Generated with Claude Code |
1 participant
Third of four for M11. The transport half of listening practice.
A phrase stops where the phrase stops
Through
AVPlayerItem.forwardPlaybackEndTime, not by watching the clock. The time observer reports four times a second, so stopping by hand overshoots by up to 250 ms — audible, and enough to leak the first syllable of the next phrase to a learner who is about to be asked to transcribe it. The player stops itself exactly and reports it as the end of the item, which is the path a finished track already took.The drill
Play a phrase N times with a gap after each. The gap is the point rather than a detail — it is where the learner says the phrase back or writes it down, and it is what separates a drill from a loop. WorkAudioBook spells the same two settings out as "repeat phrase" and "pause between phrases".
Advancing to the next phrase is deliberately not done here. Which phrase comes next depends on the text and on what the reader has already passed, and neither belongs in a player.
onSegmentFinishedreports; PR 4 decides.Writing the tests found a real bug
pause()guarded onisPlaying. During the gap between repeats nothing is playing, so the guard returned before cancelling the pending repeat: press pause, hear silence, hear the phrase start again a moment later. Cancelling now happens before the guard, because pausing has to mean stop whether or not sound is coming out at that instant.A correction to the plan
audioTimePitchAlgorithmwas written up as a latent M7 defect — that a rate change would resample and deepen the voice at 0.75×. Measured rather than assumed:.timeDomainis already the default, so M7 never shipped a chipmunk voice and there was nothing to repair.It is set explicitly anyway, and the comment now says honestly that this is pinning a behaviour rather than fixing one: the documented default has been
.spectralin the past, and this app also runs on macOS 14 and iOS 17 where it may differ. The plan has been corrected too.Testing note
One test was flaky as first written — a 500 ms sleep for a 300 ms gap, which passes on this machine and fails under parallel execution or on a slower runner. It polls to a generous deadline now: the claim is that the repeat happens, not that it happens within half a second. The one genuinely negative assertion (pausing stops it) still waits, because "nothing happened" can only be established by waiting.
465 core and 58 playback tests. Five mutants planted and killed. Both app targets built locally before pushing.
🤖 Generated with Claude Code