diff --git a/.planning/NOTES.md b/.planning/NOTES.md index c108b99..86e4dcd 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3483,3 +3483,26 @@ public tap. A guard added today does not protect a tag that points at yesterday. When re-pointing a tag, check what the workflows looked like *there*. + +## A tag reader looks at exactly one spelling of "total" (measured 2026-08-18) + +Writing #16's totals means matching the reader, which is +`dhowden/tag`, and it is narrower than the specs are: + +- **Vorbis (FLAC, OGG): `TRACKTOTAL` and `DISCTOTAL` only.** + `vorbis.go`'s `Track()` reads `tracknumber` and `tracktotal` and + nothing else, so `TOTALTRACKS` — which several taggers write and + which xiph lists — and a `1/12` packed into `TRACKNUMBER` both read + back as *no total*. They write successfully. Nothing errors. +- **ID3v2 (MP3): `TRCK`/`TPOS` as `n/N`**, via `parseXofN`. That is one + frame carrying two facts, which is why `applyPositionFrame` reads the + existing frame before writing either half. +- **WAV: nothing at all.** There is no RIFF reader in the module, so a + WAV's `id3 ` chunk is invisible to `metadata.ExtractTags` — every + field, not just the totals. Filed as #104. + +The general shape, and the reason this is written down: a tag written +under a name the reader does not look at is indistinguishable from one +never written. So the tests assert the round trip through +`metadata.ExtractTags` — the reader the *scan* uses — rather than +through the bytes the writer produced. diff --git a/CLAUDE.md b/CLAUDE.md index ee56427..7d861bc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1547,11 +1547,55 @@ shape as the encoding probe beside it. What neither side can give is *which* tracks are missing, only how many — so an incomplete album still browses, and that is now the exception -rather than every album load. Two smaller consequences: existing databases -read "unknown" until a rescan repopulates the column (which degrades to -exactly the old behaviour, so nothing breaks), and our own `tagwriter` -writes track and disc *numbers* but not totals, so autotagging a folder -currently degrades the field this rests on. +rather than every album load. One smaller consequence: existing databases +read "unknown" until a rescan repopulates the column, which degrades to +exactly the old behaviour, so nothing breaks. + +**And our own writers declare the total, because for a long time they +did not.** `tagwriter` wrote track and disc *numbers* and dropped the +totals, so autotagging an album actively **erased** the evidence this +rests on: the release became MBID-matched — a green tick — while the +field `GetAlbumCompleteness` reads stayed absent, which is exactly the +"2 of 10 tracks, reported as in your library" the report described. +`FieldTotalTracks` / `FieldTotalDiscs` are written by the autotag apply +pass and by the download importer, and `dbsync` persists the track +total to the row so the album page agrees with the file without waiting +for a rescan. + +Five things about it are load-bearing, and four of them fail silently: + +- **The total is per *disc*, not per release**, because that is what + the tag form declares and what `GetAlbumCompleteness` **sums** per + disc — a release total written on every file multiplies a two-disc + album's expectation by two, and no library can then satisfy it. + `backend/tagtotals` is that derivation, once, because the two callers + must not import each other or the writer. +- **The Vorbis names are `TRACKTOTAL` and `DISCTOTAL` and no other + spelling.** `dhowden/tag`'s Vorbis reader looks at exactly those two + keys, so a perfectly reasonable `TOTALTRACKS`, or a `1/12` inside + `TRACKNUMBER`, is written successfully and reads back as no total at + all. The tests assert the round trip through the reader the *scan* + uses rather than through the bytes, for that reason. +- **ID3's number and total share one frame**, so writing either alone + has to read the other off the existing tag or it silently discards + it. A total with no number is not written: `/12` is what a reader + parses as track 0. +- **The totals are written unconditionally, not on a diff.** The case + this exists for is a file that declares *no* total, which compares + equal to nothing and is exactly what a "only if it changed" guard + skips. +- **A single-track download must not be totalled.** A `RecordingMBID` + anchor resolves `Expected` to that one track, so the same code would + tag a track off a twelve-track album "1 of 1" — and a declared total + outranks the catalog total that would otherwise have answered + correctly. Confidently wrong is worse than absent here, which is the + same rule `Known` exists for. + +One gap this did not close, and it is older: **`dhowden/tag` has no +RIFF reader**, so nothing the tag writer puts in a WAV's `id3 ` chunk +is visible to `metadata.ExtractTags` — not the totals and not the title +either. `wav_test.go` reads that chunk itself, which is why no test +ever noticed. **The absence is what gets marked, not the presence.** The tracklist put a green tick against every owned track and a legend underneath diff --git a/backend/autotag/apply.go b/backend/autotag/apply.go index 578798f..757a89b 100644 --- a/backend/autotag/apply.go +++ b/backend/autotag/apply.go @@ -8,6 +8,7 @@ import ( "log/slog" "yellowjacket/backend/database/sql/sqlcgen" + "yellowjacket/backend/tagtotals" ) // TagChanges mirrors tagwriter.TagChanges — redefined here so the @@ -28,6 +29,8 @@ const ( FieldYear = "year" FieldTrackNumber = "track_number" FieldDiscNumber = "disc_number" + FieldTotalTracks = "total_tracks" + FieldTotalDiscs = "total_discs" FieldCoverArt = "cover_art" ) @@ -418,5 +421,32 @@ func buildChanges( changes[FieldDiscNumber] = track.DiscNumber } + // The totals are what says "2 of 10" rather than a bare tick, and + // dropping them here is what made autotagging an album *erase* the + // evidence: the release becomes MBID-matched while the field + // GetAlbumCompleteness reads stays absent. + // + // They are written unconditionally where the candidate has a + // tracklist, not only when they differ from the local value, because + // the common case is a file that declares no total at all -- which + // compares equal to nothing and would be skipped by a diff guard. + if tracks, discs := tagtotals.For( + candidatePositions(cand), track.DiscNumber, + ); tracks > 0 { + changes[FieldTotalTracks] = tracks + changes[FieldTotalDiscs] = discs + } + return changes } + +// candidatePositions is the candidate's tracklist as bare positions. +func candidatePositions(cand Candidate) []tagtotals.Position { + out := make([]tagtotals.Position, 0, len(cand.Tracks)) + + for _, t := range cand.Tracks { + out = append(out, tagtotals.Position{Disc: t.DiscNumber, Track: t.Position}) + } + + return out +} diff --git a/backend/autotag/buildchanges_test.go b/backend/autotag/buildchanges_test.go new file mode 100644 index 0000000..21615fc --- /dev/null +++ b/backend/autotag/buildchanges_test.go @@ -0,0 +1,90 @@ +package autotag + +import "testing" + +// Autotagging an album used to *erase* the evidence that says "2 of 10": +// the release became MBID-matched while the totals the files declared +// went unwritten, so the album page showed a plain tick. These pin the +// two halves of the fix that are easy to get wrong silently. +func TestBuildChanges_Totals(t *testing.T) { + t.Parallel() + + twoDiscs := Candidate{ + Tracks: []CandidateTrack{ + {DiscNumber: 1, Position: 1}, + {DiscNumber: 1, Position: 2}, + {DiscNumber: 2, Position: 1}, + {DiscNumber: 2, Position: 2}, + {DiscNumber: 2, Position: 3}, + }, + } + + tests := []struct { + name string + cand Candidate + local LocalTrack + track CandidateTrack + wantTracks any + wantDiscs any + }{ + { + // The common case, and the one a diff guard would skip: the + // file declares no total at all, so the total "has not + // changed" and would never be written. + name: "a file with no total gets one", + cand: Candidate{Tracks: []CandidateTrack{ + {Position: 1}, {Position: 2}, {Position: 3}, + }}, + local: LocalTrack{TrackNumber: 1}, + track: CandidateTrack{Position: 1}, + wantTracks: 3, + wantDiscs: 1, + }, + { + // 5 here would be the release's track count. Summed once + // per disc by GetAlbumCompleteness that claims a ten-track + // expectation for a five-track album, which no library can + // ever satisfy. + name: "a multi-disc release totals the track's own disc", + cand: twoDiscs, + local: LocalTrack{}, + track: CandidateTrack{DiscNumber: 2, Position: 1}, + wantTracks: 3, + wantDiscs: 2, + }, + { + name: "the other disc gets its own total", + cand: twoDiscs, + local: LocalTrack{}, + track: CandidateTrack{DiscNumber: 1, Position: 1}, + wantTracks: 2, + wantDiscs: 2, + }, + { + // A candidate with no tracklist knows nothing, and writing + // a zero would claim it did. + name: "a candidate with no tracklist writes no total", + cand: Candidate{}, + local: LocalTrack{}, + track: CandidateTrack{Position: 1}, + wantTracks: nil, + wantDiscs: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + changes := buildChanges(tc.local, tc.cand, tc.track) + + if got := changes[FieldTotalTracks]; got != tc.wantTracks { + t.Errorf("%s: got %v, want %v", FieldTotalTracks, got, tc.wantTracks) + } + + if got := changes[FieldTotalDiscs]; got != tc.wantDiscs { + t.Errorf("%s: got %v, want %v", FieldTotalDiscs, got, tc.wantDiscs) + } + }) + } +} diff --git a/backend/autotagservice/fieldnames_test.go b/backend/autotagservice/fieldnames_test.go new file mode 100644 index 0000000..bbfd2cd --- /dev/null +++ b/backend/autotagservice/fieldnames_test.go @@ -0,0 +1,38 @@ +package autotagservice + +import ( + "testing" + + "yellowjacket/backend/autotag" + "yellowjacket/backend/tagwriter" +) + +// twAdapter passes the diff map through unchanged, so autotag's field +// constants and tagwriter's are the same keys written down twice -- +// deliberately, to keep autotag out of the write pipeline's import +// graph. A key that drifts does not fail to compile and does not fail +// to write: the writer simply finds no entry under the name it looks +// for, and the field is silently dropped. That is what this pins, and +// this package is the one place that imports both. +func TestAutotagAndTagwriterAgreeOnFieldNames(t *testing.T) { + t.Parallel() + + pairs := map[string][2]string{ + "title": {autotag.FieldTitle, tagwriter.FieldTitle}, + "artist": {autotag.FieldArtist, tagwriter.FieldArtist}, + "album": {autotag.FieldAlbum, tagwriter.FieldAlbum}, + "album artist": {autotag.FieldAlbumArtist, tagwriter.FieldAlbumArtist}, + "year": {autotag.FieldYear, tagwriter.FieldYear}, + "track number": {autotag.FieldTrackNumber, tagwriter.FieldTrackNumber}, + "disc number": {autotag.FieldDiscNumber, tagwriter.FieldDiscNumber}, + "total tracks": {autotag.FieldTotalTracks, tagwriter.FieldTotalTracks}, + "total discs": {autotag.FieldTotalDiscs, tagwriter.FieldTotalDiscs}, + "cover art": {autotag.FieldCoverArt, tagwriter.FieldCoverArt}, + } + + for name, pair := range pairs { + if pair[0] != pair[1] { + t.Errorf("%s: autotag says %q, tagwriter says %q", name, pair[0], pair[1]) + } + } +} diff --git a/backend/download/importer.go b/backend/download/importer.go index 6754be5..45020a5 100644 --- a/backend/download/importer.go +++ b/backend/download/importer.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" + "yellowjacket/backend/tagtotals" "yellowjacket/backend/tagwriter" ) @@ -275,6 +276,25 @@ func (i *Importer) tagFile(p plannedFile, dl Download) error { changes[tagwriter.FieldDiscNumber] = p.Track.DiscNumber } + // An imported file should arrive knowing how much of the album it + // is one of, or the album reads as "in your library" from its first + // imported track onward. + // + // A *track* download is the case this must not touch: a + // RecordingMBID anchor resolves Expected to exactly that one track, + // so totalling it would write "1 of 1" onto a track off a + // twelve-track album -- a confident lie, and one that outranks the + // catalog's own total, which is the fallback that would otherwise + // have answered correctly. + if dl.RecordingMBID == "" { + if tracks, discs := tagtotals.For( + expectedPositions(dl.Expected), p.Track.DiscNumber, + ); tracks > 0 { + changes[tagwriter.FieldTotalTracks] = tracks + changes[tagwriter.FieldTotalDiscs] = discs + } + } + if err := i.tags.WriteUntrackedFileTags(p.Source, changes); err != nil { return fmt.Errorf("write tags: %w", err) } @@ -282,6 +302,18 @@ func (i *Importer) tagFile(p plannedFile, dl Download) error { return nil } +// expectedPositions is the download's resolved tracklist as bare +// positions. +func expectedPositions(expected []ExpectedTrack) []tagtotals.Position { + out := make([]tagtotals.Position, 0, len(expected)) + + for _, t := range expected { + out = append(out, tagtotals.Position{Disc: t.DiscNumber, Track: t.Position}) + } + + return out +} + // destinationFor computes a file's library path from the template. func (i *Importer) destinationFor( p plannedFile, diff --git a/backend/download/importer_test.go b/backend/download/importer_test.go index fbac0e7..8ff8889 100644 --- a/backend/download/importer_test.go +++ b/backend/download/importer_test.go @@ -446,3 +446,77 @@ func keysOf(m map[string]tagwriter.TagChanges) []string { return out } + +// An imported album should arrive knowing its own size, or the album +// page reads "in your library" from its first imported track onward -- +// which is the badge complaint this exists to answer. +func TestImportWritesTheAlbumTotals(t *testing.T) { + t.Parallel() + + f := newImportFixture(t, + "01 - Airbag.flac", + "02 - Paranoid Android.flac", + "03 - Subterranean Homesick Alien.flac", + "04 - Exit Music (For a Film).flac", + ) + + if _, err := f.importer.Import( + context.Background(), + fourTrackDownload(), + Result{Dir: f.dir, Files: f.files}, + ImportOptions{LibraryRoot: f.root, WriteTags: true}, + ); err != nil { + t.Fatalf("Import: %v", err) + } + + changes := f.tags.writes["01 - Airbag.flac"] + if changes == nil { + t.Fatal("no tag write recorded for the first track") + } + + if got := changes[tagwriter.FieldTotalTracks]; got != 4 { + t.Errorf("%s: got %v, want 4", tagwriter.FieldTotalTracks, got) + } + + if got := changes[tagwriter.FieldTotalDiscs]; got != 1 { + t.Errorf("%s: got %v, want 1", tagwriter.FieldTotalDiscs, got) + } +} + +// A RecordingMBID anchor resolves Expected to exactly the one track it +// asked for, so totalling it would tag a track off a twelve-track album +// as "1 of 1" -- worse than saying nothing, because a declared total +// outranks the catalog total that would have answered correctly. +func TestImportWritesNoTotalsForATrackDownload(t *testing.T) { + t.Parallel() + + f := newImportFixture(t, "01 - Airbag.flac") + + dl := Download{ + ID: "dl-track", + LibraryID: 1, + RecordingMBID: "mbid-recording", + Artist: "Radiohead", + Album: "OK Computer", + Expected: []ExpectedTrack{{Position: 1, Title: "Airbag"}}, + } + + if _, err := f.importer.Import( + context.Background(), + dl, + Result{Dir: f.dir, Files: f.files}, + ImportOptions{LibraryRoot: f.root, WriteTags: true}, + ); err != nil { + t.Fatalf("Import: %v", err) + } + + changes := f.tags.writes["01 - Airbag.flac"] + if changes == nil { + t.Fatal("no tag write recorded") + } + + if _, ok := changes[tagwriter.FieldTotalTracks]; ok { + t.Errorf("%s written for a single-track download: %v", + tagwriter.FieldTotalTracks, changes[tagwriter.FieldTotalTracks]) + } +} diff --git a/backend/tagtotals/tagtotals.go b/backend/tagtotals/tagtotals.go new file mode 100644 index 0000000..68afc84 --- /dev/null +++ b/backend/tagtotals/tagtotals.go @@ -0,0 +1,56 @@ +// Package tagtotals derives the totals a tag's "5/12" form declares. +// +// It exists because the two writers that know a release's full +// tracklist -- the autotag apply pass and the download importer -- +// must not import each other or the tag writer, and because getting +// the denominator wrong is invisible: a total that is too large marks +// a complete album incomplete forever, and nothing fails. +package tagtotals + +// Position is one track's place in a release. A zero Disc means the +// release did not say, which is disc 1. +type Position struct { + Disc int + Track int +} + +// For returns the totals to write on a file sitting on disc `disc`: +// how many tracks that disc has, and how many discs the release has. +// +// The track total is **per disc** and not the release's track count, +// because that is what the tag form means and what +// GetAlbumCompleteness sums -- summing a release total once per disc +// would multiply a two-disc album's expectation by two. +// +// Tracks are counted by distinct position rather than by row: a +// tracklist that lists a position twice is a defect in the source, and +// counting it twice would put an album permanently out of reach of its +// own total. +func For(all []Position, disc int) (tracks, discs int) { + disc = normaliseDisc(disc) + + seenTracks := make(map[int]struct{}, len(all)) + seenDiscs := make(map[int]struct{}, 1) + + for _, p := range all { + d := normaliseDisc(p.Disc) + seenDiscs[d] = struct{}{} + + if d != disc || p.Track <= 0 { + continue + } + + seenTracks[p.Track] = struct{}{} + } + + return len(seenTracks), len(seenDiscs) +} + +// normaliseDisc treats an undeclared disc as disc 1. +func normaliseDisc(d int) int { + if d <= 0 { + return 1 + } + + return d +} diff --git a/backend/tagtotals/tagtotals_test.go b/backend/tagtotals/tagtotals_test.go new file mode 100644 index 0000000..d89bf43 --- /dev/null +++ b/backend/tagtotals/tagtotals_test.go @@ -0,0 +1,92 @@ +package tagtotals_test + +import ( + "testing" + + "yellowjacket/backend/tagtotals" +) + +func TestFor(t *testing.T) { + t.Parallel() + + singleDisc := []tagtotals.Position{ + {Disc: 0, Track: 1}, {Disc: 0, Track: 2}, {Disc: 0, Track: 3}, + } + + twoDiscs := []tagtotals.Position{ + {Disc: 1, Track: 1}, + {Disc: 1, Track: 2}, + {Disc: 2, Track: 1}, + {Disc: 2, Track: 2}, + {Disc: 2, Track: 3}, + } + + tests := []struct { + name string + all []tagtotals.Position + disc int + wantTracks int + wantDiscs int + }{ + { + name: "a single-disc release totals its own tracks", + all: singleDisc, disc: 0, wantTracks: 3, wantDiscs: 1, + }, + { + // An undeclared disc is disc 1, on both sides of the + // question -- a file tagged "disc 1" and a tracklist that + // declares no disc describe the same disc. + name: "an undeclared disc is disc 1", + all: singleDisc, disc: 1, wantTracks: 3, wantDiscs: 1, + }, + { + // The whole point: 5 here would be the release's track + // count, which summed once per disc claims a ten-track + // expectation for a five-track album. + name: "a multi-disc release totals the file's own disc", + all: twoDiscs, disc: 2, wantTracks: 3, wantDiscs: 2, + }, + { + name: "the other disc gets its own total", + all: twoDiscs, disc: 1, wantTracks: 2, wantDiscs: 2, + }, + { + // A disc the tracklist does not mention cannot be totalled, + // and 0 is how the caller is told to write nothing. + name: "a disc with no tracks totals nothing", + all: twoDiscs, disc: 3, wantTracks: 0, wantDiscs: 2, + }, + { + name: "an empty tracklist totals nothing", + all: nil, disc: 1, wantTracks: 0, wantDiscs: 0, + }, + { + // A source that lists a position twice would otherwise put + // the album permanently one track short of its own total. + name: "a repeated position counts once", + all: []tagtotals.Position{ + {Disc: 1, Track: 1}, {Disc: 1, Track: 1}, {Disc: 1, Track: 2}, + }, + disc: 1, wantTracks: 2, wantDiscs: 1, + }, + { + name: "a track with no position is not counted", + all: []tagtotals.Position{ + {Disc: 1, Track: 0}, {Disc: 1, Track: 1}, + }, + disc: 1, wantTracks: 1, wantDiscs: 1, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tracks, discs := tagtotals.For(tc.all, tc.disc) + if tracks != tc.wantTracks || discs != tc.wantDiscs { + t.Errorf("For(%v, %d) = (%d, %d), want (%d, %d)", + tc.all, tc.disc, tracks, discs, tc.wantTracks, tc.wantDiscs) + } + }) + } +} diff --git a/backend/tagwriter/dbsync.go b/backend/tagwriter/dbsync.go index 68e57ce..d644d9e 100644 --- a/backend/tagwriter/dbsync.go +++ b/backend/tagwriter/dbsync.go @@ -183,6 +183,15 @@ func syncDatabase( discNum = toNullInt64(v) } + // The completeness evidence. Without this the row keeps whatever + // the last scan read while the file on disk now declares a total, + // so the album stays "unknown" until a full rescan -- which is the + // state the report describes. + totalTracks := old.TotalTracks + if v, ok := asInt(params.changes[FieldTotalTracks]); ok { + totalTracks = toNullInt64(v) + } + composer := old.Composer if v, ok := params.changes[FieldComposer].(string); ok { composer = v @@ -207,7 +216,7 @@ func syncDatabase( AlbumID: albumID, TrackNumber: trackNum, DiscNumber: discNum, - TotalTracks: old.TotalTracks, + TotalTracks: totalTracks, Year: year, Composer: composer, Comment: old.Comment, diff --git a/backend/tagwriter/flac.go b/backend/tagwriter/flac.go index bca99c1..54a2036 100644 --- a/backend/tagwriter/flac.go +++ b/backend/tagwriter/flac.go @@ -101,6 +101,11 @@ func applyFlacTextChanges(cmt *flacvorbis.MetaDataBlockVorbisComment, changes Ta {FieldYear, flacvorbis.FIELD_DATE, true}, {FieldTrackNumber, flacvorbis.FIELD_TRACKNUMBER, true}, {FieldDiscNumber, "DISCNUMBER", true}, + // TRACKTOTAL/DISCTOTAL and no other spelling: dhowden/tag's + // Vorbis reader looks at exactly these two keys, so TOTALTRACKS + // or a "1/12" inside TRACKNUMBER reads back as no total at all. + {FieldTotalTracks, "TRACKTOTAL", true}, + {FieldTotalDiscs, "DISCTOTAL", true}, {FieldComposer, "COMPOSER", false}, } diff --git a/backend/tagwriter/mp3.go b/backend/tagwriter/mp3.go index b654a7d..768a435 100644 --- a/backend/tagwriter/mp3.go +++ b/backend/tagwriter/mp3.go @@ -6,6 +6,7 @@ import ( "log/slog" "os" "strconv" + "strings" id3v2 "github.com/bogem/id3v2/v2" @@ -66,17 +67,10 @@ func applyTextChanges(tag *id3v2.Tag, changes TagChanges) { tag.SetYear(strconv.Itoa(v)) } - if v, ok := asInt(changes[FieldTrackNumber]); ok { - trckID := tag.CommonID("Track number/Position in set") - tag.DeleteFrames(trckID) - tag.AddTextFrame(trckID, id3v2.EncodingUTF8, strconv.Itoa(v)) - } - - if v, ok := asInt(changes[FieldDiscNumber]); ok { - tposID := tag.CommonID("Part of a set") - tag.DeleteFrames(tposID) - tag.AddTextFrame(tposID, id3v2.EncodingUTF8, strconv.Itoa(v)) - } + applyPositionFrame(tag, "Track number/Position in set", changes, + FieldTrackNumber, FieldTotalTracks) + applyPositionFrame(tag, "Part of a set", changes, + FieldDiscNumber, FieldTotalDiscs) if v, ok := changes[FieldComposer].(string); ok { tag.DeleteFrames("TCOM") @@ -90,6 +84,64 @@ func applyTextChanges(tag *id3v2.Tag, changes TagChanges) { } } +// applyPositionFrame writes an ID3v2 position frame (TRCK or TPOS) in +// the "n/N" form the readers parse. +// +// The number and the total are separate diff entries and either may be +// absent, so the frame's *existing* value is the base: writing a total +// alone must not discard the number that is already there, and writing +// a number alone must not discard a total the file already declared. +// A total with no number at all is not written, since "/12" says +// nothing a reader can use. +func applyPositionFrame( + tag *id3v2.Tag, description string, changes TagChanges, numKey, totalKey string, +) { + _, hasNum := changes[numKey] + _, hasTotal := changes[totalKey] + + if !hasNum && !hasTotal { + return + } + + frameID := tag.CommonID(description) + + num, total := parseXofN( + strings.TrimRight(tag.GetTextFrame(frameID).Text, "\x00 \t\n\r"), + ) + + if v, ok := asInt(changes[numKey]); ok { + num = v + } + + if v, ok := asInt(changes[totalKey]); ok { + total = v + } + + if num <= 0 { + return + } + + value := strconv.Itoa(num) + if total > 0 { + value += "/" + strconv.Itoa(total) + } + + tag.DeleteFrames(frameID) + tag.AddTextFrame(frameID, id3v2.EncodingUTF8, value) +} + +// parseXofN splits an ID3v2 "n/N" position value. A bare "n" yields a +// zero total, and anything unparseable yields zeros — the same reading +// dhowden/tag gives the frame. +func parseXofN(s string) (int, int) { + numText, totalText, _ := strings.Cut(s, "/") + + num, _ := strconv.Atoi(strings.TrimSpace(numText)) + total, _ := strconv.Atoi(strings.TrimSpace(totalText)) + + return num, total +} + // applyCoverArtChanges handles the FieldCoverArt entry in the diff map. // // - []byte with len > 0: embed the given image as front cover. diff --git a/backend/tagwriter/ogg_vorbis.go b/backend/tagwriter/ogg_vorbis.go index 44d8d86..69d0b26 100644 --- a/backend/tagwriter/ogg_vorbis.go +++ b/backend/tagwriter/ogg_vorbis.go @@ -166,6 +166,8 @@ var oggFieldMappings = []struct { //nolint:gochecknoglobals // field mapping tab {FieldYear, "DATE", true}, {FieldTrackNumber, "TRACKNUMBER", true}, {FieldDiscNumber, "DISCNUMBER", true}, + {FieldTotalTracks, "TRACKTOTAL", true}, + {FieldTotalDiscs, "DISCTOTAL", true}, {FieldComposer, "COMPOSER", false}, } diff --git a/backend/tagwriter/pipeline_test.go b/backend/tagwriter/pipeline_test.go index f59a32f..ad1cf48 100644 --- a/backend/tagwriter/pipeline_test.go +++ b/backend/tagwriter/pipeline_test.go @@ -333,3 +333,31 @@ func TestWriteTrackTags_DBSync(t *testing.T) { t.Error("expected FTS5 result for 'New Title'") } } + +// The row is what the album page reads, and it is only refreshed by a +// scan. Leaving total_tracks at whatever the last scan saw means an +// album autotagged just now stays "unknown" -- a plain tick on an album +// the user holds two tracks of -- until a full rescan happens to run. +func TestWriteTrackTags_PersistsTheTotal(t *testing.T) { + db := database.NewTestDB(t) + dir := t.TempDir() + trackID := seedTestTrack(t, db, createPipelineTestMP3(t, dir)) + + tw := NewTagWriter(testLogger(), db, &mockPlayer{}, &mockPipelineLocker{}) + + if err := tw.WriteTrackTags(trackID, TagChanges{ + FieldTrackNumber: 2, + FieldTotalTracks: 10, + }); err != nil { + t.Fatalf("WriteTrackTags: %v", err) + } + + af, err := db.Queries.GetAudioFile(context.Background(), trackID) + if err != nil { + t.Fatalf("get audio file: %v", err) + } + + if !af.TotalTracks.Valid || af.TotalTracks.Int64 != 10 { + t.Errorf("total_tracks: got %v, want 10", af.TotalTracks) + } +} diff --git a/backend/tagwriter/tagwriter.go b/backend/tagwriter/tagwriter.go index 433adcf..87c2d70 100644 --- a/backend/tagwriter/tagwriter.go +++ b/backend/tagwriter/tagwriter.go @@ -26,6 +26,15 @@ const ( FieldDiscNumber = "disc_number" FieldComposer = "composer" FieldCoverArt = "cover_art" // []byte for set, nil for clear + + // FieldTotalTracks is how many tracks are on *this file's disc*, not + // in the whole release. That is what the "5/12" form declares and + // what GetAlbumCompleteness sums per disc; a release total written + // here would multiply the expectation by the number of discs. + FieldTotalTracks = "total_tracks" + + // FieldTotalDiscs is how many discs the release has. + FieldTotalDiscs = "total_discs" ) // AudioFormat represents a supported audio file format. diff --git a/backend/tagwriter/totals_test.go b/backend/tagwriter/totals_test.go new file mode 100644 index 0000000..860b4ea --- /dev/null +++ b/backend/tagwriter/totals_test.go @@ -0,0 +1,199 @@ +package tagwriter + +import ( + "path/filepath" + "testing" + + "yellowjacket/backend/metadata" +) + +// The totals are the evidence GetAlbumCompleteness reads, and every way +// of getting them wrong is silent: a tag written under a name the +// reader does not look at reads back as no total at all, which is +// indistinguishable from never having written one. So these assert the +// round trip through the *reader the scan uses*, not the bytes. +// +// WAV is the exception and it is not this change's: dhowden/tag has no +// RIFF reader at all, so metadata.ExtractTags cannot see a WAV's ID3 +// chunk -- which is why every other test here reads that chunk itself. +func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) { + t.Parallel() + + changes := TagChanges{ + FieldTitle: "Some Song", + FieldTrackNumber: 2, + FieldTotalTracks: 10, + FieldDiscNumber: 1, + FieldTotalDiscs: 2, + } + + viaScanner := func(t *testing.T, path string) *metadata.TrackMetadata { + t.Helper() + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + return meta + } + + tests := []struct { + name string + write func(t *testing.T, dir string) string + read func(t *testing.T, path string) *metadata.TrackMetadata + }{ + { + name: "mp3", + read: viaScanner, + write: func(t *testing.T, dir string) string { + t.Helper() + + path := createTestMP3(t, dir, "totals.mp3", nil) + if err := writeMp3Tags(testLogger(), path, changes); err != nil { + t.Fatalf("writeMp3Tags: %v", err) + } + + return path + }, + }, + { + name: "flac", + read: viaScanner, + write: func(t *testing.T, dir string) string { + t.Helper() + + path := filepath.Join(dir, "totals.flac") + makeMinimalFLAC(t, path) + + if err := writeFlacTags(testLogger(), path, changes); err != nil { + t.Fatalf("writeFlacTags: %v", err) + } + + return path + }, + }, + { + name: "ogg", + read: viaScanner, + write: func(t *testing.T, dir string) string { + t.Helper() + + path := filepath.Join(dir, "totals.ogg") + createTestOGG(t, path) + + if err := writeOggTags(testLogger(), path, changes); err != nil { + t.Fatalf("writeOggTags: %v", err) + } + + return path + }, + }, + { + name: "wav", + read: readWavID3Tags, + write: func(t *testing.T, dir string) string { + t.Helper() + + path := createTestWAV(t, dir, "totals.wav", nil) + + if err := writeWavTags(testLogger(), path, changes); err != nil { + t.Fatalf("writeWavTags: %v", err) + } + + return path + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + meta := tc.read(t, tc.write(t, t.TempDir())) + + assertIntField(t, "TrackNumber", meta.TrackNumber, 2) + assertIntField(t, "TotalTracks", meta.TotalTracks, 10) + assertIntField(t, "DiscNumber", meta.DiscNumber, 1) + assertIntField(t, "TotalDiscs", meta.TotalDiscs, 2) + }) + } +} + +// A number and a total are separate diff entries, so writing one must +// not discard the other. For ID3v2 they share a single "n/N" frame, +// which is the only place this can go wrong -- and it goes wrong by +// silently zeroing a total the file already declared. +func TestWriteMp3Totals_PartialUpdateKeepsTheOtherHalf(t *testing.T) { + t.Parallel() + + t.Run("writing the number keeps the total", func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := createTestMP3(t, dir, "seeded.mp3", TagChanges{ + FieldTrackNumber: 2, + FieldTotalTracks: 10, + }) + + if err := writeMp3Tags(testLogger(), path, TagChanges{ + FieldTrackNumber: 4, + }); err != nil { + t.Fatalf("writeMp3Tags: %v", err) + } + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + assertIntField(t, "TrackNumber", meta.TrackNumber, 4) + assertIntField(t, "TotalTracks", meta.TotalTracks, 10) + }) + + t.Run("writing the total keeps the number", func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := createTestMP3(t, dir, "seeded.mp3", TagChanges{ + FieldTrackNumber: 7, + }) + + if err := writeMp3Tags(testLogger(), path, TagChanges{ + FieldTotalTracks: 12, + }); err != nil { + t.Fatalf("writeMp3Tags: %v", err) + } + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + assertIntField(t, "TrackNumber", meta.TrackNumber, 7) + assertIntField(t, "TotalTracks", meta.TotalTracks, 12) + }) + + // "/12" says nothing a reader can use, and dhowden/tag reads it as + // track 0 -- which the scan would store as a real track number. + t.Run("a total with no number writes nothing", func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := createTestMP3(t, dir, "bare.mp3", nil) + + if err := writeMp3Tags(testLogger(), path, TagChanges{ + FieldTotalTracks: 12, + }); err != nil { + t.Fatalf("writeMp3Tags: %v", err) + } + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + assertIntField(t, "TrackNumber", meta.TrackNumber, 0) + assertIntField(t, "TotalTracks", meta.TotalTracks, 0) + }) +} diff --git a/backend/tagwriter/wav_test.go b/backend/tagwriter/wav_test.go index 3b54280..e8fa853 100644 --- a/backend/tagwriter/wav_test.go +++ b/backend/tagwriter/wav_test.go @@ -522,19 +522,20 @@ func readWavID3Tags( } } - // Track number (TRCK). + // Track number and total (TRCK), disc number and total (TPOS). + // Both carry the "n/N" form, so they are read the way a reader + // reads them rather than with Atoi -- which sees "2/10" as 0. trckID := parsed.CommonID("Track number/Position in set") if frames := parsed.GetFrames(trckID); len(frames) > 0 { if tf, ok := frames[0].(id3v2.TextFrame); ok { - meta.TrackNumber = atoiSafe(tf.Text) + meta.TrackNumber, meta.TotalTracks = parseXofN(tf.Text) } } - // Disc number (TPOS). tposID := parsed.CommonID("Part of a set") if frames := parsed.GetFrames(tposID); len(frames) > 0 { if tf, ok := frames[0].(id3v2.TextFrame); ok { - meta.DiscNumber = atoiSafe(tf.Text) + meta.DiscNumber, meta.TotalDiscs = parseXofN(tf.Text) } }