Compare commits

..
Author SHA1 Message Date
logan a2ff0aed4c fix(ui): make the queue button say whether the queue is open
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m27s
CI / e2e (pull_request) Canceled after 5m49s
It looked identical in both states, so the only way to tell what
pressing it would do was to look at the other side of the window and
infer it -- and for anyone not looking there was nothing to infer from:
no aria-expanded, no aria-controls, no drawn state.

The state is reflected *from the panel* rather than kept beside the
click. This button is not the only thing that opens the queue --
now-playing-view sets the same attribute, because it hides the bar the
button lives in -- so a flag maintained by the click handler would be
right until something else opened the panel and then quietly wrong.
The panel's `open` attribute stays the one fact; a MutationObserver
reflects it.

Refs #26
2026-08-18 11:15:32 -04:00
yonlu 3bf27e3fd5 Merge pull request 'Fix/explore art scanner requests' (#21) from fix/explore-art-scanner-requests into main
CI / check (push) Skipped
CI / e2e (push) Skipped
Release / release (push) Successful in 32s
Build & publish the Android APK / apk (push) Successful in 1m26s
Build & publish Arch package / arch-package (push) Successful in 2m35s
Attach the desktop build to the release / linux (push) Successful in 1m12s
Sync Homebrew formula / sync-formula (push) Successful in 9s
CI / check (pull_request) Canceled after 0s
CI / e2e (pull_request) Canceled after 0s
Reviewed-on: #21
2026-08-18 13:48:37 +00:00
yonlu 48abecb830 Merge remote-tracking branch 'origin/main' into fix/explore-art-scanner-requests
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m26s
CI / e2e (pull_request) Successful in 6m13s
2026-08-18 07:43:25 -04:00
logan e1c07438e9 docs: record what shipping the release pipeline taught us (#4)
CI / check (push) Successful in 2m21s
Release / release (push) Successful in 31s
CI / e2e (push) Successful in 6m4s
2026-08-18 03:49:00 +00:00
logan 6e563f3846 docs: record what shipping the release pipeline taught us
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m27s
CI / e2e (pull_request) Successful in 6m8s
Moves plan 017 to completed with a recap, and lifts the three findings
that generalise into NOTES.md: a preset major that renders empty notes
with everything green, a 403 that looks like branch protection and is a
token scope, and tag-triggered workflows running the tagged commit's
own definitions.
2026-08-17 23:35:59 -04:00
yonluandClaude Opus 5 590a0d86dd perf(library): size the scan to the drive, and prefetch what it reads
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 2m47s
CI / e2e (pull_request) Successful in 5m59s
Every parser in `backend/metadata` is header-only -- a few hundred
bytes and return -- so on a spinning disk a scan is not waiting on CPU
or on bytes, it is waiting on the head to arrive. Two things follow,
and the drive says which.

**How many reads should be in flight.** This was a flat 2 for anything
rotational, which is a pre-NCQ assumption: a modern SATA disk reports a
queue depth of 32 and reorders outstanding reads into the order its
head passes over them, and was being handed a quarter of what it can
use. It gets 4 now. A drive that reports 1 -- a USB bridge, a pre-2004
disk -- services one command at a time in the order given, where every
extra worker is one more seek competing for one head and the scan gets
*slower* the harder it is pushed; that keeps 2.

**And that the next seek should already be queued.** A prefetch stage
between the walk and the workers issues `POSIX_FADV_WILLNEED` over the
first 512 KB of each file -- enough for an ID3v2 tag carrying cover
art, or FLAC's STREAMINFO and PICTURE blocks. The buffered channel *is*
the lookahead: the goroutine runs 16 files ahead of the workers,
hinting as it goes, so the read a worker needs has been in flight for
sixteen files' worth of parsing by the time it asks. Rotational only;
an SSD gets the channel back unwrapped and pays nothing, since it has
no seek to hide and already has one worker per core.

`workersForProfile` is the policy on its own so it can be tested
against drives this machine does not have, and the scan logs the
device, its rotational flag and its queue depth, so the decision is
inspectable rather than inferred.

Also: `ScanConcurrency` has been a validated three-value config field
with exactly one caller, passing the constant `auto` -- so choosing
`ssd` or `hdd` by hand did nothing at all. It reads the config now.
The two modes overrule detection about the *disk* and not about its
queue, since a user who picks `hdd` on a queueing drive still wants
that drive's queue used.

What is not here is inode-ordered dispatch. It needs the streaming walk
restructured to buffer per directory, and with queueing the drive is
already reordering what the hints put in front of it; that wants a
measurement on real hardware before the complexity.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:12:15 -04:00
yonluandClaude Opus 5 36af7090d9 fix(system): resolve a path to its own disk, not the first on its major
`deviceForPath` scanned `/sys/block` comparing device numbers and, when
no entry matched exactly, took the first one whose *major* agreed. Every
SATA disk is major 8. A filesystem's `st_dev` is its **partition**, so
the exact match never hits for anything on one, and the fallback then
resolved `/dev/sdb3` to whatever `/sys/block` listed first -- which is
alphabetical, which is `sda`.

On the machine this was found on that is a Samsung SSD sitting next to
the 6 TB spinning disk the library is actually on, so
`IsRotationalDisk` answered false and the scanner ran one worker per
core across a drive with one head. Matching on major alone cannot be
right on any machine with two disks, which is the case this exists for.

It goes through `/sys/dev/block/<major>:<minor>` instead -- a symlink
the kernel maintains to the device's own sysfs directory -- and climbs
to the parent when that turns out to be a partition. One readlink, no
scan, no ambiguity. The dev_t decode goes with it: Linux packs 12 bits
of major and 20 of minor split across the word, and masking the low
byte of each is right only for the first 256 of either.

`ProfileForPath` returns what the scanner needs to ask next, and the
new half is `queue_depth`: how many commands the drive will accept and
reorder at once. A SATA disk with NCQ enabled reports 31 or 32 and one
without reports 1, which is the difference between concurrency helping
and hurting. An absent file is read as "queues", because everything
that does not publish it -- NVMe, virtio, device-mapper -- is a device
where concurrency is fine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:11:51 -04:00
yonluandClaude Opus 5 3e142f8c35 test(downloads): guard the service fixture on something the fake sets
`newServiceFixture` stops auto-pick from starting a grab, because none
of its tests is about the download and a detached `go m.grab(...)`
racing `t.TempDir()`'s cleanup is how they fail. It did that with
`MaxSizeMB: 1` -- and the size gates read `Candidate.TotalSize`, which
real providers fill and the fake leaves at zero. Zero is under every
ceiling, so the guard never fired and the race it was written to
prevent kept happening, roughly one run in fifteen:

    TempDir RemoveAll cleanup: unlinkat ... : directory not empty

The guard is a format the fake never produces. Thirty consecutive
whole-package runs, none.

`TestManualDownloadSatisfiesRequestOnSuccess` was relying on the guard
being broken -- it is the one test here that wants the download -- so
it now clears the preferences itself rather than depending on a bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:11:10 -04:00
yonluandClaude Opus 5 3d375adab1 feat(downloads): bound auto-pick by bitrate, and take a good copy
Three faults, one subsystem, and the middle one is why a request that
looked obviously satisfiable came back refused.

**The guardrails were in megabytes, which cannot mean anything.** 300 MB
is a generous FLAC single and a suspiciously small boxset, and whoever
fills the field in has no idea which release the pipeline will apply it
to. `MinKbps`/`MaxKbps`/`PreferredKbps` are the same statement divided
by how long the music is, so one number holds across a nine-minute EP
and a three-hour opera. The runtime comes from `Download.Expected`,
which every anchored request already carries, so this costs no lookup;
the rate is audio bytes over that, falling back to the mean stated
per-file bitrate when the runtime is unknown. Artwork is excluded from
the numerator, or a folder with 30 MB of scans reads as a better rip.

An unknown runtime *passes* the window rather than failing it: the
window is a statement about quality, and refusing everything the moment
MusicBrainz is missing a track length would be a silent embargo.
`MaxFileSizeMB` survives as a separate ceiling, still in megabytes on
purpose -- it is a question about disk space, and it has to apply to a
candidate whose bitrate cannot be worked out at all.

**Auto-pick required daylight over the runner-up**, 0.08 on the
combined score, and so 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 it 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 condition under which that matters least. A candidate no longer has
to beat the field, only clear the bars on its own terms; where several
do, ranking puts the one closest to the preferred bitrate first.

That tie-break needed the preference to carry weight or it would have
been decorative in a new unit: `BitrateFit` was 0.05 against format's
0.42, so asking for 320 and being handed a FLAC every time was the
designed behaviour. When a preference is set the weights shift to fit
0.40 / format 0.20 / bitrate 0.10, taking it off the two heuristics
that exist as stand-ins for the preference the user has now given.
Health and priority are untouched. And the fit spans 0.5 to 1.0 rather
than 0 to 1, so a preference can promote the copy that matches it and
can never push the others under `minQuality` -- turning "I like 320"
into "never take anything else" silently is what `MinKbps`/`MaxKbps`
are for, out loud.

**And a refusal quoted numbers that passed.** The request list built its
message from `ranked[0]` -- the best candidate *before* the guardrails
and before the lead check -- so a request killed by the size window, or
by having too many good copies, reported "best of 12 found is not a
confident enough match (match 96%, quality 88%)". `AutoPickVeto` names
the gate that actually refused, and `AutoPickable` is that returning
empty.

Existing configs: the old `MinFileSizeMB`/`PreferredFileSizeMB` are not
migrated. A number meaning "300 MB" cannot be reinterpreted as a rate
without knowing the album it was aimed at, so carrying it over would be
inventing an intent nobody expressed. Those two fall back to no window,
which is the permissive default and what a fresh install gets;
`MaxFileSizeMB` carries over unchanged, because a ceiling on bytes
still means exactly what it did.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:10:51 -04:00
yonluandClaude Opus 5 e3d492e130 fix(downloads): call a request a request, and mark it with a bookmark
The feature was renamed to requests and the copy was not. The badge on
every Explore card and track row still offered "Want track X", the
album page's button read "Want this" / "Wanted", the artist page's
release menu said "Want This", and the Downloads empty state told the
user to look for a control by a name nothing rendered.

The `queued` badge is a bookmark rather than an hourglass. 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 list is
somewhere a user can leave one 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.

The backend's `'wanted'` request state is deliberately untouched: it is
a stored enum, not copy.

Also removes a dead duplicate branch in the badge's `render()`. The
first `if (this.actionable)` returned before the ring was built, so a
partly-held album that could still be requested drew a plus instead of
its progress arc.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:10:23 -04:00
yonluandClaude Opus 5 e6f30b6e43 fix(a11y): draw an unfavourited track as an outline, not a dimmer fill
`favCtrl.iconName` returned the solid glyph in both states, so "not a
favourite" was a filled heart in a duller colour and the only thing
separating the two states was hue. That fails outright for anyone who
cannot tell the two colours apart (WCAG 1.4.1), and reads as
"everything is a favourite" to everyone else.

`iconFor(favorited)` returns the outline or the fill, and the nine
`<wa-icon>` call sites split into the two cases they always were. The
three that show a *state* -- the mini player, the phone's now-playing
view, and the sidebar's marker for the favourites playlist itself --
pass it. The rest are context-menu items, which are actions rather than
states and take the outline `iconName` still returns.

`track-list` and `album-dropdown` already had this right, from inline
SVG paths of their own; this is the same rule for the call sites that
go through the icon library. `regular/star` is vendored to go with
`regular/heart`, which was already there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:10:06 -04:00
yonluandClaude Opus 5 351798fd66 fix(ui): spend a row's leftover space on the gaps, not the margins
The three card grids -- albums, artists, genres -- laid out with
`justify: 'center'` and a fixed 8px gap and padding, which gives the
row a fixed width and pushes everything left over to the two margins.
Measured on a 1440px window: cards 16px apart inside 78px of nothing
down each side. The outside was five times the inside.

`utils/grid-spacing.ts` computes one number instead, from what the row
could not spend on another card: the same value between two cards,
between two rows, and down each edge. That window now reads 30px
outside against 34px between, and it holds at any width.

The virtualizer has a word for this -- `justify: 'space-evenly'` with
`gap: 'auto'` -- and it cannot be used. It fits `floor(width /
cardWidth)` columns without reserving the gap it is about to need, so a
width one card short of exact leaves seven cards a pixel apart. On the
window above it would fit 7 columns with 1px between them. Deciding the
column count here is what puts a floor under the spacing.

Two consequences. The layout is rebuilt when the container width
changes the spacing rather than only when the cover size changes, so
each grid observes its own scroller -- keyed on the spacing, or every
pixel of a drag rebuilds a layout that comes out the same. And
`cover-grid`'s ScrollManager took `GRID_GAP`/`GRID_PADDING` as
constants, which stopped describing anything the moment the spacing
became elastic: it asks the host for the geometry now, since a scroll
position rebuilt from a stale 8px lands in the wrong row.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:09:50 -04:00
yonluandClaude Opus 5 40984f6086 fix(explore): let a slow archive node finish, and read the 404 back
Explore's album art was almost entirely missing: 5 of 24 cards on the
shelves had a cover, and those five were the ones already on disk.

The Cover Art Archive answers `front-250` with a 307 to an Internet
Archive storage node, and those nodes are 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, against a client timeout of 10. So every
live fetch died, and a timeout writes nothing and says nothing -- which
is why this reads as "Explore has no album art" rather than as a slow
upstream. The timeout is 30 s, chosen to clear the measured range: the
fetch is off the critical path, so waiting costs nothing and giving up
early costs the whole page.

Two things beside it, both found on the way.

`writeCache(mbid, nil)` has recorded "the archive has no art for this"
as an empty file since it 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 CAA on every render that asked about it. A third of the
shelves are art-less, so that was a third of the page spending a live
request to be told again what the last one said. `knownMissing` reads
it, on both the release-group and the release path.

And the frontend marked a failed fetch as permanently answered for the
session, so a timed-out cover never retried within it. It drops the
marker instead; a genuine 404 is now answered from disk, so re-asking
one costs nothing.

Measured after: 23 of 24.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MeQt5hgXg5YGoNZQ9ozG7L
2026-08-17 22:09:14 -04:00
44 changed files with 2000 additions and 741 deletions
+38
View File
@@ -3445,3 +3445,41 @@ their own output, and leave the previous snapshots in place.
makes it worth having: a restored snapshot resolves to `refresh` and makes it worth having: a restored snapshot resolves to `refresh` and
folds in the incremental listens since — minutes, against the 323 h a folds in the incremental listens since — minutes, against the 323 h a
rebuild was estimating. rebuild was estimating.
## A green release pipeline can ship an empty changelog (2026-08-18)
`conventional-changelog-conventionalcommits@10` is silently incompatible
with the writer `@semantic-release/release-notes-generator@14` depends on
(`conventional-changelog-writer@^8`). Every release note renders as a bare
`## 0.0.1 (date)` heading with **no sections and no commits under it**, no
step fails, and the release ships with an empty body.
It is pinned to `9` in `.gitea/workflows/release.yml` and in
`make release-dry`, which must stay identical. **Check the rendered notes,
never the exit code** — this is invisible to every tick in the pipeline.
## semantic-release needs push rights to the branch even when it never pushes to it (2026-08-18)
Core runs `git push --dry-run HEAD:<branch>` as a permission check, before
and independently of any plugin. With `@semantic-release/git` removed
nothing ever pushes to `main`, and the check still runs.
Two things this looked like and was not:
- **Not branch protection.** A `--dry-run` push does not reach the
pre-receive hook: pushing one to protected `main` with a write-scoped
token succeeds. So `main`'s `enable_push: false` is not what fails here.
- **A flat `403 Forbidden`, not Gitea's protection message.** That is the
tell. `PACKAGE_TOKEN` had package-write and repo-*read* — enough to
clone a private repo, so every other workflow was fine — and needed
`write:repository`.
## A tag-triggered workflow runs the workflow file at the *tagged* commit (2026-08-18)
Not the one on `main`. Moving `v0.0.0` onto a pre-merge commit ran that
commit's version of `homebrew-formula.yml`, which predated the `v0.0.0`
skip guard added in the same plan, and it pushed a `0.0.0` formula to the
public tap.
A guard added today does not protect a tag that points at yesterday. When
re-pointing a tag, check what the workflows looked like *there*.
@@ -1,358 +0,0 @@
# 017 — Releases that happen by themselves
> **Status: built, not yet run.** Phases 04 have landed on this branch;
> phase 5 is the merge itself and cannot be done until then. The old
> `v1.x` tags are already deleted from `origin`. Verified locally against
> a scratch remote: semantic-release computes **0.0.1** from these
> commits and renders correct sectioned notes.
>
> **One thing found by testing that no amount of reading would have
> caught.** `conventional-changelog-conventionalcommits@10` — the current
> release, and my first pin — is silently incompatible with the writer
> `release-notes-generator@14` depends on: the version is right, the tag
> is right, every step reports success, and the release body is a bare
> `## 0.0.1 (date)` heading with **nothing under it**. It is pinned to 9
> in both `release.yml` and `make release-dry`, with the reason written
> beside it. Four of my seven original pins were wrong majors besides;
> they were guesses, and `npm view` was the fix.
The goal in one sentence: **a merge to `main` computes the next version
from the commits it contains, cuts a tag and a Gitea release whose body
is the changelog, and every publishing channel builds that tag.** The
first release under this scheme is `v0.0.1`, and the five existing `v1.x`
tags go.
## What is there now
Measured, not remembered:
- **Five tags and zero releases.** `v1.3.0`, `v1.4.0`, `v1.4.1`,
`v1.5.0`, `v1.6.0` exist on `origin`;
`GET /api/v1/repos/yonlu/yellowjacket/releases` returns `[]`. So there
is no release page to preserve and nothing but the tags to remove.
- **`CHANGELOG.md` is stale and belongs to another repo.** Its newest
entry is `1.3.0` and every link in it points at
`github.com/onion-4-dinner/yellowjacket` — it was written by a
semantic-release run against a GitHub remote this project no longer
has.
- **`.releaserc.yml` is a complete semantic-release config that nothing
invokes**, which CLAUDE.md already says in as many words.
- **Root `package.json` is literally `{}`** — the stub left behind by
whatever was going to run it.
- The triggers today are: `arch-package` on **push to `main`**,
`homebrew-formula` on **`v*`**, `android-apk` on **`v*`**, `ci` on
every branch, `index-artifact` on cron/dispatch. So Arch publishes a
`git describe` version on every merge and the other two publish only
when a human remembers to push a tag.
## Decision 1 — semantic-release, with `exec` in place of the `github` plugin
**Revised: the first draft of this plan proposed a shell script and the
argument for it does not hold.** Recorded here rather than deleted,
because the reasoning is what the decision rests on.
What I said, and what checking it showed:
- *"The two plugins that would carry the work do not fit."* Half true.
`@semantic-release/github` genuinely does not speak Gitea's `/api/v1`
— but the replacement is **`@semantic-release/exec`**, which is
first-party, published 2026-06, and peer-deps `semantic-release >=24.1`.
Its `publishCmd` is one `curl` at the Gitea release endpoint with
`${nextRelease.notes}` as the body. The Gitea-shaped part of this is
five lines, and the part I proposed to hand-roll — parsing conventional
commits, ordering semver, rendering grouped notes — is the part with
the edge cases and none of it is Gitea-shaped at all.
- *"`@semantic-release/git` commits the changelog back to `main`, which
re-triggers everything."* True, and it is the one real risk — but it
is a two-line guard (skip the job when `HEAD`'s subject is
`chore(release):`), not a reason to write a version calculator. That
guard is needed under **either** design, since either one writes a
changelog commit.
- *"A Node dependency tree at the root of a Go repo."* The commitlint
precedent does not transfer. commitlint was a dependency to regex one
line; this is a dependency to do something with real complexity, it is
`npx`-only so nothing lands in the repo, and Node is already installed
in CI for the frontend.
- *"It cannot be told to produce `0.0.1`."* Wrong — that is a property
of which commits are in the range, not of the tool. Identical under
both designs. See below.
Note also that **`@saithodev/semantic-release-gitea` is a dead end** and
should not be reached for: last published 2022, depends on `got@10` and
`fs-extra@8`, and declares no peer dependency on semantic-release at all
— i.e. it is untested against anything since v19, against a core now at
v25. `exec` + `curl` is both simpler and maintained.
So `.releaserc.yml` stays, and its plugin list becomes five **first-party**
plugins, all published within the last six months:
| plugin | job |
| --- | --- |
| `commit-analyzer` | the version |
| `release-notes-generator` | the notes |
| `changelog` | writes `CHANGELOG.md` |
| `git` | commits it back |
| `exec` | `curl`s the Gitea release |
The `releaseRules` and `presetConfig` blocks already in the file are
kept verbatim — they are the same bump table `commit-check.sh` already
enforces the grammar for, and nothing about the project's commit
convention changes.
Two mechanical details that decide whether this works at all:
- **semantic-release pushes the tag itself**, as core behaviour, using
`repositoryUrl`. The remote here is `ssh://git@git.ljones.me:2222/…`,
which would need an SSH key in CI — so the run passes
`--repository-url "https://x-access-token:$PACKAGE_TOKEN@git.ljones.me/yonlu/yellowjacket.git"`
on the command line rather than committing a token to the config.
**That is also what satisfies Decision 2**: the tag push is attributed
to a real user, not to the Actions token.
- **The empty root `package.json` (`{}`) goes.** semantic-release does
not need one when `--repository-url` is explicit, and leaving a
package manifest at the root of a Go repo invites the npm plugin and
every tool that looks for one.
Invocation is pinned in the workflow, not installed into the repo:
```
npx --yes \
-p semantic-release@25 \
-p @semantic-release/commit-analyzer@14 \
-p @semantic-release/release-notes-generator@15 \
-p @semantic-release/changelog@6 \
-p @semantic-release/git@10 \
-p @semantic-release/exec@7 \
-p conventional-changelog-conventionalcommits@9 \
semantic-release --repository-url "…"
```
(Exact majors get pinned from `npm view` at implementation time;
`conventional-changelog-conventionalcommits` is in the list because both
the analyzer and the notes generator name that preset and neither
depends on it.)
## Decision 2 — how the publish workflows learn about the tag
**Gitea, like GitHub, does not start a workflow from a tag pushed by a
workflow's own token** (go-gitea#33123, and the forum thread it points
at). This is the one load-bearing unknown in the plan.
The remedy is to push the tag with a *user* PAT — `secrets.PACKAGE_TOKEN`
is already in this repo and already used by `arch-package` and
`android-apk` to clone and to publish — so the push is attributed to a
person and the `v*` triggers fire normally. That keeps the three publish
workflows completely unchanged in shape.
**It is verified in phase 5, not assumed.** The fallback, if it does not
fire, is an explicit `POST
/api/v1/repos/{owner}/{repo}/actions/workflows/{file}/dispatches` per
channel from the release job. That needs `workflow_dispatch` (with a
`version` input) added to `homebrew-formula.yml` and `arch-package.yml`;
`android-apk.yml` already has both. **Add those inputs in phase 3
regardless** — a hand-triggered rebuild of one channel is worth having
whether or not the fallback is needed.
The alternative — one `release.yml` with the three publishes as
`needs:` jobs — is rejected: it means either copying ~400 lines of
Android and Arch setup into it or relying on `workflow_call`, and it
puts every merge to `main` behind an up-to-60-minute Android build on a
runner with capacity 1.
## Decision 3 — 1.6.0 → 0.0.1 is a downgrade, and the answer is reinstall
**Decided: no version-code offset, no epoch. The version number stays
honest and existing installs are replaced by hand.** Every channel is a
downgrade and each declines differently, so what to expect:
- **Arch: no upgrade is offered, silently.** `pkgver()` derives from
`git describe`, so after the wipe it reads `0.0.1.rN.gHASH`, which
pacman orders *below* the `1.3.0.rN.*` in the registry. `pacman -R
yellowjacket && pacman -S yellowjacket` is the remedy. (`epoch=1` in
the PKGBUILD would have avoided it for one line — but an epoch can
never be removed, and it puts a permanent `1:` in front of every
version string this project will ever have.)
- **Homebrew: no upgrade is offered, silently.** Brew has no epoch at
all. `brew uninstall yellowjacket && brew install …`.
- **Android: a hard refusal.** `versionCode` is
`maj*10000 + min*100 + pat`, so `0.0.1` is **1** against the **10300**
an installed 1.3.0 carries, and the install fails with
`INSTALL_FAILED_VERSION_DOWNGRADE`. Uninstall first — **and that takes
the app's library and config with it**, which is the same data loss
`android-apk.yml`'s keystore guard exists to prevent, arrived at from
the other direction. The workflow's own `code -le 0` guard still passes
at 1, so nothing in CI stops or warns about this.
All three go in the release notes for `v0.0.1` and in
`packaging/homebrew/README.md` / `docs/android-release.md`, because a
channel that silently offers no upgrade is indistinguishable from a
broken pipeline six months from now.
## Landing exactly `v0.0.1`
Determinism comes from two things:
1. **Seed `v0.0.0` on `6fb7b5e`** (current `origin/main`) after wiping
the old tags. That is the floor, and the analyser's range starts
there.
2. **This branch carries no `feat:` commit.** Everything in it is
`ci:`/`docs:`/`chore:`/`build:`, plus at least one `fix:` — which is
honest, since wiring up release machinery that was configured and
never run *is* a fix. One patch-level commit in `v0.0.0..HEAD`
computes `0.0.1` and nothing else can.
This is a property of the commit range, not of the tool — it would have
been the same constraint under the shell script.
This is a real constraint on the branch, not an accounting trick: a
single `feat:` commit here makes the first release `v0.1.0`.
`v0.0.0` itself gets no release object — it is a floor, not a shipment.
## Decision 4 — what the release page carries
Four artifacts, and the fourth is the interesting one. Measured on this
machine rather than assumed:
| asset | built by | state |
| --- | --- | --- |
| `yellowjacket-<v>-android-arm64.apk` | `android-apk.yml` | already built, verified, signed |
| `yellowjacket-<v>-linux-amd64.tar.gz` | new job | binary + `.desktop` + icon |
| `yellowjacket-<v>-x86_64.pkg.tar.zst` | `arch-package.yml` | already built; free to attach |
| `yellowjacket-<v>-windows-amd64.zip` | new job | **compiles; has never been run** |
**macOS cannot be one of them.** `GOOS=darwin CGO_ENABLED=0` fails at
`wails/v3/pkg/mac: build constraints exclude all Go files` — the darwin
backend is Objective-C behind cgo, so a `.app` needs a macOS host and
the runner is a Linux container. That is precisely why the Homebrew
channel builds from source on the user's own Mac, and it stays the
answer for macOS.
**Windows is newly possible and should be labelled honestly.**
`GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build -tags production`
succeeds in 2.5 s and produces a 40 MB `.exe` — nothing in the audio,
database or webview path needs cgo on Windows (oto uses WinMM through
`x/sys`, sqlite is modernc's pure-Go driver, WebView2 is COM syscalls,
and MPRIS is `linux && !android`-tagged). But **compiling is not
running**: no Windows build of this app has ever been started, no CI tier
can exercise one, and `backend/system`'s `%LOCALAPPDATA%` path has never
resolved on a real machine. It ships marked as untested in the release
notes, or it does not ship — an unlabelled Windows download is a promise
nothing here can keep.
### The race the ordering creates
semantic-release runs **prepare** (changelog commit, tag push) before
**publish** (the `exec` curl that creates the release object). The tag
push is what starts the publishing workflows — so a fast one can reach
its upload step *before the release exists*, and
`POST /releases/{id}/assets` needs an id.
The capacity-1 runner serialises things enough that this would usually
work, which is the worst kind of bug. So each upload step **polls
`GET /api/v1/repos/…/releases/tags/{tag}` with a bounded retry** before
uploading, and fails loudly on timeout rather than skipping the asset.
That is ~8 lines of shell, shared by all three publishers.
## Phases
**Phase 0 — clear the ground.**
Delete `v1.3.0``v1.6.0` locally and on `origin`; push `v0.0.0` at
`6fb7b5e` — this is the floor semantic-release reads, and without it the
first release is `1.0.0` by its own rule. Delete the empty root
`package.json`. Truncate `CHANGELOG.md` to a header plus a line saying
history before `0.0.1` is in `git log` — the existing content is another
repo's links and cannot be repaired, only replaced, and the `changelog`
plugin prepends to whatever it finds.
**Phase 1 — `.releaserc.yml`.**
Swap `@semantic-release/github` for `@semantic-release/exec`, whose
`publishCmd` POSTs to
`/api/v1/repos/yonlu/yellowjacket/releases` with `tag_name`, `name` and
`body` taken from `${nextRelease.*}`. Keep `commit-analyzer`,
`release-notes-generator`, `changelog` and `git` exactly as written; fix
the `git` plugin's commit message so it passes `commit-check`
(`chore(release): ${nextRelease.version}` — the existing one already
does, but the trailing `${nextRelease.notes}` in the body is worth
keeping deliberate rather than incidental). `make release-dry` wraps
`semantic-release --dry-run` so the next version is answerable without
pushing anything.
`scripts/commit-check.sh`'s header already points at `.releaserc.yml`
for the type list and stays correct — that coupling survives this plan
rather than being broken by it.
**Phase 2 — `.gitea/workflows/release.yml`.**
On `push: branches: [main]`. Node 22, the pinned `npx` line from
Decision 1, `--repository-url` carrying `PACKAGE_TOKEN`. Concurrency
group `release-main` with `cancel-in-progress: false` — cutting a tag is
not a thing to cancel halfway.
The one guard that matters: **the job exits early when `HEAD`'s subject
starts `chore(release):`**, so the changelog commit the `git` plugin
pushes cannot re-enter this workflow. That is checked in shell rather
than left to `[skip ci]`, whose handling in Gitea is one more thing that
would have to be verified.
**Phase 3 — rewire the publish workflows.**
`arch-package.yml` moves from `push: branches: [main]` to
`push: tags: ['v*']` plus `workflow_dispatch`, so a merge no longer
publishes an untagged package. `homebrew-formula.yml` gains
`workflow_dispatch` with a `version` input and takes its version from
the input when there is no tag. `android-apk.yml` needs neither.
**Phase 3b — the assets.**
`scripts/release-asset.sh` is the shared uploader: wait for the release
by tag, then `POST /releases/{id}/assets?name=…`. `android-apk.yml` and
`arch-package.yml` each call it with the artifact they already built.
A new `desktop-assets` job — `push: tags: ['v*']`, in the same
`ubuntu:24.04` container `ci.yml` uses — builds the Linux binary via
`make build-prod` and the Windows one via the `CGO_ENABLED=0`
cross-compile, and uploads both. It is a separate job from the Arch one
because that runs in an `archlinux` container as an unprivileged
`makepkg` user, and grafting two unrelated builds onto it would make one
failure look like the other.
**Phase 4 — say that the upgrade is a reinstall, and that Windows is untried.**
No code change: a note in `packaging/homebrew/README.md`, one in
`docs/android-release.md`, the three-channel downgrade warning written
into the `v0.0.1` release notes, and a standing line in the notes
template marking the Windows asset unverified until someone runs it.
**Phase 5 — cut it and watch.** *(the only phase left)*
Merge, then verify with `gitea_ci` that (a) `release.yml` ran, seeded
`v0.0.0` and produced `v0.0.1`, (b) the release exists **with a non-empty
body** — check the body, not the exit code — and (c) **all four publish
workflows started from the tag**. If (c) is empty, that is Decision 2's
fallback and the `workflow_dispatch` inputs added in phase 3 are already
there to drive it.
The expected sequence on the merge is: `release.yml` seeds `v0.0.0`
(triggering nothing), releases `0.0.1`, and pushes both the changelog
commit and the tag — at which point `release.yml` fires a second time on
the changelog commit and exits at the `chore(release):` guard, while the
four `v*` workflows start. On a capacity-1 runner they will queue behind
each other, Android last and longest.
**Phase 6 — the documentation that will otherwise be wrong.**
CLAUDE.md's *Commits* section currently explains `.releaserc.yml` and
says nothing runs it; the CI section says there are five workflows and
that only `ci.yml` gates. Both change. `docs/android-release.md`
describes tags as hand-pushed. `make skill-check` fails on a `.pi/`
reference to a make target that does not exist, so `make release-dry`
gets documented or nothing does.
## Open questions for you
1. **Ship the Windows `.exe` or not?** It builds, and it has never run.
Marked-as-untested is the assumption; say if you would rather hold it
back until someone boots it.
Resolved: semantic-release stays, with `exec` in place of the `github`
plugin (Decision 1). Reinstalls are accepted, so no epoch and no
versionCode offset (Decision 3). The release carries the APK, a Linux
tarball, the Arch package and — pending (1) — a Windows zip; macOS is
not buildable here and stays a Homebrew-from-source channel (Decision 4).
`v0.0.0` has to be a real tag under this design — semantic-release reads
git tags for its floor and has no "treat absence as 0.0.0" knob that
also stops it calling the first release `1.0.0`.
@@ -0,0 +1,87 @@
# 017 — Releases that happen by themselves
**Shipped as `v0.0.1`.** A merge to `main` now reads the Conventional
Commits since the last tag, cuts the tag and the Gitea release whose body
is the generated changelog, and the four publishing workflows build that
tag and attach their artifacts. Nothing is released by hand.
## What it looks like now
`release.yml` on push to `main` → semantic-release → tag → four `v*`
workflows in parallel (serialised in practice by the capacity-1 runner):
| workflow | publishes | attaches |
| --- | --- | --- |
| `arch-package` | pacman registry | `…-x86_64.pkg.tar.zst` |
| `android-apk` | generic registry (Obtainium) | `…-android-arm64.apk` |
| `desktop-assets` | — | `…-linux-amd64.tar.gz` |
| `homebrew-formula` | the public tap | — (builds from source) |
Verified on the real thing: all five green, three assets on the release,
the tap at `0.0.1`, and the Obtainium `latest` URL serving 200.
## The five decisions, and what they cost
1. **semantic-release, not a shell script.** The first draft of this plan
proposed hand-rolling it and the argument did not survive checking:
`@semantic-release/exec` is first-party and current, and the
Gitea-shaped part is one `curl`. What I would have hand-rolled —
commit parsing, semver ordering, note rendering — is the part with the
edge cases and none of it is Gitea-shaped.
2. **`@saithodev/semantic-release-gitea` is a dead end** and was offered
before it was checked: last published 2022, `got@10`, and no peer
dependency on semantic-release at all.
3. **No `@semantic-release/git`.** `main` is protected, so a changelog
commit-back is rejected by the pre-receive hook — and would be
rejected *after* the tag was pushed, leaving a tagged release the run
reports as failed. The release page is the changelog;
`.release-notes.md` is a gitignored carrier and `CHANGELOG.md` is a
signpost.
4. **Versions restart at `0.0.1`**, a downgrade on every channel. No
`epoch`, no `versionCode` offset: both are permanent, a reinstall is
once. Documented in `packaging/homebrew/README.md` and
`docs/android-release.md`.
5. **No macOS and no Windows.** `GOOS=darwin CGO_ENABLED=0` fails at
`wails/v3/pkg/mac` and there is no macOS runner, so Homebrew-from-source
stays that channel. Windows cross-compiles in ~2.5 s and is withheld
because no build of it has ever been *run*.
## Four things that only showed up by running it
- **`conventional-changelog-conventionalcommits@10` renders empty
notes.** Silently: right version, right tag, every step green, and a
release body that is a bare `## 0.0.1 (date)` heading with nothing
beneath it. Held at `9`, in `release.yml` and `make release-dry`, with
the reason beside both. **Check the rendered notes, never the exit
code.**
- **semantic-release core dry-run-pushes to the release branch** as a
permission check, independently of any plugin. `PACKAGE_TOKEN` had
package-write and repo-*read* — enough to clone, not enough for this —
and it failed with a flat `403 Forbidden` that reads exactly like
branch protection. It is not: a `--dry-run` push never reaches the
pre-receive hook, which a one-line experiment settled. The token needed
`write:repository`.
- **The floor tag must go on `HEAD^`, not `HEAD`.** Seeded on the merge
commit itself it leaves nothing between the floor and HEAD, and
semantic-release correctly reports there is nothing to release. The
first run did exactly that and cut nothing.
- **A tag-triggered workflow runs from the tagged commit's tree.**
Moving `v0.0.0` back to `6fb7b5e` ran the *pre-merge* homebrew
workflow, which predates the `v0.0.0` skip guard, and pushed a `0.0.0`
formula to the public tap. Self-corrected at `0.0.1`. The corollary is
general: a guard added today does not protect a tag pointing at
yesterday.
## Two mechanisms confirmed, having been assumptions
- **A tag pushed with a user PAT does start the `v*` workflows**; one
pushed with the Actions token does not (go-gitea#33123). Both halves
are load-bearing and both were observed: the floor seed triggered
nothing, and the release tag triggered all four.
- **Tags are not protected** on this repo, only `main` — which is what
lets semantic-release tag at all.
## Left behind deliberately
`v0.0.0` stays on `origin` as the floor. It carries no release, and all
four publishers skip it by name.
+3 -2
View File
@@ -411,9 +411,10 @@ func (c *Config) SetDownloadPreferences(prefs download.AutoDownloadPrefs) error
formats = append(formats, string(f)) 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.MaxFileSizeMB = prefs.MaxSizeMB
c.Downloads.PreferredFileSizeMB = prefs.PreferredSizeMB
c.Downloads.AllowedFormats = formats c.Downloads.AllowedFormats = formats
if err := c.Save(); err != nil { if err := c.Save(); err != nil {
+28 -11
View File
@@ -34,13 +34,29 @@ type UserConfig struct {
// in one burst that every provider sees as a flood. // in one burst that every provider sees as a flood.
WantedBatch int `toml:"WantedBatch"` WantedBatch int `toml:"WantedBatch"`
// MinFileSizeMB, MaxFileSizeMB and PreferredFileSizeMB bound and // MinKbps, MaxKbps and PreferredKbps bound and nudge what auto-pick
// nudge what auto-pick (interactive or via the request list) may // (interactive or via the request list) may grab without asking.
// grab without asking. Zero on any of them is permissive: see // Zero on any of them is permissive: see AutoDownloadPrefs.
// AutoDownloadPrefs. //
MinFileSizeMB int `toml:"MinFileSizeMB"` // They replaced MinFileSizeMB / MaxFileSizeMB /
MaxFileSizeMB int `toml:"MaxFileSizeMB"` // PreferredFileSizeMB, which were megabytes and so said nothing
PreferredFileSizeMB int `toml:"PreferredFileSizeMB"` // 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 // AllowedFormats restricts auto-pick to these formats. Empty means
// no restriction. Values are Format strings ("flac", "mp3", ...). // no restriction. Values are Format strings ("flac", "mp3", ...).
@@ -56,10 +72,11 @@ func (c *UserConfig) AutoDownloadPrefs() AutoDownloadPrefs {
} }
return AutoDownloadPrefs{ return AutoDownloadPrefs{
MinSizeMB: c.MinFileSizeMB, MinKbps: c.MinKbps,
MaxSizeMB: c.MaxFileSizeMB, MaxKbps: c.MaxKbps,
PreferredSizeMB: c.PreferredFileSizeMB, PreferredKbps: c.PreferredKbps,
AllowedFormats: formats, MaxSizeMB: c.MaxFileSizeMB,
AllowedFormats: formats,
} }
} }
+8 -10
View File
@@ -236,6 +236,12 @@ func (m *Manager) AutoPickable(dl Download, ranked []Candidate) bool {
return AutoPickable(dl, ranked, m.preferences()) 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 // Reload rebuilds every provider from stored config. Called at startup
// and after any provider settings change. // and after any provider settings change.
// //
@@ -612,16 +618,8 @@ func (m *Manager) Attempt(
return false, "", err return false, "", err
} }
if !m.AutoPickable(dl, ranked) { if veto := m.AutoPickVeto(dl, ranked); veto != "" {
best := ranked[0] return false, veto, nil
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 err := m.store.CreateDownload(ctx, dl); err != nil { if err := m.store.CreateDownload(ctx, dl); err != nil {
+44 -6
View File
@@ -218,8 +218,17 @@ func TestManagerEndToEndAutoPick(t *testing.T) {
}, "staging was never released, or the library was never rescanned") }, "staging was never released, or the library was never rescanned")
} }
// An ambiguous result set must park for the user rather than guess. // Two equally good copies are not an ambiguity — they are a spare.
func TestManagerWaitsWhenAmbiguous(t *testing.T) { //
// 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() t.Parallel()
f := newManagerFixture(t) f := newManagerFixture(t)
@@ -237,11 +246,41 @@ func TestManagerWaitsWhenAmbiguous(t *testing.T) {
t.Fatalf("Start: %v", err) t.Fatalf("Start: %v", err)
} }
if f.manager.AutoPickable(dl, ranked) { if veto := f.manager.AutoPickVeto(dl, ranked); veto != "" {
t.Fatal("two equivalent candidates must not auto-pick") 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 { if a.GrabCalls != 0 || b.GrabCalls != 0 {
t.Errorf( t.Errorf(
"grabs happened without a pick: a=%d b=%d", "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) t.Errorf("stored request id = %s, want %s", stored.ID, dl.ID)
} }
// The user picks the second one explicitly.
if err := f.manager.Pick( if err := f.manager.Pick(
context.Background(), dl.ID, ranked[1].ID, context.Background(), dl.ID, ranked[1].ID,
); err != nil { ); err != nil {
+313 -77
View File
@@ -1,6 +1,7 @@
package download package download
import ( import (
"fmt"
"math" "math"
"sort" "sort"
"strings" "strings"
@@ -34,38 +35,102 @@ const (
weightArtistFit = 0.12 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 ( const (
weightFormat = 0.42 weightFormat = 0.42
weightBitrate = 0.23 weightBitrate = 0.23
weightHealth = 0.20 weightHealth = 0.20
weightPriority = 0.10 weightPriority = 0.10
weightSizeFit = 0.05 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 // unanchoredCap bounds the match score of a free-text request. Without
// an MBID there is no tracklist to be right about, so a confident- // 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. // looking score would be a lie — and auto-pick keys off this.
const unanchoredCap = 0.65 const unanchoredCap = 0.65
// AutoDownloadPrefs gates and scores what AutoPickable may choose // AutoDownloadPrefs gates and scores what AutoPickable may choose
// without asking. Zero values are permissive: no size window and no // without asking. Zero values are permissive: no bitrate window, no
// format restriction. // 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 { type AutoDownloadPrefs struct {
// MinSizeMB and MaxSizeMB bound what auto-pick will grab. Zero // MinKbps and MaxKbps bound the average bitrate auto-pick will
// means no bound on that side. A candidate outside the window is // grab. Zero means no bound on that side. A candidate outside the
// filtered out of auto-pick entirely, not merely scored down — a // window is filtered out of auto-pick entirely, not merely scored
// tiny "sampler" torrent or a boxset ten times the expected size is // down — a 96 kbps rip of the right album is not a worse copy the
// usually the wrong thing entirely, not a worse copy of the right // user might accept, it is one they said not to take unattended.
// thing. //
MinSizeMB int `json:"minSizeMb"` // For reference: 320 is the top of MP3, ~5001000 is FLAC depending
MaxSizeMB int `json:"maxSizeMb"` // 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 // PreferredKbps nudges the score toward a target rate within the
// min/max window (a lossless rip and a heavily-padded lossless rip // window, and breaks the tie when several candidates are equally
// can both pass the window). Zero disables the nudge; sizeFit then // good matches. Zero disables the nudge; bitrateFit then returns a
// returns a neutral value that does not affect ranking. // neutral value that does not affect ranking.
PreferredSizeMB int `json:"preferredSizeMb"` 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 // AllowedFormats restricts auto-pick to candidates whose audio
// files are all in one of these formats. Empty means no // 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 // eligible reports whether a candidate may be auto-picked under these
// preferences: within the size window (when set) and, when a format // preferences: inside the bitrate window and the size ceiling (when
// list is given, every audio file in an allowed format. // set) and, when a format list is given, every audio file in an
func (p AutoDownloadPrefs) eligible(c Candidate) bool { // 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 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 { if p.MaxSizeMB > 0 && c.TotalSize > int64(p.MaxSizeMB)*bytesPerMB {
return false 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 { if len(p.AllowedFormats) == 0 {
return true return true
} }
@@ -107,11 +186,14 @@ func (p AutoDownloadPrefs) eligible(c Candidate) bool {
// filter returns only the candidates these preferences allow to be // filter returns only the candidates these preferences allow to be
// auto-picked, in the same (already ranked) order. // 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)) out := make([]Candidate, 0, len(ranked))
for _, c := range ranked { for _, c := range ranked {
if p.eligible(c) { if p.eligible(c, runtimeMillis) {
out = append(out, c) out = append(out, c)
} }
} }
@@ -119,32 +201,116 @@ func (p AutoDownloadPrefs) filter(ranked []Candidate) []Candidate {
return out return out
} }
// sizeFit scores how close totalSize is to PreferredSizeMB, 0..1, // bitrateFit scores how close a candidate's average bitrate is to
// falling off linearly as the size doubles or halves away from it. // PreferredKbps, falling off linearly as it doubles or halves away
// Returns a neutral 0.5 when no preference is set, so the absence of a // from it.
// preference does not bias ranking. //
func (p AutoDownloadPrefs) sizeFit(totalSize int64) float64 { // 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 ( 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 return neutral
} }
preferred := float64(p.PreferredSizeMB) * bytesPerMB kbps := candidateKbps(c, runtimeMillis)
ratio := float64(totalSize) / preferred if kbps <= 0 {
return neutral
}
ratio := kbps / float64(p.PreferredKbps)
if ratio < 1 { if ratio < 1 {
ratio = 1 / ratio ratio = 1 / ratio
} }
// ratio is now >= 1: 1.0 is an exact match, 2.0 is double or half // 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. // the preferred rate, where the closeness term reaches 0.
fit := 1 - (ratio - 1) 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. // 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.Files = mergeMatched(c.Files, matched)
c.Match = scoreMatch(dl, c, audio, titleFit) 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 c.Score = weightMatch*c.Match.Overall + weightQuality*c.Quality.Overall
@@ -279,11 +447,12 @@ func scoreQuality(
audio []CandidateFile, audio []CandidateFile,
priority int, priority int,
prefs AutoDownloadPrefs, prefs AutoDownloadPrefs,
runtimeMillis int64,
) QualityScore { ) QualityScore {
q := QualityScore{ q := QualityScore{
Health: clamp01(c.Health), Health: clamp01(c.Health),
Priority: clamp01(float64(priority) / 100.0), Priority: clamp01(float64(priority) / 100.0),
SizeFit: prefs.sizeFit(c.TotalSize), BitrateFit: prefs.bitrateFit(c, runtimeMillis),
} }
if len(audio) == 0 { if len(audio) == 0 {
@@ -310,11 +479,13 @@ func scoreQuality(
q.FormatRank = worst q.FormatRank = worst
q.Bitrate = bitrateScore(audio) q.Bitrate = bitrateScore(audio)
q.Overall = weightFormat*q.FormatRank + wFormat, wBitrate, wHealth, wPriority, wFit := qualityWeights(prefs)
weightBitrate*q.Bitrate +
weightHealth*q.Health + q.Overall = wFormat*q.FormatRank +
weightPriority*q.Priority + wBitrate*q.Bitrate +
weightSizeFit*q.SizeFit wHealth*q.Health +
wPriority*q.Priority +
wFit*q.BitrateFit
if q.Mixed { if q.Mixed {
q.Overall *= 0.9 q.Overall *= 0.9
@@ -444,6 +615,19 @@ func Rank(
return out[i].Match.Overall > out[j].Match.Overall 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 { if out[i].Quality.Priority != out[j].Quality.Priority {
return 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 return out
} }
// AutoPickable reports whether a ranked list has a clear enough winner // Auto-pick gates. Named rather than inlined because AutoPickVeto
// to grab without asking. It demands an anchored request, a high match, // reports which of them refused, and a number in a sentence the user
// decent quality, and daylight between first and second place — if two // reads should be the same number the decision used.
// candidates are close, the choice is the user's. const (
func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool { minMatch = 0.85
const ( minQuality = 0.5
minMatch = 0.85 )
minQuality = 0.5
minLead = 0.08
)
if !dl.Anchored() || len(ranked) == 0 { // AutoPickable reports whether a ranked list has a candidate worth
return false // 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: // 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 // is exactly the evidence a wrong-album candidate also has. This
// matters most for the request list, where nobody is watching. // matters most for the request list, where nobody is watching.
if len(dl.Expected) == 0 { 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 // The guardrails apply before the match and quality checks: a
// candidate outside the allowed size or format is not a worse // candidate outside the allowed bitrate, size or format is not a
// choice, it is not a choice auto-pick may make at all, so it must // worse choice, it is not a choice auto-pick may make at all, so it
// not count as "the winner" nor as "second place" for the lead // must not count as "the winner" either.
// check below. eligible := prefs.filter(ranked, dl.runtimeMillis())
eligible := prefs.filter(ranked)
if len(eligible) == 0 { 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] 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 { if best.Quality.Overall < minQuality {
return false 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 // mergeMatched copies MatchedTo assignments from the audio-only slice
+360 -57
View File
@@ -1,6 +1,34 @@
package download 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. // okComputer is the reference request used across ranking tests.
func okComputer() Download { func okComputer() Download {
@@ -8,11 +36,15 @@ func okComputer() Download {
ReleaseMBID: "mbid-ok-computer", ReleaseMBID: "mbid-ok-computer",
Artist: "Radiohead", Artist: "Radiohead",
Album: "OK Computer", 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{ Expected: []ExpectedTrack{
{Position: 1, Title: "Airbag"}, {Position: 1, Title: "Airbag", LengthMillis: trackMillis},
{Position: 2, Title: "Paranoid Android"}, {Position: 2, Title: "Paranoid Android", LengthMillis: trackMillis},
{Position: 3, Title: "Subterranean Homesick Alien"}, {Position: 3, Title: "Subterranean Homesick Alien", LengthMillis: trackMillis},
{Position: 4, Title: "Exit Music (For a Film)"}, {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() t.Parallel()
dl := okComputer() 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() t.Parallel()
twin := best twin := best
twin.ID = "twin" twin.ID = "twin"
if AutoPickable(dl, []Candidate{best, twin}, AutoDownloadPrefs{}) { if !AutoPickable(dl, []Candidate{best, twin}, AutoDownloadPrefs{}) {
t.Error("identical candidates must not auto-pick") 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) { func TestAutoDownloadPrefsEligible(t *testing.T) {
t.Parallel() t.Parallel()
flacCandidate := candidateFor("c", allTitles(), ".flac", 30_000_000) flacCandidate := kbpsCandidate("c", ".flac", 900)
flacCandidate.Files = AnnotateFiles(flacCandidate.Files) mp3Candidate := kbpsCandidate("c", ".mp3", 128)
flacCandidate.TotalSize = 300 * mb
mp3Candidate := candidateFor("c", allTitles(), ".mp3", 3_000_000)
mp3Candidate.Files = AnnotateFiles(mp3Candidate.Files)
mp3Candidate.TotalSize = 30 * mb
tests := []struct { tests := []struct {
name string name string
@@ -321,18 +350,25 @@ func TestAutoDownloadPrefsEligible(t *testing.T) {
}{ }{
{"zero value is permissive", AutoDownloadPrefs{}, flacCandidate, true}, {"zero value is permissive", AutoDownloadPrefs{}, flacCandidate, true},
{ {
"within min/max window", "within the bitrate window",
AutoDownloadPrefs{MinSizeMB: 100, MaxSizeMB: 500}, AutoDownloadPrefs{MinKbps: 320, MaxKbps: 1200},
flacCandidate, true, flacCandidate, true,
}, },
{ {
"below minimum", "below the minimum bitrate",
AutoDownloadPrefs{MinSizeMB: 400}, AutoDownloadPrefs{MinKbps: 500},
mp3Candidate, false,
},
{
"above the maximum bitrate",
AutoDownloadPrefs{MaxKbps: 500},
flacCandidate, false, flacCandidate, false,
}, },
{ {
"above maximum", // The ceiling is bytes, not a rate, and it is the guard
AutoDownloadPrefs{MaxSizeMB: 200}, // that still works when the bitrate cannot be worked out.
"above the hard size ceiling",
AutoDownloadPrefs{MaxSizeMB: 50},
flacCandidate, false, flacCandidate, false,
}, },
{ {
@@ -351,57 +387,131 @@ func TestAutoDownloadPrefsEligible(t *testing.T) {
t.Run(tt.name, func(t *testing.T) { t.Run(tt.name, func(t *testing.T) {
t.Parallel() 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) 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) { func TestAutoDownloadPrefsFilter(t *testing.T) {
t.Parallel() t.Parallel()
small := candidateFor("small", allTitles(), ".flac", 10_000_000) lossy := kbpsCandidate("lossy", ".mp3", 128)
small.TotalSize = 50 * mb lossless := kbpsCandidate("lossless", ".flac", 900)
big := candidateFor("big", allTitles(), ".flac", 30_000_000) prefs := AutoDownloadPrefs{MinKbps: 500}
big.TotalSize = 500 * mb
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 != "lossless" {
if len(filtered) != 1 || filtered[0].ID != "big" {
t.Errorf("filter() = %v, want only the in-window candidate", filtered) t.Errorf("filter() = %v, want only the in-window candidate", filtered)
} }
} }
func TestAutoDownloadPrefsSizeFit(t *testing.T) { func TestAutoDownloadPrefsBitrateFit(t *testing.T) {
t.Parallel() t.Parallel()
const neutral = 0.5 const neutral = 0.5
tests := []struct { tests := []struct {
name string name string
prefs AutoDownloadPrefs prefs AutoDownloadPrefs
totalSize int64 c Candidate
want float64 want float64
}{ }{
{"no preference is neutral", AutoDownloadPrefs{}, 300 * mb, neutral}, {
"no preference is neutral",
AutoDownloadPrefs{},
kbpsCandidate("c", ".flac", 900), neutral,
},
{ {
"exact match scores 1", "exact match scores 1",
AutoDownloadPrefs{PreferredSizeMB: 300}, AutoDownloadPrefs{PreferredKbps: 320},
300 * mb, 1.0, kbpsCandidate("c", ".mp3", 320), 1.0,
}, },
{ {
"double the preferred size scores 0", // The floor is neutral, not zero: this term carries 0.40
AutoDownloadPrefs{PreferredSizeMB: 300}, // of the quality score once a preference is set, and a
600 * mb, 0.0, // 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", "half the preferred rate falls to the neutral floor",
AutoDownloadPrefs{PreferredSizeMB: 300}, AutoDownloadPrefs{PreferredKbps: 320},
150 * mb, 0.0, 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.Run(tt.name, func(t *testing.T) {
t.Parallel() t.Parallel()
if got := tt.prefs.sizeFit(tt.totalSize); got != tt.want { // The last case deliberately withholds the runtime.
t.Errorf("sizeFit(%d) = %f, want %f", tt.totalSize, got, tt.want) 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 // An otherwise-perfect candidate must not auto-pick when it falls
// outside the configured size guard: the guardrail applies before the // outside the configured guardrails: they apply before the match and
// match/quality/lead checks, not as one more input averaged into them. // quality checks, not as one more input averaged into them.
func TestAutoPickableRejectsCandidateOutsideSizeGuard(t *testing.T) { func TestAutoPickableRejectsCandidateOutsideTheGuardrails(t *testing.T) {
t.Parallel() t.Parallel()
dl := okComputer() dl := okComputer()
best := Score(dl, candidateFor("a", allTitles(), ".flac", 30_000_000), 50, AutoDownloadPrefs{}) best := Score(dl, kbpsCandidate("a", ".flac", 900), 50, AutoDownloadPrefs{})
best.TotalSize = 500 * mb
if !AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{}) { if !AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{}) {
t.Fatal("expected this candidate to be auto-pickable with no guardrails") 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) { if AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{MaxSizeMB: 1}) {
t.Error("candidate outside the size guard must not auto-pick") 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)
} }
} }
+18 -2
View File
@@ -36,13 +36,24 @@ func newServiceFixture(t *testing.T) serviceFixture {
// assertion read it; the second is that same goroutine still writing // assertion read it; the second is that same goroutine still writing
// into `t.TempDir()` after the test returned. One cause, two shapes. // 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 // 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 // is no goroutine to be slow, so the tests state what they mean
// ("the request exists, in this state") without a timing assumption // ("the request exists, in this state") without a timing assumption
// underneath. A test that does want the download has `managerFixture` // underneath. A test that does want the download has `managerFixture`
// and sets its own preferences. // 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} return serviceFixture{managerFixture: mf, svc: svc}
} }
@@ -182,6 +193,11 @@ func TestManualDownloadSatisfiesRequestOnSuccess(t *testing.T) {
f := newServiceFixture(t) f := newServiceFixture(t)
ctx := context.Background() 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") provider := fakeWithAlbum(1, "source", ".flac")
f.manager.installProvider(Config{ID: 1, Priority: 50}, provider) f.manager.installProvider(Config{ID: 1, Priority: 50}, provider)
+6 -1
View File
@@ -302,7 +302,12 @@ type QualityScore struct {
Bitrate float64 `json:"bitrate"` Bitrate float64 `json:"bitrate"`
Health float64 `json:"health"` // seeders, free slots Health float64 `json:"health"` // seeders, free slots
Priority float64 `json:"priority"` // user's per-provider preference 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, // Mixed marks a candidate whose files are not all the same format,
// which usually means a hand-assembled folder rather than a rip. // which usually means a hand-assembled folder rather than a rip.
+49 -5
View File
@@ -25,8 +25,26 @@ const (
// where cached cover art thumbnails are stored. // where cached cover art thumbnails are stored.
thumbnailDir = CoverArtCacheDirName thumbnailDir = CoverArtCacheDirName
// thumbnailTimeout is the HTTP timeout for fetching a thumbnail. // thumbnailTimeout is the HTTP timeout for fetching a thumbnail,
thumbnailTimeout = 10 * time.Second // 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 1416 s
// and a failing one 1317 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 is the maximum image size to cache (2 MB).
thumbnailMaxSize = 2 * 1024 * 1024 thumbnailMaxSize = 2 * 1024 * 1024
@@ -97,6 +115,20 @@ func (p *CoverArtProxy) GetThumbnail(
return "" 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). // Source 3: fetch from Cover Art Archive (slow, cached to disk).
url := CoverArtGroupURL(releaseGroupMBID) url := CoverArtGroupURL(releaseGroupMBID)
data, cacheable, err := p.fetch(url) data, cacheable, err := p.fetch(url)
@@ -177,8 +209,9 @@ func (p *CoverArtProxy) GetCandidateThumbnail(
} }
} }
// Network fetch on release group. // Network fetch on release group — unless a previous one was told
if releaseGroupMBID != "" { // there is none. See `knownMissing`.
if releaseGroupMBID != "" && !p.knownMissing(releaseGroupMBID) {
url := CoverArtGroupURL(releaseGroupMBID) url := CoverArtGroupURL(releaseGroupMBID)
data, cacheable, err := p.fetch(url) data, cacheable, err := p.fetch(url)
@@ -194,7 +227,7 @@ func (p *CoverArtProxy) GetCandidateThumbnail(
} }
// Network fetch on release (fallback). // Network fetch on release (fallback).
if releaseMBID != "" { if releaseMBID != "" && !p.knownMissing(releaseMBID) {
url := CoverArtURL(releaseMBID) url := CoverArtURL(releaseMBID)
data, cacheable, err := p.fetch(url) data, cacheable, err := p.fetch(url)
@@ -285,6 +318,17 @@ func (p *CoverArtProxy) cachePath(mbid string) string {
return filepath.Join(p.cacheDir, mbid+".jpg") 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 { func (p *CoverArtProxy) readCache(mbid string) string {
path := p.cachePath(mbid) path := p.cachePath(mbid)
+141 -15
View File
@@ -289,8 +289,13 @@ func (l *Library) scanInternal(
l.mu.Unlock() 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( workerCount := resolveScanWorkerCount(
ScanConcurrencyAuto, l.conf.ScanConcurrency,
libraryPath, libraryPath,
) )
@@ -300,6 +305,10 @@ func (l *Library) scanInternal(
"libraryName", libraryName, "libraryName", libraryName,
"libraryPath", libraryPath, "libraryPath", libraryPath,
"workers", workerCount, "workers", workerCount,
"mode", l.conf.ScanConcurrency,
"device", diskProfile.Device,
"rotational", diskProfile.Rotational,
"queueDepth", diskProfile.QueueDepth,
) )
// Helper to build a ScanProgress with library identification. // Helper to build a ScanProgress with library identification.
@@ -818,7 +827,7 @@ func (l *Library) scanInternal(
g := new(errgroup.Group) g := new(errgroup.Group)
g.SetLimit(workerCount) g.SetLimit(workerCount)
for work := range workChan { for work := range readaheadWork(scanCtx, workChan, diskProfile) {
g.Go(func() error { g.Go(func() error {
if err := l.waitIfPaused(scanCtx); err != nil { if err := l.waitIfPaused(scanCtx); err != nil {
return err return err
@@ -1285,9 +1294,101 @@ func surveyAudioFiles(
return count, maxModTime return count, maxModTime
} }
// hddWorkerCount is the maximum number of concurrent extraction // How many extraction workers a spinning disk gets, and why it is two
// workers when the library resides on a spinning disk. // numbers rather than one.
const hddWorkerCount = 2 //
// 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/<dev>/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 // resolveScanWorkerCount returns the number of concurrent
// extraction workers based on the configured concurrency mode // extraction workers based on the configured concurrency mode
@@ -1296,20 +1397,45 @@ func resolveScanWorkerCount(
mode ScanConcurrency, mode ScanConcurrency,
libraryPath string, libraryPath string,
) int { ) 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 { switch mode {
case ScanConcurrencySSD: case ScanConcurrencySSD:
return goruntime.NumCPU() spinning = false
case ScanConcurrencyHDD: case ScanConcurrencyHDD:
return min(hddWorkerCount, goruntime.NumCPU()) spinning = true
default: // auto case ScanConcurrencyAuto:
if system.IsRotationalDisk(libraryPath) {
return min(
hddWorkerCount, goruntime.NumCPU(),
)
}
return goruntime.NumCPU()
} }
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. // scanWork represents a file to be processed by a worker.
+38
View File
@@ -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,
)
}
+13
View File
@@ -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) {}
+122
View File
@@ -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")
}
}
+134 -57
View File
@@ -16,32 +16,107 @@ var errNoBlockDevice = errors.New(
"no matching block device found", "no matching block device found",
) )
// IsRotationalDisk reports whether the block device backing the // DiskProfile is what the scanner needs to know about the device a
// given path is a rotational (spinning) disk. Detection uses the // library sits on. Both fields are about the same question — how many
// Linux sysfs interface at /sys/block/<dev>/queue/rotational. // reads should be in flight at once — and they answer different halves
// Returns false on any error (assumes SSD). // of it, so they travel together rather than as two probes.
func IsRotationalDisk(path string) bool { type DiskProfile struct {
dev, err := deviceForPath(path) // Device is the whole-disk kernel name ("sdb"), or "" when the
if err != nil { // path could not be resolved to one.
return false Device string
}
rotational, err := os.ReadFile( // Rotational is /sys/block/<dev>/queue/rotational: true for a
filepath.Join( // spinning disk, where a seek costs milliseconds.
"/sys/block", dev, "queue", "rotational", Rotational bool
),
)
if err != nil {
return false
}
return strings.TrimSpace(string(rotational)) == "1" // QueueDepth is /sys/block/<dev>/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 // Queues reports whether the drive can reorder outstanding commands.
// device name (e.g. "sda") by matching the device major:minor //
// from stat(2) against /sys/block/ entries. // An unknown depth (0) counts as queueing: everything that does not
func deviceForPath(path string) (string, error) { // 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/<dev>/<parts...>,
// 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/<major>:<minor>, 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 var st syscall.Stat_t
if err := syscall.Stat(path, &st); err != nil { if err := syscall.Stat(path, &st); err != nil {
return "", fmt.Errorf( return "", fmt.Errorf(
@@ -49,48 +124,50 @@ func deviceForPath(path string) (string, error) {
) )
} }
// Extract major and minor device numbers. // Linux packs dev_t as 12 bits of major and 20 of minor, split
major := (st.Dev >> 8) & 0xff // across the word. Masking the low byte of each — which is what
minor := st.Dev & 0xff // 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. link := filepath.Join(
entries, err := os.ReadDir("/sys/block") "/sys/dev/block",
strconv.FormatUint(major, 10)+":"+
strconv.FormatUint(minor, 10),
)
target, err := filepath.EvalSymlinks(link)
if err != nil { if err != nil {
return "", fmt.Errorf( return "", fmt.Errorf(
"could not read /sys/block: %w", err, "%w: %s (%w)", errNoBlockDevice, link, err,
) )
} }
majorStr := strconv.FormatUint(major, 10) // A partition's directory sits inside its disk's, and only the
devStr := majorStr + ":" + // disk carries `queue`. Climb at most one level: sysfs nests a
strconv.FormatUint(minor, 10) // partition exactly one deep under its disk.
name := filepath.Base(target)
for _, entry := range entries { if _, err := os.Stat(filepath.Join(target, "queue")); err != nil {
devFile := filepath.Join( name = filepath.Base(filepath.Dir(target))
"/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
}
} }
return "", fmt.Errorf( if name == "" || name == "." || name == string(filepath.Separator) {
"%w for %s", errNoBlockDevice, devStr, 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))
} }
+25
View File
@@ -2,9 +2,34 @@
package system 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 // IsRotationalDisk reports whether the block device backing the
// given path is a rotational (spinning) disk. On non-Linux // given path is a rotational (spinning) disk. On non-Linux
// platforms this always returns false (assumes SSD). // platforms this always returns false (assumes SSD).
func IsRotationalDisk(_ string) bool { func IsRotationalDisk(_ string) bool {
return false 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{}
}
+66
View File
@@ -0,0 +1,66 @@
import { test, expect } from '../support/fixtures.js';
/**
* The queue button says whether the queue is open.
*
* It used to look identical in both states, so the only way to tell
* what pressing it would do was to look at the other side of the window
* and infer it — and for anyone not looking at all there was nothing to
* infer from: no `aria-expanded`, no `aria-controls`, no pressed state.
*
* The state is reflected *from the panel*, not kept beside the click,
* because the button is not the only thing that opens the queue —
* `now-playing-view` sets the same attribute, since it hides the bar
* this button lives in. A flag maintained by the click handler would be
* right until something else opened the panel and then quietly wrong,
* which is the second test here.
*/
test.describe('the queue toggle', () => {
test('reports open and closed, and names what it controls', async ({
app,
}) => {
const toggle = app.locator('#queue-button');
await expect(toggle).toHaveAttribute('aria-controls', 'queue-panel');
await expect(toggle).toHaveAttribute('aria-expanded', 'false');
await toggle.click();
await expect(toggle).toHaveAttribute('aria-expanded', 'true');
// The state is not only in the accessibility tree: a control that
// announces a state it does not draw is half a fix.
//
// Background rather than colour, because the pointer is still on
// the button after the click and `:hover` paints it the same accent
// the open state does -- so a colour comparison here passes on the
// broken build and proves nothing.
const [open, closed] = await toggle.evaluate((el) => {
const now = getComputedStyle(el).backgroundColor;
el.setAttribute('aria-expanded', 'false');
const shut = getComputedStyle(el).backgroundColor;
el.setAttribute('aria-expanded', 'true');
return [now, shut];
});
expect(open).not.toBe(closed);
await toggle.click();
await expect(toggle).toHaveAttribute('aria-expanded', 'false');
});
test('follows the panel when something else opens it', async ({ app }) => {
const toggle = app.locator('#queue-button');
await expect(toggle).toHaveAttribute('aria-expanded', 'false');
// Exactly what `now-playing-view`'s queue button does.
await app.evaluate(() =>
document.getElementById('queue-panel')?.setAttribute('open', ''),
);
await expect(toggle).toHaveAttribute('aria-expanded', 'true');
});
});
+2 -2
View File
@@ -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 * 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 * 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 * 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 * 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 * 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') const ds = document.querySelector('explore-album-details')
?.shadowRoot; ?.shadowRoot;
const btn = [...(ds?.querySelectorAll('wa-button') ?? [])].find( const btn = [...(ds?.querySelectorAll('wa-button') ?? [])].find(
(b) => /Wanted/.test(b.textContent ?? ''), (b) => /Requested/.test(b.textContent ?? ''),
); );
return btn?.querySelector('wa-icon')?.getAttribute('name') ?? ''; return btn?.querySelector('wa-icon')?.getAttribute('name') ?? '';
@@ -3,28 +3,52 @@
/** /**
* AutoDownloadPrefs gates and scores what AutoPickable may choose * AutoDownloadPrefs gates and scores what AutoPickable may choose
* without asking. Zero values are permissive: no size window and no * without asking. Zero values are permissive: no bitrate window, no
* format restriction. * 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 { export interface AutoDownloadPrefs {
/** /**
* MinSizeMB and MaxSizeMB bound what auto-pick will grab. Zero * MinKbps and MaxKbps bound the average bitrate auto-pick will
* means no bound on that side. A candidate outside the window is * grab. Zero means no bound on that side. A candidate outside the
* filtered out of auto-pick entirely, not merely scored down — a * window is filtered out of auto-pick entirely, not merely scored
* tiny "sampler" torrent or a boxset ten times the expected size is * down — a 96 kbps rip of the right album is not a worse copy the
* usually the wrong thing entirely, not a worse copy of the right * user might accept, it is one they said not to take unattended.
* thing. *
* For reference: 320 is the top of MP3, ~5001000 is FLAC depending
* on the material, and anything under ~128 is a transcode.
*/ */
"minSizeMb": number; "minKbps": number;
"maxSizeMb": number; "maxKbps": number;
/** /**
* PreferredSizeMB nudges the score toward a target size within the * PreferredKbps nudges the score toward a target rate within the
* min/max window (a lossless rip and a heavily-padded lossless rip * window, and breaks the tie when several candidates are equally
* can both pass the window). Zero disables the nudge; sizeFit then * good matches. Zero disables the nudge; bitrateFit then returns a
* returns a neutral value that does not affect ranking. * 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 * AllowedFormats restricts auto-pick to candidates whose audio
@@ -487,9 +511,13 @@ export interface QualityScore {
"priority": number; "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, * Mixed marks a candidate whose files are not all the same format,
+11
View File
@@ -207,6 +207,17 @@ body div.sidebar {
color: var(--yj-accent, #ffd43b); color: var(--yj-accent, #ffd43b);
} }
/* An open queue is a state this button can be in, and it used to
look exactly like the closed one -- so the only way to tell what
pressing it would do was to look at the other side of the window
and infer it. `aria-expanded` is the same fact for anyone not
looking at all, and it points at the panel it controls. */
#queue-button[aria-expanded='true'] {
color: var(--yj-accent, #ffd43b);
background: var(--yj-bg-overlay, #404040);
border-radius: 4px;
}
#queue-button.drag-over { #queue-button.drag-over {
color: var(--yj-accent, #ffd43b); color: var(--yj-accent, #ffd43b);
outline: 2px dashed var(--yj-accent, #ffd43b); outline: 2px dashed var(--yj-accent, #ffd43b);
+2 -1
View File
@@ -37,7 +37,8 @@
<footer class="bottom-bar"> <footer class="bottom-bar">
<now-playing></now-playing> <now-playing></now-playing>
<audio-player></audio-player> <audio-player></audio-player>
<button aria-label="Toggle queue" id="queue-button"> <button aria-label="Toggle queue" aria-controls="queue-panel" aria-expanded="false"
id="queue-button">
<wa-icon name="list"></wa-icon> <wa-icon name="list"></wa-icon>
</button> </button>
</footer> </footer>
+22
View File
@@ -521,6 +521,28 @@ if (queueButton && queuePanel) {
} }
}); });
// The button says whether the panel is open, and it learns that
// from the panel rather than from its own click handler.
//
// It is not the only thing that opens the queue -- `now-playing-view`
// sets the same attribute, because it hides the bar this button
// lives in -- so a state kept beside the click would be right until
// something else opened the panel and then quietly wrong. The panel's
// `open` attribute is the one fact; this reflects it.
const reflectQueueState = () => {
queueButton.setAttribute(
'aria-expanded',
String(queuePanel.hasAttribute('open')),
);
};
new MutationObserver(reflectQueueState).observe(queuePanel, {
attributes: true,
attributeFilter: ['open'],
});
reflectQueueState();
// --------------------------------------------------------------- // ---------------------------------------------------------------
// Queue button as drop target (when queue panel is closed) // Queue button as drop target (when queue panel is closed)
// --------------------------------------------------------------- // ---------------------------------------------------------------
@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 576 512"><!--! Font Awesome Free 7.3.1 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) Copyright 2026 Fonticons, Inc. --><path fill="currentColor" d="M288.1-32c9 0 17.3 5.1 21.4 13.1L383 125.3 542.9 150.7c8.9 1.4 16.3 7.7 19.1 16.3s.5 18-5.8 24.4L441.7 305.9 467 465.8c1.4 8.9-2.3 17.9-9.6 23.2s-17 6.1-25 2L288.1 417.6 143.8 491c-8 4.1-17.7 3.3-25-2s-11-14.2-9.6-23.2L134.4 305.9 20 191.4c-6.4-6.4-8.6-15.8-5.8-24.4s10.1-14.9 19.1-16.3l159.9-25.4 73.6-144.2c4.1-8 12.4-13.1 21.4-13.1zm0 76.8L230.3 158c-3.5 6.8-10 11.6-17.6 12.8l-125.5 20 89.8 89.9c5.4 5.4 7.9 13.1 6.7 20.7l-19.8 125.5 113.3-57.6c6.8-3.5 14.9-3.5 21.8 0l113.3 57.6-19.8-125.5c-1.2-7.6 1.3-15.3 6.7-20.7l89.8-89.9-125.5-20c-7.6-1.2-14.1-6-17.6-12.8L288.1 44.8z"/></svg>

After

Width:  |  Height:  |  Size: 889 B

@@ -10,6 +10,7 @@ import type {
VisibilityChangedEvent, VisibilityChangedEvent,
} from '@lit-labs/virtualizer'; } from '@lit-labs/virtualizer';
import { grid } from '@lit-labs/virtualizer/layouts/grid.js'; import { grid } from '@lit-labs/virtualizer/layouts/grid.js';
import { gridSpacingFor } from '@utils/grid-spacing';
import { import {
GetAlbumsByArtist, GetAlbumsByArtist,
GetFilePathsByAlbums, GetFilePathsByAlbums,
@@ -147,8 +148,6 @@ export class ArtistsView
// ----- Grid spacing constants ----- // ----- Grid spacing constants -----
private static readonly GRID_GAP = 8;
private static readonly GRID_PADDING = 8;
private static readonly CARD_PADDING = 5; private static readonly CARD_PADDING = 5;
private get imageSize(): number { private get imageSize(): number {
@@ -177,20 +176,41 @@ export class ArtistsView
private createGridLayout() { private createGridLayout() {
const w = this.cardSize ?? CARD_SIZE_DEFAULT; const w = this.cardSize ?? CARD_SIZE_DEFAULT;
const h = w + this.cardTextHeight; 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({ return grid({
itemSize: { itemSize: {
width: `${w}px`, width: `${w}px`,
height: `${h}px`, height: `${h}px`,
}, },
gap: `${gap}px`, gap: `${spacing}px`,
padding: `${pad}px`, padding: `${spacing}px`,
justify: 'center', justify: 'start',
}); });
} }
/** The width the grid lays itself out in. */
private get containerWidth(): number {
return (
this.renderRoot?.querySelector<HTMLElement>(
'.grid-scroll-container',
)?.clientWidth ||
this.clientWidth ||
0
);
}
private spacingFor(width: number): number {
return gridSpacingFor(width, this.cardSize);
}
/** Sort direction for the artist grid. /** Sort direction for the artist grid.
* *
* There is only one key to sort by: `library.Artist` carries a * There is only one key to sort by: `library.Artist` carries a
@@ -478,6 +498,8 @@ export class ArtistsView
override disconnectedCallback() { override disconnectedCallback() {
super.disconnectedCallback(); super.disconnectedCallback();
this.detachWheelListener(); this.detachWheelListener();
this.gridResizeObserver?.disconnect();
this.gridResizeObserver = null;
} }
/** The wheel listener and the scroll debounce belong to the grid /** The wheel listener and the scroll debounce belong to the grid
@@ -730,10 +752,34 @@ export class ArtistsView
* ================================================================ */ * ================================================================ */
private lastLayoutWidth = 0; 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<HTMLElement>(
'.grid-scroll-container',
);
if (!container || this.gridResizeObserver) return;
this.gridResizeObserver = new ResizeObserver(() =>
this.requestUpdate(),
);
this.gridResizeObserver.observe(container);
}
private updateGridLayout() { private updateGridLayout() {
this.observeGridWidth();
if ( if (
this.cardSize === this.lastLayoutWidth this.cardSize === this.lastLayoutWidth &&
this.lastLayoutSpacing ===
this.spacingFor(this.containerWidth)
) { ) {
return; return;
} }
@@ -86,9 +86,10 @@ export class DownloadClients extends LitElement {
/** Working copy of the auto-download guardrails. */ /** Working copy of the auto-download guardrails. */
@state() @state()
private prefs: download.AutoDownloadPrefs = { private prefs: download.AutoDownloadPrefs = {
minSizeMb: 0, minKbps: 0,
maxKbps: 0,
preferredKbps: 0,
maxSizeMb: 0, maxSizeMb: 0,
preferredSizeMb: 0,
allowedFormats: [], allowedFormats: [],
} as download.AutoDownloadPrefs; } as download.AutoDownloadPrefs;
@@ -284,25 +285,72 @@ export class DownloadClients extends LitElement {
: nothing} : nothing}
<div class="form"> <div class="form">
<!-- Bitrate, not megabytes. A size means nothing
on its own: 300 MB is a generous single and a
suspiciously small boxset, and whoever fills
this in has no idea which release it will be
applied to. A rate is the same statement
divided by how long the music is, so one number
holds across an EP and an opera. -->
<div class="field-row"> <div class="field-row">
<wa-input <wa-input
label="Minimum size (MB)" label="Minimum bitrate (kbps)"
type="number" type="number"
min="0" min="0"
placeholder="No minimum" placeholder="No minimum"
.value=${this.prefs.minSizeMb ? String(this.prefs.minSizeMb) : ''} .value=${this.prefs.minKbps ? String(this.prefs.minKbps) : ''}
@input=${(e: Event) => { @input=${(e: Event) => {
this.prefs = { this.prefs = {
...this.prefs, ...this.prefs,
minSizeMb: Number((e.target as HTMLInputElement).value) || 0, minKbps: Number((e.target as HTMLInputElement).value) || 0,
}; };
}} }}
></wa-input> ></wa-input>
<wa-input <wa-input
label="Maximum size (MB)" label="Maximum bitrate (kbps)"
type="number" type="number"
min="0" min="0"
placeholder="No maximum" placeholder="No maximum"
.value=${this.prefs.maxKbps ? String(this.prefs.maxKbps) : ''}
@input=${(e: Event) => {
this.prefs = {
...this.prefs,
maxKbps: Number((e.target as HTMLInputElement).value) || 0,
};
}}
></wa-input>
<wa-input
label="Preferred bitrate (kbps)"
type="number"
min="0"
placeholder="No preference"
.value=${this.prefs.preferredKbps
? String(this.prefs.preferredKbps)
: ''}
@input=${(e: Event) => {
this.prefs = {
...this.prefs,
preferredKbps:
Number((e.target as HTMLInputElement).value) || 0,
};
}}
></wa-input>
</div>
<div class="requires">
320 is the top of MP3; a FLAC rip is usually
5001000 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.
</div>
<div class="field-row">
<wa-input
label="Never grab more than (MB)"
type="number"
min="0"
placeholder="No limit"
.value=${this.prefs.maxSizeMb ? String(this.prefs.maxSizeMb) : ''} .value=${this.prefs.maxSizeMb ? String(this.prefs.maxSizeMb) : ''}
@input=${(e: Event) => { @input=${(e: Event) => {
this.prefs = { this.prefs = {
@@ -311,22 +359,14 @@ export class DownloadClients extends LitElement {
}; };
}} }}
></wa-input> ></wa-input>
<wa-input </div>
label="Preferred size (MB)"
type="number" <div class="requires">
min="0" A ceiling on the download itself, in case a
placeholder="No preference" mislabelled boxset gets through. Still a size
.value=${this.prefs.preferredSizeMb because it is a question about disk space, and
? String(this.prefs.preferredSizeMb) because it has to apply to a candidate whose
: ''} bitrate cannot be worked out at all.
@input=${(e: Event) => {
this.prefs = {
...this.prefs,
preferredSizeMb:
Number((e.target as HTMLInputElement).value) || 0,
};
}}
></wa-input>
</div> </div>
<div> <div>
@@ -19,6 +19,7 @@ import { LibraryController } from '@store/controllers/library-controller';
import { SearchController } from '@store/controllers/search-controller'; import { SearchController } from '@store/controllers/search-controller';
import { ViewLifecycleMixin } from '@utils/view-lifecycle'; import { ViewLifecycleMixin } from '@utils/view-lifecycle';
import { RovingGridController } from '@utils/roving-grid'; import { RovingGridController } from '@utils/roving-grid';
import { gridColumnsFor, gridSpacingFor } from '@utils/grid-spacing';
import { queueStore } from '@store/queue-store'; import { queueStore } from '@store/queue-store';
import type { QueueSource } from '@store/queue-store'; import type { QueueSource } from '@store/queue-store';
import '@awesome.me/webawesome/dist/components/popup/popup.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js';
@@ -97,19 +98,36 @@ export class CoverGrid
private lastAlbumsRef: library.Album[] | null = private lastAlbumsRef: library.Album[] | null =
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 static readonly CARD_PADDING = 5;
private ctxMenu = new ContextMenuController(this); private ctxMenu = new ContextMenuController(this);
private favCtrl = new FavoritesController(this); private favCtrl = new FavoritesController(this);
private selMgr = new AlbumSelectionManager(); private selMgr = new AlbumSelectionManager();
private scrollMgr = new ScrollManager(this, { private scrollMgr = new ScrollManager(this, {
GRID_GAP: CoverGrid.GRID_GAP, columnsFor: (width: number) => this.columnsFor(width),
GRID_PADDING: CoverGrid.GRID_PADDING, 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 lastSelectedAlbumIndex: number | null = null;
private lastSelectedTrackIndex: number | null = null; private lastSelectedTrackIndex: number | null = null;
@@ -148,10 +166,30 @@ export class CoverGrid
} }
// Virtualizer grid layout instance — recreated when // Virtualizer grid layout instance — recreated when
// the card size changes. // the card size or the container width changes.
private gridLayout = this.createGridLayout(); private gridLayout = this.createGridLayout();
private gridLayoutWidth = 0; 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 * Secondary layout for the "after" virtualizer in
* split mode. Uses zero top padding so there is no * split mode. Uses zero top padding so there is no
@@ -169,22 +207,49 @@ export class CoverGrid
} }
const h = w + this.cardTextHeight; 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({ return grid({
itemSize: { itemSize: {
width: `${w}px`, width: `${w}px`,
height: `${h}px`, height: `${h}px`,
}, },
gap: `${gap}px`, gap: `${spacing}px`,
padding: noTopPad padding: noTopPad
? `0 ${pad}px ${pad}px` ? `0 ${spacing}px ${spacing}px`
: `${pad}px`, : `${spacing}px`,
justify: 'center', 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; private dragImageEl: HTMLElement | null = null;
// -- Memoisation caches for filtered albums -- // -- Memoisation caches for filtered albums --
@@ -466,6 +531,9 @@ export class CoverGrid
); );
this.wheelListenerAttached = false; this.wheelListenerAttached = false;
this.gridResizeObserver?.disconnect();
this.gridResizeObserver = null;
this.scrollMgr.teardown(); this.scrollMgr.teardown();
this.scrollMgr.revealContainer( this.scrollMgr.revealContainer(
this.scrollContainer, this.scrollContainer,
@@ -603,10 +671,18 @@ export class CoverGrid
this.wheelListenerAttached = true; this.wheelListenerAttached = true;
} }
// Recreate the virtualizer grid layout when this.observeGridWidth();
// the card size changes.
// 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 = const cardSizeChanged =
this.gridLayoutWidth !== this.cardWidth; this.gridLayoutWidth !== this.cardWidth ||
this.gridLayoutSpacing !==
this.spacingFor(this.containerWidth);
if (cardSizeChanged) { if (cardSizeChanged) {
this.gridLayout = this.createGridLayout(); this.gridLayout = this.createGridLayout();
@@ -6,12 +6,21 @@ import type { LibraryController } from '@store/controllers/library-controller';
import type { GridEntry } from './cover-grid-types.js'; import type { GridEntry } from './cover-grid-types.js';
/** /**
* Grid spacing constants shared between the scroll * Grid geometry, asked of the host rather than written down.
* manager and the host component. *
* 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 { export interface GridConstants {
readonly GRID_GAP: number; /** Columns that fit across `width`. */
readonly GRID_PADDING: number; 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; return;
} }
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
const pad = this.gc.GRID_PADDING; const pad = gap;
const rowStep = const rowStep =
this.host.cardHeight + gap; this.host.cardHeight + gap;
@@ -293,7 +302,7 @@ export class ScrollManager {
() => { () => {
const rowStep = const rowStep =
this.host.cardHeight + this.host.cardHeight +
this.gc.GRID_GAP; this.spacing(container);
if (this.pendingFocus === null) { if (this.pendingFocus === null) {
this.isResizing = true; this.isResizing = true;
@@ -351,7 +360,7 @@ export class ScrollManager {
container: HTMLElement, container: HTMLElement,
rowStep: number, rowStep: number,
): void { ): void {
const pad = this.gc.GRID_PADDING; const pad = this.spacing(container);
const cols = this.currentColumnCount; const cols = this.currentColumnCount;
const filtered = const filtered =
this.host.cachedFilteredAlbums; this.host.cachedFilteredAlbums;
@@ -410,17 +419,15 @@ export class ScrollManager {
): number { ): number {
if (!container) return 1; if (!container) return 1;
const gap = this.gc.GRID_GAP; return this.gc.columnsFor(
const pad = this.gc.GRID_PADDING; container.clientWidth,
const availableWidth = );
container.clientWidth - pad * 2; }
return Math.max( /** The grid's current spacing, which is also its padding. */
1, private spacing(container?: HTMLElement): number {
Math.floor( return this.gc.spacingFor(
(availableWidth + gap) / container?.clientWidth ?? 800,
(this.host.cardWidth + gap),
),
); );
} }
@@ -439,7 +446,7 @@ export class ScrollManager {
container?: HTMLElement, container?: HTMLElement,
): number { ): number {
const cols = this.getColumnCount(container); const cols = this.getColumnCount(container);
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
return ( return (
cols * this.host.cardWidth + cols * this.host.cardWidth +
@@ -460,7 +467,7 @@ export class ScrollManager {
const cols = this.getColumnCount(container); const cols = this.getColumnCount(container);
const colIndex = idx % cols; const colIndex = idx % cols;
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
return ( return (
colIndex * colIndex *
@@ -597,8 +604,8 @@ export class ScrollManager {
if (!this.host.splitMode) return raw; if (!this.host.splitMode) return raw;
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
const pad = this.gc.GRID_PADDING; const pad = gap;
const columns = const columns =
this.getColumnCount(container); this.getColumnCount(container);
const rowStep = this.host.cardHeight + gap; const rowStep = this.host.cardHeight + gap;
@@ -678,8 +685,8 @@ export class ScrollManager {
if (expandedIndex < 0) return; if (expandedIndex < 0) return;
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
const pad = this.gc.GRID_PADDING; const pad = gap;
const columns = const columns =
this.getColumnCount(container); this.getColumnCount(container);
const rowStep = this.host.cardHeight + gap; const rowStep = this.host.cardHeight + gap;
@@ -772,8 +779,8 @@ export class ScrollManager {
if (idx < 0) return; if (idx < 0) return;
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
const pad = this.gc.GRID_PADDING; const pad = gap;
const cols = const cols =
this.getColumnCount(container); this.getColumnCount(container);
const rowStep = this.host.cardHeight + gap; const rowStep = this.host.cardHeight + gap;
@@ -854,9 +861,8 @@ export class ScrollManager {
this.getExpandedAlbumIndex(); this.getExpandedAlbumIndex();
if (idx >= 0) { if (idx >= 0) {
const gap = this.gc.GRID_GAP; const gap = this.spacing(container);
const pad = const pad = gap;
this.gc.GRID_PADDING;
const cols = const cols =
this.getColumnCount( this.getColumnCount(
container, container,
@@ -400,7 +400,7 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) {
private renderEmptyRequests() { private renderEmptyRequests() {
return html` return html`
<div class="empty"> <div class="empty">
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. to add it here.
</div> </div>
`; `;
@@ -2680,7 +2680,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
slot="start" slot="start"
name=${this.isRequested ? 'solid/bookmark' : 'regular/bookmark'} name=${this.isRequested ? 'solid/bookmark' : 'regular/bookmark'}
></wa-icon> ></wa-icon>
${this.isRequested ? 'Wanted' : 'Want this'} ${this.isRequested ? 'Requested' : 'Request this'}
</wa-button> </wa-button>
`; `;
} }
@@ -2753,7 +2753,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
slot="icon" slot="icon"
name=${requested ? 'xmark' : 'bookmark'} name=${requested ? 'xmark' : 'bookmark'}
></wa-icon> ></wa-icon>
${requested ? 'Cancel Request' : 'Want This'} ${requested ? 'Cancel Request' : 'Request This'}
</wa-dropdown-item> </wa-dropdown-item>
` `
: nothing} : nothing}
@@ -1509,9 +1509,24 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
if (url) { if (url) {
this.thumbnailCache.set(req.mbid, url); this.thumbnailCache.set(req.mbid, url);
this.requestUpdate(); 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(() => { .catch(() => {
@@ -10,6 +10,7 @@ import type {
VisibilityChangedEvent, VisibilityChangedEvent,
} from '@lit-labs/virtualizer'; } from '@lit-labs/virtualizer';
import { grid } from '@lit-labs/virtualizer/layouts/grid.js'; import { grid } from '@lit-labs/virtualizer/layouts/grid.js';
import { gridSpacingFor } from '@utils/grid-spacing';
import { import {
GetFilePathsByGenres, GetFilePathsByGenres,
} from '@go/library/library.js'; } from '@go/library/library.js';
@@ -155,8 +156,6 @@ export class GenresView
// ----- Grid spacing constants ----- // ----- Grid spacing constants -----
private static readonly GRID_GAP = 8;
private static readonly GRID_PADDING = 8;
private static readonly CARD_PADDING = 5; private static readonly CARD_PADDING = 5;
private get imageSize(): number { private get imageSize(): number {
@@ -185,20 +184,41 @@ export class GenresView
private createGridLayout() { private createGridLayout() {
const w = this.cardSize ?? CARD_SIZE_DEFAULT; const w = this.cardSize ?? CARD_SIZE_DEFAULT;
const h = w + this.cardTextHeight; 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({ return grid({
itemSize: { itemSize: {
width: `${w}px`, width: `${w}px`,
height: `${h}px`, height: `${h}px`,
}, },
gap: `${gap}px`, gap: `${spacing}px`,
padding: `${pad}px`, padding: `${spacing}px`,
justify: 'center', justify: 'start',
}); });
} }
/** The width the grid lays itself out in. */
private get containerWidth(): number {
return (
this.renderRoot?.querySelector<HTMLElement>(
'.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). */ /** Sort key and direction for the genre grid (H-19: it had none). */
@state() @state()
private sortField: 'name' | 'tracks' = 'name'; private sortField: 'name' | 'tracks' = 'name';
@@ -483,6 +503,8 @@ export class GenresView
override disconnectedCallback() { override disconnectedCallback() {
super.disconnectedCallback(); super.disconnectedCallback();
this.detachWheelListener(); this.detachWheelListener();
this.gridResizeObserver?.disconnect();
this.gridResizeObserver = null;
} }
/** See artists-view: off-screen the grid cannot be scrolled, and /** See artists-view: off-screen the grid cannot be scrolled, and
@@ -737,10 +759,34 @@ export class GenresView
* ================================================================ */ * ================================================================ */
private lastLayoutWidth = 0; 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<HTMLElement>(
'.grid-scroll-container',
);
if (!container || this.gridResizeObserver) return;
this.gridResizeObserver = new ResizeObserver(() =>
this.requestUpdate(),
);
this.gridResizeObserver.observe(container);
}
private updateGridLayout() { private updateGridLayout() {
this.observeGridWidth();
if ( if (
this.cardSize === this.lastLayoutWidth this.cardSize === this.lastLayoutWidth &&
this.lastLayoutSpacing ===
this.spacingFor(this.containerWidth)
) { ) {
return; return;
} }
@@ -48,7 +48,7 @@ export type LibraryStatus =
* *
* Colours and glyphs: * Colours and glyphs:
* - in-library → green circle, check mark * - in-library → green circle, check mark
* - queued → amber circle, hourglass * - queued → amber circle, bookmark ("on your list")
* - not-in-library → grey circle, plus sign * - not-in-library → grey circle, plus sign
* *
* Usage: * 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 { private iconName(): string {
switch (this.status) { switch (this.status) {
case 'in-library': case 'in-library':
return 'check'; return 'check';
case 'queued': case 'queued':
return 'hourglass-half'; return 'bookmark';
default: default:
return 'plus'; return 'plus';
} }
@@ -276,7 +287,7 @@ export class LibraryStatusIndicator extends LitElement {
if (this.actionable) { if (this.actionable) {
return this.status === 'queued' return this.status === 'queued'
? `Cancel the request for ${kind}${name}` ? `Cancel the request for ${kind}${name}`
: `Want ${kind}${name}`; : `Request ${kind}${name}`;
} }
switch (this.status) { switch (this.status) {
@@ -354,25 +365,6 @@ export class LibraryStatusIndicator extends LitElement {
} }
const title = this.tooltip(); const title = this.tooltip();
const icon = this.iconName()
? html`<wa-icon name=${this.iconName()} aria-hidden="true"></wa-icon>`
: nothing;
if (this.actionable) {
return html`
<button
class="badge"
type="button"
title=${title}
aria-label=${title}
?disabled=${this.busy}
@click=${this.onActivate}
@keydown=${this.onKeydown}
>
${icon}
</button>
`;
}
// The ring stands in for the icon wherever the icon would go — // The ring stands in for the icon wherever the icon would go —
// including inside the button, because a partly-held album is // including inside the button, because a partly-held album is
@@ -307,7 +307,7 @@ export class NowPlayingView extends LitElement {
: `Add ${track.title} to ${this.favCtrl.playlistName}`} : `Add ${track.title} to ${this.favCtrl.playlistName}`}
@click=${this.toggleFavorite} @click=${this.toggleFavorite}
> >
<wa-icon name=${this.favCtrl.iconName}></wa-icon> <wa-icon name=${this.favCtrl.iconFor(favorited)}></wa-icon>
</button> </button>
</div> </div>
@@ -527,7 +527,7 @@ export class NowPlaying extends LitElement {
)} )}
> >
<wa-icon <wa-icon
name=${this.favCtrl.iconName} name=${this.favCtrl.iconFor(isFav)}
variant=${favVariant} variant=${favVariant}
></wa-icon> ></wa-icon>
</button> </button>
@@ -1756,7 +1756,7 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) {
${entry.summary.ID === this.favCtrl.playlistId ${entry.summary.ID === this.favCtrl.playlistId
? html`<wa-icon ? html`<wa-icon
class="playlist-icon" class="playlist-icon"
name=${this.favCtrl.iconName} name=${this.favCtrl.iconFor(true)}
></wa-icon>` ></wa-icon>`
: entry.summary.IsSmart : entry.summary.IsSmart
? html`<wa-icon ? html`<wa-icon
+1
View File
@@ -16,6 +16,7 @@
# fetch-icons.mjs for why that is not negotiable. Re-vendor with: # fetch-icons.mjs for why that is not negotiable. Re-vendor with:
# node frontend/scripts/fetch-icons.mjs # node frontend/scripts/fetch-icons.mjs
regular/heart regular/heart
regular/star
solid/arrow-down-wide-short solid/arrow-down-wide-short
solid/arrow-left solid/arrow-left
solid/arrow-rotate-right solid/arrow-rotate-right
@@ -74,12 +74,36 @@ export class FavoritesController
} }
/** /**
* Returns the icon name for the current icon style. * The icon name for the current icon style, unfilled.
*
* Prefer `iconFor(isFav)` — this getter is the name of the *empty*
* glyph, which is what every caller that does not know the state
* should draw.
*/ */
get iconName(): string { get iconName(): string {
return this.iconStyle === 'star' return this.iconFor(false);
? 'star' }
: 'heart';
/**
* The glyph for one track's favourite state.
*
* **A filled shape means favourited and an outline means not**, in
* every list in the app. Nine components rendered `iconName`, which
* was the *solid* glyph in both states — so "not a favourite" was a
* filled heart in a duller colour, and the only thing separating
* the two states was hue. That fails for anyone who cannot see the
* difference between them, and reads as "everything is a favourite"
* to everyone else. `track-list` and `album-dropdown` already drew
* it correctly, from inline SVG paths of their own; this is the
* same rule for the `<wa-icon>` 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}`;
} }
// =============================================================== // ===============================================================
+58
View File
@@ -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)),
);
}
@@ -120,7 +120,7 @@ describe('the context menu on an artist page release', () => {
expect(items).toContain('Add to Queue'); expect(items).toContain('Add to Queue');
expect(items).toContain('Play Next'); expect(items).toContain('Play Next');
// Owned: there is nothing left to ask for. // 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 () => { 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('Play');
expect(items).not.toContain('Add to Queue'); expect(items).not.toContain('Add to Queue');
expect(items).toContain('Want This'); expect(items).toContain('Request This');
expect(items).toContain('View on MusicBrainz'); 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 // …but a `local:` id names nothing upstream, and wanting something
// already in the library is not a thing to offer. // already in the library is not a thing to offer.
expect(items).not.toContain('View on MusicBrainz'); 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 () => { it('opens from the keyboard on Shift+F10', async () => {
+1 -1
View File
@@ -166,7 +166,7 @@ describe('<library-status-indicator>', () => {
glyphs.push(shadow(el, 'wa-icon')?.getAttribute('name')); 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 () => { it('phrases its label around the entity it describes', async () => {
@@ -249,7 +249,7 @@ describe('<library-status-indicator> as a control', () => {
const el = await badge({ requestMbid: 'rg-1' }); const el = await badge({ requestMbid: 'rg-1' });
expect(shadow(el, '.badge')?.getAttribute('aria-label')).toBe( expect(shadow(el, '.badge')?.getAttribute('aria-label')).toBe(
'Want album "Abbey Road"', 'Request album "Abbey Road"',
); );
await update(el, { status: 'queued' }); await update(el, { status: 'queued' });