Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
97f2eb85ee |
@@ -1,107 +0,0 @@
|
|||||||
name: Unclaim
|
|
||||||
|
|
||||||
# A `Closes #N` footer in a commit body closes the issue on merge — and
|
|
||||||
# leaves `Status/In Progress` on it, because Gitea's auto-close touches
|
|
||||||
# state and nothing else. So #100 was closed and simultaneously marked
|
|
||||||
# as being actively worked on, and `scripts/issue.sh close` (which does
|
|
||||||
# drop the label) is exactly the thing the footer exists to avoid
|
|
||||||
# calling.
|
|
||||||
#
|
|
||||||
# **This hooks the close, not the merge.** Stripping the label in the
|
|
||||||
# PR would work and would be a per-PR habit; habits are what the footer
|
|
||||||
# removed. `issues: [closed]` covers every path an issue can close by —
|
|
||||||
# the footer on merge, `issue.sh close`, someone clicking Close in the
|
|
||||||
# web UI — and asks nothing of anyone at any of them.
|
|
||||||
#
|
|
||||||
# **Reopening deliberately does not restore it.** Reopening says the
|
|
||||||
# work was not finished, not that somebody is at a keyboard doing it
|
|
||||||
# now; the claim gets re-made by whoever picks it up.
|
|
||||||
#
|
|
||||||
# **This is not instant, and should not be described as it.** The
|
|
||||||
# runner has capacity 1 and is shared with an index build that can hold
|
|
||||||
# it for three hours, so a label tweak can queue behind one. Stale for
|
|
||||||
# an afternoon beats stale forever, which is what it was.
|
|
||||||
#
|
|
||||||
# The audit that answers "is this still firing" stays in CLAUDE.md and
|
|
||||||
# is one command:
|
|
||||||
#
|
|
||||||
# ./scripts/issue.sh list --state closed --label "Status/In Progress"
|
|
||||||
#
|
|
||||||
# A workflow that silently stops working is the failure mode this whole
|
|
||||||
# area has already produced once.
|
|
||||||
|
|
||||||
on:
|
|
||||||
issues:
|
|
||||||
types: [closed]
|
|
||||||
|
|
||||||
jobs:
|
|
||||||
unclaim:
|
|
||||||
runs-on: ubuntu-latest
|
|
||||||
container:
|
|
||||||
image: ubuntu:24.04
|
|
||||||
|
|
||||||
steps:
|
|
||||||
- name: Drop the claim label
|
|
||||||
# **Inside a container the act runner selects `sh`, not bash**, so
|
|
||||||
# `set -o pipefail` fails the job on its second line with "Illegal
|
|
||||||
# option" and the step never reaches the API. `homebrew-formula.yml`
|
|
||||||
# carries the same `set -euo pipefail` without trouble because it
|
|
||||||
# runs with **no container**, on the host image where bash is the
|
|
||||||
# default — so "another workflow does it" is not evidence here.
|
|
||||||
shell: bash
|
|
||||||
env:
|
|
||||||
# The automatic Actions token, as release.yml uses for the
|
|
||||||
# floor tag. It needs no more than write access to this repo.
|
|
||||||
TOKEN: ${{ secrets.GITEA_TOKEN }}
|
|
||||||
API: ${{ github.server_url }}/api/v1/repos/${{ github.repository }}
|
|
||||||
ISSUE: ${{ github.event.issue.number }}
|
|
||||||
run: |
|
|
||||||
set -euo pipefail
|
|
||||||
|
|
||||||
# `ca-certificates` is named because `--no-install-recommends`
|
|
||||||
# skips it, and `ubuntu:24.04` ships no CA bundle of its own —
|
|
||||||
# so curl comes up unable to verify TLS against our own Gitea
|
|
||||||
# and fails with "error setting certificate file" (exit 77).
|
|
||||||
# Every other containerised workflow here spells it out for the
|
|
||||||
# same reason; this one did not, and cost a release cycle.
|
|
||||||
apt-get update -qq
|
|
||||||
apt-get install -y -qq --no-install-recommends \
|
|
||||||
ca-certificates curl jq >/dev/null
|
|
||||||
|
|
||||||
label_id=$(
|
|
||||||
curl -sSf -H "Authorization: token $TOKEN" "$API/labels?limit=100" |
|
|
||||||
jq -r '.[] | select(.name == "Status/In Progress") | .id'
|
|
||||||
)
|
|
||||||
|
|
||||||
# The label not existing is a repo somebody reorganised, not a
|
|
||||||
# failure of this run — say so and stop, rather than failing a
|
|
||||||
# job on every close from then on.
|
|
||||||
if [ -z "$label_id" ]; then
|
|
||||||
echo "unclaim: no 'Status/In Progress' label in this repo; nothing to do"
|
|
||||||
exit 0
|
|
||||||
fi
|
|
||||||
|
|
||||||
# DELETE is idempotent here: an issue that never carried the
|
|
||||||
# label answers the same as one that did, which is what makes
|
|
||||||
# this safe to run on *every* close rather than only the ones
|
|
||||||
# that were claimed.
|
|
||||||
# The body is captured, not discarded, so a refusal is
|
|
||||||
# diagnosable from this log alone. Whether the automatic
|
|
||||||
# token carries issue-write scope is still unproven, and
|
|
||||||
# "DELETE returned 403" without Gitea's own sentence costs
|
|
||||||
# another merge to find out which of the two it is.
|
|
||||||
body=$(mktemp)
|
|
||||||
code=$(
|
|
||||||
curl -sS -o "$body" -w '%{http_code}' -X DELETE \
|
|
||||||
-H "Authorization: token $TOKEN" \
|
|
||||||
"$API/issues/$ISSUE/labels/$label_id"
|
|
||||||
)
|
|
||||||
|
|
||||||
case "$code" in
|
|
||||||
204) echo "unclaim: #$ISSUE is closed and unclaimed" ;;
|
|
||||||
*)
|
|
||||||
echo "unclaim: DELETE returned $code for #$ISSUE" >&2
|
|
||||||
cat "$body" >&2
|
|
||||||
exit 1
|
|
||||||
;;
|
|
||||||
esac
|
|
||||||
@@ -3483,26 +3483,3 @@ public tap.
|
|||||||
|
|
||||||
A guard added today does not protect a tag that points at yesterday. When
|
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*.
|
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.
|
|
||||||
|
|||||||
@@ -85,18 +85,8 @@ release decision. The rule that the issue number stays out of the
|
|||||||
still drops the footer. `./scripts/issue.sh list --state open` after a
|
still drops the footer. `./scripts/issue.sh list --state open` after a
|
||||||
merge, looking for what you just shipped; `./scripts/issue.sh close
|
merge, looking for what you just shipped; `./scripts/issue.sh close
|
||||||
<n>` for whatever did not take, with a comment naming the commit.
|
<n>` for whatever did not take, with a comment naming the commit.
|
||||||
|
`close` also drops `Status/In Progress`, because a claim outlives the
|
||||||
**Unclaiming is automatic, and it is hooked to the close rather than
|
work if nothing takes the label off.
|
||||||
to the merge.** Gitea's auto-close changes state and nothing else, so a
|
|
||||||
footer left `Status/In Progress` on a closed issue — #100 was closed
|
|
||||||
and marked as being actively worked on at the same time.
|
|
||||||
`.gitea/workflows/unclaim.yml` runs on `issues: [closed]`, which covers
|
|
||||||
the footer, `issue.sh close` and a click in the web UI alike; stripping
|
|
||||||
the label in the PR instead would have been a per-PR habit, and habits
|
|
||||||
are what the footer removed. It is not instant — the runner has
|
|
||||||
capacity 1 — and reopening deliberately does not restore the label.
|
|
||||||
`./scripts/issue.sh list --state closed --label "Status/In Progress"`
|
|
||||||
is how you find out it has stopped firing.
|
|
||||||
|
|
||||||
## Planning
|
## Planning
|
||||||
|
|
||||||
@@ -1547,55 +1537,11 @@ shape as the encoding probe beside it.
|
|||||||
|
|
||||||
What neither side can give is *which* tracks are missing, only how many
|
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
|
— so an incomplete album still browses, and that is now the exception
|
||||||
rather than every album load. One smaller consequence: existing databases
|
rather than every album load. Two smaller consequences: existing databases
|
||||||
read "unknown" until a rescan repopulates the column, which degrades to
|
read "unknown" until a rescan repopulates the column (which degrades to
|
||||||
exactly the old behaviour, so nothing breaks.
|
exactly the old behaviour, so nothing breaks), and our own `tagwriter`
|
||||||
|
writes track and disc *numbers* but not totals, so autotagging a folder
|
||||||
**And our own writers declare the total, because for a long time they
|
currently degrades the field this rests on.
|
||||||
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
|
**The absence is what gets marked, not the presence.** The tracklist
|
||||||
put a green tick against every owned track and a legend underneath
|
put a green tick against every owned track and a legend underneath
|
||||||
@@ -2230,11 +2176,10 @@ Pre-commit runs vet, lint, codegen check, and frontend typecheck in parallel. Pr
|
|||||||
|
|
||||||
## CI
|
## CI
|
||||||
|
|
||||||
Eight workflows in `.gitea/workflows/`. Five of them package and
|
Seven workflows in `.gitea/workflows/`. Five of them package and
|
||||||
publish (`arch-package`, `homebrew-formula`, `index-artifact`,
|
publish (`arch-package`, `homebrew-formula`, `index-artifact`,
|
||||||
`android-apk`, `desktop-assets`); `release.yml` decides *whether* four of
|
`android-apk`, `desktop-assets`); `release.yml` decides *whether* four of
|
||||||
those run at all; `unclaim.yml` is housekeeping on the tracker and
|
those run at all; only `ci.yml` gates, and it is the one to look at when
|
||||||
touches no code; only `ci.yml` gates, and it is the one to look at when
|
|
||||||
deciding whether a push was healthy.
|
deciding whether a push was healthy.
|
||||||
|
|
||||||
**`release.yml` is the entry point for all of it.** On every push to
|
**`release.yml` is the entry point for all of it.** On every push to
|
||||||
|
|||||||
@@ -8,7 +8,6 @@ import (
|
|||||||
"log/slog"
|
"log/slog"
|
||||||
|
|
||||||
"yellowjacket/backend/database/sql/sqlcgen"
|
"yellowjacket/backend/database/sql/sqlcgen"
|
||||||
"yellowjacket/backend/tagtotals"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
// TagChanges mirrors tagwriter.TagChanges — redefined here so the
|
// TagChanges mirrors tagwriter.TagChanges — redefined here so the
|
||||||
@@ -29,8 +28,6 @@ const (
|
|||||||
FieldYear = "year"
|
FieldYear = "year"
|
||||||
FieldTrackNumber = "track_number"
|
FieldTrackNumber = "track_number"
|
||||||
FieldDiscNumber = "disc_number"
|
FieldDiscNumber = "disc_number"
|
||||||
FieldTotalTracks = "total_tracks"
|
|
||||||
FieldTotalDiscs = "total_discs"
|
|
||||||
FieldCoverArt = "cover_art"
|
FieldCoverArt = "cover_art"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -421,32 +418,5 @@ func buildChanges(
|
|||||||
changes[FieldDiscNumber] = track.DiscNumber
|
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
|
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
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -1,90 +0,0 @@
|
|||||||
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)
|
|
||||||
}
|
|
||||||
})
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,38 +0,0 @@
|
|||||||
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])
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -12,7 +12,6 @@ import (
|
|||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"yellowjacket/backend/tagtotals"
|
|
||||||
"yellowjacket/backend/tagwriter"
|
"yellowjacket/backend/tagwriter"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -276,25 +275,6 @@ func (i *Importer) tagFile(p plannedFile, dl Download) error {
|
|||||||
changes[tagwriter.FieldDiscNumber] = p.Track.DiscNumber
|
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 {
|
if err := i.tags.WriteUntrackedFileTags(p.Source, changes); err != nil {
|
||||||
return fmt.Errorf("write tags: %w", err)
|
return fmt.Errorf("write tags: %w", err)
|
||||||
}
|
}
|
||||||
@@ -302,18 +282,6 @@ func (i *Importer) tagFile(p plannedFile, dl Download) error {
|
|||||||
return nil
|
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.
|
// destinationFor computes a file's library path from the template.
|
||||||
func (i *Importer) destinationFor(
|
func (i *Importer) destinationFor(
|
||||||
p plannedFile,
|
p plannedFile,
|
||||||
|
|||||||
@@ -446,77 +446,3 @@ func keysOf(m map[string]tagwriter.TagChanges) []string {
|
|||||||
|
|
||||||
return out
|
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])
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -1,56 +0,0 @@
|
|||||||
// 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
|
|
||||||
}
|
|
||||||
@@ -1,92 +0,0 @@
|
|||||||
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)
|
|
||||||
}
|
|
||||||
})
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -183,15 +183,6 @@ func syncDatabase(
|
|||||||
discNum = toNullInt64(v)
|
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
|
composer := old.Composer
|
||||||
if v, ok := params.changes[FieldComposer].(string); ok {
|
if v, ok := params.changes[FieldComposer].(string); ok {
|
||||||
composer = v
|
composer = v
|
||||||
@@ -216,7 +207,7 @@ func syncDatabase(
|
|||||||
AlbumID: albumID,
|
AlbumID: albumID,
|
||||||
TrackNumber: trackNum,
|
TrackNumber: trackNum,
|
||||||
DiscNumber: discNum,
|
DiscNumber: discNum,
|
||||||
TotalTracks: totalTracks,
|
TotalTracks: old.TotalTracks,
|
||||||
Year: year,
|
Year: year,
|
||||||
Composer: composer,
|
Composer: composer,
|
||||||
Comment: old.Comment,
|
Comment: old.Comment,
|
||||||
|
|||||||
@@ -101,11 +101,6 @@ func applyFlacTextChanges(cmt *flacvorbis.MetaDataBlockVorbisComment, changes Ta
|
|||||||
{FieldYear, flacvorbis.FIELD_DATE, true},
|
{FieldYear, flacvorbis.FIELD_DATE, true},
|
||||||
{FieldTrackNumber, flacvorbis.FIELD_TRACKNUMBER, true},
|
{FieldTrackNumber, flacvorbis.FIELD_TRACKNUMBER, true},
|
||||||
{FieldDiscNumber, "DISCNUMBER", 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},
|
{FieldComposer, "COMPOSER", false},
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+11
-63
@@ -6,7 +6,6 @@ import (
|
|||||||
"log/slog"
|
"log/slog"
|
||||||
"os"
|
"os"
|
||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
|
||||||
|
|
||||||
id3v2 "github.com/bogem/id3v2/v2"
|
id3v2 "github.com/bogem/id3v2/v2"
|
||||||
|
|
||||||
@@ -67,10 +66,17 @@ func applyTextChanges(tag *id3v2.Tag, changes TagChanges) {
|
|||||||
tag.SetYear(strconv.Itoa(v))
|
tag.SetYear(strconv.Itoa(v))
|
||||||
}
|
}
|
||||||
|
|
||||||
applyPositionFrame(tag, "Track number/Position in set", changes,
|
if v, ok := asInt(changes[FieldTrackNumber]); ok {
|
||||||
FieldTrackNumber, FieldTotalTracks)
|
trckID := tag.CommonID("Track number/Position in set")
|
||||||
applyPositionFrame(tag, "Part of a set", changes,
|
tag.DeleteFrames(trckID)
|
||||||
FieldDiscNumber, FieldTotalDiscs)
|
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))
|
||||||
|
}
|
||||||
|
|
||||||
if v, ok := changes[FieldComposer].(string); ok {
|
if v, ok := changes[FieldComposer].(string); ok {
|
||||||
tag.DeleteFrames("TCOM")
|
tag.DeleteFrames("TCOM")
|
||||||
@@ -84,64 +90,6 @@ 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.
|
// applyCoverArtChanges handles the FieldCoverArt entry in the diff map.
|
||||||
//
|
//
|
||||||
// - []byte with len > 0: embed the given image as front cover.
|
// - []byte with len > 0: embed the given image as front cover.
|
||||||
|
|||||||
@@ -166,8 +166,6 @@ var oggFieldMappings = []struct { //nolint:gochecknoglobals // field mapping tab
|
|||||||
{FieldYear, "DATE", true},
|
{FieldYear, "DATE", true},
|
||||||
{FieldTrackNumber, "TRACKNUMBER", true},
|
{FieldTrackNumber, "TRACKNUMBER", true},
|
||||||
{FieldDiscNumber, "DISCNUMBER", true},
|
{FieldDiscNumber, "DISCNUMBER", true},
|
||||||
{FieldTotalTracks, "TRACKTOTAL", true},
|
|
||||||
{FieldTotalDiscs, "DISCTOTAL", true},
|
|
||||||
{FieldComposer, "COMPOSER", false},
|
{FieldComposer, "COMPOSER", false},
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -333,31 +333,3 @@ func TestWriteTrackTags_DBSync(t *testing.T) {
|
|||||||
t.Error("expected FTS5 result for 'New Title'")
|
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)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -26,15 +26,6 @@ const (
|
|||||||
FieldDiscNumber = "disc_number"
|
FieldDiscNumber = "disc_number"
|
||||||
FieldComposer = "composer"
|
FieldComposer = "composer"
|
||||||
FieldCoverArt = "cover_art" // []byte for set, nil for clear
|
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.
|
// AudioFormat represents a supported audio file format.
|
||||||
|
|||||||
@@ -1,199 +0,0 @@
|
|||||||
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)
|
|
||||||
})
|
|
||||||
}
|
|
||||||
@@ -522,20 +522,19 @@ func readWavID3Tags(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Track number and total (TRCK), disc number and total (TPOS).
|
// Track number (TRCK).
|
||||||
// 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")
|
trckID := parsed.CommonID("Track number/Position in set")
|
||||||
if frames := parsed.GetFrames(trckID); len(frames) > 0 {
|
if frames := parsed.GetFrames(trckID); len(frames) > 0 {
|
||||||
if tf, ok := frames[0].(id3v2.TextFrame); ok {
|
if tf, ok := frames[0].(id3v2.TextFrame); ok {
|
||||||
meta.TrackNumber, meta.TotalTracks = parseXofN(tf.Text)
|
meta.TrackNumber = atoiSafe(tf.Text)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Disc number (TPOS).
|
||||||
tposID := parsed.CommonID("Part of a set")
|
tposID := parsed.CommonID("Part of a set")
|
||||||
if frames := parsed.GetFrames(tposID); len(frames) > 0 {
|
if frames := parsed.GetFrames(tposID); len(frames) > 0 {
|
||||||
if tf, ok := frames[0].(id3v2.TextFrame); ok {
|
if tf, ok := frames[0].(id3v2.TextFrame); ok {
|
||||||
meta.DiscNumber, meta.TotalDiscs = parseXofN(tf.Text)
|
meta.DiscNumber = atoiSafe(tf.Text)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user