From cc21036ac0af02288b8b7e370a2cee5e372810d9 Mon Sep 17 00:00:00 2001 From: revtex Date: Fri, 14 Aug 2026 22:41:50 -0400 Subject: [PATCH] Start recording when playback starts, not only when the track changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing record with Spotify paused and then pressing play recorded nothing. The level meter moved, the counter ran and the page named the song, but no file appeared until the user pressed stop and start again. Being played is half of what makes a track recordable, and the admission check ran only on a track change. Track equality ignores the play state on purpose, so that a pause mid-song does not read as a new song — which means a track that starts playing raises no track change at all. The session had already passed it over once as not recordable, and went on waiting for something that had already happened. The predecessor never met this: it blocked on Spotify producing audio before it began watching. Offstream starts listening the moment record is pressed, which is the better behaviour and is worth keeping. The admission check moves into Consider, called from the track-changed handler as before and now from the play-state handler too. The resume path is narrower than the track-changed one, deliberately: - only when nothing is being recorded, so a pause mid-song leaves the running recorder alone rather than starting a second one on top of it; - not once the recording timer has elapsed, since the session is winding down and "recording timer elapsed" would be a lie about a resume; - only for the track already showing. One poll can see both a new song and a change of play state — starting with Spotify already playing is exactly that — and the track change is raised straight after, with the stop-the-outgoing- recorder handling this path does not have. Admitting from both would start a recorder for the other to tear down, and discard the fragment as too short. The play-state event now carries the track it observed. The poller does not store the observation until the poll finishes, so a handler reading its current track gets the previous one, whose play state is the one that just stopped being true. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 13 +++++ .../Recording/RecordingSession.cs | 54 +++++++++++++++++- src/Offstream.Core/Spotify/SpotifyPoller.cs | 2 +- .../Spotify/SpotifyPollerEvents.cs | 11 +++- .../Recording/RecordingSessionTests.cs | 56 +++++++++++++++++++ 5 files changed, 132 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e565045..9c46afb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -274,6 +274,19 @@ phase plan these entries follow. ### Fixed +- **Pressing record with Spotify paused, then pressing play, recorded nothing.** The level meter + moved, the counter ran and the page named the song, but no file was ever written until the user + pressed stop and start again. Being played is half of what makes a track recordable, and that + check ran only when the track changed — while two observations of the same song count as the + same track whether or not it is playing, by design, so that a pause mid-song does not read as a + new one. A song that started playing therefore raised nothing the session was listening for: it + had already passed the track over once as not recordable, and went on waiting for a change that + had happened. The predecessor never met this because it waited for Spotify to produce audio + before it began watching at all; Offstream starts listening the moment record is pressed, which + is worth keeping. The check now also runs when playback starts, for the track already showing + and only when nothing is being recorded — a pause mid-song still leaves the recorder alone, and + starting with music already playing still produces exactly one recording rather than one that is + immediately torn down and discarded as too short. - **Every track after the first showed nothing but the artist and title.** The album, cover art and destination arrived a second into each song and were gone a moment later, on every track except the first one of a session — while the files themselves were tagged correctly, so the diff --git a/src/Offstream.Core/Recording/RecordingSession.cs b/src/Offstream.Core/Recording/RecordingSession.cs index 1e9f60c..b007a5a 100644 --- a/src/Offstream.Core/Recording/RecordingSession.cs +++ b/src/Offstream.Core/Recording/RecordingSession.cs @@ -419,11 +419,49 @@ private void OnTrackTimeChanged(object? sender, TrackTimeChangedEventArgs e) TimeSpan.FromSeconds(e.TrackTimeSeconds)); } - private void OnPlayStateChanged(object? sender, PlayStateChangedEventArgs e) => + /// + /// Playback started or stopped. Starting is also a chance to record what is already on. + /// + /// + /// + /// A track is only admitted while it is playing — + /// is half of — and the admission check used to + /// run in alone. ignores the play + /// state, so a song that goes from paused to playing is not a new track and raised nothing + /// but this event. Pressing record while Spotify was paused and then pressing play therefore + /// left the session waiting for a track change that had already happened: the meter moved, + /// the counter ran, and nothing was ever written until the user stopped and started again. + /// The predecessor never met this because it blocked on Spotify producing audio before it + /// began watching at all; Offstream starts listening immediately, and pays for it here. + /// + /// + /// Only when nothing is being recorded. A pause mid-song leaves the recorder alone — what + /// Spotify stops sending is silence, and the trim handles it — so a resume with a recorder + /// still running is the ordinary case and must not start a second one. + /// + /// + private void OnPlayStateChanged(object? sender, PlayStateChangedEventArgs e) + { Report( e.Playing ? RecordingStage.Recording : RecordingStage.WaitingForTrack, message: e.Playing ? null : "Playback paused."); + if (!e.Playing || CurrentTrack is not null) return; + + // Not when the recording timer has already elapsed: the session is winding down, and the + // next thing it should do is stop rather than take on another track. + if (_stopAfterCurrentTrack) return; + + // Only for the track already known. A single poll can see both a new song and a change of + // play state — starting from a stopped Spotify does exactly that — and the track change + // is raised straight after this, with the stop-the-outgoing-recorder handling that this + // path deliberately lacks. Admitting the same track from both would start a recorder only + // for the other to tear it down a moment later and discard the fragment as too short. + // The poller has not yet stored this observation, so its current track is the previous + // one; equality ignores the play state, which is precisely the comparison wanted here. + if (e.Track.Equals(_poller.CurrentTrack)) Consider(e.Track); + } + /// /// The centre of the session: one track ends, the next begins. /// @@ -449,8 +487,20 @@ private void OnTrackChanged(object? sender, TrackChangedEventArgs e) return; } - var track = e.NewTrack; + Consider(e.NewTrack); + } + /// + /// Records if the settings allow it, and says why when they do not. + /// + /// + /// Shared by the two moments a track can become recordable: it changed, or it started + /// playing. Everything specific to a track ending — stopping the outgoing recorder, honouring + /// a recording timer that elapsed — stays with , because neither + /// is true of a song resuming. + /// + private void Consider(Track track) + { if (!_policy.IsTypeAllowed(track)) { Report(RecordingStage.WaitingForTrack, track, message: DescribeSkipped(track)); diff --git a/src/Offstream.Core/Spotify/SpotifyPoller.cs b/src/Offstream.Core/Spotify/SpotifyPoller.cs index e2ba1fc..eeccc76 100644 --- a/src/Offstream.Core/Spotify/SpotifyPoller.cs +++ b/src/Offstream.Core/Spotify/SpotifyPoller.cs @@ -124,7 +124,7 @@ public async Task PollOnceAsync(CancellationToken cancellationToken = default) if (_playing) ResumeClock(); else PauseClock(); - PlayStateChanged?.Invoke(this, new PlayStateChangedEventArgs(newest.Playing)); + PlayStateChanged?.Invoke(this, new PlayStateChangedEventArgs(newest.Playing, newest)); } var isSameTrack = newest.Equals(previous); diff --git a/src/Offstream.Core/Spotify/SpotifyPollerEvents.cs b/src/Offstream.Core/Spotify/SpotifyPollerEvents.cs index 1c67392..abb59d8 100644 --- a/src/Offstream.Core/Spotify/SpotifyPollerEvents.cs +++ b/src/Offstream.Core/Spotify/SpotifyPollerEvents.cs @@ -12,9 +12,18 @@ public sealed class TrackChangedEventArgs(Track? oldTrack, Track newTrack) : Eve } /// Playback started or stopped. -public sealed class PlayStateChangedEventArgs(bool playing) : EventArgs +/// Whether Spotify is playing now. +/// +/// What it is playing, as this observation saw it. Carried on the event because +/// is not updated until the poll finishes — a handler +/// reading it here would get the previous observation, whose play state is the one that just +/// stopped being true. +/// +public sealed class PlayStateChangedEventArgs(bool playing, Track track) : EventArgs { public bool Playing { get; } = playing; + + public Track Track { get; } = track; } /// The elapsed position within the current track changed. diff --git a/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs b/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs index 667d3ae..238335f 100644 --- a/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs +++ b/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs @@ -209,6 +209,10 @@ public Harness( public static Track Playing(string artist, string title) => new() { Artist = artist, Title = title, Playing = true }; + /// The same song, showing in Spotify but stopped. + public static Track Paused(string artist, string title) => + new() { Artist = artist, Title = title, Playing = false }; + /// /// Changes what Spotify reports. The session owns the poller, so the running poll loop /// picks this up on its own — driving PollOnceAsync by hand here would race with it. @@ -746,6 +750,58 @@ await WaitFor( r => r.ConcernsNowPlaying && r.Track is not null && r.Track != r.NowPlaying); } + /// + /// Pressing record with Spotify paused, then pressing play, has to start recording. + /// + /// + /// Playing is half of what makes a track recordable, and the check ran only when the track + /// changed. Since ignores the play state, a song that starts + /// playing is not a new track — so the session skipped it once as not recordable and then + /// waited for a change that never came. What the user saw was a level meter moving, a counter + /// running, and no file, until they stopped and started again. + /// + [Fact] + public async Task Session_WhenPlaybackStartsOnTheTrackAlreadyShowing_RecordsIt() + { + await using var harness = new Harness(); + + harness.Session.Start(); + harness.Play(Harness.Paused("Artist", "Title")); + + await WaitFor( + () => harness.Reports.Any(r => r.Message?.Contains("Not a recordable track", StringComparison.Ordinal) == true), + "the paused track to be passed over"); + + Assert.Null(harness.Session.CurrentTrack); + + harness.Play(Harness.Playing("Artist", "Title")); + + await WaitFor( + () => harness.Session.CurrentTrack?.Title == "Title", + "the recorder to start once playback begins"); + } + + /// + /// And exactly one recorder for it. Starting with Spotify already playing is a single poll + /// that sees both a new track and a change of play state, and both used to be a reason to + /// start — the second one tearing down the first and discarding the fragment as too short. + /// + [Fact] + public async Task Session_WhenPlaybackIsAlreadyUnderWay_StartsOneRecorder() + { + await using var harness = new Harness(); + + harness.Session.Start(); + + await RecordTrackAsync(harness, Harness.Playing("Artist", "Title")); + + await harness.Session.StopAsync(); + + await WaitFor(() => !harness.Recorded.IsEmpty, "the recording to finish"); + + Assert.Single(harness.Recorded); + } + [Fact] public async Task Level_MeasuresWhatCaptureDelivers() {