From a7588728a6bfd9f6447f16be52e2489424beb464 Mon Sep 17 00:00:00 2001 From: revtex Date: Sun, 30 Aug 2026 11:19:22 -0400 Subject: [PATCH 1/4] Let the user find the right track when the match is wrong There was no way to correct a bad match. The three fields are editable, so a wrong title could be typed over - and doing that forfeits the year, the genre and the artwork that come with a real match, which is most of why anyone opens this page. Re-fetch could not help, and the reason is structural rather than a bug: the automatic lookup builds its query from the file's own artist and title, then discards any result whose artist disagrees with them. Both halves are right. Spotify always returns something, and the top hit for a misparsed filename is routinely a different song, so the gate is what stops the page writing a confident wrong answer into a correctly named file. But it means the one case the automatic path can never reach is a file whose artist is the thing that is wrong, and pressing Re-fetch asks the identical question and gets the identical answer. Auto-fetch skips the row too, because all three fields are filled in. So there is a second lookup with a deliberately different shape: the user's words instead of the file's, several results instead of a verdict, and nothing filtered on the way out. A person reading a list can tell a live take from a studio one, and a filter that could hide the right answer is worse here than a list with a few wrong ones in it. Each result carries its release year, because a search for a song recorded twice returns the original and the remaster with identical titles by identical artists, and the year is the only thing on the row that separates them. The chosen candidate is looked up again rather than tagged from the search result. A search result is not a full track - the release date lives on the album and the genre on the artist - and a hand-picked match arriving thinner than an automatic one would be a strange thing to have built. Both paths share one enrichment step for the same reason: the rule about putting back what Spotify does not offer is the kind that goes wrong quietly, and it would drift the moment there were two copies. Picking a result also drops the file's own cover art, which is the one place the rule that an embedded picture beats a provider's URL is inverted. That rule protects artwork while a lookup confirms what a file already says; choosing a different song says the opposite, and keeping the old sleeve would write correct tags and the previous artist's cover into the same file. That decision then exposed the display bug underneath it. The thumbnail was bound to decoded bytes, but a lookup normally returns a URL and no bytes - the writer downloads it when saving - so it showed the artwork the file already had while claiming to show what saving would do. On a hand-picked match it put the replaced track's sleeve on both sides of the before-and-after. It now binds to either an image or a URI and lets WPF fetch the second, which also keeps the network out of the view model and off whatever thread a fetch happened to finish on. Last.fm is not offered here. Its lookup answers a question about a named artist and title, so it has no results for someone who does not yet know what the track is called, and an empty box that can never return anything would be worse than saying it needs Spotify. Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 8 +- CLAUDE.md | 2 +- README.md | 15 + docs/MODERNIZATION-PLAN.md | 10 + src/Offstream.App/Resources/Strings.fr.resx | 36 +++ src/Offstream.App/Resources/Strings.resx | 36 +++ .../Services/LibraryMetadataChain.cs | 31 ++- .../ViewModels/LibraryMatchViewModel.cs | 34 +++ .../ViewModels/LibraryTrackViewModel.cs | 81 +++++- .../ViewModels/MetadataViewModel.cs | 92 +++++++ .../Views/Pages/MetadataPage.xaml | 91 ++++++- .../Metadata/Library/ILibraryMatchSearch.cs | 39 +++ .../Metadata/Library/LibraryMatchCandidate.cs | 29 ++ .../Library/SpotifyCatalogEnricher.cs | 80 ++++++ .../Metadata/Library/SpotifyMatchSearch.cs | 124 +++++++++ .../Library/SpotifySearchMetadataProvider.cs | 62 +---- .../Library/SpotifyMatchSearchTests.cs | 257 ++++++++++++++++++ .../LibraryTrackViewModelTests.cs | 24 ++ .../MetadataViewModelTests.cs | 193 +++++++++++++ 19 files changed, 1175 insertions(+), 69 deletions(-) create mode 100644 src/Offstream.App/ViewModels/LibraryMatchViewModel.cs create mode 100644 src/Offstream.Core/Metadata/Library/ILibraryMatchSearch.cs create mode 100644 src/Offstream.Core/Metadata/Library/LibraryMatchCandidate.cs create mode 100644 src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs create mode 100644 src/Offstream.Core/Metadata/Library/SpotifyMatchSearch.cs create mode 100644 tests/Offstream.Core.Tests/Metadata/Library/SpotifyMatchSearchTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e35f93..f04342c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,7 +91,13 @@ phase plan these entries follow. scroll — it changes what is drawn and nothing else, so Save still writes every ticked row it is hiding. Beneath the editable fields, **Also from the match** shows the year, genre and artwork a lookup found: Save writes all three and they appeared nowhere, so a row could announce that it - would change while every field on screen matched the file exactly. + would change while every field on screen matched the file exactly. And when the match itself is + wrong, **Not the right track?** searches Spotify for whatever you type and lists what it finds, + year included, so a remaster can be told from the original. Until this there was no way to + correct a bad match except to type all three fields by hand and give up the year, genre and + artwork that come with a real one — re-fetching could not do it, because the automatic lookup + builds its query from the file's own fields and then refuses any result whose artist disagrees + with them, which is precisely the case where the file is what is wrong. - **Looking a track up now falls back from Spotify to Last.fm instead of stopping at whichever one the Settings page names.** That setting answers "who tags a recording as it is made", where one source keeps a library consistent. Repairing files already on disk is the opposite problem — diff --git a/CLAUDE.md b/CLAUDE.md index ca04418..d8149f3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,7 +63,7 @@ Set by the user on 2026-08-13 and binding like the constraints above. Most were - **Endpoints, parameters and field names come from the [OpenAPI schema](https://developer.spotify.com/reference/web-api/open-api-schema.yaml).** Never guessed. In practice `SpotifyAPI.Web` is the typed SDK and nothing here hand-rolls HTTP; check the schema before adding a call, not after. - **[Authorization Code with PKCE](https://developer.spotify.com/documentation/web-api/tutorials/code-pkce-flow) for user data.** Client Credentials only for public non-user data. **Implicit Grant is banned** — it is deprecated. A desktop app is a public client, so there is no Client Secret to protect and none may be introduced. - **Redirect URIs are HTTPS**, except `http://127.0.0.1` for local development. Never bare `localhost` (Spotify rejects it), never wildcards. `SpotifyAuthOptions.DefaultRedirectUri` is the one in use and a test pins its shape. -- **Request the minimum [scopes](https://developer.spotify.com/documentation/web-api/concepts/scopes) the shipped features need, never a scope for a feature that might arrive later.** Offstream makes five calls, and only two of them cost a scope: `/me/player/currently-playing`, which needs `user-read-currently-playing`; `/me`, which puts the signed-in account's name on the Settings page and needs `user-read-private`; and `/albums/{id}`, `/artists/{id}` and `/search`, which are public catalogue data and need no user scope. Two scopes, and `SpotifyAuthOptionsTests` fails the build if the list grows. **The count is not the thing to guard — the scope list is.** This line said "exactly three calls" while the code already made four, because `/artists/{id}` was added for genre and nobody updated the prose; `/search` joined them for the Metadata page on 2026-08-29 and added no scope either. Adding a call to an endpoint that needs no user scope is ordinary work. Adding one that needs a scope is the thing that has to be argued for. `user-read-email` would identify an account unambiguously and is deliberately **not** requested: Spotify removed the `email` field in its late-2024 cull, so the permission now covers data the endpoint no longer returns. `SpotifyAuthOptions.DefaultScopes` carries the full reasoning and is the place to change it. A scope requested ahead of its feature is a permission the user grants for nothing, on a consent screen where the spare lines look identical to the load-bearing one. +- **Request the minimum [scopes](https://developer.spotify.com/documentation/web-api/concepts/scopes) the shipped features need, never a scope for a feature that might arrive later.** Offstream makes six calls, and only two of them cost a scope: `/me/player/currently-playing`, which needs `user-read-currently-playing`; `/me`, which puts the signed-in account's name on the Settings page and needs `user-read-private`; and `/albums/{id}`, `/artists/{id}`, `/search` and `/tracks/{id}`, which are public catalogue data and need no user scope. Two scopes, and `SpotifyAuthOptionsTests` fails the build if the list grows. **The count is not the thing to guard — the scope list is.** This line said "exactly three calls" while the code already made four, because `/artists/{id}` was added for genre and nobody updated the prose; `/search` joined them for the Metadata page on 2026-08-29, and `/tracks/{id}` on 2026-08-30 for the manual match picker, neither adding a scope. Adding a call to an endpoint that needs no user scope is ordinary work. Adding one that needs a scope is the thing that has to be argued for. `user-read-email` would identify an account unambiguously and is deliberately **not** requested: Spotify removed the `email` field in its late-2024 cull, so the permission now covers data the endpoint no longer returns. `SpotifyAuthOptions.DefaultScopes` carries the full reasoning and is the place to change it. A scope requested ahead of its feature is a permission the user grants for nothing, on a consent screen where the spare lines look identical to the load-bearing one. - **Tokens are stored protected, refreshed, and abandoned when dead.** DPAPI covers the refresh token; the access token never reaches disk. Spotify rotates the refresh token on every renewal, so the replacement must be persisted or a long-running install silently stops working. A **401** means the refresh token itself is gone — clear it and put the user back through sign-in rather than retrying it forever. Only 401: treating a rate limit or an outage that way would sign the user out over a transient fault. - **On 429, honour `Retry-After` exactly; back off exponentially only where there is no such instruction.** Guessing shorter is what gets an application throttled harder. Never retry in a tight loop. `SpotifyRetryHandler` is the implementation — note that `SpotifyClientConfig.CreateDefault()` attaches **no** retry handler, so a client built without one treats rate limiting as fatal. - **Log throttling at `Warning`.** The Record page's activity log shows Information and above, so anything quieter is invisible to everyone who has not gone looking. Transient 5xx stays at `Information` — it usually clears, and promoting it makes the Problems filter too noisy to read. diff --git a/README.md b/README.md index d2cd361..f396fce 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,21 @@ change while every box matches the file otherwise looks like a mistake. type. It only changes what is on screen: Save still writes every ticked row, including the ones the filter is hiding. +### When the match is wrong + +Untick the row and nothing happens to that file. To fix it instead, either type the correct values +into Title, Artist and Album — they're yours to edit and they win over anything a lookup found — +or use **Not the right track?** at the bottom of the opened row. That searches Spotify for whatever +you type and lists what it finds, with the year beside each result so you can tell a remaster from +the original, and **Use this** fills the row in from the one you pick. + +Reach for the search rather than **Re-fetch** whenever the artist is the thing that's wrong. +Re-fetch asks the same question the automatic lookup already asked — built from the file's own +fields, and refusing any result whose artist disagrees with them — so it returns the same answer +however many times you press it. The search takes your words instead, which is the only way to +reach a track the file's tags point away from. Picking a result also replaces the cover art, since +the old one belongs to whatever the file used to claim to be. + A few things worth knowing: - **`.wav` files are skipped**, and the page says how many. WAV has no tag format players agree on, diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index f583cca..1d7d304 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -945,6 +945,16 @@ The layout that replaced the table is one compact row per file — title, then a The lesson is about the order the two were found in. The artwork bug was found by looking at the running app; **this one was found by building the feature that displays the data** — the "Also from the match" block exists because a row could say it would change while all three editable fields matched the file, and the first thing it drew on a real library was `Genre — was Hip-Hop, rap, hip hop`, an offer to erase a tag nobody had asked to erase. A value that is written on Save but rendered nowhere is a value no amount of looking at the page can check, and the suite could not check it either: the fixtures asserted what a lookup *adds*, never what it silently takes away. +### Finding: an automatic matcher cannot be its own escape hatch (2026-08-30) + +`SpotifySearchMetadataProvider` builds its query from the file's own artist and title, and then discards any result whose artist disagrees with them. Both halves are right: Spotify always returns *something*, and the top hit for a misparsed filename is routinely a different song, so a strict gate is what stops the page writing a confident wrong answer into a correctly named file. + +The consequence is that **the automatic path cannot fix a wrong artist, and Re-fetch is not the escape hatch it was documented as.** A file tagged `AC — Who Made Who` searches for that artist and rejects everything that is not it; pressing Re-fetch again asks the identical question and gets the identical answer. Auto-fetch will not touch the row either, because all three fields are filled in. The only remedy was to type the correct values by hand — which works, and forfeits the year, genre and artwork that come with a real match. + +So `ILibraryMatchSearch` is a second, deliberately different shape: **the user's words instead of the file's, several results instead of a verdict, and no filtering on the way out.** A person reading a list can tell a live take from a studio one, and a filter that could hide the right answer is worse here than a list with a few wrong ones in it. Two details are load-bearing. The chosen candidate is fetched again through `/tracks/{id}` rather than tagged from the search result, because a search result is not a full track — the release date is on the album and the genre on the artist — and a hand-picked match arriving thinner than an automatic one would be a strange thing to have built. And it is the one place the "an embedded picture beats a provider's URL" rule is inverted: picking a different song says the file's own cover belongs to the track it used to claim to be, so it is dropped and the new URL is taken. + +That last decision then exposed the display bug beneath it. `CoverArt` was bound to decoded bytes, but a lookup normally returns a **URL** and no bytes — the writer downloads it at save time — so the thumbnail showed the artwork the file already had while claiming to show what saving would do. On a hand-picked match it put the replaced track's sleeve on both sides of the before-and-after. The fix is to bind to either a decoded image or a `Uri` and let WPF's `Image.Source` fetch the second, which also keeps the network off the view model and off whatever thread a fetch finished on. **Both times on this page, the thing that made a bug visible was drawing the value rather than testing it.** + --- ## 12. Risks diff --git a/src/Offstream.App/Resources/Strings.fr.resx b/src/Offstream.App/Resources/Strings.fr.resx index 8ab3f82..d3b3ad7 100644 --- a/src/Offstream.App/Resources/Strings.fr.resx +++ b/src/Offstream.App/Resources/Strings.fr.resx @@ -654,4 +654,40 @@ Après enregistrement Caption under the artwork that would be written. + + Ce n'est pas le bon morceau ? + Heading over the manual catalogue search in an opened row. + + + Rechercher + Label for the manual search box. + + + Recherche sur Spotify ce que vous saisissez et affiche tous les résultats pour que vous choisissiez le bon. À utiliser quand la correspondance automatique s'est trompée de morceau : relancer la recherche pose la même question et donne la même réponse. + Tooltip for the manual search box. + + + Rechercher sur Spotify + Button that runs the manual search. + + + Spotify n'a renvoyé aucun résultat pour cette recherche. + Shown when a manual search matched nothing. + + + La recherche d'une autre correspondance nécessite Spotify. Connectez-vous depuis la page Paramètres. + Shown when a manual search is attempted without a Spotify sign-in. + + + Utiliser + Button that applies one search result to the row. + + + Remplit les champs de cette ligne à partir du résultat sélectionné. Rien n'est écrit dans le fichier avant l'enregistrement. + Tooltip for the button that applies a search result. + + + Recherche en cours… + Shown while a manual search is running. + diff --git a/src/Offstream.App/Resources/Strings.resx b/src/Offstream.App/Resources/Strings.resx index 007b9aa..5e424f9 100644 --- a/src/Offstream.App/Resources/Strings.resx +++ b/src/Offstream.App/Resources/Strings.resx @@ -814,4 +814,40 @@ After saving Caption under the artwork that would be written. + + Not the right track? + Heading over the manual catalogue search in an opened row. + + + Search + Label for the manual search box. + + + Searches Spotify for whatever you type, and shows every result so you can pick the right one. Use this when the automatic match found the wrong song — re-fetching asks the same question and gets the same answer. + Tooltip for the manual search box. + + + Search Spotify + Button that runs the manual search. + + + Spotify returned no results for that search. + Shown when a manual search matched nothing. + + + Searching for a different match needs Spotify. Sign in on the Settings page. + Shown when a manual search is attempted without a Spotify sign-in. + + + Use this + Button that applies one search result to the row. + + + Fills this row's fields from the selected result. Nothing is written to the file until you save. + Tooltip for the button that applies a search result. + + + Searching… + Shown while a manual search is running. + diff --git a/src/Offstream.App/Services/LibraryMetadataChain.cs b/src/Offstream.App/Services/LibraryMetadataChain.cs index 194acb1..a1ff5f1 100644 --- a/src/Offstream.App/Services/LibraryMetadataChain.cs +++ b/src/Offstream.App/Services/LibraryMetadataChain.cs @@ -3,6 +3,7 @@ using Offstream.Core.Metadata.Providers; using Offstream.Core.Settings; using Serilog; +using SpotifyAPI.Web; namespace Offstream.App.Services; @@ -17,6 +18,17 @@ public interface ILibraryMetadataChain /// effect on the next fetch instead of at the next restart. /// FallbackMetadataProvider Create(); + + /// + /// The catalogue search behind "find a different match", or null when nobody is signed in. + /// + /// + /// Spotify only, and null rather than a chain. Last.fm's track.getInfo answers a + /// question about a named artist and title — it is a lookup, not a search — so it has no + /// results to offer someone who does not yet know what the track is called. Offering an empty + /// box that can never return anything would be worse than saying it needs Spotify. + /// + ILibraryMatchSearch? CreateMatchSearch(); } /// @@ -68,6 +80,14 @@ public FallbackMetadataProvider Create() return new FallbackMetadataProvider(providers); } + /// + public ILibraryMatchSearch? CreateMatchSearch() + { + var client = CreateClient(settingsDocument.Current); + + return client is null ? null : new SpotifyMatchSearch(client); + } + /// The search provider, or null when nobody is signed in. /// /// Deliberately and not the recording path's @@ -77,14 +97,17 @@ public FallbackMetadataProvider Create() /// private SpotifySearchMetadataProvider? CreateSpotify(OffstreamSettings settings) { - var client = spotifyAccount.CreateClient( - settings.Metadata.SpotifyClientId, - settings.Metadata.SpotifyRefreshToken, - StoreRotatedRefreshToken); + var client = CreateClient(settings); return client is null ? null : new SpotifySearchMetadataProvider(client); } + private ISpotifyClient? CreateClient(OffstreamSettings settings) => + spotifyAccount.CreateClient( + settings.Metadata.SpotifyClientId, + settings.Metadata.SpotifyRefreshToken, + StoreRotatedRefreshToken); + /// /// Persists the refresh token Spotify hands back on renewal. /// diff --git a/src/Offstream.App/ViewModels/LibraryMatchViewModel.cs b/src/Offstream.App/ViewModels/LibraryMatchViewModel.cs new file mode 100644 index 0000000..f585e72 --- /dev/null +++ b/src/Offstream.App/ViewModels/LibraryMatchViewModel.cs @@ -0,0 +1,34 @@ +using Offstream.Core.Metadata.Library; + +namespace Offstream.App.ViewModels; + +/// One result of a manual search, offered to replace the row's current suggestion. +/// +/// Carries its own row, because the command that applies it lives on the page rather than on the +/// row — a candidate needs both to be useful, and a WPF CommandParameter is one object. +/// +public sealed class LibraryMatchViewModel(LibraryTrackViewModel row, LibraryMatchCandidate candidate) +{ + /// The row this result would be applied to. + public LibraryTrackViewModel Row { get; } = + row ?? throw new ArgumentNullException(nameof(row)); + + /// The result itself. + public LibraryMatchCandidate Candidate { get; } = + candidate ?? throw new ArgumentNullException(nameof(candidate)); + + /// The track title. + public string Title => Candidate.Title; + + /// + /// Artist, album and year on one line — what actually tells two results apart. + /// + /// + /// The year is the discriminating field and is why it is here. A search for a song people + /// have recorded twice returns the original and the remaster with identical titles and + /// identical artists, and the release year is the only thing on the row that separates them. + /// + public string Summary => Candidate.Year is { } year + ? $"{Candidate.Artist} · {Candidate.Album} · {year}" + : $"{Candidate.Artist} · {Candidate.Album}"; +} diff --git a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs index c2809d5..55b6b04 100644 --- a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs +++ b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs @@ -1,3 +1,4 @@ +using System.Collections.ObjectModel; using System.ComponentModel.DataAnnotations; using System.Globalization; using System.IO; @@ -107,6 +108,7 @@ public LibraryTrackViewModel(LibraryTrack track) /// Cover art to preview, from the file or from a fetched suggestion. [ObservableProperty] + [NotifyPropertyChangedFor(nameof(SuggestedCoverArtSource))] private BitmapImage? _coverArt; /// Whether the row's editable fields are showing. @@ -213,6 +215,34 @@ public LibraryTrackViewModel(LibraryTrack track) /// Whether saving would put a different picture in the file. public bool HasCoverArtChange => _track.CoverArtWouldChange; + /// The artwork that would actually be written, however it is going to get there. + /// + /// + /// Returns either a decoded picture or a , and WPF's Image.Source + /// accepts both — the second is left for the framework to fetch rather than downloaded here, + /// which keeps the network off the view model and off whatever thread a fetch happened to + /// finish on. + /// + /// + /// Both cases are real. A lookup that finds artwork usually hands back a URL and no bytes, + /// and the writer downloads it at save time — so binding the thumbnail to the decoded image + /// alone showed the artwork the file already had while claiming to show what saving would do. + /// After a hand-picked match, where the old picture is dropped deliberately, that meant the + /// before-and-after displayed the replaced track's sleeve on both sides. + /// + /// + public object? SuggestedCoverArtSource + { + get + { + if (_track.Suggested.AlbumArtImage is { Length: > 0 }) return CoverArt; + + return Uri.TryCreate(_track.Suggested.AlbumArtUrl, UriKind.Absolute, out var url) + ? url + : CoverArt; + } + } + /// /// Whether the match changed anything the three editable boxes do not show. /// @@ -255,6 +285,7 @@ public void RefreshFromSuggestion() OnPropertyChanged(nameof(HasGenreChange)); OnPropertyChanged(nameof(HasCoverArtChange)); OnPropertyChanged(nameof(HasMatchDetails)); + OnPropertyChanged(nameof(SuggestedCoverArtSource)); } /// @@ -312,9 +343,57 @@ private void MarkEdited() } } + /// Results of the last manual search on this row. + public ObservableCollection Candidates { get; } = []; + + /// What to search the catalogue for. + /// + /// Seeded from the row rather than left blank, because the query the user wants is almost + /// always a correction of the one already there — an artist misspelt, a featured act that + /// belongs in the title, a remaster suffix to drop. + /// + [ObservableProperty] + private string _matchQuery = string.Empty; + + /// Whether a manual search is running on this row. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(IsNotSearching))] + private bool _isSearching; + + /// Whether the row's search button is available. + public bool IsNotSearching => !IsSearching; + + /// Why the last search returned nothing, or null. + [ObservableProperty] + private string? _searchMessage; + + /// Whether there are results to choose from. + public bool HasCandidates => Candidates.Count > 0; + + /// Announces that has been refilled. + public void NotifyCandidatesChanged() => OnPropertyChanged(nameof(HasCandidates)); + + /// Fills the search box from whatever the row says now. + /// + /// Called when the fields open. Doing it on expand rather than in the constructor means the + /// box reflects a fetch or an edit that happened in between, instead of the values the file + /// carried when it was scanned. + /// + public void SeedMatchQuery() + { + if (!string.IsNullOrWhiteSpace(MatchQuery)) return; + + MatchQuery = string.Join(' ', new[] { Artist, Title }.Where(part => !string.IsNullOrWhiteSpace(part))); + } + /// Opens or closes this row's fields. [RelayCommand] - private void ToggleExpand() => IsExpanded = !IsExpanded; + private void ToggleExpand() + { + IsExpanded = !IsExpanded; + + if (IsExpanded) SeedMatchQuery(); + } private static string Or(string? value, string fallback) => string.IsNullOrWhiteSpace(value) ? fallback : value; diff --git a/src/Offstream.App/ViewModels/MetadataViewModel.cs b/src/Offstream.App/ViewModels/MetadataViewModel.cs index 20269d8..2d83ea7 100644 --- a/src/Offstream.App/ViewModels/MetadataViewModel.cs +++ b/src/Offstream.App/ViewModels/MetadataViewModel.cs @@ -310,6 +310,98 @@ private async Task RefetchAsync(LibraryTrackViewModel? row) } } + /// Searches the catalogue for what the user typed on one row. + /// + /// The escape hatch of last resort, and the only one that can correct a match the automatic + /// path is certain about. Re-fetch asks the same question again and gets the same answer; + /// this asks a different question, and the automatic path's own rule — reject any result + /// whose artist disagrees with the file — is precisely what stops it helping when the file's + /// artist is the thing that is wrong. + /// + [RelayCommand] + private async Task SearchMatchesAsync(LibraryTrackViewModel? row) + { + if (row is null || string.IsNullOrWhiteSpace(row.MatchQuery)) return; + + var search = _chain.CreateMatchSearch(); + + if (search is null) + { + row.SearchMessage = Strings.MetadataSearchNeedsSpotify; + + return; + } + + row.IsSearching = true; + row.SearchMessage = null; + row.Candidates.Clear(); + row.NotifyCandidatesChanged(); + + try + { + var results = await search.SearchAsync(row.MatchQuery, CancellationToken.None); + + foreach (var result in results) row.Candidates.Add(new LibraryMatchViewModel(row, result)); + + row.SearchMessage = results.Count == 0 ? Strings.MetadataSearchNoResults : null; + } + catch (MetadataLookupException ex) + { + row.SearchMessage = ex.Message; + } + finally + { + row.IsSearching = false; + row.NotifyCandidatesChanged(); + } + } + + /// Applies a chosen search result to its row. + /// + /// The results disappear afterwards. The choice has been made and its effect is visible in + /// the fields above, so leaving the list open invites picking a second one on top of the + /// first — which works, but reads as though neither had been applied. + /// + [RelayCommand] + private async Task UseMatchAsync(LibraryMatchViewModel? choice) + { + if (choice is null) return; + + var search = _chain.CreateMatchSearch(); + + if (search is null) + { + choice.Row.SearchMessage = Strings.MetadataSearchNeedsSpotify; + + return; + } + + var row = choice.Row; + + row.IsSearching = true; + + try + { + await search.ApplyAsync(row.Track.Suggested, choice.Candidate, CancellationToken.None); + + row.Track.Status = LibraryTrackStatus.Fetched; + row.Track.FailureReason = null; + row.RefreshFromSuggestion(); + + row.Candidates.Clear(); + row.SearchMessage = null; + } + catch (MetadataLookupException ex) + { + row.SearchMessage = ex.Message; + } + finally + { + row.IsSearching = false; + row.NotifyCandidatesChanged(); + } + } + /// Writes the ticked rows back to their files. [RelayCommand] private async Task SaveSelectedAsync(CancellationToken cancellationToken) diff --git a/src/Offstream.App/Views/Pages/MetadataPage.xaml b/src/Offstream.App/Views/Pages/MetadataPage.xaml index ee33f4d..0a0e4d6 100644 --- a/src/Offstream.App/Views/Pages/MetadataPage.xaml +++ b/src/Offstream.App/Views/Pages/MetadataPage.xaml @@ -87,7 +87,7 @@ VerticalAlignment="Center" Background="{DynamicResource ControlFillColorSecondaryBrush}" CornerRadius="4"> - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Offstream.Core/Metadata/Library/ILibraryMatchSearch.cs b/src/Offstream.Core/Metadata/Library/ILibraryMatchSearch.cs new file mode 100644 index 0000000..15725f8 --- /dev/null +++ b/src/Offstream.Core/Metadata/Library/ILibraryMatchSearch.cs @@ -0,0 +1,39 @@ +namespace Offstream.Core.Metadata.Library; + +/// Searching the catalogue by hand, for when the automatic match is wrong. +/// +/// +/// Why this is separate from . A provider answers +/// "what is this track", takes its question from the track's own fields, and either matches or +/// does not. That is the right shape for filling gaps and the wrong one for correcting a mistake: +/// the automatic path refuses any result whose artist disagrees with the file, which is exactly +/// the case where the file is wrong and the user knows better. Re-running it returns the same +/// answer however many times it is asked. +/// +/// +/// So this takes the user's words instead of the file's, returns several results rather than a +/// verdict, and leaves the choosing to the person who can see them. +/// +/// +public interface ILibraryMatchSearch +{ + /// Searches the catalogue for what the user typed. + /// Free text, exactly as entered. + /// Cancels the request. + /// Up to a handful of candidates, best first, or empty when nothing matched. + /// + /// The lookup failed for a reason the user can act on, carrying the provider's own message. + /// + Task> SearchAsync( + string query, + CancellationToken cancellationToken = default); + + /// Applies a chosen candidate to a track, filling in everything a tag needs. + /// The suggestion to overwrite. + /// The result the user picked. + /// Cancels the requests. + Task ApplyAsync( + Track track, + LibraryMatchCandidate candidate, + CancellationToken cancellationToken = default); +} diff --git a/src/Offstream.Core/Metadata/Library/LibraryMatchCandidate.cs b/src/Offstream.Core/Metadata/Library/LibraryMatchCandidate.cs new file mode 100644 index 0000000..e95cf1b --- /dev/null +++ b/src/Offstream.Core/Metadata/Library/LibraryMatchCandidate.cs @@ -0,0 +1,29 @@ +namespace Offstream.Core.Metadata.Library; + +/// One result of a manual catalogue search, as the user sees it before choosing. +/// +/// +/// A deliberately flat description rather than the provider's own object. The page needs four +/// strings and a picture to let someone tell two recordings of the same song apart, and keeping +/// the provider's type out of the view models means the second provider — whenever one arrives — +/// is a new implementation rather than a change to the page. +/// +/// +/// is what applying the choice acts on, because the search response does not +/// carry everything a tag needs: genre lives on the artist and the release date on the album, so +/// the chosen candidate is looked up properly rather than tagged from the search result alone. +/// +/// +/// The provider's identifier for the track. +/// Track title. +/// Artists, joined, as the search returned them. +/// Album name. +/// Release year, when the search result carried one. +/// A thumbnail to show beside the row, when there is one. +public sealed record LibraryMatchCandidate( + string Id, + string Title, + string Artist, + string Album, + int? Year, + string? CoverArtUrl); diff --git a/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs b/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs new file mode 100644 index 0000000..dcf2ffc --- /dev/null +++ b/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs @@ -0,0 +1,80 @@ +using Offstream.Core.Metadata.Providers; +using SpotifyAPI.Web; + +namespace Offstream.Core.Metadata.Library; + +/// Turns one Spotify track into a full set of tags. +/// +/// +/// Shared by the automatic lookup and the manual one, which is the point of it existing. The two +/// differ entirely in how they arrive at a track — one guesses from the file and refuses a result +/// whose artist disagrees, the other takes whatever the user picked — and not at all in what to +/// do once they have one. Keeping the second half in one place matters more than the saving, +/// because the rule about putting back what Spotify does not offer is the kind that goes wrong +/// quietly and would drift the moment there were two copies. +/// +/// +internal static class SpotifyCatalogEnricher +{ + /// + /// How many artist genres reach the tag — the same three the recording path takes, so a + /// library tagged by both does not end up with two ideas of how long a genre tag is. + /// + private const int MaximumGenres = 3; + + /// Copies the match onto the track, then fills in what a track object cannot say. + /// + /// + /// The same three-step shape the recording path uses: the track carries title, artist and + /// position; the album carries year and cover art; and genre lives on the artist, because + /// Spotify has no genre field on a track and stopped populating the album's for most of the + /// catalogue in late 2024. + /// + /// + /// What the track already had is put back when Spotify offers nothing. The album + /// mapping assigns genre and year unconditionally — right for a recording, where the track + /// starts empty and Spotify is the only source there is, and wrong on the Metadata page, + /// where it starts as the file's own tags. Spotify returns an empty genre list for most of + /// its catalogue, so without this a lookup silently blanks a genre the user curated. Nothing + /// was ever written — the writer skips empty values — but the row reported a change it would + /// not make, and once the page started showing before-and-after it read as an offer to erase. + /// + /// + public static async Task ApplyAsync( + ISpotifyClient client, + Track track, + FullTrack match, + CancellationToken cancellationToken) + { + var seededGenres = track.Genres; + var seededYear = track.Year; + + SpotifyTrackMapper.Apply(track, match); + + var albumId = match.Album?.Id; + + if (!string.IsNullOrEmpty(albumId)) + { + SpotifyTrackMapper.Apply(track, await client.Albums.Get(albumId, cancellationToken)); + } + + track.Year ??= seededYear; + + if (track.Genres is { Length: > 0 }) return; + + track.Genres = seededGenres; + + if (track.Genres is { Length: > 0 }) return; + + var artistId = match.Artists?.FirstOrDefault(artist => !string.IsNullOrEmpty(artist.Id))?.Id; + + if (string.IsNullOrEmpty(artistId)) return; + + var artistDetail = await client.Artists.Get(artistId, cancellationToken); + + if (artistDetail?.Genres is { Count: > 0 } genres) + { + track.Genres = [.. genres.Take(MaximumGenres)]; + } + } +} diff --git a/src/Offstream.Core/Metadata/Library/SpotifyMatchSearch.cs b/src/Offstream.Core/Metadata/Library/SpotifyMatchSearch.cs new file mode 100644 index 0000000..7235a13 --- /dev/null +++ b/src/Offstream.Core/Metadata/Library/SpotifyMatchSearch.cs @@ -0,0 +1,124 @@ +using Offstream.Core.Metadata.Providers; +using Serilog; +using SpotifyAPI.Web; + +namespace Offstream.Core.Metadata.Library; + +/// Searching Spotify's catalogue with the user's own words. +/// +/// +/// The answer to a wrong automatic match. builds its +/// query from the file's own fields and then rejects any result whose artist disagrees with them, +/// which is right when the file is roughly correct and useless when it is not: a file whose +/// artist is wrong cannot be corrected by a search that requires the wrong artist to match. Asking +/// again returns the same answer, however many times it is asked. +/// +/// +/// So the query is free text, taken verbatim, and every result comes back for the user to choose +/// from. Nothing is filtered on the way out — the person reading the list can tell a live version +/// from a studio one, and a filter that could hide the right answer is worse here than a list with +/// a few wrong ones in it. +/// +/// +public sealed class SpotifyMatchSearch(ISpotifyClient client) : ILibraryMatchSearch +{ + /// + /// How many results to show. + /// + /// + /// Enough to cover the remaster, the live take and the compilation appearance of one song, + /// short enough to read without scrolling the row off the screen. A longer list is not more + /// useful: past this the results stop being versions of what was asked for. + /// + private const int SearchLimit = 5; + + /// + public async Task> SearchAsync( + string query, + CancellationToken cancellationToken = default) + { + if (string.IsNullOrWhiteSpace(query)) return []; + + try + { + var request = new SearchRequest(SearchRequest.Types.Track, query.Trim()) + { + Limit = SearchLimit, + }; + + var response = await client.Search.Item(request, cancellationToken); + var results = response?.Tracks?.Items; + + if (results is null or { Count: 0 }) return []; + + return [.. results.Where(result => result is not null).Select(Describe)]; + } + catch (APIException ex) + { + Log.Warning(ex, "Searching Spotify for {Query} failed.", query); + + throw new MetadataLookupException(SpotifySearchMetadataProvider.Describe(ex), ex); + } + } + + /// + /// + /// The chosen track is fetched again rather than tagged from the search result, because a + /// search result is not a full track: the release date lives on the album and the genre on the + /// artist. Going through the same enrichment the automatic path uses is also what keeps a + /// hand-picked match from behaving differently to a found one. + /// + public async Task ApplyAsync( + Track track, + LibraryMatchCandidate candidate, + CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(track); + ArgumentNullException.ThrowIfNull(candidate); + + try + { + var match = await client.Tracks.Get(candidate.Id, cancellationToken); + + if (match is null) return; + + // The file's own artwork is dropped, and only on this path. Everywhere else the rule + // is that an embedded picture wins over a provider's URL, which is right when a + // lookup is confirming what the file already says. Here the user has just declared + // the file to be a different song, so its cover belongs to the wrong track — keeping + // it would write correct tags and the previous artist's sleeve into the same file. + track.AlbumArtImage = null; + + await SpotifyCatalogEnricher.ApplyAsync(client, track, match, cancellationToken); + } + catch (APIException ex) + { + Log.Warning(ex, "Applying the chosen Spotify match {Id} failed.", candidate.Id); + + throw new MetadataLookupException(SpotifySearchMetadataProvider.Describe(ex), ex); + } + } + + /// Flattens one result into the four things that tell two versions apart. + private static LibraryMatchCandidate Describe(FullTrack result) => new( + result.Id ?? string.Empty, + result.Name ?? string.Empty, + string.Join(", ", result.Artists?.Select(artist => artist.Name) ?? []), + result.Album?.Name ?? string.Empty, + ReleaseYear(result.Album?.ReleaseDate), + SmallestImage(result.Album?.Images)); + + private static int? ReleaseYear(string? releaseDate) => + releaseDate is { Length: >= 4 } && int.TryParse(releaseDate[..4], out var year) ? year : null; + + /// + /// The smallest image Spotify offers, because this is a thumbnail in a list. + /// + /// + /// Eight results at full album-art resolution is several megabytes downloaded to draw eight + /// squares of forty pixels. Spotify orders its images widest first, so the last is the one to + /// take. + /// + private static string? SmallestImage(List? images) => + images is { Count: > 0 } ? images[^1].Url : null; +} diff --git a/src/Offstream.Core/Metadata/Library/SpotifySearchMetadataProvider.cs b/src/Offstream.Core/Metadata/Library/SpotifySearchMetadataProvider.cs index 623c7ff..59378d5 100644 --- a/src/Offstream.Core/Metadata/Library/SpotifySearchMetadataProvider.cs +++ b/src/Offstream.Core/Metadata/Library/SpotifySearchMetadataProvider.cs @@ -47,12 +47,6 @@ public sealed class SpotifySearchMetadataProvider(ISpotifyClient client) : IMeta /// private const int SearchLimit = 5; - /// - /// How many artist genres reach the tag — the same three the recording path takes, so a - /// library tagged by both does not end up with two ideas of how long a genre tag is. - /// - private const int MaximumGenres = 3; - /// public MetadataProvider Kind => MetadataProvider.Spotify; @@ -85,7 +79,7 @@ public async Task EnrichAsync(Track track, CancellationToken cancellationT return false; } - await ApplyAsync(track, match, cancellationToken); + await SpotifyCatalogEnricher.ApplyAsync(client, track, match, cancellationToken); return true; } @@ -137,65 +131,13 @@ private static bool Same(string? left, string? right) => && !string.IsNullOrWhiteSpace(right) && string.Equals(left.Trim(), right.Trim(), StringComparison.OrdinalIgnoreCase); - /// Copies the match onto the track, then fills in what a track object cannot say. - /// - /// - /// The same three-step shape the recording path uses: the track carries title, artist and - /// position; the album carries year and cover art; and genre lives on the artist, because - /// Spotify has no genre field on a track and stopped populating the album's for most of the - /// catalogue in late 2024. - /// - /// - /// What the file already had is put back when Spotify offers nothing. The album - /// mapping assigns genre and year unconditionally — right for a recording, where the track - /// starts empty and Spotify is the only source there is, and wrong here, where the track - /// starts as the file's own tags. Spotify returns an empty genre list for most of its - /// catalogue, so without this a lookup silently blanks a genre the user curated. Nothing was - /// ever written — the writer skips empty values — but the row reported a change it would not - /// make, and once the page started showing before-and-after it read as an offer to erase. - /// - /// - private async Task ApplyAsync(Track track, FullTrack match, CancellationToken cancellationToken) - { - var seededGenres = track.Genres; - var seededYear = track.Year; - - SpotifyTrackMapper.Apply(track, match); - - var albumId = match.Album?.Id; - - if (!string.IsNullOrEmpty(albumId)) - { - SpotifyTrackMapper.Apply(track, await client.Albums.Get(albumId, cancellationToken)); - } - - track.Year ??= seededYear; - - if (track.Genres is { Length: > 0 }) return; - - track.Genres = seededGenres; - - if (track.Genres is { Length: > 0 }) return; - - var artistId = match.Artists?.FirstOrDefault(artist => !string.IsNullOrEmpty(artist.Id))?.Id; - - if (string.IsNullOrEmpty(artistId)) return; - - var artistDetail = await client.Artists.Get(artistId, cancellationToken); - - if (artistDetail?.Genres is { Count: > 0 } genres) - { - track.Genres = [.. genres.Take(MaximumGenres)]; - } - } - /// Turns Spotify's refusal into a sentence, keeping Spotify's own words in it. /// /// The status decides what to add, never what to replace. A 403 in particular means either /// "this account is not on the dashboard app's list" or "the app is past its quota", and only /// the body tells them apart — so the body leads and the hint follows. /// - private static string Describe(APIException ex) + internal static string Describe(APIException ex) { var reason = SpotifyMetadataProvider.ReasonFor(ex) ?? "no reason given"; diff --git a/tests/Offstream.Core.Tests/Metadata/Library/SpotifyMatchSearchTests.cs b/tests/Offstream.Core.Tests/Metadata/Library/SpotifyMatchSearchTests.cs new file mode 100644 index 0000000..88e3125 --- /dev/null +++ b/tests/Offstream.Core.Tests/Metadata/Library/SpotifyMatchSearchTests.cs @@ -0,0 +1,257 @@ +using System.Net; +using Moq; +using Offstream.Core.Metadata; +using Offstream.Core.Metadata.Library; +using SpotifyAPI.Web; +using SpotifyAPI.Web.Http; +using Xunit; + +namespace Offstream.Core.Tests.Metadata.Library; + +/// +/// Searching the catalogue by hand, for the match the automatic path got wrong. +/// +/// +/// The interesting difference from is that this +/// one has no opinion about the results. The automatic lookup exists to refuse a wrong answer; +/// this exists because the user has already seen the wrong answer and wants the list. +/// +public sealed class SpotifyMatchSearchTests +{ + /// Every result comes back, in Spotify's order. + /// + /// Deliberately unfiltered. A search for a song with a live version and a remaster returns + /// three rows that a person can tell apart at a glance and a matcher cannot — and a filter + /// that could hide the right answer is worse here than a list with wrong ones in it. + /// + [Fact] + public async Task Search_ReturnsEveryResultWithoutJudgingThem() + { + var harness = new Harness(); + harness.Returns( + Result("1", "Mr. Wendal", "Arrested Development", "3 Years", "1992-03-24"), + Result("2", "Mr. Wendal - Live", "Someone Else Entirely", "Unplugged", "1993")); + + var results = await harness.Search.SearchAsync("mr wendal"); + + Assert.Equal(2, results.Count); + Assert.Equal("Mr. Wendal", results[0].Title); + Assert.Equal("Someone Else Entirely", results[1].Artist); + } + + /// The year is carried, because it is what separates two versions of one song. + [Fact] + public async Task Search_CarriesTheReleaseYear() + { + var harness = new Harness(); + harness.Returns(Result("1", "Mr. Wendal", "Arrested Development", "3 Years", "1992-03-24")); + + var results = await harness.Search.SearchAsync("mr wendal"); + + Assert.Equal(1992, results[0].Year); + } + + /// Several artists are joined rather than truncated to the first. + [Fact] + public async Task Search_JoinsEveryArtistOnAResult() + { + var harness = new Harness(); + + var result = Result("1", "Thank Me", "Able Heart", "Thank Me", "2020"); + result.Artists = [new SimpleArtist { Name = "Able Heart" }, new SimpleArtist { Name = "Qveen Herby" }]; + + harness.Returns(result); + + var results = await harness.Search.SearchAsync("thank me"); + + Assert.Equal("Able Heart, Qveen Herby", results[0].Artist); + } + + /// An empty query is not sent. + /// + /// It would match the catalogue and cost the user a request to learn nothing. + /// + [Theory] + [InlineData("")] + [InlineData(" ")] + public async Task Search_SkipsAnEmptyQuery(string query) + { + var harness = new Harness(); + + Assert.Empty(await harness.Search.SearchAsync(query)); + + harness.SearchClient.Verify( + x => x.Item(It.IsAny(), It.IsAny()), + Times.Never); + } + + /// Spotify's own explanation reaches the caller. + [Fact] + public async Task Search_ThrowsWithSpotifysMessage() + { + var harness = new Harness(); + harness.Fails(HttpStatusCode.Forbidden, "User not registered in the Developer Dashboard"); + + var problem = await Assert.ThrowsAsync( + () => harness.Search.SearchAsync("anything")); + + Assert.Contains("User not registered in the Developer Dashboard", problem.Message, StringComparison.Ordinal); + } + + /// + /// Applying a choice looks the track up properly rather than tagging from the search result. + /// + /// + /// A search result is not a full track: the release date lives on the album and the genre on + /// the artist, so a hand-picked match tagged from the list alone would come out thinner than + /// an automatic one — the same tags, fetched a different way, arriving incomplete. + /// + [Fact] + public async Task Apply_FetchesTheChosenTrackAndItsAlbum() + { + var harness = new Harness(); + harness.ReturnsTrack("1", Result("1", "Mr. Wendal", "Arrested Development", "3 Years", "1992-03-24")); + harness.ReturnsAlbum("album-1", new FullAlbum + { + Id = "album-1", + Name = "3 Years, 5 Months and 2 Days in the Life Of...", + ReleaseDate = "1992-03-24", + Genres = [], + Images = [], + }); + harness.ReturnsArtist("artist-1", "hip hop"); + + var track = new Track { Artist = "Wrong Artist", Title = "Mr Wendal" }; + + await harness.Search.ApplyAsync( + track, + new LibraryMatchCandidate("1", "Mr. Wendal", "Arrested Development", "3 Years", 1992, null)); + + Assert.Equal("Mr. Wendal", track.Title); + Assert.Equal("Arrested Development", track.Artist); + Assert.Equal("3 Years, 5 Months and 2 Days in the Life Of...", track.Album); + Assert.Equal(1992, track.Year); + Assert.Equal(["hip hop"], track.Genres!); + } + + /// A chosen match overrules the file, which is the whole point of choosing it. + /// + /// The automatic path refuses a result whose artist disagrees with the file. This one must do + /// the opposite: the user picked it *because* the file is wrong, so the artist has to be + /// written through 's API tier, which outranks the scraped value. + /// + [Fact] + public async Task Apply_OverwritesAWrongArtistOnTheFile() + { + var harness = new Harness(); + harness.ReturnsTrack("1", Result("1", "Mr. Wendal", "Arrested Development", "3 Years", "1992")); + harness.ReturnsAlbum("album-1", new FullAlbum { Id = "album-1", Name = "3 Years", Genres = [], Images = [] }); + harness.ReturnsArtist("artist-1", "hip hop"); + + var track = new Track { Artist = "Completely Wrong", Title = "Mr Wendal" }; + + await harness.Search.ApplyAsync( + track, + new LibraryMatchCandidate("1", "Mr. Wendal", "Arrested Development", "3 Years", 1992, null)); + + Assert.Equal("Arrested Development", track.Artist); + } + + /// A chosen match drops the artwork belonging to the track it replaced. + /// + /// The one place the "an embedded picture beats a provider's URL" rule is wrong. That rule + /// protects artwork while a lookup confirms what the file already says; picking a different + /// song out of a list says the opposite, and keeping the old cover would write correct tags + /// and the previous artist's sleeve into the same file. + /// + [Fact] + public async Task Apply_DropsTheArtworkOfTheTrackItReplaced() + { + var harness = new Harness(); + harness.ReturnsTrack("1", Result("1", "One More Time", "Daft Punk", "Discovery", "2001")); + harness.ReturnsAlbum("album-1", new FullAlbum + { + Id = "album-1", + Name = "Discovery", + ReleaseDate = "2001-03-12", + Genres = [], + // 300 and not larger: the mapper takes the biggest cover at or under 300px. + Images = [new SpotifyAPI.Web.Image { Url = "https://example.invalid/discovery.jpg", Width = 300 }], + }); + harness.ReturnsArtist("artist-1", "french house"); + + var track = new Track + { + Artist = "AC", + Title = "Who Made Who", + AlbumArtImage = [1, 2, 3, 4], + }; + + await harness.Search.ApplyAsync( + track, + new LibraryMatchCandidate("1", "One More Time", "Daft Punk", "Discovery", 2001, null)); + + Assert.Null(track.AlbumArtImage); + Assert.Equal("https://example.invalid/discovery.jpg", track.AlbumArtUrl); + } + + private static FullTrack Result(string id, string name, string artist, string album, string released) => new() + { + Id = id, + Name = name, + Album = new SimpleAlbum { Id = "album-1", Name = album, ReleaseDate = released, Images = [] }, + Artists = [new SimpleArtist { Id = "artist-1", Name = artist }], + }; + + private sealed class Harness + { + public Harness() + { + Client.SetupGet(x => x.Search).Returns(SearchClient.Object); + Client.SetupGet(x => x.Albums).Returns(Albums.Object); + Client.SetupGet(x => x.Artists).Returns(Artists.Object); + Client.SetupGet(x => x.Tracks).Returns(Tracks.Object); + } + + public Mock Client { get; } = new(); + + public Mock SearchClient { get; } = new(); + + public Mock Albums { get; } = new(); + + public Mock Artists { get; } = new(); + + public Mock Tracks { get; } = new(); + + public SpotifyMatchSearch Search => new(Client.Object); + + public void Returns(params FullTrack[] results) => + SearchClient + .Setup(x => x.Item(It.IsAny(), It.IsAny())) + .ReturnsAsync(new SearchResponse + { + Tracks = new Paging { Items = [.. results] }, + }); + + public void ReturnsTrack(string id, FullTrack track) => + Tracks.Setup(x => x.Get(id, It.IsAny())).ReturnsAsync(track); + + public void ReturnsAlbum(string albumId, FullAlbum album) => + Albums.Setup(x => x.Get(albumId, It.IsAny())).ReturnsAsync(album); + + public void ReturnsArtist(string artistId, params string[] genres) => + Artists + .Setup(x => x.Get(artistId, It.IsAny())) + .ReturnsAsync(new FullArtist { Id = artistId, Genres = [.. genres] }); + + public void Fails(HttpStatusCode status, string message) + { + var response = new Mock(); + response.SetupGet(x => x.StatusCode).Returns(status); + + SearchClient + .Setup(x => x.Item(It.IsAny(), It.IsAny())) + .ThrowsAsync(new APIException(message) { Response = response.Object }); + } + } +} diff --git a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs index e8350b2..6a43db7 100644 --- a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs +++ b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs @@ -220,6 +220,30 @@ public void MatchDetails_IgnoreArtworkTheFileAlreadyHas() Assert.False(row.HasMatchDetails); } + /// The thumbnail follows a match that brought a URL rather than bytes. + /// + /// A lookup normally returns a link and no picture — the writer downloads it at save time — + /// so a thumbnail bound to the decoded image alone showed the artwork the file already had + /// while claiming to show what saving would do. + /// + [Fact] + public void CoverPreview_FollowsAUrlWhenTheMatchBroughtNoBytes() + { + var track = new LibraryTrack( + @"C:\Music\01 Who Made Who.mp3", + new Track { Title = "Who Made Who", Artist = "AC", Album = "Who Made Who" }); + + var row = new LibraryTrackViewModel(track); + + track.Suggested.AlbumArtImage = null; + track.Suggested.AlbumArtUrl = "https://example.invalid/discovery.jpg"; + row.RefreshFromSuggestion(); + + var source = Assert.IsType(row.SuggestedCoverArtSource); + + Assert.Equal("https://example.invalid/discovery.jpg", source.ToString()); + } + private static LibraryTrackViewModel Row( string? title = "Running Up That Hill", string? artist = "Kate Bush", diff --git a/tests/Offstream.UI.Tests/MetadataViewModelTests.cs b/tests/Offstream.UI.Tests/MetadataViewModelTests.cs index f1a1a16..2aa2f33 100644 --- a/tests/Offstream.UI.Tests/MetadataViewModelTests.cs +++ b/tests/Offstream.UI.Tests/MetadataViewModelTests.cs @@ -1,3 +1,4 @@ +using Offstream.App.Resources; using Offstream.App.Services; using Offstream.App.ViewModels; using Offstream.Core.Metadata; @@ -380,6 +381,158 @@ public async Task Filter_ClearedShowsEverythingAgain() Assert.False(viewModel.IsFiltered); } + /// A manual search fills the row with results to choose from. + [Fact] + public async Task SearchMatches_OffersWhatSpotifyReturned() + { + var search = new FakeMatchSearch( + Candidate("1", "Mr. Wendal", "Arrested Development", "3 Years", 1992), + Candidate("2", "Mr. Wendal - Live", "Arrested Development", "Unplugged", 1993)); + + var viewModel = Build( + new FakeScanner(Scanned("a.mp3", "Wrong Artist", "Mr Wendal", null)), + chain: new FakeChain(null) { MatchSearch = search }); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var row = viewModel.Tracks[0]; + row.MatchQuery = "arrested development mr wendal"; + + await viewModel.SearchMatchesCommand.ExecuteAsync(row); + + Assert.Equal(2, row.Candidates.Count); + Assert.True(row.HasCandidates); + Assert.Equal("arrested development mr wendal", search.LastQuery); + } + + /// + /// Choosing a result rewrites the row, including over a wrong artist. + /// + /// + /// The case nothing else on the page can reach. Auto-fetch skips a row with all three fields + /// filled in, and re-fetch searches on those same fields and then refuses any result whose + /// artist disagrees with them — so a file confidently tagged with the wrong artist is exactly + /// the file the automatic path can never correct, however many times it is run. + /// + [Fact] + public async Task UseMatch_RewritesTheRowFromTheChosenResult() + { + var search = new FakeMatchSearch( + Candidate("1", "Mr. Wendal", "Arrested Development", "3 Years", 1992)); + + var viewModel = Build( + new FakeScanner(Scanned("a.mp3", "Wrong Artist", "Mr Wendal", "Wrong Album")), + chain: new FakeChain(null) { MatchSearch = search }); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var row = viewModel.Tracks[0]; + row.MatchQuery = "arrested development mr wendal"; + + await viewModel.SearchMatchesCommand.ExecuteAsync(row); + await viewModel.UseMatchCommand.ExecuteAsync(row.Candidates[0]); + + Assert.Equal("Mr. Wendal", row.Title); + Assert.Equal("Arrested Development", row.Artist); + Assert.Equal("3 Years", row.Album); + Assert.Equal(LibraryTrackStatus.Fetched, row.Status); + Assert.True(row.HasPendingChanges); + } + + /// The results close once one has been taken. + [Fact] + public async Task UseMatch_ClearsTheResultsAfterwards() + { + var search = new FakeMatchSearch( + Candidate("1", "Mr. Wendal", "Arrested Development", "3 Years", 1992)); + + var viewModel = Build( + new FakeScanner(Scanned("a.mp3", "A", "T", null)), + chain: new FakeChain(null) { MatchSearch = search }); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var row = viewModel.Tracks[0]; + row.MatchQuery = "anything"; + + await viewModel.SearchMatchesCommand.ExecuteAsync(row); + await viewModel.UseMatchCommand.ExecuteAsync(row.Candidates[0]); + + Assert.Empty(row.Candidates); + Assert.False(row.HasCandidates); + } + + /// A search that matched nothing says so on the row. + [Fact] + public async Task SearchMatches_SaysWhenNothingMatched() + { + var viewModel = Build( + new FakeScanner(Scanned("a.mp3", "A", "T", null)), + chain: new FakeChain(null) { MatchSearch = new FakeMatchSearch() }); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var row = viewModel.Tracks[0]; + row.MatchQuery = "nothing at all"; + + await viewModel.SearchMatchesCommand.ExecuteAsync(row); + + Assert.Empty(row.Candidates); + Assert.Equal(Strings.MetadataSearchNoResults, row.SearchMessage); + } + + /// Spotify's own words reach the row when a search fails. + [Fact] + public async Task SearchMatches_SurfacesTheProvidersMessage() + { + var search = new FakeMatchSearch { Failure = "Spotify refused the request: quota exceeded." }; + + var viewModel = Build( + new FakeScanner(Scanned("a.mp3", "A", "T", null)), + chain: new FakeChain(null) { MatchSearch = search }); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var row = viewModel.Tracks[0]; + row.MatchQuery = "anything"; + + await viewModel.SearchMatchesCommand.ExecuteAsync(row); + + Assert.Equal("Spotify refused the request: quota exceeded.", row.SearchMessage); + Assert.False(row.IsSearching); + } + + /// Without a Spotify sign-in the row says what is missing. + /// + /// Last.fm cannot stand in here. Its lookup answers a question about a named artist and + /// title — it has no results to offer someone who does not yet know what the track is called. + /// + [Fact] + public async Task SearchMatches_SaysWhenSpotifyIsNotSignedIn() + { + var viewModel = Build( + new FakeScanner(Scanned("a.mp3", "A", "T", null)), + chain: new FakeChain(null)); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var row = viewModel.Tracks[0]; + row.MatchQuery = "anything"; + + await viewModel.SearchMatchesCommand.ExecuteAsync(row); + + Assert.Equal(Strings.MetadataSearchNeedsSpotify, row.SearchMessage); + Assert.Empty(row.Candidates); + } + + private static LibraryMatchCandidate Candidate( + string id, + string title, + string artist, + string album, + int? year) => + new(id, title, artist, album, year, null); + private static MetadataViewModel Build( ILibraryScanner? scanner = null, ILibraryTagWriter? writer = null, @@ -424,8 +577,48 @@ public Task SaveAsync(LibraryTrack track, CancellationToken private sealed class FakeChain(IMetadataProvider? provider) : ILibraryMetadataChain { + public ILibraryMatchSearch? MatchSearch { get; init; } + public FallbackMetadataProvider Create() => provider is null ? new FallbackMetadataProvider() : new FallbackMetadataProvider(provider); + + public ILibraryMatchSearch? CreateMatchSearch() => MatchSearch; + } + + /// A manual search that answers with whatever it was handed. + private sealed class FakeMatchSearch(params LibraryMatchCandidate[] results) : ILibraryMatchSearch + { + public string? LastQuery { get; private set; } + + public LibraryMatchCandidate? Applied { get; private set; } + + public string? Failure { get; init; } + + public Task> SearchAsync( + string query, + CancellationToken cancellationToken = default) + { + LastQuery = query; + + if (Failure is not null) throw new MetadataLookupException(Failure); + + return Task.FromResult>(results); + } + + public Task ApplyAsync( + Track track, + LibraryMatchCandidate candidate, + CancellationToken cancellationToken = default) + { + Applied = candidate; + + track.SetTitleFromApi(candidate.Title); + track.SetArtistFromApi(candidate.Artist); + track.Album = candidate.Album; + track.Year = candidate.Year; + + return Task.CompletedTask; + } } private sealed class CountingProvider : IMetadataProvider From 9c87474edd1ce0222f14420352738038e693d042 Mon Sep 17 00:00:00 2001 From: revtex Date: Sun, 30 Aug 2026 11:54:20 -0400 Subject: [PATCH 2/4] Rebuild the row editor, and fix three things under it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The panel that opens under a track was laid out as a form when what it shows is a comparison. Labels hung in a right-aligned gutter of the page's own invention, one field per line, eight rows deep — a shape no other page in the application uses, so it read as a foreign object, and it pushed the answer to "is this match right?" off the bottom of the list. It is now the recessed well already used elsewhere for "here is what was found", with the label above its field the way every other page writes one, the four fields paired into two columns, and what the match found beside them instead of in a section of its own. Capping the panel's width turned out to be the interesting part. A MaxWidth on a panel aligned Left arranges it at its *desired* width, and a Grid's desired width is what its columns' content asked for — so every box shrank to the word inside it, a 130px box for "Thank Me" beside a 270px one for "Able Heart, Qveen Herby". The cap is now a star column carrying MaxWidth beside an empty Auto one, which is the idiom the rest of the file already uses. Underneath that, three defaults were wrong. The list scrolled by item, so a wheel notch moved three rows and a row here is a three-line block — most of a screen at a time; pixel scrolling moves 48 DIP instead, which was measured rather than assumed. A row had a selected state that nothing on the page acts on, drawn brightly over rows that were neither ticked nor open; removing it takes a template of a bare ContentPresenter, because the themed one paints an accent bar the brush setters do not reach. Focus still moves, and the focus rectangle still shows where it is. And the search box was seeded from the toggle command, which only the row's title runs, so opening a row by its chevron — the control that looks like the way to do it — left the box empty and searching from there asked Spotify for nothing. The last one is a fourth member of the genre false-positive family, and the earlier fix had been made in the wrong place. Putting the file's own genre and year back when the lookup has none was done inside SpotifyCatalogEnricher, which is no help when the chain falls through to Last.fm — its EnrichAsync assigns the artist's tags just as unconditionally and returns nothing for an artist nobody has tagged. The rule now sits at the call site every library lookup passes through, so it holds for whichever provider answers and for any provider added later. Nothing was ever written wrongly — the writer skips empty values — but the row claimed a change Save would not make, and the rebuilt panel spelled it out as an offer to erase a curated tag. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 17 +- CLAUDE.md | 14 + README.md | 11 +- docs/MODERNIZATION-PLAN.md | 16 + .../ViewModels/LibraryTrackViewModel.cs | 14 +- .../ViewModels/MetadataViewModel.cs | 16 + .../Views/Pages/MetadataPage.xaml | 569 ++++++++++-------- .../LibraryTrackViewModelTests.cs | 28 + .../MetadataViewModelTests.cs | 48 ++ 9 files changed, 461 insertions(+), 272 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f04342c..194582d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,7 +97,22 @@ phase plan these entries follow. correct a bad match except to type all three fields by hand and give up the year, genre and artwork that come with a real one — re-fetching could not do it, because the automatic lookup builds its query from the file's own fields and then refuses any result whose artist disagrees - with them, which is precisely the case where the file is what is wrong. + with them, which is precisely the case where the file is what is wrong. The list scrolls by + pixel rather than by row, because a wheel notch moved three rows and a row here is three lines + of text — most of a screen at a time. A row has no selected state either: nothing on the page + acts on "the selected row", so a highlight following the arrow keys was a third kind of emphasis + competing with the tick box, which says what Save writes, and the chevron, which says what is + open. The panel that opens is a recessed well with its labels above its fields, the shape the + rest of the application uses, with the four fields in two columns and what the match found + beside them; two earlier versions were laid out as a form instead of as a comparison, and the + second one hung its labels in a right-aligned gutter of its own invention and ran eight rows + deep. Opening a row by its chevron now fills the search box the way opening it by its title + always did — the seeding hung off the title's command, so the control that looks like the way to + open a row left the box empty and searching from there asked Spotify for nothing at all. And a + lookup that has no genre for a track no longer offers to remove the one the file has: every + provider assigns genre and year unconditionally, which is right where they were written, tagging + a recording that starts with nothing, and wrong here, where the track starts as the file's own + tags. - **Looking a track up now falls back from Spotify to Last.fm instead of stopping at whichever one the Settings page names.** That setting answers "who tags a recording as it is made", where one source keeps a library consistent. Repairing files already on disk is the opposite problem — diff --git a/CLAUDE.md b/CLAUDE.md index d8149f3..102666f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,6 +54,20 @@ These are load-bearing; violating them breaks the app in ways that are not obvio - **Never enable aggressive trimming.** WPF trims poorly and settings/localisation are reflection-driven. Publish self-contained and untrimmed. - **`Offstream.Core` must not reference WPF or `System.Windows`.** The predecessor passed the form itself into its watcher and recorder; Offstream uses events or `IProgress` instead. - **All conversion goes through ffmpeg.** No NAudio.Lame, no bundled LAME DLLs. This governs *conversion*, and only that. **Retagging a file that already exists goes through TagLib#** (decided 2026-08-29, `Metadata/Library/TagLibTagStore.cs`), because ffmpeg cannot edit a tag in place — the equivalent is remuxing the whole file to change one string, which rewrites audio that had nothing wrong with it. So "ffmpeg writes every textual tag" stays true of the recording pipeline and is false of the Metadata page. Two rules follow from having a second writer: it writes tags and cover art in **one** TagLib# session rather than calling `CoverArtWriter` after itself, which would save the file twice for one edit; and it pins `Id3v2.Tag.DefaultVersion = 3`, because TagLib# defaults to v2.4 while this project ships v2.3 — a retag that silently upgraded the tag would make tags vanish from Windows Explorer and Media Player on files that displayed correctly before Offstream touched them. +- **A lookup on the Metadata page adds tags; it never takes one away** (decided 2026-08-30). Every + provider assigns genre and year unconditionally — right where they were written, tagging a + recording that starts with nothing and where the provider is the only source there is, and wrong + on this page, where the track starts as the file's own tags. Spotify returns an empty genre list + for most of its catalogue since late 2024 and Last.fm returns none for an artist nobody has + tagged, so without this a lookup blanks a genre the user curated. Nothing ever reaches the file — + `TagLibTagStore` writes a genre only when there is one — but the row reports a change Save will + not make, and the editor spells it out as an offer to erase. It lives in **two** places, and + both are load-bearing. `MetadataViewModel.FetchOneAsync` is the call site every automatic lookup + passes through, so the rule holds there for whichever provider in the chain answers and for any + provider added later — it was fixed once inside the Spotify path alone and the Last.fm fallback + still had the hole. `SpotifyCatalogEnricher` keeps its own copy because the manual-match path + (**Use this**, via `SpotifyMatchSearch.ApplyAsync`) reaches the enricher without going through + `FetchOneAsync`. Deleting either one restores the bug on one of the two paths. - **Use `ProcessStartInfo.ArgumentList`, never a command string.** Track metadata comes from Spotify window titles and is untrusted. The argv array prevents argument injection structurally; the old app needed hand-written `CommandLineToArgvW` escaping because .NET Framework lacked `ArgumentList`. ## Spotify Web API rules diff --git a/README.md b/README.md index f396fce..627d735 100644 --- a/README.md +++ b/README.md @@ -148,10 +148,12 @@ Nothing reaches a file until you press Save. Each file is one row showing what * click on the text) opens Title, Artist and Album as editable boxes, with a **was …** line under anything you'd be changing, so a match that got the remix wrong is a correction rather than a reason to start over. Rows that would actually alter a file are marked **Will change**, which is -how you find the three that need attention without opening the other hundred. Under the boxes, +how you find the three that need attention without opening the other hundred. Beside the boxes, **Also from the match** shows what a lookup found that has no box of its own — the year, the genre and the artwork, before and after — because Save writes all three and a row that says it will -change while every box matches the file otherwise looks like a mistake. +change while every box matches the file otherwise looks like a mistake. Only what a lookup +actually found appears there: a provider that has no genre for a track leaves the file's own alone +rather than proposing to remove it. **Filter** narrows the list to the rows whose title, artist, album or file name contain what you type. It only changes what is on screen: Save still writes every ticked row, including the ones the @@ -161,8 +163,9 @@ filter is hiding. Untick the row and nothing happens to that file. To fix it instead, either type the correct values into Title, Artist and Album — they're yours to edit and they win over anything a lookup found — -or use **Not the right track?** at the bottom of the opened row. That searches Spotify for whatever -you type and lists what it finds, with the year beside each result so you can tell a remaster from +or use **Not the right track?** at the bottom of the opened row. The box starts filled in with the +row's artist and title, so it is usually a word or two away from the right query. It searches +Spotify for whatever you type and lists what it finds, with the year beside each result so you can tell a remaster from the original, and **Use this** fills the row in from the one you pick. Reach for the search rather than **Re-fetch** whenever the artist is the thing that's wrong. diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index 1d7d304..ec9d202 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -955,6 +955,22 @@ So `ILibraryMatchSearch` is a second, deliberately different shape: **the user's That last decision then exposed the display bug beneath it. `CoverArt` was bound to decoded bytes, but a lookup normally returns a **URL** and no bytes — the writer downloads it at save time — so the thumbnail showed the artwork the file already had while claiming to show what saving would do. On a hand-picked match it put the replaced track's sleeve on both sides of the before-and-after. The fix is to bind to either a decoded image or a `Uri` and let WPF's `Image.Source` fetch the second, which also keeps the network off the view model and off whatever thread a fetch finished on. **Both times on this page, the thing that made a bug visible was drawing the value rather than testing it.** +### Finding: the row editor was laid out as a form, and three WPF defaults were wrong underneath it (2026-08-30) + +Four defects on one page, all of them invisible to the build and to the suite, and all found by running the app and looking at it. + +**A star column inside a panel aligned `Left` is a content-sized column.** The editor was capped with `` around a two-column `Grid`. A `Left`-aligned element is *arranged* at its desired width, a `Grid`'s desired width is the sum of what its columns' content asked for, and a star column contributes its content — not its share of the available space. So every text box shrank to the width of the word inside it: a 130px box for "Thank Me" beside a 270px one for "Able Heart, Qveen Herby". The working idiom is the one already used elsewhere in the file — a star column carrying `MaxWidth` beside an empty `Auto` one, inside a Grid that is allowed to stretch. `MaxWidth` on a `Stretch` element is not the answer either: WPF centres what it cannot stretch, which pushes the whole panel off its own indent. + +**`VirtualizingPanel.ScrollUnit` defaults to `Item`, which is wrong for any list whose rows are taller than a line.** Three items per wheel notch is a sensible default for a list of names; here a row is a three-line block and an opened one is a whole editor, so a notch moved most of a screen. `ScrollUnit="Pixel"` is a one-attribute fix and it works with `VirtualizationMode="Recycling"`. Measure it rather than trusting it: with UIAutomation, read a `ListBoxItem`'s `BoundingRectangle.Y`, send one `mouse_event` wheel notch over the list, and read it again. It moved **72 physical pixels** (48 DIP at 150%) against a row height of 115, where the default had been moving three whole rows. + +**Removing a selection highlight means replacing the `ListBoxItem` template, not overriding its brushes.** The themed template paints its own accent bar as well as a background, so setters for `Background` and the `IsSelected` triggers leave a band behind. A `ControlTemplate` of a bare `ContentPresenter` removes the paint and keeps everything else: focus still moves, arrow keys still work, and the default focus rectangle still shows where the keyboard is. Worth doing on this page because nothing here acts on "the selected row" — the tick box and the chevron each carry their own meaning, and a third highlight tracking the arrow keys only competes with them. + +**A behaviour hung off a `[RelayCommand]` is a behaviour the other route does not get.** The search box was seeded in `ToggleExpandCommand`, which only the row's title runs. Expanding by the chevron sets `IsExpanded` through its `IsChecked` binding and never touches the command, so the affordance that *looks* like the way to open a row left the box empty and a search from there asked Spotify for the empty string. `partial void OnIsExpandedChanged` is the seam that both routes cross. This is a general trap with `[ObservableProperty]` two-way bound to a control: the property is the event, the command is not. + +And a fourth member of the genre false-positive family, which the earlier fix had missed because it was made in the wrong place. Putting the file's genre and year back when Spotify offers nothing was done inside `SpotifyCatalogEnricher` — correct for the Spotify path and no help at all when `FallbackMetadataProvider` falls through to Last.fm, whose `EnrichAsync` assigns `track.Genres = await ArtistTagsAsync(...)` just as unconditionally and returns nothing for an artist nobody has tagged. The rule belongs at the call site every library lookup passes through, `MetadataViewModel.FetchOneAsync`, where it holds for whichever provider answers and for any provider added later. The enricher keeps its own copy because the manual-match path does not go through `FetchOneAsync`. + +One process note, because it cost a wrong diagnosis. Restoring a file with `mv` from a `.bak` preserves the backup's **mtime**, which can be older than the compiled output — so MSBuild considers the project up to date and the next test run silently exercises the previous build. A test that passed, then failed with the fix visibly present in the source, was that and nothing else. `touch` the file after any restore of this kind. + --- ## 12. Risks diff --git a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs index 55b6b04..d85852c 100644 --- a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs +++ b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs @@ -388,11 +388,17 @@ public void SeedMatchQuery() /// Opens or closes this row's fields. [RelayCommand] - private void ToggleExpand() - { - IsExpanded = !IsExpanded; + private void ToggleExpand() => IsExpanded = !IsExpanded; - if (IsExpanded) SeedMatchQuery(); + /// Seeds the search box whichever way the row was opened. + /// + /// This hung off the command, so a row opened by clicking its title got a seeded query and + /// the same row opened by its chevron — the affordance that looks like the way to do it — + /// got an empty box, and searching from there asked Spotify for nothing. + /// + partial void OnIsExpandedChanged(bool value) + { + if (value) SeedMatchQuery(); } private static string Or(string? value, string fallback) => diff --git a/src/Offstream.App/ViewModels/MetadataViewModel.cs b/src/Offstream.App/ViewModels/MetadataViewModel.cs index 2d83ea7..62faa3e 100644 --- a/src/Offstream.App/ViewModels/MetadataViewModel.cs +++ b/src/Offstream.App/ViewModels/MetadataViewModel.cs @@ -461,10 +461,26 @@ private static async Task FetchOneAsync( row.Status = LibraryTrackStatus.Fetching; row.FailureReason = null; + var genres = row.Track.Suggested.Genres; + var year = row.Track.Suggested.Year; + try { var updated = await chain.EnrichAsync(row.Track.Suggested, cancellationToken); + // A lookup on this page adds tags. It never takes one away. + // + // Every provider assigns genre and year unconditionally, which is right where they + // were written — a recording starts with an empty track and the provider is the only + // source there is. Here the track starts as the file's own tags, and a provider that + // knows the song but has no genre for it (Last.fm returns none for an artist nobody + // has tagged; Spotify returns none for most of its catalogue since late 2024) hands + // back an empty list that overwrites one the user curated. Nothing reaches the file — + // the writer skips empty values — but the row said "will change" over a change Save + // would not make, and the panel underneath it spelled the loss out as an offer. + row.Track.Suggested.Genres = row.Track.Suggested.Genres is { Length: > 0 } found ? found : genres; + row.Track.Suggested.Year ??= year; + row.Track.Status = updated ? LibraryTrackStatus.Fetched : LibraryTrackStatus.Untagged; row.Track.FailureReason = null; row.RefreshFromSuggestion(); diff --git a/src/Offstream.App/Views/Pages/MetadataPage.xaml b/src/Offstream.App/Views/Pages/MetadataPage.xaml index 0a0e4d6..361c2f1 100644 --- a/src/Offstream.App/Views/Pages/MetadataPage.xaml +++ b/src/Offstream.App/Views/Pages/MetadataPage.xaml @@ -20,10 +20,15 @@ - + + + - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - - - - - - - - - - - - - - - - - - - - - + + - + - - + + + + - - - - + + - - + + - - - - - - - - - - + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + - + + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -650,11 +667,37 @@ ScrollViewer.HorizontalScrollBarVisibility="Disabled" Visibility="{Binding HasTracks, Converter={StaticResource BooleanToVisibility}}" VirtualizingPanel.IsVirtualizing="True" + VirtualizingPanel.ScrollUnit="Pixel" VirtualizingPanel.VirtualizationMode="Recycling"> + - diff --git a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs index 6a43db7..b03f7f7 100644 --- a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs +++ b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs @@ -244,6 +244,34 @@ public void CoverPreview_FollowsAUrlWhenTheMatchBroughtNoBytes() Assert.Equal("https://example.invalid/discovery.jpg", source.ToString()); } + /// Opening a row fills its search box, however the row was opened. + /// + /// The seeding used to hang off the toggle command, which only the title runs. A row opened + /// by its chevron — the control that looks like the way to open one — got an empty box, and + /// searching from there asked Spotify for nothing at all. + /// + [Fact] + public void Expanding_SeedsTheSearchBoxFromWhatTheRowNowSays() + { + var row = Row(); + + row.IsExpanded = true; + + Assert.Equal("Kate Bush Running Up That Hill", row.MatchQuery); + } + + /// Reopening a row does not overwrite a query the user typed. + [Fact] + public void Expanding_LeavesAQueryTheUserAlreadyTyped() + { + var row = Row(); + + row.MatchQuery = "cloudbusting"; + row.IsExpanded = true; + + Assert.Equal("cloudbusting", row.MatchQuery); + } + private static LibraryTrackViewModel Row( string? title = "Running Up That Hill", string? artist = "Kate Bush", diff --git a/tests/Offstream.UI.Tests/MetadataViewModelTests.cs b/tests/Offstream.UI.Tests/MetadataViewModelTests.cs index 2aa2f33..1070635 100644 --- a/tests/Offstream.UI.Tests/MetadataViewModelTests.cs +++ b/tests/Offstream.UI.Tests/MetadataViewModelTests.cs @@ -92,6 +92,45 @@ public async Task Refetch_LooksUpARowAutoFetchWouldHaveSkipped() Assert.Equal(1, provider.Calls); } + /// A lookup that has no genre does not offer to remove the one the file has. + /// + /// Every provider assigns genre and year unconditionally, which is correct where they are + /// used to tag a recording — the track starts empty there. Here it starts as the file's own + /// tags, so a provider that knows the song and has no genre for it used to hand back an empty + /// list. The writer never wrote it, but the row said "will change" and the panel under it + /// showed the genre being replaced by nothing. + /// + [Fact] + public async Task Refetch_DoesNotOfferToEraseAGenreTheProviderHasNoAnswerFor() + { + var provider = new CountingProvider + { + Result = true, + FetchedTitle = "T", + FetchedArtist = "A", + BlanksGenreAndYear = true, + }; + + var scanned = new LibraryTrack( + @"C:\Music\curated.mp3", + new Track + { + Artist = "A", + Title = "T", + Album = "Al", + Genres = ["pop", "video"], + Year = 1999, + }); + + var viewModel = Build(new FakeScanner(scanned), chain: new FakeChain(provider)); + + await viewModel.ScanCommand.ExecuteAsync(null); + await viewModel.RefetchCommand.ExecuteAsync(viewModel.Tracks[0]); + + Assert.False(viewModel.Tracks[0].HasGenreChange); + Assert.False(viewModel.Tracks[0].HasPendingChanges); + } + /// With nothing configured, the page says so rather than failing every row. [Fact] public async Task AutoFetch_SaysWhenNoSourceIsConfigured() @@ -633,6 +672,9 @@ private sealed class CountingProvider : IMetadataProvider public string? FetchedArtist { get; init; } + /// Answers with an empty genre list and no year, the way a thin match does. + public bool BlanksGenreAndYear { get; init; } + public int Calls { get; private set; } public Task EnrichAsync(Track track, CancellationToken cancellationToken = default) @@ -645,6 +687,12 @@ public Task EnrichAsync(Track track, CancellationToken cancellationToken = track.SetTitleFromApi(FetchedTitle); track.SetArtistFromApi(FetchedArtist); + if (BlanksGenreAndYear) + { + track.Genres = []; + track.Year = null; + } + return Task.FromResult(Result); } } From 8d1f12bbe717a206ac33af9e522622f0b24b89b6 Mon Sep 17 00:00:00 2001 From: revtex Date: Mon, 31 Aug 2026 18:33:58 -0400 Subject: [PATCH 3/4] Put the tag editor beside the list instead of inside a row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The row editor was rebuilt twice and tightened once, and each version was judged by looking at it. Measuring it ended the argument. At the shell's minimum window an expanded row is 539 DIP tall and the list viewport it opens inside is 347 — the editor is 1.55x the height of its own container, so opening one file buried the library it was chosen from and pushed the search results off the bottom edge. That is not a spacing problem and no amount of trimming reaches it. The page owns about 130 DIP of the chrome above the list; the rest is the shell's title bar and nav strip. Spending all of it still leaves a deficit, and the best case is a list showing exactly one file. The cheaper variant — moving the search into a popover — lands an opened row at 347 DIP, which is precisely the viewport and the same failure at one decimal place. So the editor is a pane to the right of the list. The list holds five rows at the minimum window and eleven maximized, the pane is the same size and in the same place whatever is picked, and changing the pick reflows nothing: measured before and after a search that returned ten candidates, the list viewport read 373 DIP both times. Three things followed from the split rather than from taste. The selection highlight comes back. Removing it was right for the old page, where nothing acted on the selected row and a highlight tracking the arrow keys competed with the tick box and the chevron for meaning. Here the selection is what the pane shows, so the row has to say which one it is — a tinted background and a narrow accent edge, not the themed container's saturated fill. It also means ApplyFilter has to capture the pick and put it back, because clearing VisibleTracks makes the list null its own SelectedItem, and without that the editor emptied on every keystroke in the filter box, including the keystrokes narrowing the list towards the row being edited. The pane has two regions, not one scroller. Letting the whole thing scroll put ten results below the fold, where the only evidence the search had worked was the scroll bar getting shorter. The fields scroll; the search box and its results are pinned to the foot of the pane, because the results are what the user just asked for and the fields are what they will scroll back to. Seeding the search box moved from the row's expand command to the page's selection, which is the seam every route now crosses, and the intro paragraph became a tooltip on the heading — the same trade the Advanced page makes, for the same reason. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 44 +- CLAUDE.md | 2 + README.md | 29 +- docs/MODERNIZATION-PLAN.md | 27 + src/Offstream.App/Resources/Strings.fr.resx | 8 +- src/Offstream.App/Resources/Strings.resx | 8 +- .../ViewModels/LibraryTrackViewModel.cs | 29 +- .../ViewModels/MetadataViewModel.cs | 37 + .../Views/Pages/MetadataPage.xaml | 1054 +++++++++-------- .../LibraryTrackViewModelTests.cs | 40 +- .../MetadataViewModelTests.cs | 89 ++ 11 files changed, 744 insertions(+), 623 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 194582d..8cf3795 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,31 +84,25 @@ phase plan these entries follow. an automatic match is wrong often enough that the review step is the feature rather than friction in front of it. Files that already carry a title, artist and album are left alone and cost no request; a single row can still be looked up deliberately when its tags are complete - and wrong. Each file is one compact row showing what would be written, with the fields opening - underneath only when asked — a library is a hundred files of which three are wrong, and a row - marked **Will change** is how the three are found without opening the other ninety-seven. A - **Filter** box narrows the list by title, artist, album or file name for libraries too long to - scroll — it changes what is drawn and nothing else, so Save still writes every ticked row it is - hiding. Beneath the editable fields, **Also from the match** shows the year, genre and artwork a - lookup found: Save writes all three and they appeared nowhere, so a row could announce that it - would change while every field on screen matched the file exactly. And when the match itself is - wrong, **Not the right track?** searches Spotify for whatever you type and lists what it finds, - year included, so a remaster can be told from the original. Until this there was no way to - correct a bad match except to type all three fields by hand and give up the year, genre and - artwork that come with a real one — re-fetching could not do it, because the automatic lookup - builds its query from the file's own fields and then refuses any result whose artist disagrees - with them, which is precisely the case where the file is what is wrong. The list scrolls by - pixel rather than by row, because a wheel notch moved three rows and a row here is three lines - of text — most of a screen at a time. A row has no selected state either: nothing on the page - acts on "the selected row", so a highlight following the arrow keys was a third kind of emphasis - competing with the tick box, which says what Save writes, and the chevron, which says what is - open. The panel that opens is a recessed well with its labels above its fields, the shape the - rest of the application uses, with the four fields in two columns and what the match found - beside them; two earlier versions were laid out as a form instead of as a comparison, and the - second one hung its labels in a right-aligned gutter of its own invention and ran eight rows - deep. Opening a row by its chevron now fills the search box the way opening it by its title - always did — the seeding hung off the title's command, so the control that looks like the way to - open a row left the box empty and searching from there asked Spotify for nothing at all. And a + and wrong. The page is a list beside an editor: each file is one compact row saying what would + be written, and picking a row fills a pane on the right with its fields. A library is a hundred + files of which three are wrong, and a row marked **Will change** is how the three are found + without reading the other ninety-seven. A **Filter** box narrows the list by title, artist, + album or file name for libraries too long to scroll — it changes what is drawn and nothing else, + so Save still writes every ticked row it is hiding. Under the editable fields, **Also from the + match** shows the year, genre and artwork a lookup found: Save writes all three and they + appeared nowhere, so a row could announce that it would change while every field on screen + matched the file exactly. And when the match itself is wrong, **Not the right track?** searches + Spotify for whatever you type and lists what it finds, year included, so a remaster can be told + from the original; it is pinned to the foot of the pane, so the results arrive where the user is + already looking rather than below a fold. Until this there was no way to correct a bad match + except to type all three fields by hand and give up the year, genre and artwork that come with a + real one — re-fetching could not do it, because the automatic lookup builds its query from the + file's own fields and then refuses any result whose artist disagrees with them, which is + precisely the case where the file is what is wrong. The list scrolls by pixel rather than by + row, because a wheel notch moved three rows and a row here is three lines of text — most of a + screen at a time. Picking a row fills its search box from what the row says at that moment, so a + search asks for the track as it has been corrected rather than as it was scanned. And a lookup that has no genre for a track no longer offers to remove the one the file has: every provider assigns genre and year unconditionally, which is right where they were written, tagging a recording that starts with nothing, and wrong here, where the track starts as the file's own diff --git a/CLAUDE.md b/CLAUDE.md index 102666f..c97633c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -108,4 +108,6 @@ Beyond that: ffmpeg argv golden tests, encode-integration tests asserted with ff - MVVM via CommunityToolkit.Mvvm source generators; no code-behind logic beyond wiring. - Inline validation (`INotifyDataErrorInfo`), not modal dialogs. - **A setting's description is a tooltip, not a line under its label** (decided 2026-08-29). The Advanced page has no `ScrollViewer` and the shell's `MinHeight` promises no setting is ever out of reach, so a line of prose per row is height the page does not have — it has been clipped off the bottom three times. Put the sentence in `ToolTip` on the whole row so hovering anywhere finds it, and in `AutomationProperties.HelpText` on the control, because a tooltip is invisible to a screen reader. The text still lives in `Strings.resx` + `.fr.resx` and is still translated. Labels stay inline. +- **The Metadata page is a list beside an editor, and the editor does not go back inside the row** (decided 2026-08-31). Measured at the shell's minimum window, an expanded row was **539 DIP inside a 347 DIP list viewport** — the editor was 1.55× the height of the container it opened in, so opening one row buried the library and pushed the search results off the bottom. Trimming cannot close that: the page owns only ~130 DIP of the chrome above the list, and spending all of it still leaves a deficit. Two consequences are load-bearing. The list's **selected row is the editor's input**, so it carries a quiet highlight — a tinted background and a narrow accent edge from a replaced `ListBoxItem` template, never the themed container's saturated fill; `MetadataViewModel.ApplyFilter` must capture `SelectedTrack` and restore it after refilling `VisibleTracks`, or the editor empties on every keystroke in the filter box. And the pane has **two regions, not one scroller**: the fields scroll, while the search box and its results stay pinned to its foot, because results that arrive below a fold announce themselves only by shortening a scroll bar. Commands bound from the pane use `RelativeSource AncestorType=UserControl` — there is no `ListBox` ancestor out there. +- **Before building an expander, check that the thing it opens is smaller than the space it opens into.** That is a measurement available before the layout is written, and it is the check that two rebuilds of the panel above skipped. When it fails, the answer is master–detail, not tighter spacing. - User-facing strings live in `Offstream.App/Resources/Strings.resx` (+ `.fr.resx`) with an en/fr key-parity test. Resource **keys are re-keyed for Offstream**; do not carry the predecessor's key names across. diff --git a/README.md b/README.md index 627d735..db667ce 100644 --- a/README.md +++ b/README.md @@ -143,17 +143,19 @@ at the time. The **Metadata** tab is where you repair those. but wrong. 3. **Save Selected Tags** writes the ticked rows into the files. -Nothing reaches a file until you press Save. Each file is one row showing what *would* be written -— the title, then the artist and album, then the file it came from. The chevron on the right (or a -click on the text) opens Title, Artist and Album as editable boxes, with a **was …** line under -anything you'd be changing, so a match that got the remix wrong is a correction rather than a -reason to start over. Rows that would actually alter a file are marked **Will change**, which is -how you find the three that need attention without opening the other hundred. Beside the boxes, -**Also from the match** shows what a lookup found that has no box of its own — the year, the genre -and the artwork, before and after — because Save writes all three and a row that says it will -change while every box matches the file otherwise looks like a mistake. Only what a lookup -actually found appears there: a provider that has no genre for a track leaves the file's own alone -rather than proposing to remove it. +Nothing reaches a file until you press Save. The page is a list beside an editor. Each file is one +row showing what *would* be written — the title, then the artist and album, then the file it came +from — and clicking a row, or arrowing onto it, fills the pane on the right with Title, Artist and +Album as editable boxes. There is a **was …** line under anything you'd be changing, so a match +that got the remix wrong is a correction rather than a reason to start over. The pane stays the +same size and in the same place whichever row you pick, so the library never moves out from under +you while you're working on one of its files. Rows that would actually alter a file are marked +**Will change**, which is how you find the three that need attention without reading the other +hundred. Under the boxes, **Also from the match** shows what a lookup found that has no box of +its own — the year, the genre and the artwork, before and after — because Save writes all three +and a row that says it will change while every box matches the file otherwise looks like a +mistake. Only what a lookup actually found appears there: a provider that has no genre for a +track leaves the file's own alone rather than proposing to remove it. **Filter** narrows the list to the rows whose title, artist, album or file name contain what you type. It only changes what is on screen: Save still writes every ticked row, including the ones the @@ -163,8 +165,9 @@ filter is hiding. Untick the row and nothing happens to that file. To fix it instead, either type the correct values into Title, Artist and Album — they're yours to edit and they win over anything a lookup found — -or use **Not the right track?** at the bottom of the opened row. The box starts filled in with the -row's artist and title, so it is usually a word or two away from the right query. It searches +or use **Not the right track?** at the foot of the pane. The box starts filled in with the row's +artist and title, so it is usually a word or two away from the right query, and it keeps its place +at the bottom so results appear where you were already looking. It searches Spotify for whatever you type and lists what it finds, with the year beside each result so you can tell a remaster from the original, and **Use this** fills the row in from the one you pick. diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index ec9d202..19cb8be 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -971,6 +971,33 @@ And a fourth member of the genre false-positive family, which the earlier fix ha One process note, because it cost a wrong diagnosis. Restoring a file with `mv` from a `.bak` preserves the backup's **mtime**, which can be older than the compiled output — so MSBuild considers the project up to date and the next test run silently exercises the previous build. A test that passed, then failed with the fix visibly present in the source, was that and nothing else. `touch` the file after any restore of this kind. +### Finding: an editor that opens inside a list row has to fit inside a list row, and this one never could (2026-08-31) + +The row editor was rebuilt twice and tightened once, and every version was judged by looking at it. Measuring it instead settled the question in one reading. At the shell's minimum window of 1024×700, with UIAutomation reporting `BoundingRectangle` in physical pixels against a 150% scale factor: + +| | DIP | +| --- | --- | +| Window | 700 | +| Page chrome above the list | 542 | +| **Track list viewport** | **347** | +| **One expanded row** | **539** | + +An editor 1.55× the height of the container it opens in is not a spacing problem, and no amount of trimming reaches it. The page owns only about 130 DIP of that chrome — the intro paragraph and the folder, button and filter rows; the rest is the shell's title bar and nav strip and is not this page's to reclaim — so spending **all** of it still leaves a deficit, and the best case is a list showing exactly one file. The cheaper variant that was costed and rejected, moving the search and its results into a popover, lands an opened row at 347 DIP: precisely the viewport, which is the same failure at one decimal place. + +So the editor became a pane beside the list rather than inside it. The list holds 5 rows at the minimum window and 11 maximized, the pane is the same size and in the same place whatever is picked, and choosing a different track reflows nothing — measured before and after a search that returned ten candidates, the list viewport read 373 DIP both times. + +**The general rule is worth more than the fix: an expander is usable only when the thing it opens is smaller than the space it opens into.** That is a measurement, and it is available before the layout is written. Reach for master–detail when it is not, and reach for it early — two rebuilds of the panel went into making a shape work that could not have worked at any spacing. + +Three second-order notes from doing it: + +- **Selection had to come back, and removing it had been right on its own terms.** A highlight following the arrow keys while nothing acted on the selected row was noise. In a split view the selection is the pane's input, so the row has to say which one is showing — a tinted background and a narrow accent edge, not the themed container's saturated fill. "Nothing acts on the selection" was a fact about the layout, not about the control, and it stopped being true the moment the layout changed. + +- **Re-filtering nulls the list's `SelectedItem`.** `ApplyFilter` clears and refills `VisibleTracks`, and the clear travels out through the two-way binding — so without capturing the pick and putting it back, the editor emptied on every keystroke in the filter box, including the keystrokes narrowing the list towards the row being edited. + +- **One scroll region was the wrong instinct.** Letting the whole pane scroll put ten search results below the fold, and the only evidence the search had worked was the scroll bar getting shorter. The fields scroll and the search box and its results are pinned to the foot of the pane instead: the results are what the user just asked for, and the fields are what they will scroll back to, having already read them. + +Commands bound from inside the pane reach the page with `RelativeSource={RelativeSource AncestorType=UserControl}`; the row template's old `AncestorType=ListBox` has no such ancestor once the editor is out of the list. + --- ## 12. Risks diff --git a/src/Offstream.App/Resources/Strings.fr.resx b/src/Offstream.App/Resources/Strings.fr.resx index d3b3ad7..d995c0c 100644 --- a/src/Offstream.App/Resources/Strings.fr.resx +++ b/src/Offstream.App/Resources/Strings.fr.resx @@ -541,11 +541,11 @@ avant - - Afficher les champs + + Choisissez une piste à gauche pour vérifier et modifier ses métadonnées. - - Masquer les champs + + Métadonnées à écrire Sera modifié diff --git a/src/Offstream.App/Resources/Strings.resx b/src/Offstream.App/Resources/Strings.resx index 5e424f9..003e79f 100644 --- a/src/Offstream.App/Resources/Strings.resx +++ b/src/Offstream.App/Resources/Strings.resx @@ -692,11 +692,11 @@ was Precedes the value a file carried before, shown only under a field that changed. - - Show tag fields + + Choose a track on the left to review and edit its tags. - - Hide tag fields + + Tags to write Will change diff --git a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs index d85852c..ce2b69e 100644 --- a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs +++ b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs @@ -4,7 +4,6 @@ using System.IO; using System.Windows.Media.Imaging; using CommunityToolkit.Mvvm.ComponentModel; -using CommunityToolkit.Mvvm.Input; using Offstream.App.Resources; using Offstream.Core.Metadata.Library; using Serilog; @@ -111,15 +110,6 @@ public LibraryTrackViewModel(LibraryTrack track) [NotifyPropertyChangedFor(nameof(SuggestedCoverArtSource))] private BitmapImage? _coverArt; - /// Whether the row's editable fields are showing. - /// - /// Collapsed is the resting state. A library is a hundred files and three of them are wrong; - /// keeping every row's boxes open costs the other ninety-seven their share of the window, and - /// the first version of this page showed three of a hundred and twenty-seven files at once. - /// - [ObservableProperty] - private bool _isExpanded; - [ObservableProperty] [NotifyPropertyChangedFor(nameof(StatusText))] [NotifyPropertyChangedFor(nameof(HasFailed))] @@ -375,8 +365,8 @@ private void MarkEdited() /// Fills the search box from whatever the row says now. /// - /// Called when the fields open. Doing it on expand rather than in the constructor means the - /// box reflects a fetch or an edit that happened in between, instead of the values the file + /// Called when the row becomes the selected one, not from the constructor, so the box + /// reflects a fetch or an edit that happened in between rather than the values the file /// carried when it was scanned. /// public void SeedMatchQuery() @@ -386,21 +376,6 @@ public void SeedMatchQuery() MatchQuery = string.Join(' ', new[] { Artist, Title }.Where(part => !string.IsNullOrWhiteSpace(part))); } - /// Opens or closes this row's fields. - [RelayCommand] - private void ToggleExpand() => IsExpanded = !IsExpanded; - - /// Seeds the search box whichever way the row was opened. - /// - /// This hung off the command, so a row opened by clicking its title got a seeded query and - /// the same row opened by its chevron — the affordance that looks like the way to do it — - /// got an empty box, and searching from there asked Spotify for nothing. - /// - partial void OnIsExpandedChanged(bool value) - { - if (value) SeedMatchQuery(); - } - private static string Or(string? value, string fallback) => string.IsNullOrWhiteSpace(value) ? fallback : value; diff --git a/src/Offstream.App/ViewModels/MetadataViewModel.cs b/src/Offstream.App/ViewModels/MetadataViewModel.cs index 62faa3e..43d7d1a 100644 --- a/src/Offstream.App/ViewModels/MetadataViewModel.cs +++ b/src/Offstream.App/ViewModels/MetadataViewModel.cs @@ -93,6 +93,34 @@ public MetadataViewModel( /// public ObservableCollection VisibleTracks { get; } = []; + /// The row the editor on the right is editing, or null when none is picked. + /// + /// The editor used to open inside the row itself, and it never fitted: an opened row measured + /// 539 device-independent pixels against a list viewport of 347, so choosing a track buried + /// the library it was chosen from and pushed the search results off the bottom of the page. + /// Hoisting the choice to the page turns the same controls into a pane that is always the + /// same size and always in the same place, and nothing reflows when the pick changes. + /// + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(HasSelection))] + private LibraryTrackViewModel? _selectedTrack; + + /// Whether a row is picked, and so whether the editor has anything to show. + public bool HasSelection => SelectedTrack is not null; + + /// Fills the picked row's search box. + /// + /// Seeding on selection covers every way in — clicking a row, arrowing onto it, or the list + /// re-filtering under a pick. It hung off an expand command once, so a row opened by its + /// chevron rather than its title got an empty box and searching from there asked Spotify for + /// nothing. + /// This can fire more than once for the same pick: re-filtering clears the selection + /// and puts it back, so the row is seeded twice. That is harmless only because seeding is + /// idempotent — it returns early on a query that is already there. Anything added here has + /// to stay safe to repeat. + /// + partial void OnSelectedTrackChanged(LibraryTrackViewModel? value) => value?.SeedMatchQuery(); + /// The folder to scan. Starts at wherever recordings are being written. [ObservableProperty] private string _folder; @@ -144,6 +172,12 @@ private void ApplyFilter() { var needle = Filter?.Trim(); + // Clearing the collection makes the list null its own SelectedItem, which travels back + // here through the two-way binding. Without putting the pick back, typing in the filter + // box would empty the editor on every keystroke — including the keystrokes that still + // match the row being edited. + var picked = SelectedTrack; + VisibleTracks.Clear(); foreach (var row in Tracks) @@ -151,6 +185,8 @@ private void ApplyFilter() if (Matches(row, needle)) VisibleTracks.Add(row); } + SelectedTrack = picked is not null && VisibleTracks.Contains(picked) ? picked : null; + OnPropertyChanged(nameof(IsFiltered)); OnPropertyChanged(nameof(HasNoMatches)); OnPropertyChanged(nameof(FilterSummary)); @@ -201,6 +237,7 @@ private async Task ScanAsync(CancellationToken cancellationToken) { var scan = await _scanner.ScanAsync(Folder, cancellationToken); + SelectedTrack = null; Tracks.Clear(); foreach (var track in scan.Tracks) Tracks.Add(new LibraryTrackViewModel(track)); diff --git a/src/Offstream.App/Views/Pages/MetadataPage.xaml b/src/Offstream.App/Views/Pages/MetadataPage.xaml index 361c2f1..e66af02 100644 --- a/src/Offstream.App/Views/Pages/MetadataPage.xaml +++ b/src/Offstream.App/Views/Pages/MetadataPage.xaml @@ -22,7 +22,7 @@ @@ -32,19 +32,19 @@ + BorderThickness="0,0,0,1" + ToolTip="{Binding Path, Mode=OneTime}"> - - - - + + + + + + - - - - - - - - - + + + + + - - - - - + + + + + + - - - - - - - - - - + + - - - + + + - - + + + + - + + + + + + + + - - - - - - - - - - - - + + + + + + + + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + - - + + + - + - - - - + + + + - - + + - - + + - + - - - - - + + + + - - - - - - - - - - - - - - - - - - - - - - - - + + - - + + - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -488,6 +451,10 @@ proposes, save commits. Keeping them as three buttons rather than one "fix my library" action is deliberate — an automatic match is wrong often enough that the review step is the feature, not friction in front of it. + + Below the buttons it is a list beside an editor. That split is what makes the review step + usable: the library stays on screen while one of its files is being corrected, and the + pane holding the correction never changes size or position as the pick moves through it. --> @@ -495,21 +462,21 @@ - + - - + Style="{StaticResource OffstreamGroupHeaderStyle}" + ToolTip="{x:Static res:Strings.MetadataIntro}" /> - + @@ -538,7 +505,7 @@ IsEnabled="{Binding IsIdle, Mode=OneWay}" /> - + @@ -579,128 +546,177 @@ TextTrimming="CharacterEllipsis" /> - - - - - + + + + + + - - - - - - - + + + + + - - - - - - + + + + + + - - - - - + + + + - - - - - - - - + + + + + + + + + + + + + + + + + + + diff --git a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs index b03f7f7..c278a8e 100644 --- a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs +++ b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs @@ -9,32 +9,11 @@ namespace Offstream.UI.Tests; /// What one row of the Metadata page shows before anyone opens it. /// /// -/// The collapsed row is the only thing most files will ever show, so what it says has to be -/// enough to approve a save without opening anything. These are the properties that say it. +/// A row in the list is the only thing most files will ever show, so what it says has to be +/// enough to approve a save without picking it. These are the properties that say it. /// public sealed class LibraryTrackViewModelTests { - /// A row starts closed. - /// - /// The layout this replaced kept every row's fields open and fitted three files of a hundred - /// and twenty-seven on screen. Density is the feature, so the default matters. - /// - [Fact] - public void ARow_StartsCollapsed() => Assert.False(Row().IsExpanded); - - /// The chevron opens and closes it. - [Fact] - public void ToggleExpand_OpensAndCloses() - { - var row = Row(); - - row.ToggleExpandCommand.Execute(null); - Assert.True(row.IsExpanded); - - row.ToggleExpandCommand.Execute(null); - Assert.False(row.IsExpanded); - } - /// The second line reads "artist · album". [Fact] public void Summary_JoinsTheArtistAndAlbum() => @@ -246,28 +225,27 @@ public void CoverPreview_FollowsAUrlWhenTheMatchBroughtNoBytes() /// Opening a row fills its search box, however the row was opened. /// - /// The seeding used to hang off the toggle command, which only the title runs. A row opened - /// by its chevron — the control that looks like the way to open one — got an empty box, and - /// searching from there asked Spotify for nothing at all. + /// The query is built from the row as it stands now, not as the file was scanned, so a fetch + /// or an edit in between is what gets searched for. /// [Fact] - public void Expanding_SeedsTheSearchBoxFromWhatTheRowNowSays() + public void Seeding_FillsTheSearchBoxFromWhatTheRowNowSays() { var row = Row(); - row.IsExpanded = true; + row.SeedMatchQuery(); Assert.Equal("Kate Bush Running Up That Hill", row.MatchQuery); } - /// Reopening a row does not overwrite a query the user typed. + /// Seeding again does not overwrite a query the user typed. [Fact] - public void Expanding_LeavesAQueryTheUserAlreadyTyped() + public void Seeding_LeavesAQueryTheUserAlreadyTyped() { var row = Row(); row.MatchQuery = "cloudbusting"; - row.IsExpanded = true; + row.SeedMatchQuery(); Assert.Equal("cloudbusting", row.MatchQuery); } diff --git a/tests/Offstream.UI.Tests/MetadataViewModelTests.cs b/tests/Offstream.UI.Tests/MetadataViewModelTests.cs index 1070635..a73fc99 100644 --- a/tests/Offstream.UI.Tests/MetadataViewModelTests.cs +++ b/tests/Offstream.UI.Tests/MetadataViewModelTests.cs @@ -325,6 +325,95 @@ public async Task Filter_NarrowsTheVisibleRows() Assert.Equal("Cloudbusting", viewModel.VisibleTracks[0].Title); } + /// Picking a row fills its search box. + /// + /// Seeding lives on the page's selection rather than on the row, so it happens however the + /// row was reached — clicked, arrowed onto, or restored after a re-filter. It used to hang + /// off an expand command that only one of the two ways of opening a row ran, and the other + /// way left the box empty and searched Spotify for nothing. + /// + [Fact] + public async Task Selecting_SeedsTheRowsSearchBox() + { + var viewModel = Build(new FakeScanner(Scanned("kate.mp3", "Kate Bush", "Cloudbusting", "Hounds of Love"))); + + await viewModel.ScanCommand.ExecuteAsync(null); + + Assert.Null(viewModel.SelectedTrack); + Assert.False(viewModel.HasSelection); + + viewModel.SelectedTrack = viewModel.VisibleTracks[0]; + + Assert.True(viewModel.HasSelection); + Assert.Equal("Kate Bush Cloudbusting", viewModel.SelectedTrack.MatchQuery); + } + + /// A filter that still matches the picked row leaves it picked. + /// + /// Re-filtering empties the visible collection and refills it, which makes the list null its + /// own selection on the way through. Without putting the pick back, typing in the filter box + /// would close the editor on every keystroke — including the keystrokes that narrow the list + /// towards the very row being edited. + /// + [Fact] + public async Task Filtering_KeepsTheRowTheEditorIsShowing() + { + var viewModel = Build(new FakeScanner( + Scanned("kate.mp3", "Kate Bush", "Cloudbusting", "Hounds of Love"), + Scanned("run.mp3", "AWOLNATION", "Run", "Run"))); + + await viewModel.ScanCommand.ExecuteAsync(null); + + var picked = viewModel.VisibleTracks[0]; + viewModel.SelectedTrack = picked; + + viewModel.Filter = "kate"; + + Assert.Same(picked, viewModel.SelectedTrack); + } + + /// A filter that hides the picked row empties the editor. + /// + /// The other half of the rule above. Holding on to a row that is no longer in the list would + /// leave the pane editing a file the user cannot see, and Save would then write a change with + /// nothing on screen to account for it. + /// + [Fact] + public async Task Filtering_ClearsTheEditorWhenItsRowIsHidden() + { + var viewModel = Build(new FakeScanner( + Scanned("kate.mp3", "Kate Bush", "Cloudbusting", "Hounds of Love"), + Scanned("run.mp3", "AWOLNATION", "Run", "Run"))); + + await viewModel.ScanCommand.ExecuteAsync(null); + + viewModel.SelectedTrack = viewModel.VisibleTracks[0]; + + viewModel.Filter = "awolnation"; + + Assert.Null(viewModel.SelectedTrack); + Assert.False(viewModel.HasSelection); + } + + /// Scanning again empties the editor. + /// + /// The rows a scan produces are new objects, so a pick held across one would point at a row + /// that is no longer in any list — an editor bound to a file the new scan may not even have + /// found. + /// + [Fact] + public async Task Scanning_ClearsTheEditor() + { + var viewModel = Build(new FakeScanner(Scanned("kate.mp3", "Kate Bush", "Cloudbusting", "Hounds of Love"))); + + await viewModel.ScanCommand.ExecuteAsync(null); + viewModel.SelectedTrack = viewModel.VisibleTracks[0]; + + await viewModel.ScanCommand.ExecuteAsync(null); + + Assert.Null(viewModel.SelectedTrack); + } + /// It matches the file name too, not just the tags. /// /// The row most in need of repair is routinely the one whose tags are wrong and whose name is From fcd2623046b17c2da84c1cbf3666a88c30a13406 Mon Sep 17 00:00:00 2001 From: revtex Date: Mon, 31 Aug 2026 19:58:36 -0400 Subject: [PATCH 4/4] Let the Metadata page edit every tag the recorder writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page shipped with three editable fields — title, artist, album — and showed year, genre and artwork read-only beside them. The scope question "which fields belong here" has an answer that is not a matter of taste: whatever the recording path writes. FFmpegArguments.MetadataArguments writes title, artist, album artist, album, genre, date, track, disc and copyright, so a recording could carry a wrong disc number that no part of Offstream could correct. Composer, comment and BPM are in neither list, which is what keeps this from drifting into a general-purpose tag editor. Filling the gap turned up three defects underneath it, none of them visible while the fields were absent. Ten fields do not stack. 10 x 65 DIP is 650 in a 347 DIP pane — the same arithmetic that moved the editor out of the list row yesterday, met again a day later on the pane that fixed it. Paired into two columns it is 390 DIP: seven of ten visible at the minimum window and all ten at any real size. Numeric boxes read fine at half width, which is what makes the pairing free. An emptied box was reported as a change. HasChanges compared before against after, so clearing the album box lit "Will change" and then saved nothing, because the writer has never written a blank over a value. The predicate now asks whether saving would alter the file, which is the question the badge was always claiming to answer. Writing one artist back over a tag that held two destroyed data. ID3v2.3 separates artists with a slash, so the file recorded as AC/DC holds the two values AC and DC; Read took FirstPerformer, the box showed AC, and Write did tag.Performers = [track.Artist] — narrowing the tag on the page whose purpose is repairing tags. It had been there since the writer was written and stayed invisible until an album artist box appeared beside the artist box, showing "AC, DC" next to "AC". The original list now goes back verbatim when its first entry still matches the box, since that is where the box was filled from; a match satisfies this by construction, because the mapper sets the artist from the first performer it assigns. The tempting fix there is worse than the bug: splitting the box on commas the way the genre box splits reads "Earth, Wind & Fire" as three artists. Genres can be edited as a comma-separated line because a genre list really is a list; names cannot, because commas inside one name are ordinary. Both artist boxes keep the array they were filled from while the text still matches it, and take the whole line as a single value when it does not. The never-erase rule from yesterday became one helper, LibraryLookup, rather than two hand-written copies, and grew from genre and year to all seven fields a lookup can leave empty. There are still two call sites and both are still load-bearing, but the rule itself now exists once, so the next field cannot be added to one copy and forgotten in the other. That is exactly how the bug survived its first fix. Verified on a copy of a real recording rather than the library: a page save with one unrelated field edited left Performers as AC || DC, applied the edit, and kept the file at ID3v2.3. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 20 +- CLAUDE.md | 39 +- README.md | 21 +- docs/MODERNIZATION-PLAN.md | 16 + src/Offstream.App/Resources/Strings.fr.resx | 31 +- src/Offstream.App/Resources/Strings.resx | 31 +- .../ViewModels/LibraryTrackViewModel.cs | 271 ++- .../ViewModels/MetadataViewModel.cs | 20 +- .../Views/Pages/MetadataPage.xaml | 1597 +++++++++-------- .../Metadata/Library/LibraryLookup.cs | 69 + .../Metadata/Library/LibraryTrack.cs | 41 +- .../Library/SpotifyCatalogEnricher.cs | 20 +- .../Metadata/Library/TagLibTagStore.cs | 57 +- .../Metadata/Library/LibraryLookupTests.cs | 110 ++ .../Metadata/Library/TagLibTagStoreTests.cs | 126 ++ .../Metadata/SpotifyTrackMapperTests.cs | 23 + .../LibraryTrackViewModelTests.cs | 146 +- 17 files changed, 1787 insertions(+), 851 deletions(-) create mode 100644 src/Offstream.Core/Metadata/Library/LibraryLookup.cs create mode 100644 tests/Offstream.Core.Tests/Metadata/Library/LibraryLookupTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cf3795..339d352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,10 +89,14 @@ phase plan these entries follow. files of which three are wrong, and a row marked **Will change** is how the three are found without reading the other ninety-seven. A **Filter** box narrows the list by title, artist, album or file name for libraries too long to scroll — it changes what is drawn and nothing else, - so Save still writes every ticked row it is hiding. Under the editable fields, **Also from the - match** shows the year, genre and artwork a lookup found: Save writes all three and they - appeared nowhere, so a row could announce that it would change while every field on screen - matched the file exactly. And when the match itself is wrong, **Not the right track?** searches + so Save still writes every ticked row it is hiding. Every tag Offstream writes while recording + can be edited here: title, artist, album artist, album, genre, year, track number, tracks on + album, disc and copyright. Three of them were editable at first and the rest were not shown at + all, which left a row free to announce that it would change while every field on screen matched + the file exactly — and left a tag the recorder writes but the page could not reach as a tag + nobody could ever fix. The fields sit in two columns rather than one, because ten stacked + fields are 650 DIP inside a 347 DIP pane and seven of them would have opened below the fold. + And when the match itself is wrong, **Not the right track?** searches Spotify for whatever you type and lists what it finds, year included, so a remaster can be told from the original; it is pinned to the foot of the pane, so the results arrive where the user is already looking rather than below a fold. Until this there was no way to correct a bad match @@ -106,7 +110,13 @@ phase plan these entries follow. lookup that has no genre for a track no longer offers to remove the one the file has: every provider assigns genre and year unconditionally, which is right where they were written, tagging a recording that starts with nothing, and wrong here, where the track starts as the file's own - tags. + tags. The same holds for every other tag a lookup can leave empty, and for clearing a box by + hand — Save writes no blank over a value, so an emptied field stopped claiming a change it was + never going to make. Saving also keeps an artist tag that holds more than one name. ID3v2.3 + separates artists with a slash, so a file recorded as `AC/DC` is stored as the two values `AC` + and `DC`; the page has one artist box, it is filled from the first of them, and writing that + box back narrowed the tag to `AC` — the repair page destroying the tag it was opened to + repair. An artist nobody has typed over is now written back exactly as it was found. - **Looking a track up now falls back from Spotify to Last.fm instead of stopping at whichever one the Settings page names.** That setting answers "who tags a recording as it is made", where one source keeps a library consistent. Repairing files already on disk is the opposite problem — diff --git a/CLAUDE.md b/CLAUDE.md index c97633c..e7357f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,13 +61,38 @@ These are load-bearing; violating them breaks the app in ways that are not obvio for most of its catalogue since late 2024 and Last.fm returns none for an artist nobody has tagged, so without this a lookup blanks a genre the user curated. Nothing ever reaches the file — `TagLibTagStore` writes a genre only when there is one — but the row reports a change Save will - not make, and the editor spells it out as an offer to erase. It lives in **two** places, and - both are load-bearing. `MetadataViewModel.FetchOneAsync` is the call site every automatic lookup - passes through, so the rule holds there for whichever provider in the chain answers and for any - provider added later — it was fixed once inside the Spotify path alone and the Last.fm fallback - still had the hole. `SpotifyCatalogEnricher` keeps its own copy because the manual-match path - (**Use this**, via `SpotifyMatchSearch.ApplyAsync`) reaches the enricher without going through - `FetchOneAsync`. Deleting either one restores the bug on one of the two paths. + not make, and the editor spells it out as an offer to erase. The rule is written once, in + `LibraryLookup.Snapshot` / `KeepWhatWasThere`, and covers every field a lookup can leave empty + — not just genre and year. It has **two** call sites and both are load-bearing. + `MetadataViewModel.FetchOneAsync` is the one every automatic lookup passes through, so the rule + holds there for whichever provider in the chain answers and for any provider added later — it + was fixed once inside the Spotify path alone and the Last.fm fallback still had the hole. + `SpotifyCatalogEnricher` calls it separately because the manual-match path (**Use this**, via + `SpotifyMatchSearch.ApplyAsync`) reaches the enricher without going through `FetchOneAsync`. + Deleting either call restores the bug on one of the two paths. Add a new field to the helper, + never to a call site — the original bug survived its first fix precisely because the rule + existed twice. +- **The Metadata page edits exactly the tags the recorder writes, and a name is not a + comma-separated list** (decided 2026-08-31). The editable set is title, artist, album artist, + album, genre, year, track, tracks-on-album, disc and copyright, which is + `FFmpegArguments.MetadataArguments` — a tag Offstream writes while recording and cannot repair + afterwards is a tag nobody can fix at all. Composer, comment and BPM are in neither list, and + leaving them out is what keeps this from becoming a general-purpose tag editor. Two rules for + the fields that hold several values. **Genres split on commas; artist and album artist do + not** — `Earth, Wind & Fire` is one band, and splitting it is a worse corruption than the + multi-value tag the box was added to expose, so those boxes keep the array they were filled + from while the text still matches it and take the whole line as one value when it does not. + And **an artist tag can hold more than one name behind a single box**: ID3v2.3 separates + artists with a slash, so a file recorded as `AC/DC` reads back as the two values `AC` and `DC`. + `TagLibTagStore.Write` writes `track.Performers` back verbatim when its first entry still + equals `track.Artist` (`KeepsEveryPerformer`), because that first entry is where the box was + filled from. Writing `[track.Artist]` unconditionally is what it used to do, and it narrowed + the tag on the page whose whole purpose is repairing tags. +- **`LibraryTrack.HasChanges` means "saving would alter the file", not "these differ"** + (decided 2026-08-31). `TagLibTagStore.Write` never writes a blank over a value, so a plain + inequality made an emptied box light the **Will change** badge and then save nothing. Any field + added to the editor has to go through `Replaces`, which asks whether there is a new value at + all before asking whether it differs. - **Use `ProcessStartInfo.ArgumentList`, never a command string.** Track metadata comes from Spotify window titles and is untrusted. The argv array prevents argument injection structurally; the old app needed hand-written `CommandLineToArgvW` escaping because .NET Framework lacked `ArgumentList`. ## Spotify Web API rules diff --git a/README.md b/README.md index db667ce..4e81c7b 100644 --- a/README.md +++ b/README.md @@ -145,17 +145,24 @@ at the time. The **Metadata** tab is where you repair those. Nothing reaches a file until you press Save. The page is a list beside an editor. Each file is one row showing what *would* be written — the title, then the artist and album, then the file it came -from — and clicking a row, or arrowing onto it, fills the pane on the right with Title, Artist and -Album as editable boxes. There is a **was …** line under anything you'd be changing, so a match +from — and clicking a row, or arrowing onto it, fills the pane on the right with its tags as +editable boxes: Title, Artist, Album artist, Album, Genre, Year, Disc, Track, Tracks on album and +Copyright. That is every tag Offstream writes while recording, so anything it can put into a file +it can also repair. There is a **was …** line under anything you'd be changing, so a match that got the remix wrong is a correction rather than a reason to start over. The pane stays the same size and in the same place whichever row you pick, so the library never moves out from under you while you're working on one of its files. Rows that would actually alter a file are marked **Will change**, which is how you find the three that need attention without reading the other -hundred. Under the boxes, **Also from the match** shows what a lookup found that has no box of -its own — the year, the genre and the artwork, before and after — because Save writes all three -and a row that says it will change while every box matches the file otherwise looks like a -mistake. Only what a lookup actually found appears there: a provider that has no genre for a -track leaves the file's own alone rather than proposing to remove it. +hundred. Artwork a lookup found is shown under the boxes, before and after, since it is the one +thing Save writes that you can't type. + +Only what a lookup actually found is proposed: a provider with no genre for a track leaves the +file's own alone rather than offering to remove it. Save writes no blank over a value either, so +emptying a box leaves that tag as it was — clear a box to stop Offstream changing something, not +to erase it. Type a comma into **Genre** to give a track more than one; **Artist** and **Album +artist** take the name as you write it, commas and all, because "Earth, Wind & Fire" is one band. +A file whose artist tag already holds several names shows the first in the box and keeps all of +them on Save unless you type over it. **Filter** narrows the list to the rows whose title, artist, album or file name contain what you type. It only changes what is on screen: Save still writes every ticked row, including the ones the diff --git a/docs/MODERNIZATION-PLAN.md b/docs/MODERNIZATION-PLAN.md index 19cb8be..4db558c 100644 --- a/docs/MODERNIZATION-PLAN.md +++ b/docs/MODERNIZATION-PLAN.md @@ -998,6 +998,22 @@ Three second-order notes from doing it: Commands bound from inside the pane reach the page with `RelativeSource={RelativeSource AncestorType=UserControl}`; the row template's old `AncestorType=ListBox` has no such ancestor once the editor is out of the list. +### Finding: a tag the recorder writes and the editor cannot reach is a tag nobody can fix (2026-08-31) + +The page shipped with three editable fields — title, artist, album — and showed year, genre and artwork read-only beside them. The scope question "which fields belong here" has an answer that is not a matter of taste: **whatever the recording path writes**. `FFmpegArguments.MetadataArguments` writes title, artist, album, album artist, genre, date, track, disc and copyright, so a recording could carry a wrong disc number that no part of Offstream could correct. Composer, comment and BPM are in neither list, and that is what keeps this from drifting into a general-purpose tag editor. + +Filling the gap turned up three defects underneath it, none of which were visible while the fields were absent. + +**Ten fields do not stack.** 10 × 65 DIP is 650 in a 347 DIP pane — the same arithmetic as the finding above, discovered a day later on the pane that fixed it. Paired into two columns it is 390 DIP, so seven of ten are visible at the minimum window and all ten at any real size. Numeric boxes read fine at half width, which is what makes the pairing free. + +**An emptied box is not a change.** `HasChanges` compared before against after, so clearing the album box lit **Will change** and then saved nothing — the writer has never written a blank over a value. The predicate now asks whether saving would alter the file, which is the question the badge was always claiming to answer. + +**Writing one artist back over a tag that held two destroyed data.** ID3v2.3 separates artists with a slash, so the file recorded as `AC/DC` holds the two values `AC` and `DC`; `Read` took `FirstPerformer`, the box showed `AC`, and `Write` did `tag.Performers = [track.Artist]` — narrowing the tag on the page whose purpose is repairing tags. It had been there since the writer was written and was invisible until an album artist box appeared next to the artist box showing `AC, DC` beside `AC`. The fix writes the original list back verbatim when its first entry still matches the box, since that is where the box was filled from; a match satisfies this by construction, because the mapper sets the artist from the first performer it assigns. + +The tempting fix is worse than the bug: splitting the box on commas the way the genre box splits reads `Earth, Wind & Fire` as three artists. **A list of names cannot be edited as a comma-separated line.** Genres can, because a genre list really is a list and commas inside one genre are vanishingly rare; names cannot, because commas inside one name are ordinary. Both artist boxes therefore keep the array they were filled from whenever the text still matches it, and take the whole line as a single value when it does not. + +The never-erase rule from the day before became one helper, `LibraryLookup`, rather than two hand-written copies, and grew from genre and year to all seven fields a lookup can leave empty. There are still two call sites, and both are still load-bearing — `MetadataViewModel.FetchOneAsync` for every automatic lookup and `SpotifyCatalogEnricher` for the manual **Use this** path, which does not pass through it — but the rule itself now exists once, so the next field cannot be added to one copy and forgotten in the other. That is precisely how the bug survived its first fix. + --- ## 12. Risks diff --git a/src/Offstream.App/Resources/Strings.fr.resx b/src/Offstream.App/Resources/Strings.fr.resx index d995c0c..02ca2f5 100644 --- a/src/Offstream.App/Resources/Strings.fr.resx +++ b/src/Offstream.App/Resources/Strings.fr.resx @@ -630,10 +630,6 @@ Aucun fichier ne correspond à ce filtre. Shown in place of the list when the filter matches nothing. - - Également trouvé - Heading over the fields a match fills in that have no box of their own. - Année Label for the year a match found. @@ -690,4 +686,31 @@ Recherche en cours… Shown while a manual search is running. + + Artiste de l'album + + + Piste + + + Pistes sur l'album + + + Disque + + + Droits d'auteur + + + Pochette + + + Séparez les valeurs par des virgules. + + + Saisissez une année à quatre chiffres ou laissez le champ vide. + + + Saisissez un entier supérieur à zéro ou laissez le champ vide. + diff --git a/src/Offstream.App/Resources/Strings.resx b/src/Offstream.App/Resources/Strings.resx index 003e79f..d1056ed 100644 --- a/src/Offstream.App/Resources/Strings.resx +++ b/src/Offstream.App/Resources/Strings.resx @@ -790,10 +790,6 @@ No file matches that filter. Shown in place of the list when the filter matches nothing. - - Also from the match - Heading over the fields a match fills in that have no box of their own. - Year Label for the year a match found. @@ -850,4 +846,31 @@ Searching… Shown while a manual search is running. + + Album artist + + + Track + + + Tracks on album + + + Disc + + + Copyright + + + Artwork + + + Separate several with commas. + + + Enter a four-digit year, or leave the box empty. + + + Enter a whole number above zero, or leave the box empty. + diff --git a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs index ce2b69e..dc69c98 100644 --- a/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs +++ b/src/Offstream.App/ViewModels/LibraryTrackViewModel.cs @@ -31,6 +31,13 @@ public sealed partial class LibraryTrackViewModel : ObservableValidator private readonly string? _existingTitle; private readonly string? _existingArtist; private readonly string? _existingAlbum; + private readonly string? _existingAlbumArtist; + private readonly string? _existingGenres; + private readonly string? _existingYear; + private readonly string? _existingTrackNumber; + private readonly string? _existingTrackCount; + private readonly string? _existingDisc; + private readonly string? _existingCopyright; /// Wraps a scanned file for display. public LibraryTrackViewModel(LibraryTrack track) @@ -47,21 +54,39 @@ public LibraryTrackViewModel(LibraryTrack track) _existingTitle = track.Existing.Title; _existingArtist = track.Existing.Artist; _existingAlbum = track.Existing.Album; + _existingAlbumArtist = List(track.Existing.AlbumArtists); + _existingGenres = List(track.Existing.Genres); + _existingYear = Number(track.Existing.Year); + _existingTrackNumber = Number(track.Existing.AlbumPosition); + _existingTrackCount = Number(track.Existing.AlbumTrackCount); + _existingDisc = Number(track.Existing.Disc); + _existingCopyright = track.Existing.Copyright; CurrentTitle = Or(track.Existing.Title, Strings.MetadataNoValue); CurrentArtist = Or(track.Existing.Artist, Strings.MetadataNoValue); CurrentAlbum = Or(track.Existing.Album, Strings.MetadataNoValue); + CurrentAlbumArtist = Or(_existingAlbumArtist, Strings.MetadataNoValue); + CurrentGenres = Or(_existingGenres, Strings.MetadataNoValue); + CurrentYear = Or(_existingYear, Strings.MetadataNoValue); + CurrentTrackNumber = Or(_existingTrackNumber, Strings.MetadataNoValue); + CurrentTrackCount = Or(_existingTrackCount, Strings.MetadataNoValue); + CurrentDisc = Or(_existingDisc, Strings.MetadataNoValue); + CurrentCopyright = Or(_existingCopyright, Strings.MetadataNoValue); _title = track.Suggested.Title ?? string.Empty; _artist = track.Suggested.Artist ?? string.Empty; _album = track.Suggested.Album ?? string.Empty; + _albumArtist = List(track.Suggested.AlbumArtists) ?? string.Empty; + _genres = List(track.Suggested.Genres) ?? string.Empty; + _year = Number(track.Suggested.Year) ?? string.Empty; + _trackNumber = Number(track.Suggested.AlbumPosition) ?? string.Empty; + _trackCount = Number(track.Suggested.AlbumTrackCount) ?? string.Empty; + _disc = Number(track.Suggested.Disc) ?? string.Empty; + _copyright = track.Suggested.Copyright ?? string.Empty; _status = track.Status; CoverArt = LoadCoverArt(track.Existing.AlbumArtImage); ExistingCoverArt = CoverArt; - - CurrentYear = track.Existing.Year?.ToString(CultureInfo.CurrentCulture) ?? Strings.MetadataNoValue; - CurrentGenres = Join(track.Existing.Genres); } /// The file's own name, which is what identifies the row. @@ -87,12 +112,27 @@ public LibraryTrackViewModel(LibraryTrack track) /// public string CurrentAlbum { get; } + /// + public string CurrentAlbumArtist { get; } + /// public string CurrentYear { get; } /// public string CurrentGenres { get; } + /// + public string CurrentTrackNumber { get; } + + /// + public string CurrentTrackCount { get; } + + /// + public string CurrentDisc { get; } + + /// + public string CurrentCopyright { get; } + /// The scanned track this row edits. internal LibraryTrack Track => _track; @@ -142,6 +182,52 @@ public LibraryTrackViewModel(LibraryTrack track) [NotifyPropertyChangedFor(nameof(HasPendingChanges))] private string _album; + /// Who the album is filed under, which is not always who performed the track. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(HasAlbumArtistChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + private string _albumArtist; + + /// Genres, comma-separated, because every container stores a list. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(HasGenreChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + private string _genres; + + [ObservableProperty] + [NotifyDataErrorInfo] + [NotifyPropertyChangedFor(nameof(HasYearChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + [CustomValidation(typeof(LibraryTrackViewModel), nameof(ValidateYear))] + private string _year; + + [ObservableProperty] + [NotifyDataErrorInfo] + [NotifyPropertyChangedFor(nameof(HasTrackNumberChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + [CustomValidation(typeof(LibraryTrackViewModel), nameof(ValidateCount))] + private string _trackNumber; + + [ObservableProperty] + [NotifyDataErrorInfo] + [NotifyPropertyChangedFor(nameof(HasTrackCountChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + [CustomValidation(typeof(LibraryTrackViewModel), nameof(ValidateCount))] + private string _trackCount; + + [ObservableProperty] + [NotifyDataErrorInfo] + [NotifyPropertyChangedFor(nameof(HasDiscChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + [CustomValidation(typeof(LibraryTrackViewModel), nameof(ValidateCount))] + private string _disc; + + /// The copyright line. Filled by a match far more often than it is typed. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(HasCopyrightChange))] + [NotifyPropertyChangedFor(nameof(HasPendingChanges))] + private string _copyright; + /// The status, translated. public string StatusText => Status switch { @@ -188,19 +274,26 @@ public LibraryTrackViewModel(LibraryTrack track) /// public bool HasAlbumChange => Differs(_existingAlbum, Album); - /// The year the match found, or the placeholder. - public string SuggestedYear => - _track.Suggested.Year?.ToString(CultureInfo.CurrentCulture) ?? Strings.MetadataNoValue; + /// + public bool HasAlbumArtistChange => Differs(_existingAlbumArtist, AlbumArtist); + + /// + public bool HasGenreChange => Differs(_existingGenres, Genres); + + /// + public bool HasYearChange => Differs(_existingYear, Year); + + /// + public bool HasTrackNumberChange => Differs(_existingTrackNumber, TrackNumber); - /// The genres the match found, comma-separated, or the placeholder. - public string SuggestedGenres => Join(_track.Suggested.Genres); + /// + public bool HasTrackCountChange => Differs(_existingTrackCount, TrackCount); - /// Whether the match brought a different year. - public bool HasYearChange => _track.Existing.Year != _track.Suggested.Year; + /// + public bool HasDiscChange => Differs(_existingDisc, Disc); - /// Whether the match brought different genres. - public bool HasGenreChange => - !string.Equals(CurrentGenres, SuggestedGenres, StringComparison.Ordinal); + /// + public bool HasCopyrightChange => Differs(_existingCopyright, Copyright); /// Whether saving would put a different picture in the file. public bool HasCoverArtChange => _track.CoverArtWouldChange; @@ -233,30 +326,26 @@ public object? SuggestedCoverArtSource } } - /// - /// Whether the match changed anything the three editable boxes do not show. - /// - /// - /// This is the answer to a complaint the page earned: a row saying "will change" with title, - /// artist and album all identical to the file, and nothing on screen saying why. Year, genre - /// and artwork are written by Save and were shown nowhere, so the badge looked wrong when it - /// was right. The block this gates is hidden entirely when the match changed only the fields - /// that already have boxes, because then the "was ..." lines have said it. - /// - public bool HasMatchDetails => HasYearChange || HasGenreChange || HasCoverArtChange; /// Pulls a fetched suggestion back onto the row. /// /// Called after a provider has enriched the underlying track. The editable fields are - /// overwritten because the whole point of a fetch is to replace the guess — but only the - /// three the user can see, so a provider cannot quietly change something the page never - /// showed. + /// overwritten because the whole point of a fetch is to replace the guess — and every field + /// the writer touches now has a box, so there is nothing a provider can change that the page + /// does not show. /// public void RefreshFromSuggestion() { Title = _track.Suggested.Title ?? string.Empty; Artist = _track.Suggested.Artist ?? string.Empty; Album = _track.Suggested.Album ?? string.Empty; + AlbumArtist = List(_track.Suggested.AlbumArtists) ?? string.Empty; + Genres = List(_track.Suggested.Genres) ?? string.Empty; + Year = Number(_track.Suggested.Year) ?? string.Empty; + TrackNumber = Number(_track.Suggested.AlbumPosition) ?? string.Empty; + TrackCount = Number(_track.Suggested.AlbumTrackCount) ?? string.Empty; + Disc = Number(_track.Suggested.Disc) ?? string.Empty; + Copyright = _track.Suggested.Copyright ?? string.Empty; Status = _track.Status; FailureReason = _track.FailureReason; @@ -267,14 +356,9 @@ public void RefreshFromSuggestion() } // A fetch that confirmed what the file already said changes no field, so nothing above - // raised a notification - but it may still have brought a year, a genre or cover art. + // raised a notification - but it may still have brought cover art. OnPropertyChanged(nameof(HasPendingChanges)); - OnPropertyChanged(nameof(SuggestedYear)); - OnPropertyChanged(nameof(SuggestedGenres)); - OnPropertyChanged(nameof(HasYearChange)); - OnPropertyChanged(nameof(HasGenreChange)); OnPropertyChanged(nameof(HasCoverArtChange)); - OnPropertyChanged(nameof(HasMatchDetails)); OnPropertyChanged(nameof(SuggestedCoverArtSource)); } @@ -317,6 +401,84 @@ partial void OnAlbumChanged(string value) MarkEdited(); } + /// + partial void OnAlbumArtistChanged(string value) + { + _track.Suggested.AlbumArtists = AsList(value, _track.Suggested.AlbumArtists); + MarkEdited(); + } + + /// + partial void OnGenresChanged(string value) + { + _track.Suggested.Genres = SplitList(value); + MarkEdited(); + } + + /// + /// A number the user is halfway through typing is not a reason to throw the old one away. + /// + /// + /// The boxes update on every keystroke, so "19" is a state every four-digit year passes + /// through. Writing whatever parses would put 19 into the track and then 198 and then 1984, + /// which is harmless — but a value that does not parse has to leave the field alone + /// rather than null it, or backspacing over a year to retype it clears the tag underneath and + /// the validation message that says so never gets a chance to stop the save. + /// + partial void OnYearChanged(string value) => + SetNumber(value, number => _track.Suggested.Year = number); + + /// + partial void OnTrackNumberChanged(string value) => + SetNumber(value, number => _track.Suggested.AlbumPosition = number); + + /// + partial void OnTrackCountChanged(string value) => + SetNumber(value, number => _track.Suggested.AlbumTrackCount = number); + + /// + partial void OnDiscChanged(string value) => + SetNumber(value, number => _track.Suggested.Disc = number); + + /// + partial void OnCopyrightChanged(string value) + { + _track.Suggested.Copyright = value; + MarkEdited(); + } + + /// Assigns a numeric box, leaving the tag alone while the value is unusable. + private void SetNumber(string value, Action assign) + { + if (string.IsNullOrWhiteSpace(value)) + { + assign(null); + } + else if (ParseNumber(value) is { } number) + { + assign(number); + } + + MarkEdited(); + } + + /// A year is four digits, and everything Offstream can tag was recorded after 1000. + /// + /// Empty passes, here and in every numeric box on the page. A file that has never had a track + /// number is not in error for still not having one, and the writer leaves an empty value + /// alone rather than clearing the file's own. + /// + public static ValidationResult? ValidateYear(string? value, ValidationContext context) => + string.IsNullOrWhiteSpace(value) || ParseNumber(value) is >= 1000 and <= 9999 + ? ValidationResult.Success + : new ValidationResult(Strings.MetadataYearInvalid); + + /// + public static ValidationResult? ValidateCount(string? value, ValidationContext context) => + string.IsNullOrWhiteSpace(value) || ParseNumber(value) is not null + ? ValidationResult.Success + : new ValidationResult(Strings.MetadataNumberInvalid); + /// Takes a row back out of the saved state when it is edited again. /// /// Save is what makes a row Saved, and "Saved" is also what suppresses the "will change" @@ -379,8 +541,47 @@ public void SeedMatchQuery() private static string Or(string? value, string fallback) => string.IsNullOrWhiteSpace(value) ? fallback : value; - private static string Join(string[]? genres) => - genres is { Length: > 0 } ? string.Join(", ", genres) : Strings.MetadataNoValue; + /// A tag list as one editable line, or nothing at all. + private static string? List(string[]? values) => + values is { Length: > 0 } ? string.Join(", ", values) : null; + + /// The inverse of . Blank entries are dropped, not stored empty. + private static string[]? SplitList(string? value) => + string.IsNullOrWhiteSpace(value) + ? null + : value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + + /// A name list edited as one line, without reading the commas inside a name. + /// + /// Artist names contain commas — "Earth, Wind & Fire" is one band — so splitting this box + /// the way splits genres would file that album under three artists, + /// which is a worse corruption than the multi-value tag it was meant to preserve. The box is + /// filled from , so text that still matches means nobody has typed + /// over it and the list goes back untouched, however many values it holds; text that does not + /// match is one name the user chose. Genres keep the split — a genre list really is a list, + /// and commas inside a single genre are vanishingly rare. + /// + private static string[]? AsList(string? value, string[]? current) => + string.IsNullOrWhiteSpace(value) ? null + : string.Equals(List(current), value, StringComparison.Ordinal) ? current + : [value.Trim()]; + + private static string? Number(int? value) => + value is > 0 ? value.Value.ToString(CultureInfo.InvariantCulture) : null; + + /// + /// Reads a box back as a number, invariantly. + /// + /// + /// is what rejects "-3", "1,984" and " 12 " — a tag number is + /// digits and nothing else, and a group separator that parses in one locale and fails in the + /// next would make the same typed year valid or invalid depending on the machine. + /// + private static int? ParseNumber(string? value) => + int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out var number) + && number > 0 + ? number + : null; /// Whether a proposed value says something the file does not already say. /// diff --git a/src/Offstream.App/ViewModels/MetadataViewModel.cs b/src/Offstream.App/ViewModels/MetadataViewModel.cs index 43d7d1a..89eae4e 100644 --- a/src/Offstream.App/ViewModels/MetadataViewModel.cs +++ b/src/Offstream.App/ViewModels/MetadataViewModel.cs @@ -498,25 +498,17 @@ private static async Task FetchOneAsync( row.Status = LibraryTrackStatus.Fetching; row.FailureReason = null; - var genres = row.Track.Suggested.Genres; - var year = row.Track.Suggested.Year; + var before = LibraryLookup.Snapshot(row.Track.Suggested); try { var updated = await chain.EnrichAsync(row.Track.Suggested, cancellationToken); - // A lookup on this page adds tags. It never takes one away. - // - // Every provider assigns genre and year unconditionally, which is right where they - // were written — a recording starts with an empty track and the provider is the only - // source there is. Here the track starts as the file's own tags, and a provider that - // knows the song but has no genre for it (Last.fm returns none for an artist nobody - // has tagged; Spotify returns none for most of its catalogue since late 2024) hands - // back an empty list that overwrites one the user curated. Nothing reaches the file — - // the writer skips empty values — but the row said "will change" over a change Save - // would not make, and the panel underneath it spelled the loss out as an offer. - row.Track.Suggested.Genres = row.Track.Suggested.Genres is { Length: > 0 } found ? found : genres; - row.Track.Suggested.Year ??= year; + // A lookup on this page adds tags. It never takes one away. This is the call site + // every automatic lookup passes through, so the rule holds for whichever provider in + // the chain answers and for any provider added later; SpotifyCatalogEnricher keeps + // its own call because the manual "Use this" path never comes through here. + LibraryLookup.KeepWhatWasThere(row.Track.Suggested, before); row.Track.Status = updated ? LibraryTrackStatus.Fetched : LibraryTrackStatus.Untagged; row.Track.FailureReason = null; diff --git a/src/Offstream.App/Views/Pages/MetadataPage.xaml b/src/Offstream.App/Views/Pages/MetadataPage.xaml index e66af02..47fc327 100644 --- a/src/Offstream.App/Views/Pages/MetadataPage.xaml +++ b/src/Offstream.App/Views/Pages/MetadataPage.xaml @@ -1,724 +1,873 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/Offstream.Core/Metadata/Library/LibraryLookup.cs b/src/Offstream.Core/Metadata/Library/LibraryLookup.cs new file mode 100644 index 0000000..7b96c14 --- /dev/null +++ b/src/Offstream.Core/Metadata/Library/LibraryLookup.cs @@ -0,0 +1,69 @@ +namespace Offstream.Core.Metadata.Library; + +/// +/// The rule that a lookup on the Metadata page adds tags and never takes one away. +/// +/// +/// +/// Every provider assigns its fields unconditionally, which is right where they were written: a +/// recording starts as an empty track and the provider is the only source there is. On this page +/// the track starts as the file's own tags, so a provider that knows the song but has nothing to +/// say about one field hands back a blank that overwrites something the user curated. Spotify +/// returns an empty genre list for most of its catalogue since late 2024 and Last.fm returns none +/// for an artist nobody has tagged, so this is the common case rather than the odd one. +/// +/// +/// Nothing ever reached the file — the writer skips empty values — but the row reported a change +/// Save would not make, and once the page started showing before-and-after it read as an offer to +/// erase. +/// +/// +/// This is a helper, not the rule's only home. It has to be applied at both call sites: the +/// automatic lookup in MetadataViewModel.FetchOneAsync, which covers whichever provider in +/// the chain answers and any provider added later, and , which +/// the manual "Use this" path reaches without going through the first. The logic lives here so the +/// two cannot drift — it was once fixed inside the Spotify path alone and the Last.fm fallback +/// still had the hole. +/// +/// +/// Title, artist and album are deliberately not on the list. Replacing those is what a match is +/// for, and the manual picker exists precisely to correct them. +/// +/// +/// is off the list for a different reason: it is not independent of +/// the artist, it is the same tag read whole, and TagLibTagStore decides what to write with +/// it by asking whether it still starts with . Restoring the old list +/// under a new artist would make those two disagree, and the store would then discard the list it +/// had just been handed back. Leaving it alone gives the right answer either way — a lookup that +/// changed the artist writes the new one, and a lookup that did not keeps every name the file had. +/// +/// +public static class LibraryLookup +{ + /// Takes the "before" a lookup will be measured against. + public static Track Snapshot(Track track) + { + ArgumentNullException.ThrowIfNull(track); + + return new Track(track); + } + + /// Puts back every field the lookup left empty. + /// The enriched track, edited in place. + /// What captured before enrichment. + public static void KeepWhatWasThere(Track track, Track before) + { + ArgumentNullException.ThrowIfNull(track); + ArgumentNullException.ThrowIfNull(before); + + track.Year ??= before.Year; + track.AlbumPosition ??= before.AlbumPosition; + track.AlbumTrackCount ??= before.AlbumTrackCount; + track.Disc ??= before.Disc; + + if (track.Genres is not { Length: > 0 }) track.Genres = before.Genres; + if (track.AlbumArtists is not { Length: > 0 }) track.AlbumArtists = before.AlbumArtists; + if (string.IsNullOrWhiteSpace(track.Copyright)) track.Copyright = before.Copyright; + if (string.IsNullOrWhiteSpace(track.ReleaseDate)) track.ReleaseDate = before.ReleaseDate; + } +} diff --git a/src/Offstream.Core/Metadata/Library/LibraryTrack.cs b/src/Offstream.Core/Metadata/Library/LibraryTrack.cs index 1140d4a..210af34 100644 --- a/src/Offstream.Core/Metadata/Library/LibraryTrack.cs +++ b/src/Offstream.Core/Metadata/Library/LibraryTrack.cs @@ -59,19 +59,32 @@ public LibraryTrack(string path, Track existing) /// public string? FailureReason { get; set; } - /// Whether anything about differs from . + /// Whether saving would put something different in the file than it holds now. /// + /// /// What the Save button acts on. A row that came back from a provider saying exactly what the /// file already said is not worth rewriting the file for — and rewriting it would re-encode /// nothing but would still touch the modified time, which is enough to disturb anything /// syncing the folder. + /// + /// + /// An emptied field is not a change, because the writer never writes a blank over a + /// value — clearing a box leaves the file's own tag alone. Comparing the two sides plainly + /// made a cleared box light the "will change" badge and then save nothing, which is the same + /// defect as a lookup offering to erase a genre, arrived at from the other direction. + /// /// public bool HasChanges => - !SameText(Existing.Title, Suggested.Title) - || !SameText(Existing.Artist, Suggested.Artist) - || !SameText(Existing.Album, Suggested.Album) - || Existing.Year != Suggested.Year - || !SameGenres(Existing.Genres, Suggested.Genres) + Replaces(Existing.Title, Suggested.Title) + || Replaces(Existing.Artist, Suggested.Artist) + || Replaces(Existing.Album, Suggested.Album) + || Replaces(Existing.Copyright, Suggested.Copyright) + || Replaces(Existing.Year, Suggested.Year) + || Replaces(Existing.AlbumPosition, Suggested.AlbumPosition) + || Replaces(Existing.AlbumTrackCount, Suggested.AlbumTrackCount) + || Replaces(Existing.Disc, Suggested.Disc) + || Replaces(Existing.Genres, Suggested.Genres) + || Replaces(Existing.AlbumArtists, Suggested.AlbumArtists) || CoverArtWouldChange; /// Whether saving would put a different picture in the file than the one it has. @@ -112,11 +125,19 @@ internal static bool IsComplete(Track track) => && !string.IsNullOrWhiteSpace(track.Artist) && !string.IsNullOrWhiteSpace(track.Album); - private static bool SameText(string? left, string? right) => - string.Equals(left ?? string.Empty, right ?? string.Empty, StringComparison.Ordinal); + /// Whether the suggestion says something the writer would actually put in the file. + private static bool Replaces(string? existing, string? suggested) => + !string.IsNullOrWhiteSpace(suggested) + && !string.Equals(existing ?? string.Empty, suggested, StringComparison.Ordinal); + + /// + private static bool Replaces(int? existing, int? suggested) => + suggested is > 0 && existing != suggested; - private static bool SameGenres(string[]? left, string[]? right) => - (left ?? []).SequenceEqual(right ?? [], StringComparer.Ordinal); + /// + private static bool Replaces(string[]? existing, string[]? suggested) => + suggested is { Length: > 0 } + && !(existing ?? []).SequenceEqual(suggested, StringComparer.Ordinal); /// Whether two pictures are the same one. /// diff --git a/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs b/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs index dcf2ffc..1716c26 100644 --- a/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs +++ b/src/Offstream.Core/Metadata/Library/SpotifyCatalogEnricher.cs @@ -31,13 +31,10 @@ internal static class SpotifyCatalogEnricher /// catalogue in late 2024. /// /// - /// What the track already had is put back when Spotify offers nothing. The album - /// mapping assigns genre and year unconditionally — right for a recording, where the track - /// starts empty and Spotify is the only source there is, and wrong on the Metadata page, - /// where it starts as the file's own tags. Spotify returns an empty genre list for most of - /// its catalogue, so without this a lookup silently blanks a genre the user curated. Nothing - /// was ever written — the writer skips empty values — but the row reported a change it would - /// not make, and once the page started showing before-and-after it read as an offer to erase. + /// What the track already had is put back when Spotify offers nothing, by + /// — see it for why. This call site is the manual + /// one: MetadataViewModel.FetchOneAsync covers every automatic lookup and never runs + /// for a match the user picked by hand, so the rule has to hold here too. /// /// public static async Task ApplyAsync( @@ -46,8 +43,7 @@ public static async Task ApplyAsync( FullTrack match, CancellationToken cancellationToken) { - var seededGenres = track.Genres; - var seededYear = track.Year; + var before = LibraryLookup.Snapshot(track); SpotifyTrackMapper.Apply(track, match); @@ -58,11 +54,7 @@ public static async Task ApplyAsync( SpotifyTrackMapper.Apply(track, await client.Albums.Get(albumId, cancellationToken)); } - track.Year ??= seededYear; - - if (track.Genres is { Length: > 0 }) return; - - track.Genres = seededGenres; + LibraryLookup.KeepWhatWasThere(track, before); if (track.Genres is { Length: > 0 }) return; diff --git a/src/Offstream.Core/Metadata/Library/TagLibTagStore.cs b/src/Offstream.Core/Metadata/Library/TagLibTagStore.cs index 2db3e49..ff4bbbc 100644 --- a/src/Offstream.Core/Metadata/Library/TagLibTagStore.cs +++ b/src/Offstream.Core/Metadata/Library/TagLibTagStore.cs @@ -72,6 +72,12 @@ public Track Read(string path) Album = NullIfBlank(tag.Album), Genres = tag.Genres is { Length: > 0 } genres ? genres : null, Year = tag.Year > 0 ? (int)tag.Year : null, + AlbumArtists = tag.AlbumArtists is { Length: > 0 } albumArtists ? albumArtists : null, + Performers = tag.Performers is { Length: > 0 } performers ? performers : null, + AlbumPosition = PositiveOrNull(tag.Track), + AlbumTrackCount = PositiveOrNull(tag.TrackCount), + Disc = PositiveOrNull(tag.Disc), + Copyright = NullIfBlank(tag.Copyright), AlbumArtImage = tag.Pictures is [var picture, ..] ? picture.Data?.Data : null, }; @@ -100,17 +106,30 @@ public void Write(string path, Track track, byte[]? coverArt) using var file = TagLib.File.Create(path); var tag = file.Tag; - // Only fields the user can actually see and correct on the page. Writing everything - // Track can hold would let a provider's idea of, say, disc number silently replace a - // value the user curated, on a page that never showed it to them. + // Every tag the recording path writes, and nothing beyond it. The two lists being the + // same list is the point: a tag Offstream puts into a file when it records it and + // cannot put right afterwards is a tag the user has no way to fix at all. Composer, + // comment and BPM are absent from both, so this is not a general-purpose tag editor. + // + // Empty is still never written. Clearing a box leaves the file's own value alone + // rather than erasing it, which is the same promise a lookup makes. if (!string.IsNullOrWhiteSpace(track.Title)) tag.Title = track.Title; if (!string.IsNullOrWhiteSpace(track.Album)) tag.Album = track.Album; if (track.Year is > 0) tag.Year = (uint)track.Year.Value; if (track.Genres is { Length: > 0 }) tag.Genres = track.Genres; + if (track.AlbumPosition is > 0) tag.Track = (uint)track.AlbumPosition.Value; + if (track.AlbumTrackCount is > 0) tag.TrackCount = (uint)track.AlbumTrackCount.Value; + if (track.Disc is > 0) tag.Disc = (uint)track.Disc.Value; + if (!string.IsNullOrWhiteSpace(track.Copyright)) tag.Copyright = track.Copyright; + + // Before the artist block on purpose: that block fills the album artist in from the + // artist only when nothing else has, so an album artist the user typed has to be in + // place by the time it runs or it would lose to the fallback. + if (track.AlbumArtists is { Length: > 0 } albumArtists) tag.AlbumArtists = albumArtists; if (!string.IsNullOrWhiteSpace(track.Artist)) { - tag.Performers = [track.Artist]; + tag.Performers = KeepsEveryPerformer(track) ? track.Performers! : [track.Artist]; // Without this the album artist keeps whatever was there before, and players that // group by album artist file the corrected track under the old, wrong name. @@ -168,4 +187,34 @@ or IOException private static string? NullIfBlank(string? value) => string.IsNullOrWhiteSpace(value) ? null : value; + + /// + /// Whether the performer list still agrees with the artist box, and so survives the save. + /// + /// + /// + /// An artist tag can hold more than one value, and the page shows one box. Writing + /// [track.Artist] unconditionally therefore threw the rest away: a file recorded as + /// AC/DC is stored as the two values AC and DC — ID3v2.3 treats the + /// slash as its separator — so the box read "AC" and Save narrowed the tag to "AC" on a page + /// whose whole purpose is repairing tags. + /// + /// + /// The first element is what the box was filled from, so it agreeing means nobody has typed + /// over it and the original list can go back verbatim. It disagreeing means the user or a + /// match replaced the artist, and their one value is what should be written. A Spotify match + /// satisfies this by construction — the mapper sets the artist from the first performer it + /// assigns — so a match still writes its full performer list, exactly as the recorder does. + /// + /// + /// Deliberately not "split the box on commas". That reads Earth, Wind & Fire as + /// three artists, which is a worse corruption than the one being fixed. + /// + /// + private static bool KeepsEveryPerformer(Track track) => + track.Performers is { Length: > 0 } performers + && string.Equals(performers[0], track.Artist, StringComparison.Ordinal); + + /// TagLib# reports an absent number as zero, which is not a track number. + private static int? PositiveOrNull(uint value) => value > 0 ? (int)value : null; } diff --git a/tests/Offstream.Core.Tests/Metadata/Library/LibraryLookupTests.cs b/tests/Offstream.Core.Tests/Metadata/Library/LibraryLookupTests.cs new file mode 100644 index 0000000..542fdf3 --- /dev/null +++ b/tests/Offstream.Core.Tests/Metadata/Library/LibraryLookupTests.cs @@ -0,0 +1,110 @@ +using Offstream.Core.Metadata; +using Offstream.Core.Metadata.Library; +using Xunit; + +namespace Offstream.Core.Tests.Metadata.Library; + +/// +/// A lookup on the Metadata page adds tags, and never takes one away. +/// +/// +/// The rule was fixed once inside the Spotify path alone and the Last.fm fallback still had the +/// hole, which is why the logic is in one place with two call sites rather than written out +/// twice. These tests cover the logic; the call sites are covered where they live. +/// +public sealed class LibraryLookupTests +{ + /// + /// A provider with nothing to say about a field says nothing, not "empty". + /// + /// + /// Spotify returns an empty genre list for most of its catalogue since late 2024 and Last.fm + /// returns none for an artist nobody has tagged, so a lookup that trusted the answer blanked + /// a genre the user had curated. Nothing ever reached the file — the writer skips empty + /// values — but the row reported a change Save would not make. + /// + [Fact] + public void EveryFieldTheLookupLeftEmpty_ComesBack() + { + var track = new Track + { + Genres = ["shoegaze"], + Year = 1991, + AlbumArtists = ["Slowdive"], + AlbumPosition = 3, + AlbumTrackCount = 9, + Disc = 2, + Copyright = "1991 Creation Records", + ReleaseDate = "1991-11-04", + }; + + var before = LibraryLookup.Snapshot(track); + + track.Genres = []; + track.Year = null; + track.AlbumArtists = null; + track.AlbumPosition = null; + track.AlbumTrackCount = null; + track.Disc = null; + track.Copyright = null; + track.ReleaseDate = null; + + LibraryLookup.KeepWhatWasThere(track, before); + + Assert.Equal(["shoegaze"], track.Genres!); + Assert.Equal(1991, track.Year); + Assert.Equal(["Slowdive"], track.AlbumArtists!); + Assert.Equal(3, track.AlbumPosition); + Assert.Equal(9, track.AlbumTrackCount); + Assert.Equal(2, track.Disc); + Assert.Equal("1991 Creation Records", track.Copyright); + Assert.Equal("1991-11-04", track.ReleaseDate); + } + + /// What the provider did answer is left exactly as it answered it. + /// + /// The other half of the rule, and the reason this is not simply "keep the file's tags". A + /// match that corrects a wrong year is the whole point of running one. + /// + [Fact] + public void WhatTheLookupAnswered_Wins() + { + var track = new Track { Genres = ["rock"], Year = 1990, Disc = 1 }; + var before = LibraryLookup.Snapshot(track); + + track.Genres = ["shoegaze"]; + track.Year = 1991; + track.Disc = 2; + + LibraryLookup.KeepWhatWasThere(track, before); + + Assert.Equal(["shoegaze"], track.Genres!); + Assert.Equal(1991, track.Year); + Assert.Equal(2, track.Disc); + } + + /// + /// Title, artist and album are deliberately not restored. + /// + /// + /// Replacing those is what a match is for, and the manual picker exists precisely to correct + /// them. Putting them back would make "Use this" unable to change the thing the user chose it + /// to change. + /// + [Fact] + public void AMatchMayStillReplaceTheTitleAndArtist() + { + var track = new Track { Title = "Track 03", Artist = "Unknown", Album = "Unknown Album" }; + var before = LibraryLookup.Snapshot(track); + + track.Title = "Alison"; + track.Artist = "Slowdive"; + track.Album = "Souvlaki"; + + LibraryLookup.KeepWhatWasThere(track, before); + + Assert.Equal("Alison", track.Title); + Assert.Equal("Slowdive", track.Artist); + Assert.Equal("Souvlaki", track.Album); + } +} diff --git a/tests/Offstream.Core.Tests/Metadata/Library/TagLibTagStoreTests.cs b/tests/Offstream.Core.Tests/Metadata/Library/TagLibTagStoreTests.cs index 66daaa4..39ef640 100644 --- a/tests/Offstream.Core.Tests/Metadata/Library/TagLibTagStoreTests.cs +++ b/tests/Offstream.Core.Tests/Metadata/Library/TagLibTagStoreTests.cs @@ -27,6 +27,11 @@ public sealed class TagLibTagStoreTests : IDisposable public void Dispose() => _workspace.Dispose(); /// What is written comes back. + /// + /// Every tag the recording path writes, because the page's promise is that a tag Offstream + /// puts into a file it can also put right. A field that reads back as null here is one the + /// user can type into a box and lose on the next scan. + /// [Fact] public async Task Write_ThenRead_RoundTripsEveryField() { @@ -37,8 +42,13 @@ public async Task Write_ThenRead_RoundTripsEveryField() Title = "The Mother We Share", Artist = "Chvrches", Album = "The Bones of What You Believe", + AlbumArtists = ["Chvrches"], Year = 2013, Genres = ["synthpop"], + AlbumPosition = 2, + AlbumTrackCount = 12, + Disc = 1, + Copyright = "2013 Goodbye Records", }, coverArt: null); var read = _store.Read(path); @@ -46,8 +56,124 @@ public async Task Write_ThenRead_RoundTripsEveryField() Assert.Equal("The Mother We Share", read.Title); Assert.Equal("Chvrches", read.Artist); Assert.Equal("The Bones of What You Believe", read.Album); + Assert.Equal(["Chvrches"], read.AlbumArtists!); Assert.Equal(2013, read.Year); Assert.Equal(["synthpop"], read.Genres!); + Assert.Equal(2, read.AlbumPosition); + Assert.Equal(12, read.AlbumTrackCount); + Assert.Equal(1, read.Disc); + Assert.Equal("2013 Goodbye Records", read.Copyright); + } + + /// + /// The numbers survive in every container the page will open. + /// + /// + /// Copyright is the field worth checking per container rather than once: it is the thinnest + /// support of the set, stored in a different place by each of ID3, Vorbis comments and the + /// MPEG-4 atom tree. A container that dropped it would give the user a box that accepts a + /// value, reports it saved and shows it gone on the next scan. + /// + [Theory] + [InlineData(MediaFormat.Mp3)] + [InlineData(MediaFormat.Flac)] + [InlineData(MediaFormat.Aac)] + [InlineData(MediaFormat.Opus)] + public async Task Write_ThenRead_KeepsTheNumbersInEveryContainer(MediaFormat format) + { + var path = await EncodeAsync(format); + + _store.Write(path, new Track + { + Title = "Recover", + Artist = "Chvrches", + AlbumPosition = 4, + AlbumTrackCount = 12, + Disc = 2, + Copyright = "2013 Goodbye Records", + }, coverArt: null); + + var read = _store.Read(path); + + Assert.Equal(4, read.AlbumPosition); + Assert.Equal(12, read.AlbumTrackCount); + Assert.Equal(2, read.Disc); + Assert.Equal("2013 Goodbye Records", read.Copyright); + } + + /// + /// An album artist the user typed outranks the one derived from the artist. + /// + /// + /// The writer fills the album artist in from the artist when the file has none, which is what + /// stops a corrected track being filed under the old name. That fallback runs after the + /// explicit write on purpose — the other order let it win, and a compilation retagged with + /// "Various Artists" came back credited to whoever performed the track. + /// + [Fact] + public async Task Write_KeepsAnAlbumArtistThatDisagreesWithTheArtist() + { + var path = await EncodeAsync(MediaFormat.Mp3); + + _store.Write(path, new Track + { + Title = "Sabotage", + Artist = "Beastie Boys", + AlbumArtists = ["Various Artists"], + }, coverArt: null); + + var read = _store.Read(path); + + Assert.Equal("Beastie Boys", read.Artist); + Assert.Equal(["Various Artists"], read.AlbumArtists!); + } + + /// + /// A scan and a save with nothing edited must give the file back exactly the artist it had, + /// however many values that tag holds. + /// + /// + /// ID3v2.3 separates artists with a slash, so a file recorded as "AC/DC" comes back as the + /// two values "AC" and "DC". The page has one artist box, it is filled from the first of + /// them, and writing that box alone used to narrow the tag to "AC" — the repair page + /// destroying the tag it was opened to repair. + /// + [Fact] + public async Task Write_KeepsEveryArtistOnATagThatHoldsMoreThanOne() + { + var path = await EncodeAsync(MediaFormat.Mp3); + + _store.Write(path, new Track { Title = "Who Made Who", Artist = "AC", Performers = ["AC", "DC"] }, coverArt: null); + + var read = _store.Read(path); + + Assert.Equal(["AC", "DC"], read.Performers!); + Assert.Equal("AC", read.Artist); + } + + /// Typing over the box is the one thing that does collapse the list. + [Fact] + public async Task Write_ReplacesEveryArtistWhenTheArtistWasEdited() + { + var path = await EncodeAsync(MediaFormat.Mp3); + + _store.Write(path, new Track { Title = "Who Made Who", Artist = "Acca Dacca", Performers = ["AC", "DC"] }, coverArt: null); + + var read = _store.Read(path); + + Assert.Equal(["Acca Dacca"], read.Performers!); + } + + /// An empty value leaves the file's own alone rather than erasing it. + [Fact] + public async Task Write_DoesNotClearATagItHasNothingToSayAbout() + { + var path = await EncodeAsync(MediaFormat.Mp3); + + _store.Write(path, new Track { Title = "Gun", Artist = "Chvrches", Disc = 3 }, coverArt: null); + _store.Write(path, new Track { Title = "Gun", Artist = "Chvrches" }, coverArt: null); + + Assert.Equal(3, _store.Read(path).Disc); } /// diff --git a/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs b/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs index 3358c5f..f4f72f8 100644 --- a/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs +++ b/tests/Offstream.Core.Tests/Metadata/SpotifyTrackMapperTests.cs @@ -36,6 +36,29 @@ public void ApplyTrack_WithAnEmptyResponse_LeavesTheWindowTitleTrackReadable() Assert.Null(track.Disc); } + /// The artist a match writes is the first performer it assigns, and stays so. + /// + /// TagLibTagStore decides whether a multi-value artist tag survives a save by asking + /// whether the performer list still starts with the artist — untouched means writable + /// verbatim. A match satisfies that by construction only while the mapper takes the artist + /// from performers[0]. Setting it from a joined string instead would silently collapse + /// every matched track's performer list to one name, with nothing else failing. + /// + [Fact] + public void ApplyTrack_LeavesTheArtistAgreeingWithTheFirstPerformer() + { + var track = WindowTitleTrack(); + + SpotifyTrackMapper.Apply(track, new FullTrack + { + Name = "Under Pressure", + Artists = [new SimpleArtist { Name = "Queen" }, new SimpleArtist { Name = "David Bowie" }], + }); + + Assert.Equal(["Queen", "David Bowie"], track.Performers!); + Assert.Equal(track.Performers![0], track.Artist); + } + [Fact] public void ApplyTrack_MapsNameArtistsTrackNumberAndDisc() { diff --git a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs index c278a8e..d34e0b6 100644 --- a/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs +++ b/tests/Offstream.UI.Tests/LibraryTrackViewModelTests.cs @@ -127,25 +127,14 @@ public void WasLine_ShowsForTheFieldThatChanged() public void WasLine_IsHiddenWhenTheFileAndTheBoxAreBothEmpty() => Assert.False(Row(album: null).HasAlbumChange); - /// A row whose match changed nothing outside the three boxes shows no extra block. - [Fact] - public void MatchDetails_AreHiddenWhenTheBoxesSayItAll() - { - var row = Row(); - - row.Title = "Something Else"; - - Assert.False(row.HasMatchDetails); - } - - /// A year the file did not have is a change the boxes cannot show. + /// A year a match brought lands in the year box, not in a read-only line. /// - /// This is the complaint the block answers. Before it, a row could say "will change" with - /// title, artist and album all identical to the file and nothing on screen saying why — - /// the match had brought a year, a genre or artwork, and Save writes all three. + /// Year and genre were shown beside the three editable boxes and could not be corrected, + /// which made a wrong match's year unfixable without an outside tag editor. Every tag the + /// recorder writes now has a box, so the check is that a fetched value reaches it. /// [Fact] - public void MatchDetails_ShowTheYearAMatchBrought() + public void AFetchedYear_LandsInTheYearBox() { var track = new LibraryTrack( @"C:\Music\01 Cloudbusting.mp3", @@ -156,15 +145,14 @@ public void MatchDetails_ShowTheYearAMatchBrought() track.Suggested.Year = 1985; row.RefreshFromSuggestion(); + Assert.Equal("1985", row.Year); Assert.True(row.HasYearChange); - Assert.True(row.HasMatchDetails); - Assert.Equal("1985", row.SuggestedYear); Assert.True(row.HasPendingChanges); } - /// Genres are shown the same way, joined for reading. + /// Genres come back as one line, because that is what the box holds. [Fact] - public void MatchDetails_ShowTheGenresAMatchBrought() + public void FetchedGenres_ArriveCommaSeparated() { var track = new LibraryTrack( @"C:\Music\01 Cloudbusting.mp3", @@ -175,13 +163,13 @@ public void MatchDetails_ShowTheGenresAMatchBrought() track.Suggested.Genres = ["art pop", "art rock"]; row.RefreshFromSuggestion(); + Assert.Equal("art pop, art rock", row.Genres); Assert.True(row.HasGenreChange); - Assert.Equal("art pop, art rock", row.SuggestedGenres); } /// Artwork the file already has is not a change, so no before-and-after appears. [Fact] - public void MatchDetails_IgnoreArtworkTheFileAlreadyHas() + public void Artwork_TheFileAlreadyHasIsNotAChange() { var picture = new byte[] { 1, 2, 3, 4 }; @@ -196,7 +184,119 @@ public void MatchDetails_IgnoreArtworkTheFileAlreadyHas() })); Assert.False(row.HasCoverArtChange); - Assert.False(row.HasMatchDetails); + } + + /// An edit to a number the page never used to show still counts as a change. + /// + /// The one that would go wrong quietly. LibraryTrack.HasChanges compared five fields, + /// so a row where the user corrected only the disc number reported nothing to save and Save + /// skipped it — the edit was accepted by the box and then dropped without a word. + /// + [Fact] + public void EditingOnlyTheDiscNumber_StillNeedsSaving() + { + var row = Row(); + + Assert.False(row.HasPendingChanges); + + row.Disc = "2"; + + Assert.True(row.HasDiscChange); + Assert.True(row.HasPendingChanges); + } + + /// Numbers are validated in the box rather than thrown away at the point of writing. + [Theory] + [InlineData("1985", false)] + [InlineData("", false)] + [InlineData("85", true)] + [InlineData("-1", true)] + [InlineData("nineteen", true)] + public void AYear_MustBeFourDigitsOrNothing(string typed, bool expectedError) + { + var row = Row(); + + row.Year = typed; + + Assert.Equal(expectedError, row.GetErrors(nameof(row.Year)).Cast().Any()); + } + + /// + /// A half-typed number leaves the tag alone rather than clearing it. + /// + /// + /// The boxes update on every keystroke, so backspacing over "1985" to retype it passes + /// through "198", "19", "1" — and then through states that do not parse at all. Nulling the + /// tag on those would erase a year while the user was in the middle of correcting it. + /// + [Fact] + public void AnUnparseableYear_LeavesTheTagWhereItWas() + { + var track = new LibraryTrack( + @"C:\Music\01 Cloudbusting.mp3", + new Track { Title = "Cloudbusting", Artist = "Kate Bush", Year = 1985 }); + + var row = new LibraryTrackViewModel(track) { Year = "nineteen" }; + + Assert.Equal(1985, track.Suggested.Year); + } + + /// Clearing a box asks for nothing, which is not the same as asking for a blank. + /// + /// The writer never writes an empty value over a real one, so a cleared box that reported a + /// change would light the "will change" badge and then save nothing at all. + /// + [Fact] + public void ClearingABox_IsNotAChange() + { + var track = new LibraryTrack( + @"C:\Music\01 Cloudbusting.mp3", + new Track { Title = "Cloudbusting", Artist = "Kate Bush", Album = "Hounds of Love" }); + + var row = new LibraryTrackViewModel(track) { Album = string.Empty }; + + Assert.False(row.HasAlbumChange); + Assert.False(row.HasPendingChanges); + } + + /// A comma inside a name is part of the name, not a second name. + /// + /// The genre box splits on commas because a genre list really is a list. Doing the same to + /// the album artist box files "Earth, Wind & Fire" under two acts — a worse corruption + /// than the multi-value tag the box was added to expose. + /// + [Fact] + public void AnAlbumArtistWithACommaInIt_StaysOneName() + { + var track = new LibraryTrack( + @"C:\Music\01 September.mp3", + new Track { Title = "September", Artist = "Earth, Wind & Fire" }); + + _ = new LibraryTrackViewModel(track) { AlbumArtist = "Earth, Wind & Fire" }; + + Assert.Equal(["Earth, Wind & Fire"], track.Suggested.AlbumArtists!); + } + + /// An album artist list nobody touched goes back the way it came. + [Fact] + public void AnUneditedAlbumArtistList_KeepsEveryName() + { + var track = new LibraryTrack( + @"C:\Music\01 Under Pressure.mp3", + new Track { Title = "Under Pressure", Artist = "Queen", AlbumArtists = ["Queen", "David Bowie"] }); + + var row = new LibraryTrackViewModel(track); + + Assert.Equal("Queen, David Bowie", row.AlbumArtist); + + row.RefreshFromSuggestion(); + + Assert.Equal(["Queen", "David Bowie"], track.Suggested.AlbumArtists!); + + // And the row stays quiet: a list that came back the way it went in is not a change, or + // every multi-value file in the library would wear the "will change" badge. + Assert.False(row.HasAlbumArtistChange); + Assert.False(row.HasPendingChanges); } /// The thumbnail follows a match that brought a URL rather than bytes.