diff --git a/internal/db/migrations/0087_library_rematch.down.sql b/internal/db/migrations/0087_library_rematch.down.sql new file mode 100644 index 00000000..66b5e525 --- /dev/null +++ b/internal/db/migrations/0087_library_rematch.down.sql @@ -0,0 +1,2 @@ +-- Nothing to undo: the refused albums are matched again either way. +SELECT 1; diff --git a/internal/db/migrations/0087_library_rematch.up.sql b/internal/db/migrations/0087_library_rematch.up.sql new file mode 100644 index 00000000..7347fff3 --- /dev/null +++ b/internal/db/migrations/0087_library_rematch.up.sql @@ -0,0 +1,9 @@ +-- The first matching (#5569) required every track's length to fit as well +-- as its title, and refused singles and near-complete albums the library +-- plainly holds. Matching now takes the titles alone under the album's own +-- name: the albums it refused are matched again. +DELETE FROM library_album_matches WHERE outcome = 'unmatched'; + +UPDATE acquisition_settings + SET library_listed_at = NULL, library_unlisted = 0 + WHERE library_listing_started_at IS NOT NULL; diff --git a/internal/wanted/library_test.go b/internal/wanted/library_test.go index eb865d1a..76410b7e 100644 --- a/internal/wanted/library_test.go +++ b/internal/wanted/library_test.go @@ -252,10 +252,22 @@ func TestTrackAgreement(t *testing.T) { {"the right title at the wrong length", []libraryTrack{{Number: 2, Title: "Song", LengthMs: 400000}}, 0}, {"a track counted once", []libraryTrack{{Title: "Song", LengthMs: 200000}, {Title: "Song", LengthMs: 200000}}, 1}, } { - if got := trackAgreement(c.tracks, edition); got != c.want { + if got, _ := trackAgreement(c.tracks, edition, false); got != c.want { t.Errorf("%s: %d agree, want %d", c.name, got, c.want) } } + // Under the album's own name the titles are enough; the reason a track + // did not agree names both sides. + edit := []libraryTrack{{Number: 2, Title: "Song (Radio Edit)", LengthMs: 150000}} + if n, miss := trackAgreement(edit, edition, false); n != 0 || miss != `"Song (Radio Edit)" runs 2:30 here; MusicBrainz's track 2 is "Song", 3:20` { + t.Errorf("by length: %d, %q", n, miss) + } + if n, _ := trackAgreement(edit, edition, true); n != 1 { + t.Errorf("by title alone: %d agree", n) + } + if _, miss := trackAgreement([]libraryTrack{{Title: "Other", LengthMs: 61000}}, edition, true); miss != `no track of its edition is like "Other" (1:01)` { + t.Errorf("unplaced: %q", miss) + } if agrees(3, 4) || !agrees(4, 5) || agrees(0, 0) { t.Error("agrees: 80% of the album's tracks, and an album has tracks") } diff --git a/internal/wanted/match.go b/internal/wanted/match.go index bcdf3ea8..04c7527e 100644 --- a/internal/wanted/match.go +++ b/internal/wanted/match.go @@ -101,10 +101,14 @@ func tracksToMatch(rows []dbq.ListAlbumTracksToMatchRow) []libraryTrack { } // trackAgreement counts the album's tracks an edition agrees with: the same -// recording, or a length that fits and a title that agrees. A track is -// paired by its disc and number first, then by title, so an album missing -// some tracks still pairs the ones it has. -func trackAgreement(tracks []libraryTrack, edition []editionTrack) int { +// recording, or a title that agrees at a length that fits. When the group is +// called just what the album is called, the titles alone are enough: the +// lengths are there to tell apart albums whose names only overlap, and a +// rip's single may be the edit MusicBrainz lists at another length. A track +// is paired by its disc and number first, then by title, so an album missing +// some tracks still pairs the ones it has. miss says how the first track +// that did not agree differs, for the reason a match was refused. +func trackAgreement(tracks []libraryTrack, edition []editionTrack, titlesEnough bool) (n int, miss string) { byPos := make(map[[2]int]int, len(edition)) for i, e := range edition { byPos[[2]int{max(e.Medium, 1), e.Position}] = i @@ -113,27 +117,47 @@ func trackAgreement(tracks []libraryTrack, edition []editionTrack) int { if t.Recording != "" && strings.EqualFold(t.Recording, e.Recording) { return true } - return release.LengthFits(t.LengthMs, e.LengthMs) && release.TitleAgrees(t.Title, e.Title) + return release.TitleAgrees(t.Title, e.Title) && (titlesEnough || release.LengthFits(t.LengthMs, e.LengthMs)) } used := make([]bool, len(edition)) - n := 0 for _, t := range tracks { + at, placed := -1, false if t.Number > 0 { - if i, ok := byPos[[2]int{max(t.Disc, 1), t.Number}]; ok && !used[i] && agree(t, edition[i]) { - used[i] = true - n++ - continue + if i, ok := byPos[[2]int{max(t.Disc, 1), t.Number}]; ok { + at = i + placed = !used[i] && agree(t, edition[i]) + if placed { + used[i] = true + } } } for i, e := range edition { - if !used[i] && agree(t, e) { - used[i] = true - n++ - break + if !placed && !used[i] && agree(t, e) { + used[i], placed = true, true } } + switch { + case placed: + n++ + case miss != "": + case at >= 0: + e := edition[at] + miss = fmt.Sprintf("%q runs %s here; MusicBrainz's track %d is %q, %s", + t.Title, minutes(t.LengthMs), e.Position, e.Title, minutes(e.LengthMs)) + default: + miss = fmt.Sprintf("no track of its edition is like %q (%s)", t.Title, minutes(t.LengthMs)) + } } - return n + return n, miss +} + +// minutes writes a length as m:ss, or "no length". +func minutes(ms int) string { + if ms <= 0 { + return "no length" + } + sec := (ms + 500) / 1000 + return fmt.Sprintf("%d:%02d", sec/60, sec%60) } // agrees: enough of the album's tracks agree for the edition to be it. @@ -173,14 +197,14 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr // discography first, then a search. tried := map[string]bool{} var best albumMatch - bestN, bestEdition, considered := -1, 0, 0 + bestN, bestEdition, considered, bestMiss := -1, 0, 0, "" check := func(cands []candidate) error { for _, c := range cands { if tried[c.Group] || len(tried) >= matchCandidates { continue } tried[c.Group] = true - n, size, err := s.agreementWith(ctx, c.Group, tracks) + n, size, miss, err := s.agreementWith(ctx, c.Group, tracks, sameTitle(c.Title, a.Title)) if err != nil { return err } @@ -189,7 +213,7 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr // one; then the edition nearest the album's length. st, bt := sameTitle(c.Title, a.Title), sameTitle(best.Title, a.Title) if n > bestN || (n == bestN && ((st && !bt) || (st == bt && abs(size-len(tracks)) < abs(bestEdition-len(tracks))))) { - best, bestN, bestEdition = albumMatch{candidate: c}, n, size + best, bestN, bestEdition, bestMiss = albumMatch{candidate: c}, n, size, miss } } return nil @@ -217,9 +241,15 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr if considered == 0 { return albumMatch{Detail: fmt.Sprintf("MusicBrainz has no album called %q by %s.", a.Title, a.ArtistName)}, nil } - return albumMatch{Detail: fmt.Sprintf( - "MusicBrainz has %d %s called %q by %s, but none has its tracks: the closest, %q, agrees on %d of its %d.", - considered, plural(considered, "album", "albums"), a.Title, a.ArtistName, best.Title, max(bestN, 0), len(tracks))}, nil + detail := fmt.Sprintf("MusicBrainz has %d %s called %q by %s, but none has its tracks: the closest, %q, agrees on %d of its %d", + considered, plural(considered, "album", "albums"), a.Title, a.ArtistName, best.Title, max(bestN, 0), len(tracks)) + switch { + case bestN < 0: + detail += " (MusicBrainz lists no edition of it)" + case bestMiss != "": + detail += ": " + bestMiss + } + return albumMatch{Detail: detail + "."}, nil } // groupCandidate reads a group the tags name. A group MusicBrainz does not @@ -296,15 +326,15 @@ func (s *Service) searchCandidates(ctx context.Context, a dbq.ListAlbumsToMatchR } // agreementWith is how many of the album's tracks the group's best edition -// agrees with, and that edition's length. A group MusicBrainz no longer has -// agrees with none. -func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack) (n, size int, err error) { +// agrees with, that edition's length, and how it differs. A group +// MusicBrainz no longer has, or lists no edition of, agrees with none (-1). +func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack, titlesEnough bool) (n, size int, miss string, err error) { eds, err := s.cat.Releases(ctx, group) switch { case errors.Is(err, catalogue.ErrNotFound): - return -1, 0, nil + return -1, 0, "", nil case err != nil: - return 0, 0, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + return 0, 0, "", fmt.Errorf("%w: %v", errMusicBrainzAway, err) } n = -1 for _, e := range eds.Editions { @@ -313,11 +343,11 @@ func (s *Service) agreementWith(ctx context.Context, group string, tracks []libr edition = append(edition, editionTrack{Medium: int(t.MediumPosition), Position: int(t.Position), Title: t.Title, LengthMs: int(t.LengthMs), Recording: t.RecordingMbid}) } - if got := trackAgreement(tracks, edition); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { - n, size = got, len(edition) + if got, m := trackAgreement(tracks, edition, titlesEnough); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { + n, size, miss = got, len(edition), m } } - return n, size, nil + return n, size, miss, nil } // creditedArtist is the artist a want is filed under: the library's own