diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index bff6edd..805dd5c 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -809,6 +809,17 @@ error paths and the scope list. - **It is a success-path step, into a gap only.** A provider that found nothing at all leaves a bare recording, and one genre on an otherwise untagged file is not worth a second request; a provider that did supply genres is not second-guessed. A fallback that throws is swallowed at `Debug` — everything else on the track is already correct by then, and losing an album because a genre lookup timed out would be the tail wagging the dog. - **SMTC cannot help here, verified rather than assumed.** `GlobalSystemMediaTransportControlsSessionMediaProperties` does define a `Genres` field. Probed live against the running Spotify client, it comes back `count=0` while Title, Artist, AlbumTitle, AlbumArtist and TrackNumber are all populated. Reading it would be code that returns nothing. + +#### The media session as a metadata floor (2026-08-14) + +- **Precedence is API first, media session underneath** (user's call, 2026-08-14). Not the other way round, and not "client first with the API as fallback" — which sounds equivalent and is not. The two sources have asymmetric coverage: SMTC supplies title, artist, album, album artist and track number, and *never* supplies year, release date, genre, album track count, disc or copyright. The API is therefore the only source for half the tag set, so a literal client-first rule would still call it on every track and save nothing. What is worth having is the other direction: the provider wins wherever it answers, and what the client already knew fills the gaps it leaves. +- **The gap it closes is a real one, and it was silent.** When the match guard rejected all four attempts — or no provider was configured, or Spotify was down — the recording was written with nothing. The media session had already reported artist, title, album, album artist and position for that exact track, with more certainty than any lookup, and all of it was being discarded. Now it stands. +- **`SmtcSnapshot` gained `AlbumArtist` and `TrackNumber`**, both optional so the window-title path — which supplies neither — and every existing construction site keep working unchanged. A `TrackNumber` of 0 maps to null: Spotify numbers from 1, so zero means "not reported" rather than a zeroth track. A blank album artist maps to null rather than to an album credited to the empty string, which would otherwise beat a provider that did know. +- **The mappers fill; they no longer clear.** This is the load-bearing half, and it is a deliberate behaviour change in both `SpotifyTrackMapper` and `LastFmTrackMapper`: `Album`, `AlbumArtists` and `AlbumPosition` are only assigned when the provider actually has a value. The `SetArtistFromApi` / `SetTitleFromApi` idiom already worked this way for artist and title — this extends it to the three fields the media session can now supply. +- **One existing test asserted the opposite** and was updated rather than worked around: `ApplyAlbum_WithAnEmptyResponse` pinned that an empty album object wrote `""` and `[]` over whatever was there. That was harmless while the only other source was a window title, which carries no album at all. It stopped being harmless the moment the media session started carrying one, because a provider that could not answer would erase what the client had said for certain. +- **Cover art is deliberately not taken from the media session.** SMTC exposes a thumbnail, and it is much smaller than the 640×640 the Web API returns. As a last resort for a file that would otherwise have no art it would be defensible; as anything that could win over the API's image it is a downgrade users notice in a library. Left out until there is a reason to want it. +- **Genre is not part of this**, because the media session has none to give — probed live, Spotify returns `count=0` for it. See the genre findings above. + **Still to do in this phase:** nothing. **Exit:** track detection survives Spotify minimised to tray; VB-CABLE presence is detected and its absence degrades to a documented link, with no vendor binaries in the repo pending question 9. diff --git a/src/Offstream.Core/Metadata/Providers/LastFmTrackMapper.cs b/src/Offstream.Core/Metadata/Providers/LastFmTrackMapper.cs index f5b6ade..8197c24 100644 --- a/src/Offstream.Core/Metadata/Providers/LastFmTrackMapper.cs +++ b/src/Offstream.Core/Metadata/Providers/LastFmTrackMapper.cs @@ -33,8 +33,10 @@ public static void Apply(Track track, LastFmTrack response) ArgumentNullException.ThrowIfNull(track); ArgumentNullException.ThrowIfNull(response); - track.Album = response.Album?.Title; - track.AlbumPosition = response.Album?.TrackPosition; + // Fills, never clears — the media session may already have supplied both, and Last.fm + // not knowing an album is not the same as the track not having one. + track.Album = string.IsNullOrWhiteSpace(response.Album?.Title) ? track.Album : response.Album.Title; + track.AlbumPosition = response.Album?.TrackPosition ?? track.AlbumPosition; // Last.fm reports milliseconds; everything downstream works in seconds. track.Length = response.Duration is > 0 ? response.Duration / 1000 : null; @@ -46,7 +48,7 @@ public static void Apply(Track track, LastFmTrack response) string[] albumArtists = track.Artist is null ? [] : [track.Artist]; track.Performers = [.. albumArtists.Concat(track.ToString().ToPerformers())]; - track.AlbumArtists = albumArtists; + track.AlbumArtists = albumArtists is { Length: > 0 } ? albumArtists : track.AlbumArtists; } /// diff --git a/src/Offstream.Core/Metadata/Providers/SpotifyTrackMapper.cs b/src/Offstream.Core/Metadata/Providers/SpotifyTrackMapper.cs index 6b29c7a..7ed8bdd 100644 --- a/src/Offstream.Core/Metadata/Providers/SpotifyTrackMapper.cs +++ b/src/Offstream.Core/Metadata/Providers/SpotifyTrackMapper.cs @@ -49,7 +49,9 @@ public static void Apply(Track track, FullTrack spotifyTrack) track.SetTitleFromApi(SpotifyTitleParser.TagAt(titleTags, 1)); track.SetTitleExtendedFromApi(SpotifyTitleParser.TagAt(titleTags, 2), separatorType); - track.AlbumPosition = PositiveOrNull(spotifyTrack.TrackNumber); + // Fills, never clears. The detected track may already carry a position from the media + // session, and a provider that has none of its own must not take that away — see ToTrack. + track.AlbumPosition = PositiveOrNull(spotifyTrack.TrackNumber) ?? track.AlbumPosition; track.Performers = performers; track.Disc = PositiveOrNull(spotifyTrack.DiscNumber); } @@ -66,8 +68,12 @@ public static void Apply(Track track, FullAlbum spotifyAlbum) ArgumentNullException.ThrowIfNull(track); ArgumentNullException.ThrowIfNull(spotifyAlbum); - track.AlbumArtists = ArtistNames(spotifyAlbum.Artists); - track.Album = spotifyAlbum.Name; + var albumArtists = ArtistNames(spotifyAlbum.Artists); + + // As above: the media session already supplied both for the track being recorded, so an + // album object that is missing either leaves what is there rather than blanking it. + track.AlbumArtists = albumArtists is { Length: > 0 } ? albumArtists : track.AlbumArtists; + track.Album = string.IsNullOrWhiteSpace(spotifyAlbum.Name) ? track.Album : spotifyAlbum.Name; track.Genres = spotifyAlbum.Genres?.ToArray() ?? []; track.Year = ParseReleaseYear(spotifyAlbum.ReleaseDate); track.AlbumArtUrl = ChooseCoverUrl(spotifyAlbum.Images); diff --git a/src/Offstream.Core/Spotify/Smtc/SmtcTrackSource.cs b/src/Offstream.Core/Spotify/Smtc/SmtcTrackSource.cs index e501d9a..d1c9ad7 100644 --- a/src/Offstream.Core/Spotify/Smtc/SmtcTrackSource.cs +++ b/src/Offstream.Core/Spotify/Smtc/SmtcTrackSource.cs @@ -7,12 +7,29 @@ namespace Offstream.Core.Spotify.Smtc; /// The track title. /// The album, when Spotify supplied one. /// Whether the session is playing rather than paused or stopped. +/// +/// Who the album is credited to, which differs from on compilations +/// and features — the distinction a library groups by. +/// +/// The position on the album, or null when Spotify did not say. /// +/// /// A snapshot rather than the live WinRT session: it makes the mapping below a pure function of -/// four values, which is what lets every rule in it be tested without a media session, an audio +/// its values, which is what lets every rule in it be tested without a media session, an audio /// endpoint, or Spotify installed. +/// +/// +/// The last two are optional so that every existing construction site — and the window-title +/// path, which has no such information — keeps working unchanged. +/// /// -public readonly record struct SmtcSnapshot(string? Artist, string? Title, string? Album, bool IsPlaying); +public readonly record struct SmtcSnapshot( + string? Artist, + string? Title, + string? Album, + bool IsPlaying, + string? AlbumArtist = null, + int? TrackNumber = null); /// Reads Spotify's media transport session, if it has one. public interface ISmtcSessions @@ -72,16 +89,28 @@ public sealed class SmtcTrackSource(ISmtcSessions sessions) : ITrackSource /// A session that is paused is never an ad, whatever it says. The placeholder lingers after /// playback stops, and treating it as an ad then would suppress the next real track. /// + /// + /// Album artist and track number are carried even though a provider usually replaces + /// them. They are the floor: when no metadata provider is configured, or when the one + /// that is cannot match the track, these are what the file is tagged with — and they come + /// from the client that is playing the track, so they describe it with certainty rather than + /// with a lookup's confidence. See the mappers, which fill rather than clear. + /// /// public static Track ToTrack(SmtcSnapshot snapshot) { var hasArtist = !string.IsNullOrWhiteSpace(snapshot.Artist); + var albumArtist = Trimmed(snapshot.AlbumArtist); return new Track { Artist = Trimmed(snapshot.Artist), Title = Trimmed(snapshot.Title), Album = Trimmed(snapshot.Album), + AlbumArtists = albumArtist is null ? null : [albumArtist], + + // Spotify numbers from 1, so a zero means "not reported" rather than a zeroth track. + AlbumPosition = snapshot.TrackNumber is > 0 ? snapshot.TrackNumber : null, Playing = snapshot.IsPlaying, Ad = snapshot.IsPlaying && (snapshot.Title.IsAdvertisement() || !hasArtist), }; diff --git a/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs b/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs index 8259068..d200f0a 100644 --- a/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs +++ b/src/Offstream.Core/Spotify/Smtc/WindowsSmtcSessions.cs @@ -52,7 +52,9 @@ public sealed class WindowsSmtcSessions : ISmtcSessions properties.Artist, properties.Title, properties.AlbumTitle, - status == GlobalSystemMediaTransportControlsSessionPlaybackStatus.Playing); + status == GlobalSystemMediaTransportControlsSessionPlaybackStatus.Playing, + properties.AlbumArtist, + properties.TrackNumber); } private static GlobalSystemMediaTransportControlsSession? FindSpotify( diff --git a/tests/Offstream.Core.Tests/Metadata/SpotifyMetadataProviderTests.cs b/tests/Offstream.Core.Tests/Metadata/SpotifyMetadataProviderTests.cs index 09d007e..5b5376e 100644 --- a/tests/Offstream.Core.Tests/Metadata/SpotifyMetadataProviderTests.cs +++ b/tests/Offstream.Core.Tests/Metadata/SpotifyMetadataProviderTests.cs @@ -419,4 +419,82 @@ public async Task EnrichAsync_WithNoArtistId_LeavesGenresEmptyWithoutAsking() Assert.Empty(track.Genres!); harness.Artists.Verify(x => x.Get(It.IsAny(), It.IsAny()), Times.Never()); } + + // ---- the floor holds when the provider cannot ---- + + /// + /// The failure this is really about: the match guard rejecting every attempt used to leave a + /// recording with nothing, even though the media session had already said — with certainty, + /// because it is the client playing the track — what the album and position were. + /// + [Fact] + public async Task EnrichAsync_WhenNothingMatches_LeavesTheDetectedMetadataIntact() + { + var harness = new Harness(); + harness.ReturnsPlayback(new CurrentlyPlaying + { + IsPlaying = true, + Item = PlayingTrack("Some Entirely Different Song"), + }); + + var track = new Track + { + Artist = "Artist", + Title = "Title", + Album = "Detected Album", + AlbumArtists = ["Detected Album Artist"], + AlbumPosition = 7, + }; + + var enriched = await harness.Provider.EnrichAsync(track); + + Assert.False(enriched); + Assert.Equal("Detected Album", track.Album); + Assert.Equal(["Detected Album Artist"], track.AlbumArtists!); + Assert.Equal(7, track.AlbumPosition); + } + + /// An API fault is the same story: tags degrade to what the client already knew. + [Fact] + public async Task EnrichAsync_WhenSpotifyFails_LeavesTheDetectedMetadataIntact() + { + var harness = new Harness(); + harness.Fails(HttpStatusCode.InternalServerError, "Spotify is having a moment."); + + var track = new Track + { + Artist = "Artist", + Title = "Title", + Album = "Detected Album", + AlbumArtists = ["Detected Album Artist"], + AlbumPosition = 7, + }; + + await harness.Provider.EnrichAsync(track); + + Assert.Equal("Detected Album", track.Album); + Assert.Equal(["Detected Album Artist"], track.AlbumArtists!); + Assert.Equal(7, track.AlbumPosition); + } + + /// + /// A partial answer fills its gaps from the floor rather than blanking them: Spotify knew the + /// album but reported no position, so the media session's position stands. + /// + [Fact] + public async Task EnrichAsync_WithAnAnswerThatHasNoPosition_KeepsTheDetectedOne() + { + var harness = new Harness(); + var spotifyTrack = PlayingTrack("Title"); + spotifyTrack.TrackNumber = 0; // The SDK's "not populated". + + harness.ReturnsPlayback(new CurrentlyPlaying { IsPlaying = true, Item = spotifyTrack }); + harness.ReturnsAlbum("album-1", new FullAlbum { Name = "Real Album", Genres = [], Images = [] }); + + var track = new Track { Artist = "Artist", Title = "Title", AlbumPosition = 7 }; + await harness.Provider.EnrichAsync(track); + + Assert.Equal("Real Album", track.Album); + Assert.Equal(7, track.AlbumPosition); + } } diff --git a/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs b/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs index f28b25d..d7746fa 100644 --- a/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs +++ b/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs @@ -114,21 +114,84 @@ public void ApplyTrack_SplitsTheTitleExactlyAsTheWindowTitleParserDoes( Assert.Equal(expectedExtended, track.TitleExtended); } + /// + /// An empty album object leaves the album fields alone rather than blanking them. + /// + /// + /// This asserted the opposite until 2026-08-14 — that an empty response wrote "" and + /// an empty array over whatever was there. That was harmless while the only other source was + /// a window title, which supplies no album at all; it stopped being harmless when the media + /// session began supplying album, album artist and track number, because a provider that + /// could not answer would erase what the client had already said for certain. + /// [Fact] - public void ApplyAlbum_WithAnEmptyResponse_ReturnsExpectedTrack() + public void ApplyAlbum_WithAnEmptyResponse_LeavesTheAlbumFieldsAlone() { var fullAlbum = new FullAlbum { Artists = [], Name = "", Genres = [], Images = [] }; var track = WindowTitleTrack(); SpotifyTrackMapper.Apply(track, fullAlbum); - Assert.Equal([], track.AlbumArtists!); - Assert.Equal("", track.Album); + Assert.Null(track.AlbumArtists); + Assert.Null(track.Album); Assert.Equal([], track.Genres!); Assert.Null(track.Year); Assert.Null(track.AlbumArtUrl); } + /// + /// The floor: what the media session already established survives a provider that has + /// nothing of its own to say about it. + /// + [Fact] + public void ApplyAlbum_WithAnEmptyResponse_KeepsWhatTheMediaSessionSupplied() + { + var track = new Track + { + Artist = "Artist", + Title = "Title", + Album = "Detected Album", + AlbumArtists = ["Detected Album Artist"], + AlbumPosition = 7, + }; + + SpotifyTrackMapper.Apply(track, new FullTrack()); + SpotifyTrackMapper.Apply(track, new FullAlbum { Artists = [], Name = "", Genres = [], Images = [] }); + + Assert.Equal("Detected Album", track.Album); + Assert.Equal(["Detected Album Artist"], track.AlbumArtists!); + Assert.Equal(7, track.AlbumPosition); + } + + /// A provider that does know better still wins, which is the whole precedence. + [Fact] + public void ApplyAlbum_WhenTheProviderKnowsBetter_OverwritesWhatWasDetected() + { + var track = new Track + { + Artist = "Artist", + Title = "Title", + Album = "Detected Album", + AlbumArtists = ["Detected Album Artist"], + AlbumPosition = 7, + }; + + SpotifyTrackMapper.Apply(track, new FullTrack { Name = "Title", TrackNumber = 3 }); + SpotifyTrackMapper.Apply( + track, + new FullAlbum + { + Artists = [new SimpleArtist { Name = "Real Album Artist" }], + Name = "Real Album", + Genres = [], + Images = [], + }); + + Assert.Equal("Real Album", track.Album); + Assert.Equal(["Real Album Artist"], track.AlbumArtists!); + Assert.Equal(3, track.AlbumPosition); + } + [Fact] public void ApplyAlbum_WithNoImages_LeavesCoverArtUrlNull() { diff --git a/tests/Offstream.Core.Tests/Spotify/SmtcTrackSourceTests.cs b/tests/Offstream.Core.Tests/Spotify/SmtcTrackSourceTests.cs index 5352149..2be9432 100644 --- a/tests/Offstream.Core.Tests/Spotify/SmtcTrackSourceTests.cs +++ b/tests/Offstream.Core.Tests/Spotify/SmtcTrackSourceTests.cs @@ -129,4 +129,79 @@ public async Task SurroundingWhitespace_IsTrimmed() [Fact] public void Constructor_RejectsAMissingSessionReader() => Assert.Throws(() => new SmtcTrackSource(null!)); + + // ---- the metadata floor: what the client knows, kept for when a provider cannot help ---- + + /// + /// Album artist and track number come across too, because they are what the file is tagged + /// with when no provider is configured or the configured one cannot match the track. + /// + [Fact] + public async Task ASession_ReportsAlbumArtistAndTrackNumber() + { + var snapshot = new SmtcSnapshot( + "Artist", + "Title", + "Album", + IsPlaying: true, + AlbumArtist: "Album Artist", + TrackNumber: 4); + + var track = await new SmtcTrackSource(new FakeSessions(snapshot)).GetCurrentTrackAsync(); + + Assert.NotNull(track); + Assert.Equal(["Album Artist"], track!.AlbumArtists!); + Assert.Equal(4, track.AlbumPosition); + } + + /// Spotify numbers from 1, so a zero means "not reported" rather than a zeroth track. + [Theory] + [InlineData(0)] + [InlineData(null)] + public async Task ATrackNumberThatIsNotAPositionIsNotReported(int? reported) + { + var snapshot = new SmtcSnapshot("Artist", "Title", "Album", IsPlaying: true, TrackNumber: reported); + + var track = await new SmtcTrackSource(new FakeSessions(snapshot)).GetCurrentTrackAsync(); + + Assert.Null(track!.AlbumPosition); + } + + /// + /// A blank album artist is absent, not an album credited to the empty string — which would + /// then win over a provider that did know, since the mappers fill rather than clear. + /// + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public async Task ABlankAlbumArtistIsNotReported(string? reported) + { + var snapshot = new SmtcSnapshot("Artist", "Title", "Album", IsPlaying: true, AlbumArtist: reported); + + var track = await new SmtcTrackSource(new FakeSessions(snapshot)).GetCurrentTrackAsync(); + + Assert.Null(track!.AlbumArtists); + } + + [Fact] + public async Task AnAlbumArtistIsTrimmed() + { + var snapshot = new SmtcSnapshot("Artist", "Title", "Album", IsPlaying: true, AlbumArtist: " Album Artist "); + + var track = await new SmtcTrackSource(new FakeSessions(snapshot)).GetCurrentTrackAsync(); + + Assert.Equal(["Album Artist"], track!.AlbumArtists!); + } + + /// The window-title path supplies neither, and must keep working unchanged. + [Fact] + public async Task ASnapshotWithoutTheOptionalFieldsReportsNeither() + { + var track = await new SmtcTrackSource(new FakeSessions(Playing("Artist", "Title", "Album"))) + .GetCurrentTrackAsync(); + + Assert.Null(track!.AlbumArtists); + Assert.Null(track.AlbumPosition); + } }