diff --git a/backend/config/config.go b/backend/config/config.go index f0ff93b..2f6430e 100644 --- a/backend/config/config.go +++ b/backend/config/config.go @@ -411,9 +411,10 @@ func (c *Config) SetDownloadPreferences(prefs download.AutoDownloadPrefs) error formats = append(formats, string(f)) } - c.Downloads.MinFileSizeMB = prefs.MinSizeMB + c.Downloads.MinKbps = prefs.MinKbps + c.Downloads.MaxKbps = prefs.MaxKbps + c.Downloads.PreferredKbps = prefs.PreferredKbps c.Downloads.MaxFileSizeMB = prefs.MaxSizeMB - c.Downloads.PreferredFileSizeMB = prefs.PreferredSizeMB c.Downloads.AllowedFormats = formats if err := c.Save(); err != nil { diff --git a/backend/download/config.go b/backend/download/config.go index bd3e7b2..b176247 100644 --- a/backend/download/config.go +++ b/backend/download/config.go @@ -34,13 +34,29 @@ type UserConfig struct { // in one burst that every provider sees as a flood. WantedBatch int `toml:"WantedBatch"` - // MinFileSizeMB, MaxFileSizeMB and PreferredFileSizeMB bound and - // nudge what auto-pick (interactive or via the request list) may - // grab without asking. Zero on any of them is permissive: see - // AutoDownloadPrefs. - MinFileSizeMB int `toml:"MinFileSizeMB"` - MaxFileSizeMB int `toml:"MaxFileSizeMB"` - PreferredFileSizeMB int `toml:"PreferredFileSizeMB"` + // MinKbps, MaxKbps and PreferredKbps bound and nudge what auto-pick + // (interactive or via the request list) may grab without asking. + // Zero on any of them is permissive: see AutoDownloadPrefs. + // + // They replaced MinFileSizeMB / MaxFileSizeMB / + // PreferredFileSizeMB, which were megabytes and so said nothing + // without knowing how long the release was. The old keys are + // deliberately *not* read back: a number that meant "300 MB" cannot + // be reinterpreted as a bitrate without knowing the album it was + // aimed at, so migrating it would be inventing an intent the user + // never expressed. An existing config falls back to no window, + // which is the permissive default and matches a fresh install — + // and MaxFileSizeMB is the one that does carry over, because a + // ceiling on total bytes still means exactly what it did. + MinKbps int `toml:"MinKbps"` + MaxKbps int `toml:"MaxKbps"` + PreferredKbps int `toml:"PreferredKbps"` + + // MaxFileSizeMB is a hard ceiling on a candidate's total size, kept + // in megabytes on purpose — it is a question about disk space, not + // about quality, and it has to apply to a candidate whose bitrate + // cannot be worked out at all. + MaxFileSizeMB int `toml:"MaxFileSizeMB"` // AllowedFormats restricts auto-pick to these formats. Empty means // no restriction. Values are Format strings ("flac", "mp3", ...). @@ -56,10 +72,11 @@ func (c *UserConfig) AutoDownloadPrefs() AutoDownloadPrefs { } return AutoDownloadPrefs{ - MinSizeMB: c.MinFileSizeMB, - MaxSizeMB: c.MaxFileSizeMB, - PreferredSizeMB: c.PreferredFileSizeMB, - AllowedFormats: formats, + MinKbps: c.MinKbps, + MaxKbps: c.MaxKbps, + PreferredKbps: c.PreferredKbps, + MaxSizeMB: c.MaxFileSizeMB, + AllowedFormats: formats, } } diff --git a/backend/download/manager.go b/backend/download/manager.go index f9eb618..2b284b5 100644 --- a/backend/download/manager.go +++ b/backend/download/manager.go @@ -236,6 +236,12 @@ func (m *Manager) AutoPickable(dl Download, ranked []Candidate) bool { return AutoPickable(dl, ranked, m.preferences()) } +// AutoPickVeto wraps the package function the same way, and is what the +// request list quotes back to the user. +func (m *Manager) AutoPickVeto(dl Download, ranked []Candidate) string { + return AutoPickVeto(dl, ranked, m.preferences()) +} + // Reload rebuilds every provider from stored config. Called at startup // and after any provider settings change. // @@ -612,16 +618,8 @@ func (m *Manager) Attempt( return false, "", err } - if !m.AutoPickable(dl, ranked) { - best := ranked[0] - - return false, fmt.Sprintf( - "best of %d found is not a confident enough match "+ - "(match %.0f%%, quality %.0f%%)", - len(ranked), - best.Match.Overall*100, //nolint:mnd // percent - best.Quality.Overall*100, - ), nil + if veto := m.AutoPickVeto(dl, ranked); veto != "" { + return false, veto, nil } if err := m.store.CreateDownload(ctx, dl); err != nil { diff --git a/backend/download/manager_test.go b/backend/download/manager_test.go index 72d504e..c120699 100644 --- a/backend/download/manager_test.go +++ b/backend/download/manager_test.go @@ -218,8 +218,17 @@ func TestManagerEndToEndAutoPick(t *testing.T) { }, "staging was never released, or the library was never rescanned") } -// An ambiguous result set must park for the user rather than guess. -func TestManagerWaitsWhenAmbiguous(t *testing.T) { +// Two equally good copies are not an ambiguity — they are a spare. +// +// This asserted the opposite for as long as auto-pick required 0.08 of +// daylight over the runner-up, and that rule was wrong in exactly the +// case it fired hardest: a popular album turns up several *correct* +// copies, all matching the tracklist, differing only in format and +// seeders. There is no question there about what to fetch, only about +// which copy, and the ranking already answers that — closest to the +// preferred bitrate first. A candidate does not have to be better than +// the field, only good enough on its own terms. +func TestManagerAutoPicksAmongEquallyGoodCopies(t *testing.T) { t.Parallel() f := newManagerFixture(t) @@ -237,11 +246,41 @@ func TestManagerWaitsWhenAmbiguous(t *testing.T) { t.Fatalf("Start: %v", err) } - if f.manager.AutoPickable(dl, ranked) { - t.Fatal("two equivalent candidates must not auto-pick") + if veto := f.manager.AutoPickVeto(dl, ranked); veto != "" { + t.Fatalf("two equally good copies must auto-pick, got veto: %s", veto) + } + + waitForDownloadState(t, f.store, dl.ID, StateComplete) + + // Exactly one of them was fetched, not both. + if grabs := a.GrabCalls + b.GrabCalls; grabs != 1 { + t.Errorf("grabs = %d, want exactly 1", grabs) + } +} + +// The user can still pick explicitly when auto-pick is not what +// happened — a candidate the ranking did not choose is still grabbable. +func TestManagerPickIsExplicit(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + + a := fakeWithAlbum(1, "source-a", ".flac") + b := fakeWithAlbum(2, "source-b", ".flac") + + f.manager.installProvider(Config{ID: 1, Priority: 50}, a) + f.manager.installProvider(Config{ID: 2, Priority: 50}, b) + + // No tracklist: never auto-picks, so the result set parks for the + // user and Pick is the only way anything is fetched. + dl := fourTrackDownload() + dl.Expected = nil + + ranked, err := f.manager.Start(context.Background(), dl) + if err != nil { + t.Fatalf("Start: %v", err) } - // Nothing was grabbed while waiting for the user. if a.GrabCalls != 0 || b.GrabCalls != 0 { t.Errorf( "grabs happened without a pick: a=%d b=%d", @@ -258,7 +297,6 @@ func TestManagerWaitsWhenAmbiguous(t *testing.T) { t.Errorf("stored request id = %s, want %s", stored.ID, dl.ID) } - // The user picks the second one explicitly. if err := f.manager.Pick( context.Background(), dl.ID, ranked[1].ID, ); err != nil { diff --git a/backend/download/rank.go b/backend/download/rank.go index e094372..ecaa53c 100644 --- a/backend/download/rank.go +++ b/backend/download/rank.go @@ -1,6 +1,7 @@ package download import ( + "fmt" "math" "sort" "strings" @@ -34,38 +35,102 @@ const ( weightArtistFit = 0.12 ) -// Quality sub-weights. They sum to 1.0 along with weightSizeFit below. +// Quality sub-weights. Each set sums to 1.0. +// +// There are two of them because a stated preference changes what the +// other numbers are *for*. `formatRank` and `bitrateScore` are the +// app guessing at how good a copy is — FLAC over MP3, 320 over 128 — +// and that guess exists precisely because the user has not said. Once +// they have, the guess should not outvote them: with the old single set +// a preference of 320 kbps moved a candidate's score by at most 0.05 +// against the 0.42 riding on format, so asking for 320 and being handed +// a FLAC every time was the *designed* behaviour. That is the same +// fault the megabyte window had — a preference the user can express and +// the ranking can ignore. const ( - weightFormat = 0.42 - weightBitrate = 0.23 - weightHealth = 0.20 - weightPriority = 0.10 - weightSizeFit = 0.05 + weightFormat = 0.42 + weightBitrate = 0.23 + weightHealth = 0.20 + weightPriority = 0.10 + weightBitrateFit = 0.05 ) +// Quality sub-weights when the user has named a preferred bitrate. +// The weight comes off format and bitrate — the two proxies the +// preference replaces — and health and priority are untouched, since +// neither is a stand-in for anything the user just said. +const ( + statedWeightFormat = 0.20 + statedWeightBitrate = 0.10 + statedWeightHealth = 0.20 + statedWeightPriority = 0.10 + statedWeightBitrateFit = 0.40 +) + +// qualityWeights picks the set, in the order scoreQuality applies them. +func qualityWeights(p AutoDownloadPrefs) ( + format, bitrate, health, priority, fit float64, +) { + if p.PreferredKbps > 0 { + return statedWeightFormat, + statedWeightBitrate, + statedWeightHealth, + statedWeightPriority, + statedWeightBitrateFit + } + + return weightFormat, + weightBitrate, + weightHealth, + weightPriority, + weightBitrateFit +} + // unanchoredCap bounds the match score of a free-text request. Without // an MBID there is no tracklist to be right about, so a confident- // looking score would be a lie — and auto-pick keys off this. const unanchoredCap = 0.65 // AutoDownloadPrefs gates and scores what AutoPickable may choose -// without asking. Zero values are permissive: no size window and no -// format restriction. +// without asking. Zero values are permissive: no bitrate window, no +// size ceiling and no format restriction. +// +// **The window is a rate, not a size.** It used to be three numbers in +// megabytes, which cannot mean anything on their own: 300 MB is a +// generous FLAC single and a suspiciously small boxset, and the user +// setting the number has no idea which release the pipeline will +// eventually apply it to. A bitrate is the same statement normalised +// by how long the music is, so one number holds across a 9-minute EP +// and a 3-hour opera — and it is the unit the thing being described is +// actually measured in. The runtime is known for every request +// auto-pick can act on (`Download.Expected` carries per-track lengths, +// and an anchored request is the only kind that reaches here), so this +// costs no extra lookup. type AutoDownloadPrefs struct { - // MinSizeMB and MaxSizeMB bound what auto-pick will grab. Zero - // means no bound on that side. A candidate outside the window is - // filtered out of auto-pick entirely, not merely scored down — a - // tiny "sampler" torrent or a boxset ten times the expected size is - // usually the wrong thing entirely, not a worse copy of the right - // thing. - MinSizeMB int `json:"minSizeMb"` - MaxSizeMB int `json:"maxSizeMb"` + // MinKbps and MaxKbps bound the average bitrate auto-pick will + // grab. Zero means no bound on that side. A candidate outside the + // window is filtered out of auto-pick entirely, not merely scored + // down — a 96 kbps rip of the right album is not a worse copy the + // user might accept, it is one they said not to take unattended. + // + // For reference: 320 is the top of MP3, ~500–1000 is FLAC depending + // on the material, and anything under ~128 is a transcode. + MinKbps int `json:"minKbps"` + MaxKbps int `json:"maxKbps"` - // PreferredSizeMB nudges the score toward a target size within the - // min/max window (a lossless rip and a heavily-padded lossless rip - // can both pass the window). Zero disables the nudge; sizeFit then - // returns a neutral value that does not affect ranking. - PreferredSizeMB int `json:"preferredSizeMb"` + // PreferredKbps nudges the score toward a target rate within the + // window, and breaks the tie when several candidates are equally + // good matches. Zero disables the nudge; bitrateFit then returns a + // neutral value that does not affect ranking. + PreferredKbps int `json:"preferredKbps"` + + // MaxSizeMB is a hard ceiling on the whole candidate, and it is + // deliberately still a size. It answers a different question from + // the window above — not "is this the quality I want" but "is this + // going to fill the disk" — and it has to hold even for a candidate + // whose bitrate cannot be worked out, which is exactly the shape a + // mislabelled boxset arrives in. Zero means no ceiling. + MaxSizeMB int `json:"maxSizeMb"` // AllowedFormats restricts auto-pick to candidates whose audio // files are all in one of these formats. Empty means no @@ -74,19 +139,33 @@ type AutoDownloadPrefs struct { } // eligible reports whether a candidate may be auto-picked under these -// preferences: within the size window (when set) and, when a format -// list is given, every audio file in an allowed format. -func (p AutoDownloadPrefs) eligible(c Candidate) bool { +// preferences: inside the bitrate window and the size ceiling (when +// set) and, when a format list is given, every audio file in an +// allowed format. +// +// `runtimeMillis` is how long the requested release is, and 0 means +// nobody knows. An unknown runtime **passes** the bitrate window +// rather than failing it: the window is a statement about quality, and +// refusing everything the moment a tracklist is missing a length would +// turn a gap in MusicBrainz into a silent embargo. The size ceiling +// still applies, which is why it exists separately. +func (p AutoDownloadPrefs) eligible(c Candidate, runtimeMillis int64) bool { const bytesPerMB = 1 << 20 - if p.MinSizeMB > 0 && c.TotalSize < int64(p.MinSizeMB)*bytesPerMB { - return false - } - if p.MaxSizeMB > 0 && c.TotalSize > int64(p.MaxSizeMB)*bytesPerMB { return false } + if kbps := candidateKbps(c, runtimeMillis); kbps > 0 { + if p.MinKbps > 0 && kbps < float64(p.MinKbps) { + return false + } + + if p.MaxKbps > 0 && kbps > float64(p.MaxKbps) { + return false + } + } + if len(p.AllowedFormats) == 0 { return true } @@ -107,11 +186,14 @@ func (p AutoDownloadPrefs) eligible(c Candidate) bool { // filter returns only the candidates these preferences allow to be // auto-picked, in the same (already ranked) order. -func (p AutoDownloadPrefs) filter(ranked []Candidate) []Candidate { +func (p AutoDownloadPrefs) filter( + ranked []Candidate, + runtimeMillis int64, +) []Candidate { out := make([]Candidate, 0, len(ranked)) for _, c := range ranked { - if p.eligible(c) { + if p.eligible(c, runtimeMillis) { out = append(out, c) } } @@ -119,32 +201,116 @@ func (p AutoDownloadPrefs) filter(ranked []Candidate) []Candidate { return out } -// sizeFit scores how close totalSize is to PreferredSizeMB, 0..1, -// falling off linearly as the size doubles or halves away from it. -// Returns a neutral 0.5 when no preference is set, so the absence of a -// preference does not bias ranking. -func (p AutoDownloadPrefs) sizeFit(totalSize int64) float64 { +// bitrateFit scores how close a candidate's average bitrate is to +// PreferredKbps, falling off linearly as it doubles or halves away +// from it. +// +// The range is **0.5 to 1.0, not 0 to 1**, and the floor is the point. +// This carries 0.40 of the quality score once a preference is set, so a +// span down to zero would let a preference of 320 kbps push a perfectly +// good FLAC under `minQuality` and out of auto-pick altogether — +// turning "I like 320" into "never take anything else", silently. A +// preference may promote the copy that matches it; it may not +// disqualify the others. That is what `MinKbps`/`MaxKbps` are for, and +// they say so out loud. +// +// Returns the neutral floor when no preference is set or the rate +// cannot be worked out, so neither an absent preference nor an absent +// runtime biases ranking. +func (p AutoDownloadPrefs) bitrateFit( + c Candidate, + runtimeMillis int64, +) float64 { const ( - bytesPerMB = 1 << 20 - neutral = 0.5 + neutral = 0.5 + span = 0.5 ) - if p.PreferredSizeMB <= 0 || totalSize <= 0 { + if p.PreferredKbps <= 0 { return neutral } - preferred := float64(p.PreferredSizeMB) * bytesPerMB - ratio := float64(totalSize) / preferred + kbps := candidateKbps(c, runtimeMillis) + if kbps <= 0 { + return neutral + } + ratio := kbps / float64(p.PreferredKbps) if ratio < 1 { ratio = 1 / ratio } // ratio is now >= 1: 1.0 is an exact match, 2.0 is double or half - // the preferred size. Falls to 0 at 2x away and beyond. - fit := 1 - (ratio - 1) + // the preferred rate, where the closeness term reaches 0. + return neutral + span*clamp01(1-(ratio-1)) +} - return clamp01(fit) +// candidateKbps is a candidate's average audio bitrate, or 0 when it +// cannot be worked out. +// +// Two sources, in this order, and the order matters: +// +// - **Derived from bytes over runtime**, which is the honest one. It +// covers lossless (where a stated bitrate rarely exists), it cannot +// be lied to by a filename, and it is what the user's window means. +// Only the *audio* files count: cover scans and a log file are not +// part of the bitrate, and a folder with 30 MB of artwork would +// otherwise read as a better rip than the same music without it. +// - **The mean stated bitrate**, when the runtime is unknown. Weaker +// — a provider that parses it from an MP3 header states it and one +// that guesses from the filename also "states" it — but a number +// from the file itself beats no number at all. +func candidateKbps(c Candidate, runtimeMillis int64) float64 { + const bitsPerByte = 8 + + audio := c.AudioFiles() + if len(audio) == 0 { + return 0 + } + + if runtimeMillis > 0 { + var bytes int64 + for _, f := range audio { + bytes += f.Size + } + + if bytes > 0 { + // bytes×8 bits over seconds, expressed in kbps: the two + // factors of 1000 (millis→seconds, bits→kilobits) cancel. + return float64(bytes) * bitsPerByte / + float64(runtimeMillis) + } + } + + var ( + sum int + count int + ) + + for _, f := range audio { + if f.Bitrate > 0 { + sum += f.Bitrate + count++ + } + } + + if count == 0 { + return 0 + } + + return float64(sum) / float64(count) +} + +// runtimeMillis is how long the requested release is, summed over its +// expected tracklist. Zero when the tracklist is absent or carries no +// lengths, which is what every caller here treats as "unknown". +func (d Download) runtimeMillis() int64 { + var total int64 + for _, t := range d.Expected { + total += t.LengthMillis + } + + return total } // Score fills a candidate's Match, Quality and Score fields. @@ -160,7 +326,9 @@ func Score(dl Download, c Candidate, priority int, prefs AutoDownloadPrefs) Cand c.Files = mergeMatched(c.Files, matched) c.Match = scoreMatch(dl, c, audio, titleFit) - c.Quality = scoreQuality(c, audio, priority, prefs) + c.Quality = scoreQuality( + c, audio, priority, prefs, dl.runtimeMillis(), + ) c.Score = weightMatch*c.Match.Overall + weightQuality*c.Quality.Overall @@ -279,11 +447,12 @@ func scoreQuality( audio []CandidateFile, priority int, prefs AutoDownloadPrefs, + runtimeMillis int64, ) QualityScore { q := QualityScore{ - Health: clamp01(c.Health), - Priority: clamp01(float64(priority) / 100.0), - SizeFit: prefs.sizeFit(c.TotalSize), + Health: clamp01(c.Health), + Priority: clamp01(float64(priority) / 100.0), + BitrateFit: prefs.bitrateFit(c, runtimeMillis), } if len(audio) == 0 { @@ -310,11 +479,13 @@ func scoreQuality( q.FormatRank = worst q.Bitrate = bitrateScore(audio) - q.Overall = weightFormat*q.FormatRank + - weightBitrate*q.Bitrate + - weightHealth*q.Health + - weightPriority*q.Priority + - weightSizeFit*q.SizeFit + wFormat, wBitrate, wHealth, wPriority, wFit := qualityWeights(prefs) + + q.Overall = wFormat*q.FormatRank + + wBitrate*q.Bitrate + + wHealth*q.Health + + wPriority*q.Priority + + wFit*q.BitrateFit if q.Mixed { q.Overall *= 0.9 @@ -444,6 +615,19 @@ func Rank( return out[i].Match.Overall > out[j].Match.Overall } + // Closest to the preferred bitrate wins the tie. + // + // This is what decides which copy is taken now that auto-pick + // no longer requires the winner to be clear of the field: when + // several candidates are equally good matches of equal overall + // quality, the one the user said they wanted the shape of is + // the answer, ahead of provider priority. With no preference + // set every BitrateFit is the same neutral value and this + // falls through, exactly as before. + if out[i].Quality.BitrateFit != out[j].Quality.BitrateFit { + return out[i].Quality.BitrateFit > out[j].Quality.BitrateFit + } + if out[i].Quality.Priority != out[j].Quality.Priority { return out[i].Quality.Priority > out[j].Quality.Priority } @@ -454,19 +638,58 @@ func Rank( return out } -// AutoPickable reports whether a ranked list has a clear enough winner -// to grab without asking. It demands an anchored request, a high match, -// decent quality, and daylight between first and second place — if two -// candidates are close, the choice is the user's. -func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool { - const ( - minMatch = 0.85 - minQuality = 0.5 - minLead = 0.08 - ) +// Auto-pick gates. Named rather than inlined because AutoPickVeto +// reports which of them refused, and a number in a sentence the user +// reads should be the same number the decision used. +const ( + minMatch = 0.85 + minQuality = 0.5 +) - if !dl.Anchored() || len(ranked) == 0 { - return false +// AutoPickable reports whether a ranked list has a candidate worth +// grabbing without asking: an anchored request with a tracklist behind +// it, and a candidate that clears the match and quality bars inside the +// user's guardrails. +// +// **It does not require the winner to be better than the runner-up.** +// It used to demand 0.08 of daylight on the combined score, which meant +// the check fired hardest in the case it was never written for: a +// popular album turns up five *correct* copies, all matching the +// tracklist at 95%+ and differing only in format and seeders, their +// scores land within a point of each other, and auto-pick refused +// forever on the grounds that the choice was the user's. It was not. +// There was no question about *what* to fetch, only about which copy — +// and abundance is the one condition under which that question matters +// least. A candidate does not need to be the best one, only one that +// meets the criteria; where several do, `Rank` puts the one closest to +// the preferred bitrate first. +func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool { + return AutoPickVeto(dl, ranked, prefs) == "" +} + +// AutoPickVeto returns the reason auto-pick declined, or "" when it +// would go ahead. +// +// It exists because "it rejected all of them" was indistinguishable +// from "it found nothing good". The request list's message was built +// from `ranked[0]` — the best candidate *before* the size and format +// guardrails, and before the lead check — so a request refused because +// the user's maximum size excluded every copy, or because three equally +// good copies were found, reported "best of 12 found is not a confident +// enough match (match 96%, quality 88%)". Numbers that clear both +// thresholds, beside a refusal, is a message that teaches the user the +// matcher is broken. Each gate names itself now. +func AutoPickVeto( + dl Download, + ranked []Candidate, + prefs AutoDownloadPrefs, +) string { + if len(ranked) == 0 { + return "nothing found" + } + + if !dl.Anchored() { + return "the request is free text, so there is no release to be right about" } // An anchor with no tracklist behind it is an anchor in name only: @@ -474,29 +697,42 @@ func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool // is exactly the evidence a wrong-album candidate also has. This // matters most for the request list, where nobody is watching. if len(dl.Expected) == 0 { - return false + return "no tracklist for this release is known yet, so a candidate cannot be checked against it" } - // The guardrails apply before the match/quality/lead checks: a - // candidate outside the allowed size or format is not a worse - // choice, it is not a choice auto-pick may make at all, so it must - // not count as "the winner" nor as "second place" for the lead - // check below. - eligible := prefs.filter(ranked) + // The guardrails apply before the match and quality checks: a + // candidate outside the allowed bitrate, size or format is not a + // worse choice, it is not a choice auto-pick may make at all, so it + // must not count as "the winner" either. + eligible := prefs.filter(ranked, dl.runtimeMillis()) if len(eligible) == 0 { - return false + return fmt.Sprintf( + "all %d found are outside the auto-download bitrate, size or format limits", + len(ranked), + ) } best := eligible[0] - if best.Match.Overall < minMatch || best.Quality.Overall < minQuality { - return false + + if best.Match.Overall < minMatch { + return fmt.Sprintf( + "best of %d found matches this release only %.0f%% (needs %.0f%%)", + len(ranked), + best.Match.Overall*100, //nolint:mnd // percent + minMatch*100, //nolint:mnd // percent + ) } - if len(eligible) > 1 && best.Score-eligible[1].Score < minLead { - return false + if best.Quality.Overall < minQuality { + return fmt.Sprintf( + "best of %d found is the right release but scores %.0f%% on quality (needs %.0f%%)", + len(ranked), + best.Quality.Overall*100, //nolint:mnd // percent + minQuality*100, //nolint:mnd // percent + ) } - return true + return "" } // mergeMatched copies MatchedTo assignments from the audio-only slice diff --git a/backend/download/rank_test.go b/backend/download/rank_test.go index 9c7915b..9c0a2cf 100644 --- a/backend/download/rank_test.go +++ b/backend/download/rank_test.go @@ -1,6 +1,34 @@ package download -import "testing" +import ( + "strings" + "testing" +) + +// trackMillis is five minutes; okComputer's four of them make a +// twenty-minute release, which is what turns a candidate's byte count +// into a bitrate the assertions below can name. +const trackMillis = 5 * 60 * 1000 + +// okComputerRuntime is that release's runtime, for the helpers that +// need it directly. +const okComputerRuntime = 4 * trackMillis + +// kbpsCandidate builds an annotated candidate whose audio adds up to +// the given average bitrate over okComputer's runtime. +func kbpsCandidate(id, ext string, kbps int) Candidate { + // bits = kbps × 1000 × (runtimeMillis / 1000), so the thousands + // cancel and the byte count is kbps × runtimeMillis / 8. + const bitsPerByte = 8 + + total := int64(kbps) * okComputerRuntime / bitsPerByte + + c := candidateFor(id, allTitles(), ext, total/int64(len(allTitles()))) + c.Files = AnnotateFiles(c.Files) + c.TotalSize = total + + return c +} // okComputer is the reference request used across ranking tests. func okComputer() Download { @@ -8,11 +36,15 @@ func okComputer() Download { ReleaseMBID: "mbid-ok-computer", Artist: "Radiohead", Album: "OK Computer", + // Four five-minute tracks: twenty minutes, so a candidate's + // bitrate is a number these tests can state exactly. Without + // lengths there is no runtime and the bitrate window has + // nothing to divide by. Expected: []ExpectedTrack{ - {Position: 1, Title: "Airbag"}, - {Position: 2, Title: "Paranoid Android"}, - {Position: 3, Title: "Subterranean Homesick Alien"}, - {Position: 4, Title: "Exit Music (For a Film)"}, + {Position: 1, Title: "Airbag", LengthMillis: trackMillis}, + {Position: 2, Title: "Paranoid Android", LengthMillis: trackMillis}, + {Position: 3, Title: "Subterranean Homesick Alien", LengthMillis: trackMillis}, + {Position: 4, Title: "Exit Music (For a Film)", LengthMillis: trackMillis}, }, } } @@ -187,7 +219,7 @@ func TestUnanchoredMatchIsCapped(t *testing.T) { } } -func TestAutoPickableRequiresAnchorAndLead(t *testing.T) { +func TestAutoPickableRequiresAnchorAndTracklist(t *testing.T) { t.Parallel() dl := okComputer() @@ -211,14 +243,18 @@ func TestAutoPickableRequiresAnchorAndLead(t *testing.T) { } }) - t.Run("two close candidates are not", func(t *testing.T) { + // Two identical copies are a spare, not an ambiguity. This + // asserted the opposite while auto-pick required daylight over the + // runner-up — a rule that made abundance the thing that stopped a + // request being satisfied, which is backwards. + t.Run("two equally good candidates still are", func(t *testing.T) { t.Parallel() twin := best twin.ID = "twin" - if AutoPickable(dl, []Candidate{best, twin}, AutoDownloadPrefs{}) { - t.Error("identical candidates must not auto-pick") + if !AutoPickable(dl, []Candidate{best, twin}, AutoDownloadPrefs{}) { + t.Error("identical good candidates must auto-pick") } }) @@ -300,18 +336,11 @@ func TestProviderPriorityBreaksTies(t *testing.T) { } } -const mb = 1 << 20 - func TestAutoDownloadPrefsEligible(t *testing.T) { t.Parallel() - flacCandidate := candidateFor("c", allTitles(), ".flac", 30_000_000) - flacCandidate.Files = AnnotateFiles(flacCandidate.Files) - flacCandidate.TotalSize = 300 * mb - - mp3Candidate := candidateFor("c", allTitles(), ".mp3", 3_000_000) - mp3Candidate.Files = AnnotateFiles(mp3Candidate.Files) - mp3Candidate.TotalSize = 30 * mb + flacCandidate := kbpsCandidate("c", ".flac", 900) + mp3Candidate := kbpsCandidate("c", ".mp3", 128) tests := []struct { name string @@ -321,18 +350,25 @@ func TestAutoDownloadPrefsEligible(t *testing.T) { }{ {"zero value is permissive", AutoDownloadPrefs{}, flacCandidate, true}, { - "within min/max window", - AutoDownloadPrefs{MinSizeMB: 100, MaxSizeMB: 500}, + "within the bitrate window", + AutoDownloadPrefs{MinKbps: 320, MaxKbps: 1200}, flacCandidate, true, }, { - "below minimum", - AutoDownloadPrefs{MinSizeMB: 400}, + "below the minimum bitrate", + AutoDownloadPrefs{MinKbps: 500}, + mp3Candidate, false, + }, + { + "above the maximum bitrate", + AutoDownloadPrefs{MaxKbps: 500}, flacCandidate, false, }, { - "above maximum", - AutoDownloadPrefs{MaxSizeMB: 200}, + // The ceiling is bytes, not a rate, and it is the guard + // that still works when the bitrate cannot be worked out. + "above the hard size ceiling", + AutoDownloadPrefs{MaxSizeMB: 50}, flacCandidate, false, }, { @@ -351,57 +387,131 @@ func TestAutoDownloadPrefsEligible(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - if got := tt.prefs.eligible(tt.c); got != tt.want { + got := tt.prefs.eligible(tt.c, okComputerRuntime) + if got != tt.want { t.Errorf("eligible() = %v, want %v", got, tt.want) } }) } } +// A release nobody knows the length of cannot be judged on bitrate, and +// the window must not become a silent embargo because MusicBrainz is +// missing a track length. The size ceiling still applies — that is why +// it is a separate field. +func TestBitrateWindowPassesAnUnknownRuntime(t *testing.T) { + t.Parallel() + + c := kbpsCandidate("c", ".mp3", 128) + prefs := AutoDownloadPrefs{MinKbps: 900} + + if !prefs.eligible(c, 0) { + t.Error("an unknown runtime must pass the bitrate window") + } + + if prefs.eligible(c, okComputerRuntime) { + t.Error("a known runtime must still be judged") + } + + ceiling := AutoDownloadPrefs{MaxSizeMB: 1} + if ceiling.eligible(c, 0) { + t.Error("the size ceiling must apply even with no runtime") + } +} + +// Artwork is not part of the bitrate. A folder carrying 30 MB of +// scans would otherwise read as a better rip than the same music +// without them, which is backwards. +func TestBitrateIgnoresNonAudioFiles(t *testing.T) { + t.Parallel() + + c := kbpsCandidate("c", ".mp3", 320) + bare := candidateKbps(c, okComputerRuntime) + + c.Files = append(c.Files, CandidateFile{ + Path: "Radiohead - OK Computer/cover.jpg", + Size: 30 << 20, + }) + c.Files = AnnotateFiles(c.Files) + + if got := candidateKbps(c, okComputerRuntime); got != bare { + t.Errorf("bitrate with artwork = %f, want %f", got, bare) + } +} + +// Where no runtime is known, a stated per-file bitrate is better than +// no answer at all. +func TestBitrateFallsBackToTheStatedRate(t *testing.T) { + t.Parallel() + + c := candidateFor("c", allTitles(), ".mp3", 3_000_000) + for i := range c.Files { + c.Files[i].Bitrate = 192 + } + + c.Files = AnnotateFiles(c.Files) + + if got := candidateKbps(c, 0); got != 192 { + t.Errorf("stated bitrate = %f, want 192", got) + } +} + func TestAutoDownloadPrefsFilter(t *testing.T) { t.Parallel() - small := candidateFor("small", allTitles(), ".flac", 10_000_000) - small.TotalSize = 50 * mb + lossy := kbpsCandidate("lossy", ".mp3", 128) + lossless := kbpsCandidate("lossless", ".flac", 900) - big := candidateFor("big", allTitles(), ".flac", 30_000_000) - big.TotalSize = 500 * mb + prefs := AutoDownloadPrefs{MinKbps: 500} - prefs := AutoDownloadPrefs{MinSizeMB: 100, MaxSizeMB: 600} + filtered := prefs.filter( + []Candidate{lossy, lossless}, okComputerRuntime, + ) - filtered := prefs.filter([]Candidate{small, big}) - - if len(filtered) != 1 || filtered[0].ID != "big" { + if len(filtered) != 1 || filtered[0].ID != "lossless" { t.Errorf("filter() = %v, want only the in-window candidate", filtered) } } -func TestAutoDownloadPrefsSizeFit(t *testing.T) { +func TestAutoDownloadPrefsBitrateFit(t *testing.T) { t.Parallel() const neutral = 0.5 tests := []struct { - name string - prefs AutoDownloadPrefs - totalSize int64 - want float64 + name string + prefs AutoDownloadPrefs + c Candidate + want float64 }{ - {"no preference is neutral", AutoDownloadPrefs{}, 300 * mb, neutral}, + { + "no preference is neutral", + AutoDownloadPrefs{}, + kbpsCandidate("c", ".flac", 900), neutral, + }, { "exact match scores 1", - AutoDownloadPrefs{PreferredSizeMB: 300}, - 300 * mb, 1.0, + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".mp3", 320), 1.0, }, { - "double the preferred size scores 0", - AutoDownloadPrefs{PreferredSizeMB: 300}, - 600 * mb, 0.0, + // The floor is neutral, not zero: this term carries 0.40 + // of the quality score once a preference is set, and a + // span to zero would let "I like 320" quietly disqualify + // every FLAC from auto-pick. + "double the preferred rate falls to the neutral floor", + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".flac", 640), neutral, }, { - "half the preferred size scores 0", - AutoDownloadPrefs{PreferredSizeMB: 300}, - 150 * mb, 0.0, + "half the preferred rate falls to the neutral floor", + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".mp3", 160), neutral, + }, + { + "an unknowable rate is neutral", + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".mp3", 320), neutral, }, } @@ -409,30 +519,223 @@ func TestAutoDownloadPrefsSizeFit(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - if got := tt.prefs.sizeFit(tt.totalSize); got != tt.want { - t.Errorf("sizeFit(%d) = %f, want %f", tt.totalSize, got, tt.want) + // The last case deliberately withholds the runtime. + runtime := int64(okComputerRuntime) + if tt.name == "an unknowable rate is neutral" { + runtime = 0 + } + + if got := tt.prefs.bitrateFit(tt.c, runtime); got != tt.want { + t.Errorf("bitrateFit() = %f, want %f", got, tt.want) } }) } } // An otherwise-perfect candidate must not auto-pick when it falls -// outside the configured size guard: the guardrail applies before the -// match/quality/lead checks, not as one more input averaged into them. -func TestAutoPickableRejectsCandidateOutsideSizeGuard(t *testing.T) { +// outside the configured guardrails: they apply before the match and +// quality checks, not as one more input averaged into them. +func TestAutoPickableRejectsCandidateOutsideTheGuardrails(t *testing.T) { t.Parallel() dl := okComputer() - best := Score(dl, candidateFor("a", allTitles(), ".flac", 30_000_000), 50, AutoDownloadPrefs{}) - best.TotalSize = 500 * mb + best := Score(dl, kbpsCandidate("a", ".flac", 900), 50, AutoDownloadPrefs{}) if !AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{}) { t.Fatal("expected this candidate to be auto-pickable with no guardrails") } - tight := AutoDownloadPrefs{MinSizeMB: 10, MaxSizeMB: 100} + if AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{MaxKbps: 320}) { + t.Error("candidate above the bitrate window must not auto-pick") + } - if AutoPickable(dl, []Candidate{best}, tight) { - t.Error("candidate outside the size guard must not auto-pick") + if AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{MaxSizeMB: 1}) { + t.Error("candidate above the size ceiling must not auto-pick") + } +} + +// The refusal has to name the gate that refused. +// +// Before AutoPickVeto, every one of these came back as the same +// sentence built from `ranked[0]` — the best candidate before the size +// and format guardrails — so a request refused because the user's size +// window excluded every copy reported a match and a quality that both +// cleared their thresholds. A refusal quoting numbers that pass is +// what made the matcher look broken from outside. +func TestAutoPickVetoNamesTheGate(t *testing.T) { + t.Parallel() + + dl := okComputer() + best := Score( + dl, + candidateFor("a", allTitles(), ".flac", 30_000_000), + 50, + AutoDownloadPrefs{}, + ) + + // candidateFor sizes the files and leaves TotalSize at 0, which is + // what the guardrails read. + sized := func(c Candidate, total int64) Candidate { + c.TotalSize = total + + return c + } + + tests := []struct { + name string + dl Download + ranked []Candidate + prefs AutoDownloadPrefs + wantSub string + }{ + { + name: "nothing found", + dl: dl, + ranked: nil, + wantSub: "nothing found", + }, + { + name: "free text", + dl: Download{Artist: "Radiohead", Album: "OK Computer"}, + ranked: []Candidate{best}, + wantSub: "free text", + }, + { + name: "no tracklist behind the anchor", + dl: Download{ + ReleaseMBID: "mbid-ok-computer", + Artist: "Radiohead", + Album: "OK Computer", + }, + ranked: []Candidate{best}, + wantSub: "no tracklist", + }, + { + // The candidate is 120 MB and the window tops out at 1 MB: + // the old message reported its match and quality instead. + name: "outside the size window", + dl: dl, + ranked: []Candidate{sized(best, 120<<20)}, + prefs: AutoDownloadPrefs{MaxSizeMB: 1}, + wantSub: "bitrate, size or format limits", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := AutoPickVeto(tt.dl, tt.ranked, tt.prefs) + if !strings.Contains(got, tt.wantSub) { + t.Errorf("veto = %q, want it to mention %q", got, tt.wantSub) + } + }) + } +} + +// A clear winner has no veto at all — the sentence is empty, which is +// what AutoPickable reads. +func TestAutoPickVetoIsEmptyForAClearWinner(t *testing.T) { + t.Parallel() + + dl := okComputer() + best := Score( + dl, + candidateFor("a", allTitles(), ".flac", 30_000_000), + 50, + AutoDownloadPrefs{}, + ) + weak := Score( + dl, + candidateFor("b", allTitles()[:2], ".mp3", 1_000_000), + 50, + AutoDownloadPrefs{}, + ) + + if got := AutoPickVeto(dl, []Candidate{best, weak}, AutoDownloadPrefs{}); got != "" { + t.Errorf("veto = %q, want none", got) + } +} + +// With several candidates that all clear the bar, the preferred +// bitrate decides which one is taken. +// +// This is what replaced the daylight requirement. Auto-pick no longer +// refuses when the field is close; it takes the copy nearest the shape +// the user asked for, which is the question they actually answered in +// Settings. +func TestPreferredBitrateBreaksTheTie(t *testing.T) { + t.Parallel() + + dl := okComputer() + prefs := AutoDownloadPrefs{PreferredKbps: 320} + + // Same album, same completeness, same health, same provider — the + // only difference between them is the rate. + lossless := kbpsCandidate("lossless", ".flac", 900) + perfect := kbpsCandidate("perfect", ".mp3", 320) + + ranked := Rank( + dl, []Candidate{lossless, perfect}, nil, prefs, + ) + + if ranked[0].ID != "perfect" { + t.Errorf( + "winner = %q (fit %f) over %q (fit %f), want the 320 kbps copy", + ranked[0].ID, ranked[0].Quality.BitrateFit, + ranked[1].ID, ranked[1].Quality.BitrateFit, + ) + } + + if AutoPickVeto(dl, ranked, prefs) != "" { + t.Error("a close field must still auto-pick") + } +} + +// With no preference set, nothing changes: BitrateFit is the same +// neutral value for every candidate and the older tie-breaks decide. +func TestNoPreferredBitrateLeavesRankingAlone(t *testing.T) { + t.Parallel() + + dl := okComputer() + + lossless := kbpsCandidate("lossless", ".flac", 900) + lossy := kbpsCandidate("lossy", ".mp3", 320) + + ranked := Rank( + dl, []Candidate{lossy, lossless}, nil, AutoDownloadPrefs{}, + ) + + if ranked[0].ID != "lossless" { + t.Errorf( + "winner = %q, want the lossless copy on format alone", + ranked[0].ID, + ) + } +} + +// A preferred bitrate promotes the copy that matches it and must never +// disqualify the ones that do not. It carries 0.40 of the quality +// score, so a fit spanning down to zero would put a perfectly good FLAC +// under minQuality and out of auto-pick — turning a preference into a +// prohibition without saying so. MinKbps and MaxKbps are how a user +// says that on purpose. +func TestAPreferredBitrateNeverDisqualifies(t *testing.T) { + t.Parallel() + + dl := okComputer() + far := AutoDownloadPrefs{PreferredKbps: 128} + + lossless := Score(dl, kbpsCandidate("flac", ".flac", 900), 50, far) + + if lossless.Quality.Overall < minQuality { + t.Errorf( + "quality = %f under a far-off preference, want >= %f", + lossless.Quality.Overall, minQuality, + ) + } + + if veto := AutoPickVeto(dl, []Candidate{lossless}, far); veto != "" { + t.Errorf("a far-off preference vetoed the candidate: %s", veto) } } diff --git a/backend/download/service_test.go b/backend/download/service_test.go index c497047..fccdd8e 100644 --- a/backend/download/service_test.go +++ b/backend/download/service_test.go @@ -36,13 +36,24 @@ func newServiceFixture(t *testing.T) serviceFixture { // assertion read it; the second is that same goroutine still writing // into `t.TempDir()` after the test returned. One cause, two shapes. // - // Putting the candidate outside the auto-pick size window stops the + // Putting the candidate outside the auto-pick guardrails stops the // grab from ever starting, which is better than waiting for it: there // is no goroutine to be slow, so the tests state what they mean // ("the request exists, in this state") without a timing assumption // underneath. A test that does want the download has `managerFixture` // and sets its own preferences. - mf.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 1}) + // + // The guard is a *format* the fake never produces, and it used to be + // `MaxSizeMB: 1`, which never fired: the size gates read + // `Candidate.TotalSize`, which real providers fill and the fake + // leaves at zero, and zero is under every ceiling. So the grab went + // ahead anyway and the second failure shape above — the TempDir + // cleanup race — kept happening, reproducibly, roughly one run in + // fifteen. A guard has to be keyed on something the fixture + // actually sets. + mf.manager.SetPreferences(AutoDownloadPrefs{ + AllowedFormats: []Format{FormatWMA}, + }) return serviceFixture{managerFixture: mf, svc: svc} } @@ -182,6 +193,11 @@ func TestManualDownloadSatisfiesRequestOnSuccess(t *testing.T) { f := newServiceFixture(t) ctx := context.Background() + // This is the one test here that is *about* the download, so it + // undoes the fixture's guard rather than relying on it — which is + // what it was doing implicitly while the guard did not work. + f.manager.SetPreferences(AutoDownloadPrefs{}) + provider := fakeWithAlbum(1, "source", ".flac") f.manager.installProvider(Config{ID: 1, Priority: 50}, provider) diff --git a/backend/download/types.go b/backend/download/types.go index c4e7886..d76c2ec 100644 --- a/backend/download/types.go +++ b/backend/download/types.go @@ -302,7 +302,12 @@ type QualityScore struct { Bitrate float64 `json:"bitrate"` Health float64 `json:"health"` // seeders, free slots Priority float64 `json:"priority"` // user's per-provider preference - SizeFit float64 `json:"sizeFit"` // closeness to the preferred download size + // BitrateFit is closeness to the preferred *rate*, which is what + // the auto-download window is expressed in. It replaced a + // `SizeFit` measured in megabytes: a size means nothing without + // knowing how long the music is, so the same number described a + // generous single and a suspiciously small boxset. + BitrateFit float64 `json:"bitrateFit"` // Mixed marks a candidate whose files are not all the same format, // which usually means a hand-assembled folder rather than a rip. diff --git a/backend/explore/coverartproxy.go b/backend/explore/coverartproxy.go index b7dee0c..25cee80 100644 --- a/backend/explore/coverartproxy.go +++ b/backend/explore/coverartproxy.go @@ -25,8 +25,26 @@ const ( // where cached cover art thumbnails are stored. thumbnailDir = CoverArtCacheDirName - // thumbnailTimeout is the HTTP timeout for fetching a thumbnail. - thumbnailTimeout = 10 * time.Second + // thumbnailTimeout is the HTTP timeout for fetching a thumbnail, + // and it has to cover a redirect the Cover Art Archive does not + // serve itself. + // + // `coverartarchive.org` answers `front-250` with a 307 to an + // Internet Archive storage node (`dn######.us.archive.org`), and + // those nodes are routinely slow: measured against the twelve + // albums on Explore's own shelves, a successful fetch took 14–16 s + // and a failing one 13–17 s. At 10 s *every* cover on the page + // timed out — 24 cards, 5 of which had art, all of those from the + // disk cache — which reads as "Explore has no album art" rather + // than as a slow upstream, because a timeout writes nothing and + // says nothing. + // + // 30 s is chosen to clear that measured range with room, not to be + // generous: the fetch is off the critical path (each one is its own + // goroutine behind an 8/s limiter, and the frontend renders a + // placeholder until it lands), so the cost of waiting is nothing + // and the cost of giving up early is a blank page. + thumbnailTimeout = 30 * time.Second // thumbnailMaxSize is the maximum image size to cache (2 MB). thumbnailMaxSize = 2 * 1024 * 1024 @@ -97,6 +115,20 @@ func (p *CoverArtProxy) GetThumbnail( return "" } + // A 404 is an answer, and it is already on disk. + // + // `writeCache(mbid, nil)` has recorded "the archive has no art for + // this" as an empty file since this was written, and nothing has + // ever read it back: `readCache` returns "" for an empty file, + // which is indistinguishable from a miss, so every art-less release + // group was re-fetched from the network on every render that asked + // about it. On Explore's shelves a third of the cards are art-less, + // so that was a third of the page spending a live CAA request to be + // told again what the last one said. + if p.knownMissing(releaseGroupMBID) { + return "" + } + // Source 3: fetch from Cover Art Archive (slow, cached to disk). url := CoverArtGroupURL(releaseGroupMBID) data, cacheable, err := p.fetch(url) @@ -177,8 +209,9 @@ func (p *CoverArtProxy) GetCandidateThumbnail( } } - // Network fetch on release group. - if releaseGroupMBID != "" { + // Network fetch on release group — unless a previous one was told + // there is none. See `knownMissing`. + if releaseGroupMBID != "" && !p.knownMissing(releaseGroupMBID) { url := CoverArtGroupURL(releaseGroupMBID) data, cacheable, err := p.fetch(url) @@ -194,7 +227,7 @@ func (p *CoverArtProxy) GetCandidateThumbnail( } // Network fetch on release (fallback). - if releaseMBID != "" { + if releaseMBID != "" && !p.knownMissing(releaseMBID) { url := CoverArtURL(releaseMBID) data, cacheable, err := p.fetch(url) @@ -285,6 +318,17 @@ func (p *CoverArtProxy) cachePath(mbid string) string { return filepath.Join(p.cacheDir, mbid+".jpg") } +// knownMissing reports whether a previous fetch was told the archive +// has no art for this MBID — the empty file `writeCache(mbid, nil)` +// leaves behind. It is deliberately separate from `readCache`, which +// answers "what are the bytes" and cannot express the difference +// between no answer and an answer of none. +func (p *CoverArtProxy) knownMissing(mbid string) bool { + info, err := os.Stat(p.cachePath(mbid)) + + return err == nil && info.Size() == 0 +} + func (p *CoverArtProxy) readCache(mbid string) string { path := p.cachePath(mbid) diff --git a/backend/library/library.go b/backend/library/library.go index 4087933..e161c0f 100644 --- a/backend/library/library.go +++ b/backend/library/library.go @@ -289,8 +289,13 @@ func (l *Library) scanInternal( l.mu.Unlock() }() + // The configured mode, not a hardcoded "auto". `ScanConcurrency` + // has been a validated config field with three values and one + // caller passing a constant, so choosing `ssd` or `hdd` by hand + // did nothing at all. + diskProfile := system.ProfileForPath(libraryPath) workerCount := resolveScanWorkerCount( - ScanConcurrencyAuto, + l.conf.ScanConcurrency, libraryPath, ) @@ -300,6 +305,10 @@ func (l *Library) scanInternal( "libraryName", libraryName, "libraryPath", libraryPath, "workers", workerCount, + "mode", l.conf.ScanConcurrency, + "device", diskProfile.Device, + "rotational", diskProfile.Rotational, + "queueDepth", diskProfile.QueueDepth, ) // Helper to build a ScanProgress with library identification. @@ -818,7 +827,7 @@ func (l *Library) scanInternal( g := new(errgroup.Group) g.SetLimit(workerCount) - for work := range workChan { + for work := range readaheadWork(scanCtx, workChan, diskProfile) { g.Go(func() error { if err := l.waitIfPaused(scanCtx); err != nil { return err @@ -1285,9 +1294,101 @@ func surveyAudioFiles( return count, maxModTime } -// hddWorkerCount is the maximum number of concurrent extraction -// workers when the library resides on a spinning disk. -const hddWorkerCount = 2 +// How many extraction workers a spinning disk gets, and why it is two +// numbers rather than one. +// +// Extraction is not CPU work — every parser here reads headers and +// returns — so on a spinning disk the whole cost is seek latency, and +// the only question worth asking is how many reads should be in flight +// at once. That has two different right answers and the drive says +// which: +// +// - A drive with command queueing (NCQ: /sys/block//device/ +// queue_depth reports 31 or 32 on any SATA disk with it enabled) +// reorders outstanding reads into the order its head passes over +// them. Handing it several at once is most of why a parallel scan +// beats a serial one at all, and four is where the returns flatten: +// the drive needs a few requests to have anything to reorder, and +// past that it is queueing requests it was already going to +// service in that order. +// - A drive without it — queue_depth 1, which is what a USB bridge +// or a pre-2004 disk reports — services one command at a time in +// the order given. Every extra worker there is one more seek +// competing for one head, and the scan gets *slower* the harder it +// is pushed. Two is kept rather than one because the readahead +// hints (see readaheadWork) do the overlapping that concurrency +// was standing in for, and one worker cannot hide a stall. +// +// This used to be a flat 2 for anything rotational, which is a +// pre-NCQ assumption: it left a modern spinning disk with a quarter of +// the queue depth it can use. +const ( + hddWorkerCountQueued = 4 + hddWorkerCountSerial = 2 +) + +// Readahead tuning. +const ( + // readaheadDepth is how many files ahead of the workers the + // prefetcher runs. It is the channel's buffer, so it is also the + // number of `WILLNEED` hints outstanding at once — comfortably more + // than a queueing drive's 32-command window is worth filling with + // one library, and small enough that a cancelled scan is not + // holding a long tail of queued reads. + readaheadDepth = 16 + + // readaheadBytes is how much of each file to pull in. Everything + // the scanner reads lives at the head: ID3v2 and FLAC's + // STREAMINFO/VORBIS_COMMENT/PICTURE blocks, and the first MPEG + // frame with its Xing header. 512 KB covers a tag carrying + // embedded cover art, which is the large case — and reading a + // little too much sequentially costs a spinning disk almost + // nothing next to the seek that got there. + readaheadBytes = 512 << 10 +) + +// readaheadWork forwards scan work while asking the kernel to fetch +// each file's header before a worker reaches it. +// +// The buffered channel *is* the lookahead: this goroutine runs ahead +// of the workers until the buffer fills, hinting every file as it goes, +// so by the time a worker takes an item the read it needs has been in +// flight for `readaheadDepth` files' worth of parsing. That is the +// only thing that helps a spinning disk here, because the per-file work +// is already header-only — every parser in `backend/metadata` reads a +// few hundred bytes and returns, so the scan is not waiting on CPU or +// on bytes, it is waiting on the head to arrive. +// +// It runs on rotational disks only. An SSD has no seek to hide and +// already has one worker per core; issuing hints there is pure syscall +// overhead against an OS readahead that is already ahead of us. +func readaheadWork( + ctx context.Context, + in <-chan scanWork, + profile system.DiskProfile, +) <-chan scanWork { + if !profile.Rotational { + return in + } + + out := make(chan scanWork, readaheadDepth) + + go func() { + defer close(out) + + for work := range in { + hintReadahead(work.absolutePath, readaheadBytes) + + select { + case out <- work: + case <-ctx.Done(): + return + } + } + }() + + return out +} // resolveScanWorkerCount returns the number of concurrent // extraction workers based on the configured concurrency mode @@ -1296,20 +1397,45 @@ func resolveScanWorkerCount( mode ScanConcurrency, libraryPath string, ) int { + return workersForProfile( + mode, + system.ProfileForPath(libraryPath), + goruntime.NumCPU(), + ) +} + +// workersForProfile is the policy on its own, so it can be tested +// against drives this machine does not have. +// +// `hdd` and `ssd` override what the device says rather than being a +// separate branch: the mode is the user overruling detection, and +// detection is right about the queue depth either way — a user who +// picks `hdd` on a queueing drive still wants that drive's queue used. +func workersForProfile( + mode ScanConcurrency, + profile system.DiskProfile, + cpus int, +) int { + spinning := profile.Rotational + switch mode { case ScanConcurrencySSD: - return goruntime.NumCPU() + spinning = false case ScanConcurrencyHDD: - return min(hddWorkerCount, goruntime.NumCPU()) - default: // auto - if system.IsRotationalDisk(libraryPath) { - return min( - hddWorkerCount, goruntime.NumCPU(), - ) - } - - return goruntime.NumCPU() + spinning = true + case ScanConcurrencyAuto: } + + if !spinning { + return cpus + } + + workers := hddWorkerCountSerial + if profile.Queues() { + workers = hddWorkerCountQueued + } + + return min(workers, cpus) } // scanWork represents a file to be processed by a worker. diff --git a/backend/library/readahead_linux.go b/backend/library/readahead_linux.go new file mode 100644 index 0000000..59f86eb --- /dev/null +++ b/backend/library/readahead_linux.go @@ -0,0 +1,38 @@ +//go:build linux + +package library + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// hintReadahead asks the kernel to start fetching the head of a file +// that is about to be read. +// +// `POSIX_FADV_WILLNEED` returns immediately and queues the read, which +// is the whole point: on a spinning disk the first access to a file +// costs a seek of several milliseconds, and that latency can only be +// hidden by having the next seek already in flight while the current +// file is being parsed. A drive with command queueing can then service +// the queued reads in head order rather than in the order they were +// asked for. +// +// Errors are dropped on purpose. This is a hint: a file that has since +// been deleted, a filesystem that does not implement fadvise, or a +// permission the walk saw and this open does not, all mean "no +// prefetch", never "fail the scan". The read that follows is what +// reports a genuine problem. +func hintReadahead(path string, bytes int64) { + f, err := os.Open(path) + if err != nil { + return + } + + defer func() { _ = f.Close() }() + + _ = unix.Fadvise( + int(f.Fd()), 0, bytes, unix.FADV_WILLNEED, + ) +} diff --git a/backend/library/readahead_other.go b/backend/library/readahead_other.go new file mode 100644 index 0000000..e862e63 --- /dev/null +++ b/backend/library/readahead_other.go @@ -0,0 +1,13 @@ +//go:build !linux + +package library + +// hintReadahead is a no-op off Linux. +// +// macOS has `F_RDADVISE` and Windows has `FILE_FLAG_SEQUENTIAL_SCAN`, +// and neither is wired up here for the reason the scan concurrency +// heuristic is not either: this package cannot tell a spinning disk +// from an SSD on those platforms (see system.ProfileForPath), so it +// would be prefetching without knowing whether prefetching is what the +// device wants. +func hintReadahead(_ string, _ int64) {} diff --git a/backend/library/scan_workers_test.go b/backend/library/scan_workers_test.go new file mode 100644 index 0000000..5720c2c --- /dev/null +++ b/backend/library/scan_workers_test.go @@ -0,0 +1,122 @@ +package library + +import ( + "context" + "testing" + + "yellowjacket/backend/system" +) + +// How many workers a scan gets is decided by two facts about the +// device, and the second one is new: a spinning disk that can queue +// commands wants several reads in flight, and one that cannot wants +// almost none. Before this it was a flat 2 for anything rotational, +// which is a pre-NCQ assumption — a modern SATA disk reports a queue +// depth of 32 and was being given a quarter of what it can use. +func TestWorkersForProfile(t *testing.T) { + t.Parallel() + + const cpus = 16 + + ssd := system.DiskProfile{Device: "sda", QueueDepth: 32} + hddQueued := system.DiskProfile{ + Device: "sdb", Rotational: true, QueueDepth: 32, + } + hddSerial := system.DiskProfile{ + Device: "sdc", Rotational: true, QueueDepth: 1, + } + // Neither NVMe nor a device-mapper volume publishes queue_depth. + // An unknown depth must not be read as "cannot queue", or every + // such device would be scanned as if it were a 2003 drive. + unknown := system.DiskProfile{Device: "dm-0", Rotational: true} + + tests := []struct { + name string + mode ScanConcurrency + profile system.DiskProfile + want int + }{ + {"ssd auto", ScanConcurrencyAuto, ssd, cpus}, + {"queueing hdd auto", ScanConcurrencyAuto, hddQueued, hddWorkerCountQueued}, + {"serial hdd auto", ScanConcurrencyAuto, hddSerial, hddWorkerCountSerial}, + {"unknown depth queues", ScanConcurrencyAuto, unknown, hddWorkerCountQueued}, + + // The mode overrules detection about the *disk*, never about + // its queue: forcing hdd on a queueing drive still uses it. + {"forced hdd on an ssd", ScanConcurrencyHDD, ssd, hddWorkerCountQueued}, + {"forced ssd on an hdd", ScanConcurrencySSD, hddQueued, cpus}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := workersForProfile(tt.mode, tt.profile, cpus); got != tt.want { + t.Errorf( + "workersForProfile(%q, %+v) = %d, want %d", + tt.mode, tt.profile, got, tt.want, + ) + } + }) + } +} + +// A machine with fewer cores than the policy asks for gets its cores. +func TestWorkersNeverExceedTheCPUCount(t *testing.T) { + t.Parallel() + + hdd := system.DiskProfile{Rotational: true, QueueDepth: 32} + + if got := workersForProfile(ScanConcurrencyAuto, hdd, 1); got != 1 { + t.Errorf("single-core hdd = %d workers, want 1", got) + } +} + +// The prefetch stage must forward every item and nothing else: it is a +// pass-through with a side effect, and a scan that drops a file because +// of a *hint* would be a spectacular way to lose part of a library. +func TestReadaheadForwardsEveryFile(t *testing.T) { + t.Parallel() + + in := make(chan scanWork, 4) + for _, p := range []string{"/a", "/b", "/c", "/d"} { + in <- scanWork{absolutePath: p} + } + + close(in) + + var got []string + for w := range readaheadWork( + context.Background(), + in, + system.DiskProfile{Rotational: true, QueueDepth: 32}, + ) { + got = append(got, w.absolutePath) + } + + want := []string{"/a", "/b", "/c", "/d"} + if len(got) != len(want) { + t.Fatalf("forwarded %v, want %v", got, want) + } + + for i := range want { + if got[i] != want[i] { + t.Errorf("item %d = %q, want %q", i, got[i], want[i]) + } + } +} + +// On an SSD the stage is not inserted at all — the channel comes back +// unchanged, so a scan there pays nothing for a feature it cannot use. +func TestReadaheadIsSkippedOnSolidState(t *testing.T) { + t.Parallel() + + in := make(chan scanWork) + out := readaheadWork( + context.Background(), in, system.DiskProfile{QueueDepth: 32}, + ) + + if out != (<-chan scanWork)(in) { + t.Error("an ssd must get the original channel, unwrapped") + } +} diff --git a/backend/system/disktype_linux.go b/backend/system/disktype_linux.go index 8639ae7..0fa502b 100644 --- a/backend/system/disktype_linux.go +++ b/backend/system/disktype_linux.go @@ -16,32 +16,107 @@ var errNoBlockDevice = errors.New( "no matching block device found", ) -// IsRotationalDisk reports whether the block device backing the -// given path is a rotational (spinning) disk. Detection uses the -// Linux sysfs interface at /sys/block//queue/rotational. -// Returns false on any error (assumes SSD). -func IsRotationalDisk(path string) bool { - dev, err := deviceForPath(path) - if err != nil { - return false - } +// DiskProfile is what the scanner needs to know about the device a +// library sits on. Both fields are about the same question — how many +// reads should be in flight at once — and they answer different halves +// of it, so they travel together rather than as two probes. +type DiskProfile struct { + // Device is the whole-disk kernel name ("sdb"), or "" when the + // path could not be resolved to one. + Device string - rotational, err := os.ReadFile( - filepath.Join( - "/sys/block", dev, "queue", "rotational", - ), - ) - if err != nil { - return false - } + // Rotational is /sys/block//queue/rotational: true for a + // spinning disk, where a seek costs milliseconds. + Rotational bool - return strings.TrimSpace(string(rotational)) == "1" + // QueueDepth is /sys/block//device/queue_depth — how many + // commands the drive will accept and reorder at once. This is + // NCQ: a SATA disk with it enabled reports 31 or 32, and one + // without reports 1. Zero means the file was not there to read, + // which is the case for anything that is not a SCSI/SATA device + // (NVMe, MMC, device-mapper, loop, a VM's virtio disk). + // + // It is the difference between concurrency helping and hurting. + // With queueing, several outstanding reads let the drive service + // them in the order its head passes over them, which is most of + // why a parallel scan is faster at all. Without it, every extra + // worker is one more seek competing for one head, and the scan + // gets slower the harder it is pushed. + QueueDepth int } -// deviceForPath resolves a filesystem path to its underlying block -// device name (e.g. "sda") by matching the device major:minor -// from stat(2) against /sys/block/ entries. -func deviceForPath(path string) (string, error) { +// Queues reports whether the drive can reorder outstanding commands. +// +// An unknown depth (0) counts as queueing: everything that does not +// publish this file is a device where concurrency is fine — NVMe has +// its own queues, virtio and device-mapper are not the physical layer +// at all. The only case worth being careful about is the one that +// says so explicitly. +func (p DiskProfile) Queues() bool { + return p.QueueDepth != 1 +} + +// IsRotationalDisk reports whether the block device backing the +// given path is a rotational (spinning) disk. Returns false on any +// error (assumes SSD). +func IsRotationalDisk(path string) bool { + return ProfileForPath(path).Rotational +} + +// ProfileForPath describes the device backing a filesystem path. A +// path that cannot be resolved yields the zero profile, which reads as +// "not rotational, queueing" — the permissive answer, since assuming a +// spinning disk on an SSD would halve a scan for nothing. +func ProfileForPath(path string) DiskProfile { + dev, err := diskForPath(path) + if err != nil { + return DiskProfile{} + } + + return DiskProfile{ + Device: dev, + Rotational: sysfsInt(dev, "queue", "rotational") == 1, + QueueDepth: sysfsInt(dev, "device", "queue_depth"), + } +} + +// sysfsInt reads one small integer out of /sys/block//, +// returning 0 when it is absent or unparseable. Every attribute here +// is optional: sysfs layout varies by driver, and a missing file is +// "this device does not say", never an error worth propagating. +func sysfsInt(dev string, parts ...string) int { + p := filepath.Join( + append([]string{"/sys/block", dev}, parts...)..., + ) + + data, err := os.ReadFile(p) //nolint:gosec // sysfs, name from the kernel + if err != nil { + return 0 + } + + n, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil { + return 0 + } + + return n +} + +// diskForPath resolves a filesystem path to the *whole disk* backing +// it — "sdb" for a file on "sdb3". +// +// It goes through /sys/dev/block/:, which the kernel +// maintains as a symlink to the device's own sysfs directory, and then +// walks up to the parent when that directory turns out to be a +// partition. The previous implementation scanned /sys/block comparing +// dev numbers and, failing an exact match, took the first entry whose +// *major* agreed — and every SATA disk shares major 8. So a library on +// /dev/sdb3 resolved to whatever /sys/block listed first, which is +// alphabetical, which is sda. On the machine this was found on that +// meant a 6 TB spinning disk was read as the SSD next to it and scanned +// with one worker per core. Matching on major alone cannot be right +// whenever a machine has two disks, which is the case this exists for. +func diskForPath(path string) (string, error) { var st syscall.Stat_t if err := syscall.Stat(path, &st); err != nil { return "", fmt.Errorf( @@ -49,48 +124,50 @@ func deviceForPath(path string) (string, error) { ) } - // Extract major and minor device numbers. - major := (st.Dev >> 8) & 0xff - minor := st.Dev & 0xff + // Linux packs dev_t as 12 bits of major and 20 of minor, split + // across the word. Masking the low byte of each — which is what + // this used to do — is right only for the first 256 of either. + major := unixMajor(uint64(st.Dev)) + minor := unixMinor(uint64(st.Dev)) - // Scan /sys/block/ for a matching device. - entries, err := os.ReadDir("/sys/block") + link := filepath.Join( + "/sys/dev/block", + strconv.FormatUint(major, 10)+":"+ + strconv.FormatUint(minor, 10), + ) + + target, err := filepath.EvalSymlinks(link) if err != nil { return "", fmt.Errorf( - "could not read /sys/block: %w", err, + "%w: %s (%w)", errNoBlockDevice, link, err, ) } - majorStr := strconv.FormatUint(major, 10) - devStr := majorStr + ":" + - strconv.FormatUint(minor, 10) + // A partition's directory sits inside its disk's, and only the + // disk carries `queue`. Climb at most one level: sysfs nests a + // partition exactly one deep under its disk. + name := filepath.Base(target) - for _, entry := range entries { - devFile := filepath.Join( - "/sys/block", entry.Name(), "dev", - ) - - data, err := os.ReadFile(devFile) - if err != nil { - continue - } - - content := strings.TrimSpace(string(data)) - - if content == devStr { - return entry.Name(), nil - } - - // The filesystem might be on a partition (e.g. sda1) - // whose parent block device is sda. Check if the - // major number matches. - parts := strings.SplitN(content, ":", 2) - if len(parts) == 2 && parts[0] == majorStr { - return entry.Name(), nil - } + if _, err := os.Stat(filepath.Join(target, "queue")); err != nil { + name = filepath.Base(filepath.Dir(target)) } - return "", fmt.Errorf( - "%w for %s", errNoBlockDevice, devStr, - ) + if name == "" || name == "." || name == string(filepath.Separator) { + return "", fmt.Errorf( + "%w for %d:%d", errNoBlockDevice, major, minor, + ) + } + + return name, nil +} + +// unixMajor and unixMinor decode a Linux dev_t. Spelled out rather +// than taken from golang.org/x/sys/unix so this file stays readable +// beside the encoding it is undoing. +func unixMajor(dev uint64) uint64 { + return (dev>>8)&0xfff | (dev >> 32 & ^uint64(0xfff)) +} + +func unixMinor(dev uint64) uint64 { + return dev&0xff | (dev >> 12 & ^uint64(0xff)) } diff --git a/backend/system/disktype_other.go b/backend/system/disktype_other.go index f5a59f1..f1f1b25 100644 --- a/backend/system/disktype_other.go +++ b/backend/system/disktype_other.go @@ -2,9 +2,34 @@ package system +// DiskProfile is what the scanner needs to know about the device a +// library sits on. See the Linux implementation for what each field +// means; off Linux nothing fills them, because neither macOS nor +// Windows publishes an equivalent of sysfs's `rotational` and +// `queue_depth` without going through platform APIs this package +// deliberately does not link. +type DiskProfile struct { + Device string + Rotational bool + QueueDepth int +} + +// Queues reports whether the drive can reorder outstanding commands. +// Always true here: an unknown depth is the permissive answer, and +// assuming otherwise would halve every scan on every Mac. +func (p DiskProfile) Queues() bool { + return p.QueueDepth != 1 +} + // IsRotationalDisk reports whether the block device backing the // given path is a rotational (spinning) disk. On non-Linux // platforms this always returns false (assumes SSD). func IsRotationalDisk(_ string) bool { return false } + +// ProfileForPath describes the device backing a filesystem path. Off +// Linux that is the zero profile, which reads as "an SSD that queues". +func ProfileForPath(_ string) DiskProfile { + return DiskProfile{} +} diff --git a/e2e/specs/requested-badge.spec.ts b/e2e/specs/requested-badge.spec.ts index 3d3520d..2d3a3de 100644 --- a/e2e/specs/requested-badge.spec.ts +++ b/e2e/specs/requested-badge.spec.ts @@ -7,7 +7,7 @@ import { test, expect, callBinding } from '../support/fixtures.js'; * and produced two: every one of the eight call sites was a two-way * ternary, so an album already on the request list showed a plus and * said "is not in your library" — on the same page, forty pixels from a - * filled button reading "Wanted". + * filled button reading "Requested". * * This spec exists at this tier rather than only in the component one * because of what it drags in with it: reaching the requested state is @@ -181,7 +181,7 @@ test.describe('the requested badge', () => { const ds = document.querySelector('explore-album-details') ?.shadowRoot; const btn = [...(ds?.querySelectorAll('wa-button') ?? [])].find( - (b) => /Wanted/.test(b.textContent ?? ''), + (b) => /Requested/.test(b.textContent ?? ''), ); return btn?.querySelector('wa-icon')?.getAttribute('name') ?? ''; diff --git a/frontend/bindings/yellowjacket/backend/download/models.ts b/frontend/bindings/yellowjacket/backend/download/models.ts index 4f2a904..fbe539a 100644 --- a/frontend/bindings/yellowjacket/backend/download/models.ts +++ b/frontend/bindings/yellowjacket/backend/download/models.ts @@ -3,28 +3,52 @@ /** * AutoDownloadPrefs gates and scores what AutoPickable may choose - * without asking. Zero values are permissive: no size window and no - * format restriction. + * without asking. Zero values are permissive: no bitrate window, no + * size ceiling and no format restriction. + * + * **The window is a rate, not a size.** It used to be three numbers in + * megabytes, which cannot mean anything on their own: 300 MB is a + * generous FLAC single and a suspiciously small boxset, and the user + * setting the number has no idea which release the pipeline will + * eventually apply it to. A bitrate is the same statement normalised + * by how long the music is, so one number holds across a 9-minute EP + * and a 3-hour opera — and it is the unit the thing being described is + * actually measured in. The runtime is known for every request + * auto-pick can act on (`Download.Expected` carries per-track lengths, + * and an anchored request is the only kind that reaches here), so this + * costs no extra lookup. */ export interface AutoDownloadPrefs { /** - * MinSizeMB and MaxSizeMB bound what auto-pick will grab. Zero - * means no bound on that side. A candidate outside the window is - * filtered out of auto-pick entirely, not merely scored down — a - * tiny "sampler" torrent or a boxset ten times the expected size is - * usually the wrong thing entirely, not a worse copy of the right - * thing. + * MinKbps and MaxKbps bound the average bitrate auto-pick will + * grab. Zero means no bound on that side. A candidate outside the + * window is filtered out of auto-pick entirely, not merely scored + * down — a 96 kbps rip of the right album is not a worse copy the + * user might accept, it is one they said not to take unattended. + * + * For reference: 320 is the top of MP3, ~500–1000 is FLAC depending + * on the material, and anything under ~128 is a transcode. */ - "minSizeMb": number; - "maxSizeMb": number; + "minKbps": number; + "maxKbps": number; /** - * PreferredSizeMB nudges the score toward a target size within the - * min/max window (a lossless rip and a heavily-padded lossless rip - * can both pass the window). Zero disables the nudge; sizeFit then - * returns a neutral value that does not affect ranking. + * PreferredKbps nudges the score toward a target rate within the + * window, and breaks the tie when several candidates are equally + * good matches. Zero disables the nudge; bitrateFit then returns a + * neutral value that does not affect ranking. */ - "preferredSizeMb": number; + "preferredKbps": number; + + /** + * MaxSizeMB is a hard ceiling on the whole candidate, and it is + * deliberately still a size. It answers a different question from + * the window above — not "is this the quality I want" but "is this + * going to fill the disk" — and it has to hold even for a candidate + * whose bitrate cannot be worked out, which is exactly the shape a + * mislabelled boxset arrives in. Zero means no ceiling. + */ + "maxSizeMb": number; /** * AllowedFormats restricts auto-pick to candidates whose audio @@ -487,9 +511,13 @@ export interface QualityScore { "priority": number; /** - * closeness to the preferred download size + * BitrateFit is closeness to the preferred *rate*, which is what + * the auto-download window is expressed in. It replaced a + * `SizeFit` measured in megabytes: a size means nothing without + * knowing how long the music is, so the same number described a + * generous single and a suspiciously small boxset. */ - "sizeFit": number; + "bitrateFit": number; /** * Mixed marks a candidate whose files are not all the same format, diff --git a/frontend/src/assets/icons/fa/regular/star.svg b/frontend/src/assets/icons/fa/regular/star.svg new file mode 100644 index 0000000..2b82988 --- /dev/null +++ b/frontend/src/assets/icons/fa/regular/star.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/frontend/src/components/artists-view/artists-view.ts b/frontend/src/components/artists-view/artists-view.ts index dba2319..209dc49 100644 --- a/frontend/src/components/artists-view/artists-view.ts +++ b/frontend/src/components/artists-view/artists-view.ts @@ -10,6 +10,7 @@ import type { VisibilityChangedEvent, } from '@lit-labs/virtualizer'; import { grid } from '@lit-labs/virtualizer/layouts/grid.js'; +import { gridSpacingFor } from '@utils/grid-spacing'; import { GetAlbumsByArtist, GetFilePathsByAlbums, @@ -147,8 +148,6 @@ export class ArtistsView // ----- Grid spacing constants ----- - private static readonly GRID_GAP = 8; - private static readonly GRID_PADDING = 8; private static readonly CARD_PADDING = 5; private get imageSize(): number { @@ -177,20 +176,41 @@ export class ArtistsView private createGridLayout() { const w = this.cardSize ?? CARD_SIZE_DEFAULT; const h = w + this.cardTextHeight; - const gap = ArtistsView.GRID_GAP; - const pad = ArtistsView.GRID_PADDING; + + // One number for the gap, the row gap and the padding: whatever + // a row could not spend on another card, shared out equally, so + // the outside is never wider than the inside. See + // `utils/grid-spacing.ts`. + const spacing = this.spacingFor(this.containerWidth); + + this.lastLayoutSpacing = spacing; return grid({ itemSize: { width: `${w}px`, height: `${h}px`, }, - gap: `${gap}px`, - padding: `${pad}px`, - justify: 'center', + gap: `${spacing}px`, + padding: `${spacing}px`, + justify: 'start', }); } + /** The width the grid lays itself out in. */ + private get containerWidth(): number { + return ( + this.renderRoot?.querySelector( + '.grid-scroll-container', + )?.clientWidth || + this.clientWidth || + 0 + ); + } + + private spacingFor(width: number): number { + return gridSpacingFor(width, this.cardSize); + } + /** Sort direction for the artist grid. * * There is only one key to sort by: `library.Artist` carries a @@ -478,6 +498,8 @@ export class ArtistsView override disconnectedCallback() { super.disconnectedCallback(); this.detachWheelListener(); + this.gridResizeObserver?.disconnect(); + this.gridResizeObserver = null; } /** The wheel listener and the scroll debounce belong to the grid @@ -730,10 +752,34 @@ export class ArtistsView * ================================================================ */ private lastLayoutWidth = 0; + private lastLayoutSpacing = 0; + + /** Watches the scroller so a window resize rebuilds the layout: + * the spacing is derived from its width, and nothing else asks + * this view to update when only that changes. */ + private gridResizeObserver: ResizeObserver | null = null; + + private observeGridWidth() { + const container = + this.renderRoot?.querySelector( + '.grid-scroll-container', + ); + + if (!container || this.gridResizeObserver) return; + + this.gridResizeObserver = new ResizeObserver(() => + this.requestUpdate(), + ); + this.gridResizeObserver.observe(container); + } private updateGridLayout() { + this.observeGridWidth(); + if ( - this.cardSize === this.lastLayoutWidth + this.cardSize === this.lastLayoutWidth && + this.lastLayoutSpacing === + this.spacingFor(this.containerWidth) ) { return; } diff --git a/frontend/src/components/config-page/download-clients.ts b/frontend/src/components/config-page/download-clients.ts index cf36c44..0fb7052 100644 --- a/frontend/src/components/config-page/download-clients.ts +++ b/frontend/src/components/config-page/download-clients.ts @@ -86,9 +86,10 @@ export class DownloadClients extends LitElement { /** Working copy of the auto-download guardrails. */ @state() private prefs: download.AutoDownloadPrefs = { - minSizeMb: 0, + minKbps: 0, + maxKbps: 0, + preferredKbps: 0, maxSizeMb: 0, - preferredSizeMb: 0, allowedFormats: [], } as download.AutoDownloadPrefs; @@ -284,25 +285,72 @@ export class DownloadClients extends LitElement { : nothing}
+
{ this.prefs = { ...this.prefs, - minSizeMb: Number((e.target as HTMLInputElement).value) || 0, + minKbps: Number((e.target as HTMLInputElement).value) || 0, }; }} > { + this.prefs = { + ...this.prefs, + maxKbps: Number((e.target as HTMLInputElement).value) || 0, + }; + }} + > + { + this.prefs = { + ...this.prefs, + preferredKbps: + Number((e.target as HTMLInputElement).value) || 0, + }; + }} + > +
+ +
+ 320 is the top of MP3; a FLAC rip is usually + 500–1000 depending on the music. Preferred + decides between copies that are otherwise equally + good — it never rules one out, which is what the + minimum and maximum are for. +
+ +
+ { this.prefs = { @@ -311,22 +359,14 @@ export class DownloadClients extends LitElement { }; }} > - { - this.prefs = { - ...this.prefs, - preferredSizeMb: - Number((e.target as HTMLInputElement).value) || 0, - }; - }} - > +
+ +
+ A ceiling on the download itself, in case a + mislabelled boxset gets through. Still a size + because it is a question about disk space, and + because it has to apply to a candidate whose + bitrate cannot be worked out at all.
diff --git a/frontend/src/components/cover-grid/cover-grid.ts b/frontend/src/components/cover-grid/cover-grid.ts index 6b3056d..540f4ad 100644 --- a/frontend/src/components/cover-grid/cover-grid.ts +++ b/frontend/src/components/cover-grid/cover-grid.ts @@ -19,6 +19,7 @@ import { LibraryController } from '@store/controllers/library-controller'; import { SearchController } from '@store/controllers/search-controller'; import { ViewLifecycleMixin } from '@utils/view-lifecycle'; import { RovingGridController } from '@utils/roving-grid'; +import { gridColumnsFor, gridSpacingFor } from '@utils/grid-spacing'; import { queueStore } from '@store/queue-store'; import type { QueueSource } from '@store/queue-store'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; @@ -97,19 +98,36 @@ export class CoverGrid private lastAlbumsRef: library.Album[] | null = null; - // Fixed grid spacing constants. - private static readonly GRID_GAP = 8; - private static readonly GRID_PADDING = 8; private static readonly CARD_PADDING = 5; private ctxMenu = new ContextMenuController(this); private favCtrl = new FavoritesController(this); private selMgr = new AlbumSelectionManager(); private scrollMgr = new ScrollManager(this, { - GRID_GAP: CoverGrid.GRID_GAP, - GRID_PADDING: CoverGrid.GRID_PADDING, + columnsFor: (width: number) => this.columnsFor(width), + spacingFor: (width: number) => this.spacingFor(width), }); + /** + * How many cards fit across `width`, by the same arithmetic the + * virtualizer's `space-evenly` grid uses — no gap and no padding + * are reserved, because both come out of what is left over. + * + * The scroll manager restores a position by rebuilding the grid's + * geometry, so this and `spacingFor` must agree with the layout + * rather than approximate it; they were two constants that no + * longer describe anything once the spacing became elastic. + */ + columnsFor(width: number): number { + return gridColumnsFor(width, this.cardWidth); + } + + /** The spacing that width produces: between columns, between rows, + * and around the outside, all the same number. */ + spacingFor(width: number): number { + return gridSpacingFor(width, this.cardWidth); + } + private lastSelectedAlbumIndex: number | null = null; private lastSelectedTrackIndex: number | null = null; @@ -148,10 +166,30 @@ export class CoverGrid } // Virtualizer grid layout instance — recreated when - // the card size changes. + // the card size or the container width changes. private gridLayout = this.createGridLayout(); private gridLayoutWidth = 0; + /** The spacing the current layouts were built with. */ + private gridLayoutSpacing = 0; + + /** Watches the scroll container so a window resize rebuilds the + * layout: the spacing is derived from its width, and nothing else + * asks this component to update when only that changes. */ + private gridResizeObserver: ResizeObserver | null = + null; + + private observeGridWidth(): void { + const container = this.scrollContainer; + + if (!container || this.gridResizeObserver) return; + + this.gridResizeObserver = new ResizeObserver( + () => this.requestUpdate(), + ); + this.gridResizeObserver.observe(container); + } + /** * Secondary layout for the "after" virtualizer in * split mode. Uses zero top padding so there is no @@ -169,22 +207,49 @@ export class CoverGrid } const h = w + this.cardTextHeight; - const gap = CoverGrid.GRID_GAP; - const pad = CoverGrid.GRID_PADDING; + + // The spacing is whatever the row could not spend on another + // card, shared out equally — so it is the same number between + // two cards, between two rows, and down each outside edge. + // See `utils/grid-spacing.ts` for why it is computed rather + // than handed to the virtualizer as `space-evenly`. + const spacing = this.spacingFor( + this.containerWidth, + ); + + if (!noTopPad) { + this.gridLayoutSpacing = spacing; + } return grid({ itemSize: { width: `${w}px`, height: `${h}px`, }, - gap: `${gap}px`, + gap: `${spacing}px`, padding: noTopPad - ? `0 ${pad}px ${pad}px` - : `${pad}px`, - justify: 'center', + ? `0 ${spacing}px ${spacing}px` + : `${spacing}px`, + justify: 'start', }); } + /** + * The width the grid lays itself out in. + * + * Read from the scroll container when there is one; before the + * first render there is not, and the fallback only has to be + * plausible — the layout is rebuilt from the real width as soon as + * one exists. + */ + private get containerWidth(): number { + return ( + this.scrollContainer?.clientWidth || + this.clientWidth || + 0 + ); + } + private dragImageEl: HTMLElement | null = null; // -- Memoisation caches for filtered albums -- @@ -466,6 +531,9 @@ export class CoverGrid ); this.wheelListenerAttached = false; + this.gridResizeObserver?.disconnect(); + this.gridResizeObserver = null; + this.scrollMgr.teardown(); this.scrollMgr.revealContainer( this.scrollContainer, @@ -603,10 +671,18 @@ export class CoverGrid this.wheelListenerAttached = true; } - // Recreate the virtualizer grid layout when - // the card size changes. + this.observeGridWidth(); + + // Recreate the virtualizer grid layout when the card size + // changes — or when the spacing the container width produces + // does, since that is now a derived number rather than a + // constant. Keyed on the spacing rather than on the width, or + // every pixel of a drag rebuilds a layout that would come out + // the same. const cardSizeChanged = - this.gridLayoutWidth !== this.cardWidth; + this.gridLayoutWidth !== this.cardWidth || + this.gridLayoutSpacing !== + this.spacingFor(this.containerWidth); if (cardSizeChanged) { this.gridLayout = this.createGridLayout(); diff --git a/frontend/src/components/cover-grid/scroll-manager.ts b/frontend/src/components/cover-grid/scroll-manager.ts index 03bc000..7f4ce93 100644 --- a/frontend/src/components/cover-grid/scroll-manager.ts +++ b/frontend/src/components/cover-grid/scroll-manager.ts @@ -6,12 +6,21 @@ import type { LibraryController } from '@store/controllers/library-controller'; import type { GridEntry } from './cover-grid-types.js'; /** - * Grid spacing constants shared between the scroll - * manager and the host component. + * Grid geometry, asked of the host rather than written down. + * + * These were two constants, `GRID_GAP` and `GRID_PADDING`, which stopped + * describing anything the moment the grid's spacing became elastic: the + * gap, the padding and the column count are all derived from the + * container width now, and a scroll position rebuilt from a stale 8px + * lands in the wrong row. */ export interface GridConstants { - readonly GRID_GAP: number; - readonly GRID_PADDING: number; + /** Columns that fit across `width`. */ + columnsFor(width: number): number; + + /** The spacing `width` produces — between columns, between rows, + * and around the outside, all the same number. */ + spacingFor(width: number): number; } /** @@ -275,8 +284,8 @@ export class ScrollManager { return; } - const gap = this.gc.GRID_GAP; - const pad = this.gc.GRID_PADDING; + const gap = this.spacing(container); + const pad = gap; const rowStep = this.host.cardHeight + gap; @@ -293,7 +302,7 @@ export class ScrollManager { () => { const rowStep = this.host.cardHeight + - this.gc.GRID_GAP; + this.spacing(container); if (this.pendingFocus === null) { this.isResizing = true; @@ -351,7 +360,7 @@ export class ScrollManager { container: HTMLElement, rowStep: number, ): void { - const pad = this.gc.GRID_PADDING; + const pad = this.spacing(container); const cols = this.currentColumnCount; const filtered = this.host.cachedFilteredAlbums; @@ -410,17 +419,15 @@ export class ScrollManager { ): number { if (!container) return 1; - const gap = this.gc.GRID_GAP; - const pad = this.gc.GRID_PADDING; - const availableWidth = - container.clientWidth - pad * 2; + return this.gc.columnsFor( + container.clientWidth, + ); + } - return Math.max( - 1, - Math.floor( - (availableWidth + gap) / - (this.host.cardWidth + gap), - ), + /** The grid's current spacing, which is also its padding. */ + private spacing(container?: HTMLElement): number { + return this.gc.spacingFor( + container?.clientWidth ?? 800, ); } @@ -439,7 +446,7 @@ export class ScrollManager { container?: HTMLElement, ): number { const cols = this.getColumnCount(container); - const gap = this.gc.GRID_GAP; + const gap = this.spacing(container); return ( cols * this.host.cardWidth + @@ -460,7 +467,7 @@ export class ScrollManager { const cols = this.getColumnCount(container); const colIndex = idx % cols; - const gap = this.gc.GRID_GAP; + const gap = this.spacing(container); return ( colIndex * @@ -597,8 +604,8 @@ export class ScrollManager { if (!this.host.splitMode) return raw; - const gap = this.gc.GRID_GAP; - const pad = this.gc.GRID_PADDING; + const gap = this.spacing(container); + const pad = gap; const columns = this.getColumnCount(container); const rowStep = this.host.cardHeight + gap; @@ -678,8 +685,8 @@ export class ScrollManager { if (expandedIndex < 0) return; - const gap = this.gc.GRID_GAP; - const pad = this.gc.GRID_PADDING; + const gap = this.spacing(container); + const pad = gap; const columns = this.getColumnCount(container); const rowStep = this.host.cardHeight + gap; @@ -772,8 +779,8 @@ export class ScrollManager { if (idx < 0) return; - const gap = this.gc.GRID_GAP; - const pad = this.gc.GRID_PADDING; + const gap = this.spacing(container); + const pad = gap; const cols = this.getColumnCount(container); const rowStep = this.host.cardHeight + gap; @@ -854,9 +861,8 @@ export class ScrollManager { this.getExpandedAlbumIndex(); if (idx >= 0) { - const gap = this.gc.GRID_GAP; - const pad = - this.gc.GRID_PADDING; + const gap = this.spacing(container); + const pad = gap; const cols = this.getColumnCount( container, diff --git a/frontend/src/components/downloads-view/downloads-view.ts b/frontend/src/components/downloads-view/downloads-view.ts index d774ab0..8047e34 100644 --- a/frontend/src/components/downloads-view/downloads-view.ts +++ b/frontend/src/components/downloads-view/downloads-view.ts @@ -400,7 +400,7 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) { private renderEmptyRequests() { return html`
- Nothing requested yet. Use “Want this” on an album or artist + Nothing requested yet. Use “Request this” on an album or artist to add it here.
`; diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index b1736ba..de44a52 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -2694,7 +2694,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { slot="start" name=${this.isRequested ? 'solid/bookmark' : 'regular/bookmark'} > - ${this.isRequested ? 'Wanted' : 'Want this'} + ${this.isRequested ? 'Requested' : 'Request this'} `; } diff --git a/frontend/src/components/explore-artist-details/explore-artist-details.ts b/frontend/src/components/explore-artist-details/explore-artist-details.ts index eafb75f..c18ca92 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -2753,7 +2753,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost slot="icon" name=${requested ? 'xmark' : 'bookmark'} > - ${requested ? 'Cancel Request' : 'Want This'} + ${requested ? 'Cancel Request' : 'Request This'} ` : nothing} diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index be05df2..b9b1257 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -1509,9 +1509,24 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte if (url) { this.thumbnailCache.set(req.mbid, url); this.requestUpdate(); + + return; } + + // An empty answer is not necessarily "there + // is no art" — a slow Internet Archive node + // is answered by a timeout, which looks + // exactly the same from here. Drop the + // in-flight marker so the next time this + // release group is on screen it is asked + // again; the backend records a genuine 404 + // on disk and answers that one instantly, + // so a real miss costs nothing to re-ask. + this.thumbnailCache.delete(req.mbid); }) - .catch(() => {}); + .catch(() => { + this.thumbnailCache.delete(req.mbid); + }); } }) .catch(() => { diff --git a/frontend/src/components/genres-view/genres-view.ts b/frontend/src/components/genres-view/genres-view.ts index fb599b2..92e45f7 100644 --- a/frontend/src/components/genres-view/genres-view.ts +++ b/frontend/src/components/genres-view/genres-view.ts @@ -10,6 +10,7 @@ import type { VisibilityChangedEvent, } from '@lit-labs/virtualizer'; import { grid } from '@lit-labs/virtualizer/layouts/grid.js'; +import { gridSpacingFor } from '@utils/grid-spacing'; import { GetFilePathsByGenres, } from '@go/library/library.js'; @@ -155,8 +156,6 @@ export class GenresView // ----- Grid spacing constants ----- - private static readonly GRID_GAP = 8; - private static readonly GRID_PADDING = 8; private static readonly CARD_PADDING = 5; private get imageSize(): number { @@ -185,20 +184,41 @@ export class GenresView private createGridLayout() { const w = this.cardSize ?? CARD_SIZE_DEFAULT; const h = w + this.cardTextHeight; - const gap = GenresView.GRID_GAP; - const pad = GenresView.GRID_PADDING; + + // One number for the gap, the row gap and the padding: whatever + // a row could not spend on another card, shared out equally, so + // the outside is never wider than the inside. See + // `utils/grid-spacing.ts`. + const spacing = this.spacingFor(this.containerWidth); + + this.lastLayoutSpacing = spacing; return grid({ itemSize: { width: `${w}px`, height: `${h}px`, }, - gap: `${gap}px`, - padding: `${pad}px`, - justify: 'center', + gap: `${spacing}px`, + padding: `${spacing}px`, + justify: 'start', }); } + /** The width the grid lays itself out in. */ + private get containerWidth(): number { + return ( + this.renderRoot?.querySelector( + '.grid-scroll-container', + )?.clientWidth || + this.clientWidth || + 0 + ); + } + + private spacingFor(width: number): number { + return gridSpacingFor(width, this.cardSize); + } + /** Sort key and direction for the genre grid (H-19: it had none). */ @state() private sortField: 'name' | 'tracks' = 'name'; @@ -483,6 +503,8 @@ export class GenresView override disconnectedCallback() { super.disconnectedCallback(); this.detachWheelListener(); + this.gridResizeObserver?.disconnect(); + this.gridResizeObserver = null; } /** See artists-view: off-screen the grid cannot be scrolled, and @@ -737,10 +759,34 @@ export class GenresView * ================================================================ */ private lastLayoutWidth = 0; + private lastLayoutSpacing = 0; + + /** Watches the scroller so a window resize rebuilds the layout: + * the spacing is derived from its width, and nothing else asks + * this view to update when only that changes. */ + private gridResizeObserver: ResizeObserver | null = null; + + private observeGridWidth() { + const container = + this.renderRoot?.querySelector( + '.grid-scroll-container', + ); + + if (!container || this.gridResizeObserver) return; + + this.gridResizeObserver = new ResizeObserver(() => + this.requestUpdate(), + ); + this.gridResizeObserver.observe(container); + } private updateGridLayout() { + this.observeGridWidth(); + if ( - this.cardSize === this.lastLayoutWidth + this.cardSize === this.lastLayoutWidth && + this.lastLayoutSpacing === + this.spacingFor(this.containerWidth) ) { return; } diff --git a/frontend/src/components/library-status-indicator/library-status-indicator.ts b/frontend/src/components/library-status-indicator/library-status-indicator.ts index b576c65..93281a4 100644 --- a/frontend/src/components/library-status-indicator/library-status-indicator.ts +++ b/frontend/src/components/library-status-indicator/library-status-indicator.ts @@ -48,7 +48,7 @@ export type LibraryStatus = * * Colours and glyphs: * - in-library → green circle, check mark - * - queued → amber circle, hourglass + * - queued → amber circle, bookmark ("on your list") * - not-in-library → grey circle, plus sign * * Usage: @@ -241,12 +241,23 @@ export class LibraryStatusIndicator extends LitElement { } `; + /** + * The glyph for each state. + * + * `queued` is a **bookmark**, not the hourglass it used to be. An + * hourglass says "wait, this is under way", which overstates what a + * request is: nothing may be downloading, nothing may ever be found, + * and the user can leave one sitting on the list indefinitely. A + * bookmark says the honest thing — it is on your list — and reads as + * the opposite of the plus that put it there, which is what a + * toggle's two states have to do. + */ private iconName(): string { switch (this.status) { case 'in-library': return 'check'; case 'queued': - return 'hourglass-half'; + return 'bookmark'; default: return 'plus'; } @@ -276,7 +287,7 @@ export class LibraryStatusIndicator extends LitElement { if (this.actionable) { return this.status === 'queued' ? `Cancel the request for ${kind}${name}` - : `Want ${kind}${name}`; + : `Request ${kind}${name}`; } switch (this.status) { @@ -354,25 +365,6 @@ export class LibraryStatusIndicator extends LitElement { } const title = this.tooltip(); - const icon = this.iconName() - ? html`` - : nothing; - - if (this.actionable) { - return html` - - `; - } // The ring stands in for the icon wherever the icon would go — // including inside the button, because a partly-held album is diff --git a/frontend/src/components/now-playing-view/now-playing-view.ts b/frontend/src/components/now-playing-view/now-playing-view.ts index cfdb99b..f90dc64 100644 --- a/frontend/src/components/now-playing-view/now-playing-view.ts +++ b/frontend/src/components/now-playing-view/now-playing-view.ts @@ -307,7 +307,7 @@ export class NowPlayingView extends LitElement { : `Add ${track.title} to ${this.favCtrl.playlistName}`} @click=${this.toggleFavorite} > - +
diff --git a/frontend/src/components/now-playing/now-playing.ts b/frontend/src/components/now-playing/now-playing.ts index 48d0524..4405695 100644 --- a/frontend/src/components/now-playing/now-playing.ts +++ b/frontend/src/components/now-playing/now-playing.ts @@ -527,7 +527,7 @@ export class NowPlaying extends LitElement { )} > diff --git a/frontend/src/components/playlist-view/playlist-view.ts b/frontend/src/components/playlist-view/playlist-view.ts index 0f69572..4e7aa62 100644 --- a/frontend/src/components/playlist-view/playlist-view.ts +++ b/frontend/src/components/playlist-view/playlist-view.ts @@ -1756,7 +1756,7 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) { ${entry.summary.ID === this.favCtrl.playlistId ? html`` : entry.summary.IsSmart ? html`` call sites. + * + * The Font Awesome family is part of the name — `regular/heart` is + * the outline, a bare `heart` is the solid one (`src/icons`). + */ + iconFor(favorited: boolean): string { + const shape = this.iconStyle === 'star' ? 'star' : 'heart'; + + return favorited ? shape : `regular/${shape}`; } // =============================================================== diff --git a/frontend/src/utils/grid-spacing.ts b/frontend/src/utils/grid-spacing.ts new file mode 100644 index 0000000..74dfbbe --- /dev/null +++ b/frontend/src/utils/grid-spacing.ts @@ -0,0 +1,58 @@ +/** + * Even spacing for the three card grids — albums, artists, genres. + * + * All three used `justify: 'center'` with a fixed 8px gap and 8px + * padding, which gives the row a fixed width and pushes everything left + * over to the two margins: on a 1440px window the albums grid drew its + * cards 16px apart inside 78px of nothing down each side. The outside + * was five times the inside. + * + * The fix is to spend the leftover on the spacing instead, so there is + * one number: between two cards, between two rows, and down each edge. + * The virtualizer has a word for that — `justify: 'space-evenly'` with + * `gap: 'auto'` — and it cannot be used, because it fits + * `floor(width / cardWidth)` columns without reserving the gap it is + * about to need: a width one card short of exact fits seven cards a + * pixel apart. Deciding the column count here is what puts a floor + * under the spacing, and the grid is then given plain numbers. + */ + +/** The narrowest the spacing is allowed to get. */ +export const MIN_GRID_SPACING = 8; + +/** + * How many cards of `cardWidth` fit across `width`. + * + * A row of c cards spends c×cardWidth on cards and (c+1)×spacing on the + * spaces between and beside them, so c is bounded by + * (width − spacing) / (cardWidth + spacing) at the minimum spacing. + */ +export function gridColumnsFor( + width: number, + cardWidth: number, +): number { + if (cardWidth <= 0) return 1; + + const fit = Math.floor( + (width - MIN_GRID_SPACING) / (cardWidth + MIN_GRID_SPACING), + ); + + return Math.max(1, fit); +} + +/** + * The spacing `width` produces — the gap, the row gap and the padding, + * which are all the same number. + */ +export function gridSpacingFor( + width: number, + cardWidth: number, +): number { + const columns = gridColumnsFor(width, cardWidth); + const leftover = width - columns * cardWidth; + + return Math.max( + MIN_GRID_SPACING, + Math.floor(leftover / (columns + 1)), + ); +} diff --git a/frontend/test/components/artist-release-menu.test.ts b/frontend/test/components/artist-release-menu.test.ts index 9829a9e..cb5ed00 100644 --- a/frontend/test/components/artist-release-menu.test.ts +++ b/frontend/test/components/artist-release-menu.test.ts @@ -120,7 +120,7 @@ describe('the context menu on an artist page release', () => { expect(items).toContain('Add to Queue'); expect(items).toContain('Play Next'); // Owned: there is nothing left to ask for. - expect(items).not.toContain('Want This'); + expect(items).not.toContain('Request This'); }); it('offers a request, and no playback, for a release nobody owns', async () => { @@ -132,7 +132,7 @@ describe('the context menu on an artist page release', () => { expect(items).not.toContain('Play'); expect(items).not.toContain('Add to Queue'); - expect(items).toContain('Want This'); + expect(items).toContain('Request This'); expect(items).toContain('View on MusicBrainz'); }); @@ -148,7 +148,7 @@ describe('the context menu on an artist page release', () => { // …but a `local:` id names nothing upstream, and wanting something // already in the library is not a thing to offer. expect(items).not.toContain('View on MusicBrainz'); - expect(items).not.toContain('Want This'); + expect(items).not.toContain('Request This'); }); it('opens from the keyboard on Shift+F10', async () => { diff --git a/frontend/test/components/chrome.test.ts b/frontend/test/components/chrome.test.ts index 6a8dec5..4d11c12 100644 --- a/frontend/test/components/chrome.test.ts +++ b/frontend/test/components/chrome.test.ts @@ -166,7 +166,7 @@ describe('', () => { glyphs.push(shadow(el, 'wa-icon')?.getAttribute('name')); } - expect(glyphs).toEqual(['check', 'hourglass-half', 'plus']); + expect(glyphs).toEqual(['check', 'bookmark', 'plus']); }); it('phrases its label around the entity it describes', async () => { diff --git a/frontend/test/components/library-status.test.ts b/frontend/test/components/library-status.test.ts index 317b2e1..6945552 100644 --- a/frontend/test/components/library-status.test.ts +++ b/frontend/test/components/library-status.test.ts @@ -249,7 +249,7 @@ describe(' as a control', () => { const el = await badge({ requestMbid: 'rg-1' }); expect(shadow(el, '.badge')?.getAttribute('aria-label')).toBe( - 'Want album "Abbey Road"', + 'Request album "Abbey Road"', ); await update(el, { status: 'queued' });