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() {