diff --git a/CHANGELOG.md b/CHANGELOG.md index 219ea20..b614c6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,28 @@ phase plan these entries follow. ## [Unreleased] +### Added + +- **Offstream can tell Spotify to move on from a track it already has.** Keeping the file on disk + was only ever half the answer: Offstream declined to record the song and Spotify played it to + nobody for three minutes, so working through a playlist that is mostly recorded meant sitting + through the parts that already are. "Also tell Spotify to move on", beside the existing-file + setting on the Advanced page, sends a skip through the Windows media transport controls — the + same command as the keyboard's next-track key, so it needs no account, no scope and no Premium + subscription, and it reaches Spotify while it is minimised to the tray. Off by default, greyed + out under the two policies that record the file again, and asked exactly once per track, because + Spotify goes on reporting the outgoing song for a moment after it takes the command and a second + skip would land on a song nobody had recorded. It is asked twice per track, though: once + immediately, and again when the metadata lookup lands, since a template built on `{album}`, + `{year}` or `{track}` names a different file before the lookup and would otherwise never match + anything already in the library. The song already under way when recording starts is never + skipped — pressing record does not mean "start rearranging what is playing", and the media + session reports the *previous* track for a few hundred milliseconds when playback begins from a + stopped Spotify, so acting on the first thing a session sees would fire the command at the song + the user had just started. It also stops after fifty tracks in a row and says so: a queue + Offstream already has, left on repeat, has no other reason to ever stop skipping. Recording + something new puts the budget back. + ### Fixed - **A recording that is thrown away no longer keeps looking its track up.** Resume Spotify on a diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index af05ec3..bd21a83 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -771,6 +771,7 @@ error paths and the scope list. - **A failing SMTC is treated as a silent one.** It is a system service Offstream does not control, so a fault costs the better metadata and never the recording — logged once per transition rather than per poll, since this runs several times a second. Cancellation is exempt: that is the session stopping, and it propagates. - **Advertisements are detected on the same two rules as the title path** — the placeholder title, or playing with no artist attached — because Spotify announces them the same way to both. A paused session is never an ad: the placeholder lingers after playback stops, and treating it as one would suppress the next real track. - **The WinRT call is kept free of decisions.** Everything decidable lives behind `ISmtcSessions` in pure code; `WindowsSmtcSessions` only talks to the system. The session manager is fetched once and kept — `RequestAsync` is a cross-process call and this is polled several times a second. Spotify is matched by a loose app-id substring, which covers both the desktop (`Spotify.exe`) and Store (`SpotifyAB.SpotifyMusic_…!Spotify`) identities. +- **The media session is also a way to talk *to* Spotify, and that turned "keep the one on disk" into a complete answer (2026-08-28).** Declining to record a track the user already has left Spotify playing it to nobody for three minutes; SMTC carries the same skip the keyboard's next-track key sends, so Offstream can move the queue on. `IPlaybackControl` is deliberately a second interface rather than a method on `ISmtcSessions` — reading what is playing and changing what plays are different privileges, and a session handed no implementation simply never skips. **The Web API was the wrong route** even though `POST /me/player/next` exists: it wants `user-modify-playback-state`, a signed-in account and a Premium subscription, and a third scope on a consent screen this project holds to two. Three things it had to get right, each a distinct failure: the ask is **once per track, marked before the call** rather than compared afterwards, because Spotify keeps reporting the outgoing track for a moment after it accepts — the same staleness that costs a new recording its first few hundred milliseconds — and a per-observation decision skips twice, the second one past a song nobody has recorded; it happens at **two checkpoints**, since the early existing-file check is the one the finding below shows cannot see an enriched path, so wired only there it would never fire for exactly the libraries organised well enough to want it; it is **armed only from the second track a session admits**, because pressing record is not an instruction to rearrange what is already playing — and because the media session's opening report is the *previous* track with the play state already true, so a session acting on the first thing it sees fires the command at the song the user has just started; and it **stops after fifty in a row**, because a fully-recorded queue on repeat has no other terminating condition, with the budget restored only by a recording actually reaching the library — a recording merely *starting* is not evidence, or a template that matches only after enrichment resets the cap on every track and never reaches it. - **Long paths go through the `\\?\` prefix, because Offstream does not write the file — ffmpeg does.** A `longPathAware` manifest plus the machine's `LongPathsEnabled` switch opts *a process* in, and ffmpeg is a separate process with its own manifest and no interest in ours. The prefix travels with the path through `ArgumentList` into whatever ffmpeg hands to `CreateFile`. Verified rather than assumed, since the feature rests on it: ffmpeg 8.1 wrote an MP3 to a 298-character destination through a prefixed path, on a machine with the registry switch **off**. - **The prefix disables normalisation, which is the trap.** Windows stops resolving `.` and `..`, stops converting `/` to `\`, and stops trimming trailing dots and spaces — a merely untidy path becomes one the filesystem rejects. So it is applied only to fully-qualified paths, normalised first, and only when the path is long enough to need it. - **Extended paths raise the total length, never the component length.** The 260 budget divided across template levels became a 32767 budget dividing into per-level allowances of thousands of characters, which renders folder names NTFS refuses outright — trading "truncated at 260" for "cannot be written at all". The per-level allowance is clamped to 255. This is the part that looks like it goes away and does not. diff --git a/src/Offstream.App/Resources/Strings.fr.resx b/src/Offstream.App/Resources/Strings.fr.resx index 0e1ddd8..9163102 100644 --- a/src/Offstream.App/Resources/Strings.fr.resx +++ b/src/Offstream.App/Resources/Strings.fr.resx @@ -325,6 +325,12 @@ Enregistrer le nouveau à côté + + Demander aussi à Spotify de passer au suivant + + + Uniquement lorsque le réglage ci-dessus conserve le fichier sur le disque. + Détection diff --git a/src/Offstream.App/Resources/Strings.resx b/src/Offstream.App/Resources/Strings.resx index 7f033ef..a24d99c 100644 --- a/src/Offstream.App/Resources/Strings.resx +++ b/src/Offstream.App/Resources/Strings.resx @@ -417,6 +417,14 @@ Save the new one alongside it Existing-file policy: keep both, numbering the newcomer. + + Also tell Spotify to move on + Checkbox beside the existing-file policy: send a skip-next command when the track playing is already in the library. + + + Only while the setting above keeps the file on disk. + Explains why the skip checkbox is greyed out under the Overwrite and Duplicate policies. + Detection Advanced page: heading over what counts as a track worth recording. The predecessor called these "spy options"; plan §0 renames them. diff --git a/src/Offstream.App/Services/RecordingSessionFactory.cs b/src/Offstream.App/Services/RecordingSessionFactory.cs index 23e8f4d..991c3cb 100644 --- a/src/Offstream.App/Services/RecordingSessionFactory.cs +++ b/src/Offstream.App/Services/RecordingSessionFactory.cs @@ -86,8 +86,13 @@ public RecordingSession Create(OffstreamSettings settings, IProgressWhat to do when the destination file already exists. public IReadOnlyList> Policies { get; } + /// + /// Whether skipping past a recorded track can do anything under the chosen policy. + /// + /// + /// Overwrite and Duplicate both write the file again, so there is nothing to skip past. The + /// checkbox greys out rather than disappearing, so the setting is still findable — and the + /// core reads the two together anyway, since a hand-edited settings file can disagree. + /// + public bool CanSkipAlreadyRecorded => ExistingFilePolicy == ExistingFilePolicy.Skip; + /// UI languages, plus following Windows. public IReadOnlyList> Languages { get; } @@ -223,6 +237,7 @@ private void Load() Template = settings.Output.Template; FileCounter = settings.Output.CurrentFileCounter.ToString(CultureInfo.CurrentCulture); ExistingFilePolicy = settings.Output.ExistingFilePolicy; + SkipAlreadyRecordedTracks = settings.Output.SkipAlreadyRecordedTracks; MuteAds = settings.Recording.MuteAds; RecordEverything = settings.Recording.RecordEverything; RecordAds = settings.Recording.RecordAds; @@ -318,6 +333,8 @@ .. FileNameTemplate.KnownTokens.Select(token => partial void OnExistingFilePolicyChanged(ExistingFilePolicy value) => Persist(); + partial void OnSkipAlreadyRecordedTracksChanged(bool value) => Persist(); + partial void OnIsTimerEnabledChanged(bool value) => Persist(); partial void OnTimerChanged(string value) => Persist(); @@ -355,6 +372,7 @@ private void Persist() { Template = Template.Trim(), ExistingFilePolicy = ExistingFilePolicy, + SkipAlreadyRecordedTracks = SkipAlreadyRecordedTracks, CurrentFileCounter = int.Parse(FileCounter, CultureInfo.CurrentCulture), }, Recording = settings.Recording with diff --git a/src/Offstream.App/Views/Pages/AdvancedPage.xaml b/src/Offstream.App/Views/Pages/AdvancedPage.xaml index 724688f..807a7d7 100644 --- a/src/Offstream.App/Views/Pages/AdvancedPage.xaml +++ b/src/Offstream.App/Views/Pages/AdvancedPage.xaml @@ -126,10 +126,19 @@ + - + @@ -236,20 +245,70 @@ - + + + + + + + + + + + + + + + + + + + + diff --git a/src/Offstream.Core/Recording/RecordingSession.cs b/src/Offstream.Core/Recording/RecordingSession.cs index 6dc0456..daf4038 100644 --- a/src/Offstream.Core/Recording/RecordingSession.cs +++ b/src/Offstream.Core/Recording/RecordingSession.cs @@ -102,6 +102,7 @@ public sealed class RecordingSession : IAsyncDisposable private readonly RecordingPolicy _policy; private readonly IFileSystem _fileSystem; private readonly ITrackEnricher? _enricher; + private readonly IPlaybackControl? _playback; private readonly EncodeBacklog _backlog; private readonly IProgress? _progress; private readonly TimeProvider _time; @@ -109,6 +110,19 @@ public sealed class RecordingSession : IAsyncDisposable private readonly CancellationTokenSource _stopping = new(); private readonly Lock _gate = new(); + /// + /// How many recorded tracks in a row to skip past before leaving Spotify alone. + /// + /// + /// Not a comfort limit — skipping fifty tracks the user already has is the feature working. + /// It is a terminating condition. Put a fully-recorded playlist on repeat and every skip + /// lands on another track that is also on disk, so without a ceiling Offstream drives Spotify + /// round the queue forever, at the speed of a media command rather than of a song. The + /// counter resets whenever a recording actually reaches the library, which is the only real + /// evidence that there is something new left in the queue. + /// + private const int MaxConsecutiveSkips = 50; + /// Maps a queued encode back to the recording it came from, for the rename. private readonly Dictionary _pending = new(StringComparer.OrdinalIgnoreCase); @@ -117,6 +131,27 @@ public sealed class RecordingSession : IAsyncDisposable private Task? _recording; private ITimer? _recordingTimer; private bool _stopAfterCurrentTrack; + + /// The track a skip has already been asked for, held until a different one turns up. + private Track? _skipRequested; + + /// + /// Whether this session has admitted a track yet. The first one is never skipped. + /// + /// + /// Never reset, because it never needs to be: disposes the poller, so + /// a session runs once and the next press of record builds a new one. That is also what makes + /// the rule mean what it says — stopping and starting again is the user saying "begin here", + /// and whatever is playing at that moment is theirs to keep. + /// + private bool _admittedATrack; + + /// Skips asked for since the last recording actually landed in the library. + private int _consecutiveSkips; + + /// Whether the cap has been reached and reported, so it is only said once. + private bool _skippingGaveUp; + private bool _disposed; private int _captureLost; @@ -135,6 +170,11 @@ public sealed class RecordingSession : IAsyncDisposable /// /// Where stage changes are reported. /// Injected for the recording timer and for dated folder names. + /// + /// Drives Spotify's transport, for . + /// Null means the session can only decline to record a track it already has — which is what it + /// did before this existed, and what it still does when the setting is off. + /// public RecordingSession( IAudioCaptureSource capture, SpotifyPoller poller, @@ -143,7 +183,8 @@ public RecordingSession( IFileSystem fileSystem, ITrackEnricher? enricher = null, IProgress? progress = null, - TimeProvider? timeProvider = null) + TimeProvider? timeProvider = null, + IPlaybackControl? playback = null) { ArgumentNullException.ThrowIfNull(capture); ArgumentNullException.ThrowIfNull(poller); @@ -157,6 +198,7 @@ public RecordingSession( _policy = new RecordingPolicy(settings); _fileSystem = fileSystem; _enricher = enricher; + _playback = playback; _progress = progress; _time = timeProvider ?? TimeProvider.System; @@ -501,6 +543,12 @@ private void OnTrackChanged(object? sender, TrackChangedEventArgs e) /// private void Consider(Track track) { + // Whatever was asked for last is no longer in flight once something else is playing. + lock (_gate) + { + if (_skipRequested is not null && !_skipRequested.Equals(track)) _skipRequested = null; + } + if (!_policy.IsTypeAllowed(track)) { Report(RecordingStage.WaitingForTrack, track, message: DescribeSkipped(track)); @@ -517,6 +565,20 @@ private void Consider(Track track) return; } + // Skipping arms on the second track this session admits, never the first. + // + // Pressing record does not mean "start rearranging what is playing". The song already + // under way is one the user chose and is part-way through; it was never going to be + // recorded whole anyway, and cutting it off is a worse answer than letting it finish. + // + // It is also the only defence against the media session's opening lie. Starting playback + // from a stopped Spotify reports the *previous* track for a few hundred milliseconds, + // with the play state already true — so the first thing a session sees can be a song that + // is not playing at all. Skipping on that fires the command at the song the user has just + // started, which is the one thing this feature must never do. + var maySkip = _admittedATrack; + _admittedATrack = true; + var paths = PathsFor(track); // A shortcut, not the decision: it can only see what the title or media session gave us, @@ -529,10 +591,12 @@ private void Consider(Track track) track, message: $"Kept the file already on disk and did not record {track}."); + if (maySkip) RequestSkip(track); + return; } - StartRecorder(track, paths); + StartRecorder(track, paths, maySkip); } /// @@ -558,7 +622,7 @@ private void Consider(Track track) /// session's own token so a teardown still ends it, and handed over with the task. /// /// - private void StartRecorder(Track detected, OutputPaths paths) + private void StartRecorder(Track detected, OutputPaths paths, bool maySkip = false) { if (_buffer is null) return; @@ -587,9 +651,150 @@ private void StartRecorder(Track detected, OutputPaths paths) _recording = Task.Run(() => recorder.RunAsync(_stopping.Token), CancellationToken.None); } + SkipWhenEnrichmentSaysItIsRecorded(recorder, paths, enrichment, maySkip); + Report(RecordingStage.Recording, track, message: $"Recording {track}."); } + /// + /// Asks Spotify to move on if the lookup turns out to have named a file already on disk. + /// + /// + /// + /// The early check in is not enough on its own. Before enrichment + /// lands, a template using {album}, {year} or {track} renders a path + /// nothing will ever be written to, so the check answers about the wrong file and always says + /// no. Skipping wired only there would silently never fire for exactly the libraries organised + /// well enough to want it. already reaches the same + /// conclusion a moment later and discards the recording; this is what makes Spotify move on + /// instead of playing a song to a recorder that is going to throw it away. + /// + /// + /// Gated on the recorder still being current, and that is the whole correctness argument. + /// A lookup outlives the track it belongs to — that is the point of starting it early — so by + /// the time it comes back the user may be two songs further on, and a skip fired then would + /// skip a song nobody has recorded. + /// + /// + private void SkipWhenEnrichmentSaysItIsRecorded( + TrackRecorder recorder, OutputPaths paths, Task? enrichment, bool maySkip) + { + // Carried from Consider rather than read here: by the time a lookup lands the session has + // long since admitted its first track, so a flag checked at this point would have + // forgotten that this recording was that one. + if (!maySkip || enrichment is null || _playback is null) return; + if (!_settings.HasSkipPastRecordedEnabled) return; + + _ = SkipIfRecordedAsync(); + + async Task SkipIfRecordedAsync() + { + try + { + await enrichment; + } +#pragma warning disable CA1031 // A lookup that failed has told us nothing new about the file name. + catch (Exception) +#pragma warning restore CA1031 + { + return; + } + + lock (_gate) + { + if (!ReferenceEquals(_recorder, recorder)) return; + } + + if (AlreadyRecorded(paths, recorder.Track)) RequestSkip(recorder.Track); + } + } + + /// + /// Tells Spotify to move past a track that is already in the library. + /// + /// + /// + /// Once per track, and the guard is held rather than compared after the fact. Spotify + /// goes on reporting the outgoing track for a moment after it accepts the command, so a + /// post-hoc "did I just skip this?" test cannot tell a repeat from a skip that has not landed + /// yet — and would fire a second time, costing the user a song they had not recorded. Marking + /// the track before the call and clearing it only when a different one is considered covers + /// both. + /// + /// + /// And never on the poll loop. runs there, and this is a + /// cross-process call; a slow or throwing transport must not hold up track detection or stop + /// it. A refusal is ordinary — Spotify declines while an advertisement is playing and at the + /// end of a queue — so only an accepted command is worth a line in the log. + /// + /// + private void RequestSkip(Track track) + { + if (_playback is null || !_settings.HasSkipPastRecordedEnabled) return; + + bool giveUp; + + lock (_gate) + { + if (_skipRequested is not null && _skipRequested.Equals(track)) return; + if (_skippingGaveUp) return; + + giveUp = _consecutiveSkips >= MaxConsecutiveSkips; + + if (giveUp) + { + _skippingGaveUp = true; + } + else + { + _skipRequested = new Track(track); + _consecutiveSkips++; + } + } + + if (giveUp) + { + // Said once, and said plainly: going quiet without a reason is indistinguishable from + // the setting having broken. + Report( + RecordingStage.WaitingForTrack, + track, + message: $"Stopped asking Spotify to move on after {MaxConsecutiveSkips} tracks in a row that " + + "were already in the library. Nothing new has been recorded since, so this queue looks " + + "like one Offstream already has."); + + return; + } + + _ = Task.Run(() => SkipAsync(track), CancellationToken.None); + } + + private async Task SkipAsync(Track track) + { + try + { + if (!await _playback!.TrySkipNextAsync(_stopping.Token)) return; + + Report( + RecordingStage.WaitingForTrack, + track, + message: $"Asked Spotify to move past {track}, which is already in the library."); + } + catch (OperationCanceledException) + { + // The session is stopping. Nothing is listening for this any more. + } +#pragma warning disable CA1031 // Detection has to survive a transport that will not take commands. + catch (Exception ex) +#pragma warning restore CA1031 + { + Report( + RecordingStage.WaitingForTrack, + track, + message: $"Could not ask Spotify to move past {track}: {ex.Message}"); + } + } + /// /// Raises once the lookup for this track has come back. /// @@ -857,6 +1062,17 @@ private void OnEncodeCompleted(object? sender, EncodeCompletedEventArgs e) Report(RecordingStage.Tagging, track); } + // A file in the library is the one thing that proves the queue still holds something + // Offstream does not have, so it — not a recording merely starting — is what puts the + // skip budget back. A recording that starts and is then discarded as already-recorded + // must not, or a template that only matches after enrichment would reset the cap on + // every track and never reach it. + lock (_gate) + { + _consecutiveSkips = 0; + _skippingGaveUp = false; + } + TrackSaved?.Invoke(this, new TrackSavedEventArgs(track, destination, recording.Duration)); Report( diff --git a/src/Offstream.Core/Settings/OffstreamSettings.cs b/src/Offstream.Core/Settings/OffstreamSettings.cs index cd8ac21..efa51c1 100644 --- a/src/Offstream.Core/Settings/OffstreamSettings.cs +++ b/src/Offstream.Core/Settings/OffstreamSettings.cs @@ -97,6 +97,7 @@ public sealed record OffstreamSettings( MediaFormat = Output.Format, BitrateKbps = Output.BitrateKbps, ExistingFilePolicy = Output.ExistingFilePolicy, + SkipAlreadyRecordedTracks = Output.SkipAlreadyRecordedTracks, MinimumRecordedLengthSeconds = Recording.MinimumLengthSeconds, MuteAdsEnabled = Recording.MuteAds, RecordEverythingEnabled = Recording.RecordEverything, @@ -132,13 +133,19 @@ public OffstreamSettings CaptureRuntimeState(RecordingSettings runtime) /// The running counter behind the {count} template token, persisted so numbering /// continues across restarts rather than overwriting yesterday's files. /// +/// +/// Tell Spotify to move to the next track when the one playing is already in the library. +/// Off by default: it reaches out and changes what the user is listening to, which is not +/// something to start doing unasked. +/// public sealed record OutputSettings( [property: JsonPropertyName("path")] string? Path = null, [property: JsonPropertyName("template")] string Template = FileNameTemplate.Default, [property: JsonPropertyName("format")] MediaFormat Format = MediaFormat.Mp3, [property: JsonPropertyName("bitrateKbps")] int BitrateKbps = 320, [property: JsonPropertyName("existingFilePolicy")] ExistingFilePolicy ExistingFilePolicy = ExistingFilePolicy.Skip, - [property: JsonPropertyName("currentFileCounter")] int CurrentFileCounter = 1); + [property: JsonPropertyName("currentFileCounter")] int CurrentFileCounter = 1, + [property: JsonPropertyName("skipAlreadyRecordedTracks")] bool SkipAlreadyRecordedTracks = false); /// What gets recorded, and for how long. /// Recordings shorter than this are discarded. diff --git a/src/Offstream.Core/Settings/RecordingSettings.cs b/src/Offstream.Core/Settings/RecordingSettings.cs index e58ecf0..01e9160 100644 --- a/src/Offstream.Core/Settings/RecordingSettings.cs +++ b/src/Offstream.Core/Settings/RecordingSettings.cs @@ -34,6 +34,29 @@ public sealed class RecordingSettings /// What to do when the output file already exists. public Recording.ExistingFilePolicy ExistingFilePolicy { get; set; } + /// + /// Tell Spotify to move on when the track playing is one already in the library. + /// + /// + /// Only meaningful alongside : under the other + /// two policies the file gets written again, so there is nothing to skip past. The setting is + /// read together with the policy rather than merely hidden in the UI, because a hand-edited + /// settings.json can set this true under any policy. + /// + public bool SkipAlreadyRecordedTracks { get; set; } + + /// + /// Whether both halves of the skip-past-recorded setting are actually in force. + /// + /// + /// Named to be hard to confuse with , which is the + /// stored half on its own: a pair differing by one letter is a pair that eventually gets + /// mistyped, and the mistake would silently send skip commands under a policy that records + /// the file again anyway. + /// + public bool HasSkipPastRecordedEnabled => + SkipAlreadyRecordedTracks && ExistingFilePolicy == Recording.ExistingFilePolicy.Skip; + /// Running counter used by the {count} token and the track-number tag. public int InternalOrderNumber { get; set; } = 1; diff --git a/src/Offstream.Core/Spotify/IPlaybackControl.cs b/src/Offstream.Core/Spotify/IPlaybackControl.cs new file mode 100644 index 0000000..84341f6 --- /dev/null +++ b/src/Offstream.Core/Spotify/IPlaybackControl.cs @@ -0,0 +1,29 @@ +namespace Offstream.Core.Spotify; + +/// Drives Spotify's transport — the buttons a user would otherwise press themselves. +/// +/// +/// Separate from on purpose. Reading what is playing and telling +/// Spotify to play something else are different privileges, and most of the app only needs the +/// first. A session handed no implementation simply never skips, which is how every existing +/// construction site keeps behaving exactly as it did. +/// +/// +/// Not the Web API. POST /me/player/next would work, but it needs the +/// user-modify-playback-state scope, a signed-in account and a Premium subscription — three +/// requirements for a convenience feature, and a third scope on a consent screen CLAUDE.md holds +/// to two. The Windows media transport controls carry the same command with no account, no scope +/// and no network, and they reach Spotify while it is minimised to the tray. +/// +/// +public interface IPlaybackControl +{ + /// + /// Asks Spotify to move to the next track. + /// + /// + /// Whether the command was accepted. False is ordinary, not a fault: Spotify refuses while an + /// advertisement is playing and when there is nothing queued after the current track. + /// + Task TrySkipNextAsync(CancellationToken cancellationToken = default); +} diff --git a/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs b/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs index c63f2af..3e38a83 100644 --- a/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs +++ b/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs @@ -25,7 +25,7 @@ namespace Offstream.Core.Spotify.Smtc; /// registers a media session whose id contains "Spotify". /// /// -public sealed class WindowsSmtcSessions : ISmtcSessions +public sealed class WindowsSmtcSessions : ISmtcSessions, IPlaybackControl { private const string SpotifyAppId = "Spotify"; @@ -58,6 +58,27 @@ public sealed class WindowsSmtcSessions : ISmtcSessions properties.AlbumTrackCount); } + /// + /// + /// Asking first is not belt-and-braces. TrySkipNextAsync returns false for a + /// command the session will not take, but Spotify advertises the control as disabled while an + /// advertisement is playing, and that is the one case where sending it anyway would be a + /// behaviour change rather than a no-op. Reading IsNextEnabled keeps the decision on + /// Windows' side, where this class likes to leave it. + /// + public async Task TrySkipNextAsync(CancellationToken cancellationToken = default) + { + var manager = _manager ??= await GlobalSystemMediaTransportControlsSessionManager + .RequestAsync() + .AsTask(cancellationToken); + + var session = FindSpotify(manager); + + if (session?.GetPlaybackInfo()?.Controls.IsNextEnabled != true) return false; + + return await session.TrySkipNextAsync().AsTask(cancellationToken); + } + private static GlobalSystemMediaTransportControlsSession? FindSpotify( GlobalSystemMediaTransportControlsSessionManager manager) { diff --git a/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs b/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs index 50f8c48..e9b532b 100644 --- a/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs +++ b/tests/Offstream.Core.Tests/Recording/RecordingSessionTests.cs @@ -118,6 +118,12 @@ private sealed class FakeEnricher : ITrackEnricher /// Whether the lookup hangs, as a real one still chasing a provider would. public bool NeverAnswers { get; set; } + /// + /// Held open, the lookup does not come back until the test lets it — which is how a + /// provider that answers three songs later is reproduced without a real delay. + /// + public TaskCompletionSource? Gate { get; set; } + /// The token the session gave the most recent lookup. public CancellationToken Token { get; private set; } @@ -129,9 +135,16 @@ public Task EnrichAsync(Track track, CancellationToken cancella Apply?.Invoke(track); - return NeverAnswers - ? NeverAsync(cancellationToken) - : Task.FromResult(new TrackEnrichment(Updated: true, CoverArtPath)); + if (NeverAnswers) return NeverAsync(cancellationToken); + + return Gate is { } gate ? AfterAsync(gate) : Task.FromResult(new TrackEnrichment(Updated: true, CoverArtPath)); + } + + private async Task AfterAsync(TaskCompletionSource gate) + { + await gate.Task; + + return new TrackEnrichment(Updated: true, CoverArtPath); } private static async Task NeverAsync(CancellationToken cancellationToken) @@ -142,6 +155,27 @@ private static async Task NeverAsync(CancellationToken cancella } } + /// Spotify's transport, counting what the session asked it to do. + private sealed class FakePlaybackControl : IPlaybackControl + { + private int _skips; + + /// Whether Spotify takes the command, as it declines to during an advertisement. + public bool Accepts { get; set; } = true; + + /// Set to make the transport throw, as one whose session has gone away does. + public Exception? Failure { get; set; } + + public int Skips => Volatile.Read(ref _skips); + + public Task TrySkipNextAsync(CancellationToken cancellationToken = default) + { + Interlocked.Increment(ref _skips); + + return Failure is not null ? Task.FromException(Failure) : Task.FromResult(Accepts); + } + } + private sealed class Harness : IAsyncDisposable { private readonly Mock _trackSource = new(); @@ -160,7 +194,8 @@ public Harness( Action? configure = null, TimeSpan? fileSystemDelay = null, WaveFormat? captureFormat = null, - ITrackEnricher? enricher = null) + ITrackEnricher? enricher = null, + FakePlaybackControl? playback = null) { Capture = new FakeCaptureSource(captureFormat); @@ -189,8 +224,10 @@ public Harness( ? new DelayedFileSystem(FileSystem, delay) : FileSystem; + Playback = playback; + Session = new RecordingSession( - Capture, Poller, Settings, Encoder, sessionFileSystem, enricher, Progress); + Capture, Poller, Settings, Encoder, sessionFileSystem, enricher, Progress, playback: playback); Session.TrackSaved += (_, e) => Saved.Enqueue(e); Session.TrackRecorded += (_, e) => Recorded.Enqueue(e); @@ -209,6 +246,9 @@ public Harness( public RecordingSession Session { get; } + /// Null when the session was built without a transport, as most tests want. + public FakePlaybackControl? Playback { get; } + public ConcurrentQueue Reports { get; } = new(); public ConcurrentQueue Saved { get; } = new(); @@ -266,6 +306,26 @@ await WaitFor( harness.Capture.Deliver(bytes); } + /// + /// Gets a session past the track it will not skip, so a test can be about skipping. + /// + /// + /// The first track a session admits is deliberately never a skip candidate — it is the song + /// already under way when the user pressed record, and it is what the media session reports + /// for a moment before it catches up. A track that is already on disk is used for + /// this on purpose: it arms the session without starting a recorder, so nothing is left in + /// flight to save itself half-way through the assertions that follow. + /// + private static async Task ArmSkippingAsync(Harness harness) + { + harness.FileSystem.AddFile(@"C:\music\Warm Up - First Track.mp3", new MockFileData("already here")); + harness.Play(Harness.Playing("Warm Up", "First Track")); + + await WaitFor( + () => harness.Reports.Any(r => r.Track?.Contains("First Track", StringComparison.Ordinal) == true), + "the session to admit its first track"); + } + [Fact] public async Task Session_RecordsATrackAndSavesItUnderTheTemplateName() { @@ -1004,4 +1064,320 @@ public async Task Session_DoesNotRaiseEndedWhenItIsStopped() Assert.Equal(0, Volatile.Read(ref ended)); } + + /// + /// Declining to record a track the user already has leaves Spotify playing it to nobody. + /// This is the setting that closes the loop and asks Spotify to move on. + /// + [Fact] + public async Task Session_AsksSpotifyToMovePastATrackItAlreadyHas() + { + var playback = new FakePlaybackControl(); + + await using var harness = new Harness( + s => s.SkipAlreadyRecordedTracks = true, playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + harness.Play(Harness.Playing("Artist", "Title")); + + await WaitFor(() => playback.Skips == 1, "Spotify to be asked to move on"); + + // And the file on disk is still the one that was there: skipping is in addition to the + // existing policy, not instead of it. + Assert.Equal("already here", harness.FileSystem.File.ReadAllText(@"C:\music\Artist - Title.mp3")); + Assert.Null(harness.Session.CurrentTrack); + } + + /// The setting is off by default, and off means Spotify is left alone. + [Fact] + public async Task Session_LeavesSpotifyAloneWhenSkippingIsOff() + { + var playback = new FakePlaybackControl(); + + await using var harness = new Harness(playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + harness.Play(Harness.Playing("Artist", "Title")); + + await WaitFor( + () => harness.Reports.Any( + r => r.Track?.Contains("Title", StringComparison.Ordinal) == true + && r.Message?.Contains("Kept the file", StringComparison.Ordinal) == true), + "the track to be declined"); + + Assert.Equal(0, playback.Skips); + } + + /// + /// The UI greys the setting out under the other two policies, but a hand-edited settings file + /// can still say true — so the pair is read together in the core rather than trusted from the + /// page. Overwrite records the track again, and there is nothing to skip past. + /// + [Fact] + public async Task Session_UnderAnOverwritePolicy_NeverAsksSpotifyToMoveOn() + { + var playback = new FakePlaybackControl(); + + await using var harness = new Harness( + s => + { + s.ExistingFilePolicy = ExistingFilePolicy.Overwrite; + s.SkipAlreadyRecordedTracks = true; + }, + playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + + await RecordTrackAsync(harness, Harness.Playing("Warm Up", "First Track")); + await RecordTrackAsync(harness, Harness.Playing("Artist", "Title")); + + Assert.Equal(0, playback.Skips); + } + + /// + /// The one that matters. Spotify goes on reporting the outgoing track for a moment after it + /// takes the command, so a session that decided per observation would ask twice — and the + /// second skip lands on a song the user has not recorded and would have wanted. + /// + [Fact] + public async Task Session_AsksToMovePastATrackOnlyOnce_HoweverLongSpotifyKeepsReportingIt() + { + var playback = new FakePlaybackControl(); + + await using var harness = new Harness( + s => s.SkipAlreadyRecordedTracks = true, playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + harness.Play(Harness.Playing("Artist", "Title")); + + await WaitFor(() => playback.Skips == 1, "Spotify to be asked to move on"); + + // The stale window: the same song, still reported, pausing and resuming as the transport + // catches up. Every one of these reaches Consider again. + for (var i = 0; i < 4; i++) + { + harness.Play(Harness.Paused("Artist", "Title")); + await Task.Delay(SpotifyPoller.PollInterval * 3); + harness.Play(Harness.Playing("Artist", "Title")); + await Task.Delay(SpotifyPoller.PollInterval * 3); + } + + Assert.Equal(1, playback.Skips); + + // A different song that is also on disk is a separate decision, and gets its own ask. + harness.FileSystem.AddFile(@"C:\music\Artist - Other.mp3", new MockFileData("also here")); + harness.Play(Harness.Playing("Artist", "Other")); + + await WaitFor(() => playback.Skips == 2, "the next recorded track to be asked about too"); + } + + /// + /// The check in Consider runs before the lookup, so a template built on {album} + /// renders a path nothing will ever be written to and the track looks new. Wired only there, + /// skipping would silently never fire for exactly the libraries organised well enough to want + /// it; this is the second checkpoint, once the lookup has landed. + /// + [Fact] + public async Task Session_WhenTheLookupRevealsTheFileIsAlreadyThere_AsksSpotifyToMoveOn() + { + var playback = new FakePlaybackControl(); + var enricher = new FakeEnricher { Apply = track => track.Album = "Album" }; + + await using var harness = new Harness( + s => + { + s.OutputTemplate = @"{album}\{artist} - {title}"; + s.SkipAlreadyRecordedTracks = true; + }, + enricher: enricher, + playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Album\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + // It starts recording, which is the early check being wrong — the un-enriched name is + // "C:\music\Artist - Title.mp3" and nothing is there. + await RecordTrackAsync(harness, Harness.Playing("Artist", "Title")); + + await WaitFor(() => playback.Skips == 1, "Spotify to be asked to move on once the album was known"); + } + + /// + /// A lookup outlives the track it belongs to — that is the point of starting it early. By the + /// time it comes back the user may be two songs on, and skipping then would skip a song + /// nobody has recorded. + /// + [Fact] + public async Task Session_DoesNotAskToMovePastATrackTheUserHasAlreadyLeft() + { + var playback = new FakePlaybackControl(); + var gate = new TaskCompletionSource(); + var enricher = new FakeEnricher { Gate = gate, Apply = track => track.Album = "Album" }; + + await using var harness = new Harness( + s => + { + s.OutputTemplate = @"{album}\{artist} - {title}"; + s.SkipAlreadyRecordedTracks = true; + }, + enricher: enricher, + playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Album\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + await RecordTrackAsync(harness, Harness.Playing("Artist", "Title")); + + // The user moves on before the provider answers. + await RecordTrackAsync(harness, Harness.Playing("Artist", "Next")); + + gate.SetResult(); + + await Task.Delay(SpotifyPoller.PollInterval * 5); + + Assert.Equal(0, playback.Skips); + Assert.Equal("Next", harness.Session.CurrentTrack?.Title); + } + + /// + /// A transport that will not take commands is a broken convenience, not a broken recorder: + /// detection has to carry on, and the next track still gets recorded. + /// + [Fact] + public async Task Session_KeepsRecordingWhenTheSkipItselfFails() + { + var playback = new FakePlaybackControl + { + Failure = new InvalidOperationException("The media session went away."), + }; + + await using var harness = new Harness( + s => s.SkipAlreadyRecordedTracks = true, playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Artist - Title.mp3", new MockFileData("already here")); + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + harness.Play(Harness.Playing("Artist", "Title")); + + await WaitFor(() => playback.Skips == 1, "the skip to be attempted"); + await WaitFor( + () => harness.Reports.Any(r => r.Message?.Contains("went away", StringComparison.Ordinal) == true), + "the reason to be reported"); + + await RecordTrackAsync(harness, Harness.Playing("Artist", "Next")); + harness.Play(Harness.Playing("Artist", "Third")); + + await WaitFor(() => !harness.Saved.IsEmpty, "the next track to be recorded anyway"); + } + + /// + /// The terminating condition. Put a queue Offstream already has on repeat and every skip lands + /// on another track that is also on disk, so without a ceiling the session drives Spotify + /// round the queue forever at the speed of a media command rather than of a song. It stops, + /// says why, and starts again as soon as a recording actually reaches the library — which is + /// the only real evidence there is anything new left to record. + /// + [Fact] + public async Task Session_StopsAskingAfterAWholeQueueOfTracksItAlreadyHas() + { + const string GaveUp = "Stopped asking Spotify to move on"; + const int Cap = 50; + + var playback = new FakePlaybackControl(); + + await using var harness = new Harness( + s => s.SkipAlreadyRecordedTracks = true, playback: playback); + + for (var i = 0; i < Cap + 5; i++) + { + harness.FileSystem.AddFile($@"C:\music\Artist - Track {i}.mp3", new MockFileData("already here")); + } + + harness.Session.Start(); + await ArmSkippingAsync(harness); + + for (var i = 0; i < Cap + 5; i++) + { + var title = $"Track {i}"; + + harness.Play(Harness.Playing("Artist", title)); + + await WaitFor( + () => harness.Reports.Any(r => r.Track?.Contains(title, StringComparison.Ordinal) == true), + $"{title} to be considered"); + } + + Assert.Equal(Cap, playback.Skips); + Assert.Equal(1, harness.Reports.Count(r => r.Message?.Contains(GaveUp, StringComparison.Ordinal) == true)); + + // Two new tracks: the first records, the second ends it so it reaches the library. + await RecordTrackAsync(harness, Harness.Playing("Artist", "Something New")); + await RecordTrackAsync(harness, Harness.Playing("Artist", "Also New")); + + await WaitFor(() => !harness.Saved.IsEmpty, "the new recording to be saved"); + + harness.Play(Harness.Playing("Artist", "Track 0")); + + await WaitFor( + () => playback.Skips == Cap + 1, "skipping to resume now the queue has produced something new"); + } + + /// + /// Pressing record does not mean "start rearranging what is playing". The song already under + /// way is one the user chose and is part-way through; it was never going to be recorded whole, + /// and cutting it off is a worse answer than letting it finish. + /// + /// + /// This is also the only defence against the media session's opening lie. Starting playback + /// from a stopped Spotify reports the previous track for a few hundred milliseconds + /// with the play state already true, so the first thing a session sees can be a song that is + /// not playing at all — and a skip fired on that lands on the song the user has just started. + /// + [Fact] + public async Task Session_DoesNotSkipTheTrackThatWasAlreadyPlayingWhenRecordingStarted() + { + var playback = new FakePlaybackControl(); + + await using var harness = new Harness( + s => s.SkipAlreadyRecordedTracks = true, playback: playback); + + harness.FileSystem.AddFile(@"C:\music\Artist - Title.mp3", new MockFileData("already here")); + harness.FileSystem.AddFile(@"C:\music\Artist - Next.mp3", new MockFileData("also here")); + + harness.Session.Start(); + harness.Play(Harness.Playing("Artist", "Title")); + + await WaitFor( + () => harness.Reports.Any( + r => r.Track?.Contains("Title", StringComparison.Ordinal) == true + && r.Message?.Contains("Kept the file", StringComparison.Ordinal) == true), + "the track already playing to be declined"); + + Assert.Equal(0, playback.Skips); + + // The next one is fair game: it began under Offstream's watch. + harness.Play(Harness.Playing("Artist", "Next")); + + await WaitFor(() => playback.Skips == 1, "the track that started afterwards to be skipped"); + } } diff --git a/tests/Offstream.UI.Tests/AdvancedViewModelTests.cs b/tests/Offstream.UI.Tests/AdvancedViewModelTests.cs index aed5bb3..13ba451 100644 --- a/tests/Offstream.UI.Tests/AdvancedViewModelTests.cs +++ b/tests/Offstream.UI.Tests/AdvancedViewModelTests.cs @@ -298,6 +298,40 @@ public void DetectionOptions_ReachTheFile() Assert.True(saved.Metadata.WriteCounterToTrackNumber); } + /// + /// Overwrite and Duplicate both write the file again, so there is nothing for a skip to move + /// past. The toggle greys out rather than disappearing — a setting that vanishes looks like + /// one that never existed. + /// + [Fact] + public void CanSkipAlreadyRecorded_FollowsTheExistingFilePolicy() + { + var viewModel = Build(); + + Assert.True(viewModel.CanSkipAlreadyRecorded); + + viewModel.ExistingFilePolicy = ExistingFilePolicy.Overwrite; + + Assert.False(viewModel.CanSkipAlreadyRecorded); + + viewModel.ExistingFilePolicy = ExistingFilePolicy.Skip; + + Assert.True(viewModel.CanSkipAlreadyRecorded); + } + + [Fact] + public void SkipAlreadyRecordedTracks_ReachesTheFile() + { + var fileSystem = new MockFileSystem(); + var viewModel = Build(fileSystem: fileSystem); + + Assert.False(viewModel.SkipAlreadyRecordedTracks); + + viewModel.SkipAlreadyRecordedTracks = true; + + Assert.True(SettingsFakes.Reload(fileSystem).Output.SkipAlreadyRecordedTracks); + } + [Fact] public void AppOptions_ReachTheFile() {