Compare commits

..
Author SHA1 Message Date
yonluandClaude Opus 5.5 399dcc05b3 docs(notes): record slskd's API as read from its source
CI / check (push) Skipped
CI / e2e (push) Skipped
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 22:11:30 -04:00
yonluandClaude Opus 5.5 ef89707bb1 feat(download): fill in the tracks a nearly complete album is missing
A grab that delivers nine of twelve tracks clears the completeness floor
and is imported, and the other three were never looked for. On Soulseek
that is the commonest way an album ends up almost right: one peer's
folder lacks a track, or one file fails.

After a successful import the manager now compares the tracks the files
were aligned to (ImportResult.Matched, new) with the expected tracklist.
When one to three are missing, and fewer than half, it searches for each
one on its own, as the track's artist and title with the album kept for
ranking. It grabs the first auto-acceptable copy that is not from the
source that already failed to supply it, and is not a delegate, which
would place it in its own library. The candidate is trimmed to the one
file aligned to the track. The import uses the album's own request with
ImportOptions.Only, which skips the completeness check and imports only
a file aligned to the missing track, so it is tagged and placed as part
of the album, and anything else the folder brought is left out.

One attempt per track, and nothing here fails the download: the album
is already imported, so a track that cannot be found is logged on the
job and left. slskd accepts one-file folders for a request that expects
one track, which the per-track search needs.

Closes #276

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 22:11:03 -04:00
yonluandClaude Opus 5.5 c88fc1c7c7 feat(download): judge a queued slskd peer by its queue position
No bytes for ten minutes usually means a peer has queued us, and the
stall timer could not tell position 2 from position 400: the first was
abandoned while it was about to start, the second was waited on for ten
minutes for nothing.

While a requested file is "Queued, Remotely", the grab asks slskd for
its place (GET .../downloads/{user}/{id}/position, which asks the peer)
once a minute. A place that improved counts as progress and restarts
the stall clock. A place beyond 50 twice running gives the peer up at
once, and the manager moves to the next copy; two readings because
slskd documents the figure as possibly inaccurate. Waiting in a queue
has an overall ceiling of an hour without a byte, since a queue moving
one place an hour would otherwise hold the grab all day. As with a
stall, files that already arrived still go forward.

Closes #275

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 22:07:18 -04:00
yonluandClaude Opus 5.5 7a9dd69d30 fix(download): own folder per slskd grab, search timeout in seconds
Checked against slskd 0.26.0's source rather than a live daemon, which
#267 never had.

slskd reads a search's searchTimeout in seconds, counted from the last
response. We sent milliseconds, telling it a search may idle for five
hours, so a search never completed on its own. It now sends seconds,
with slskd's floor of 5.

slskd writes a finished file to <downloads>/<remote leaf folder>/, and
when a name is taken it writes name_<ticks>.ext beside it. collect found
files by name there, so a file left by an earlier failed attempt, or by
the user's own download, was collected in place of this grab's. A user
who changed slskd's destination setting got nothing collected at all.

slskd 0.26 takes a batch download with an explicit destination. Each
grab now enqueues batches into yellowjacket/<uuid>/ (one per disc, since
a batch's files land flat), collects from exactly there, and removes the
folder afterwards, including after a failure. An older daemon answers
the batch route with 400, which is remembered, and the per-user enqueue
is used. There, collect skips files that were already present,
unchanged, before the enqueue, and takes the renamed copy slskd wrote
instead. The comparison is against a snapshot, not a clock, because
slskd may run on another machine. The per-folder lock from #272 is kept
only while batches are not known to work.

The test stub now writes files when they are enqueued, as slskd does,
including the rename, so tests no longer stage files before a grab.
An opt-in TestSlskdLive runs against a real daemon when YJ_SLSKD_URL,
YJ_SLSKD_API_KEY and YJ_SLSKD_DOWNLOADS are set.

Closes #274

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 22:04:50 -04:00
yonlu 3f23bb4396 Merge pull request 'Batch: explore catalog + UI, Go 1.26, Soulseek downloads, CI fixes' (#273) from batch/258-272 into main
CI / check (push) Successful in 4m8s
CI / e2e (push) Successful in 14m37s
Reviewed-on: #273
2026-09-27 01:45:18 +00:00
yonlu af2ff17342 Merge remote-tracking branch 'origin/fix/263-slskd-transfer-lifecycle' into batch/258-272
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 4m53s
CI / e2e (pull_request) Successful in 13m34s
2026-09-26 20:58:54 -04:00
yonlu fc19ca54b7 Merge remote-tracking branch 'origin/build/265-go-1.26' into batch/258-272 2026-09-26 20:58:54 -04:00
yonlu 613901847a Merge remote-tracking branch 'origin/feat/264-explore-cards' into batch/258-272 2026-09-26 20:58:54 -04:00
yonlu ecf0109331 Merge remote-tracking branch 'origin/fix/258-artifact-blob-cursor' into batch/258-272 2026-09-26 20:58:53 -04:00
yonluandClaude Opus 5.5 9710c11476 feat(download): one grab per Soulseek peer, several peers at once
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Failing after 2m51s
CI / e2e (pull_request) Skipped
slskd was capped at one transfer per daemon, on the grounds that
Soulseek peers punish clients that ask for too much. That politeness is
per peer: two different users do not compete for anyone's upload slot.
So one slow peer serialised every other Soulseek download behind it.

The manager now takes a per-(provider, peer) lock before any slot, so a
grab waiting on a busy peer does not hold a provider slot another peer
could use, and the slskd default rises to 3, which now counts peers.
The help text says so.

Running grabs at once exposed the folder collision: slskd names a
download's directory after the remote leaf folder, so two peers'
"Greatest Hits" (or any two rips' "CD1") share one directory, and
collect finds files by name there. Grabs whose local folders overlap
now take a package-level lock per folder, in sorted order, keyed on the
full path because two clients can share one daemon.

Closes #272

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 17:31:35 -04:00
yonluandClaude Opus 5.5 5e3ac8fb1b fix(download): search Soulseek more than once, and read file lengths
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Canceled after 0s
CI / e2e (pull_request) Canceled after 0s
The slskd search asked one question and ignored part of the answer.

Two queries.  Soulseek matches every term against a file's full path,
so every extra word is a filter, and several filter wrongly: an edition
qualifier from the catalog title that no one puts in a folder name, a
term with a leading "-", which Soulseek reads as an exclusion, and
"Various Artists", which is in no one's path.  When a normalised form of
the request differs, it runs alongside the original and the candidates
are merged by peer and folder.  Concurrently, not as a fallback: the
manager gives a provider one search budget, and a Soulseek search spends
most of it waiting.  A query the user typed is searched as written.

Stated options.  The search carried only its id and text, so slskd's
own defaults for its timeout and response limits applied.  Its timeout
is now set inside our wait, the limits are well above a popular album,
and slskd drops folders below the file floor and peers with a queue we
would not reach today.

A state-only poll.  Every one-second poll re-sent every response; the
responses are now fetched once at the end, falling back to the old
includeResponses form for a daemon without that endpoint.

Durations.  slskd reports each file's length and it was discarded.  It
is now carried as CandidateFile.LengthMillis and scored against the
expected tracks as DurationFit, which takes 0.15 of title fit's weight
when at least half the aligned pairs are timed: a title says which song
a file claims to be, a length says whether it is that recording.
Without lengths the score is exactly the previous formula.

freeUploadSlots is removed from the response type; slskd sends
hasFreeUploadSlot and nothing by that name.

Closes #271

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 17:11:23 -04:00
yonluandClaude Opus 5.5 f81a950916 fix(download): score a multi-disc rip as one album, and count tracks
CI / check (push) Skipped
CI / e2e (push) Skipped
Three faults in how candidates are shaped and scored, one commit because
they meet in the same completeness number.

Multi-disc albums were split in two.  Soulseek shares them as
Album/CD1 and Album/CD2, and candidates were grouped by the immediate
parent, so each disc became its own candidate titled "CD1": about half
complete, with an album title that could not match.  Such a release
essentially never cleared auto-pick.  AlbumDir groups a disc folder
under its parent, ParsePath takes the disc number from the folder (a
disc in the filename still wins), and collect keeps the disc folders in
staging, where flattened, disc 2's "01 Intro.flac" overwrote disc 1's.

A single-track request could never be served from Soulseek.  A track
search matches one file per folder, and the two-file floor that screens
out noise for an album screened out every result.  A recording request
takes one.

Completeness counted files.  Ten files against a ten-track album scored
full marks whether or not they were its tracks, and title fit is the
mean over the files that did align, so a folder where three titles
matched read as near-perfect on both.  Coverage is now counted in
aligned tracks, with the file count still setting the penalty for
extras.

Closes #270

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 17:07:28 -04:00
yonluandClaude Opus 5.5 7fbfd9c105 test(library): skip the cover-tier scan test when fixtures are absent
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 4m23s
CI / e2e (pull_request) Canceled after 0s
TestScan_StoresOnlyCoverTiers built the fixture path by hand, so in a
tree where make testdata had not run it failed on a missing covers
directory, where every other fixture test skips via testfixtures.Load.
In a fresh worktree that failure blocked the pre-push hook for every
branch.

Closes #266

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 17:02:56 -04:00
yonluandClaude Opus 5.5 792c2d9fbc build: require Go 1.26 everywhere at once
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 8m21s
CI / e2e (pull_request) Successful in 15m56s
The newest go-json-experiment/json, which wails/v3's application
package imports, declares go 1.26, so taking it raises our go
directive with it.  Every other place that names a Go version moves in
the same commit: GO_VERSION in ci, desktop-assets and android-apk, the
Arch makedepends, and CONTRIBUTING's table.

index-artifact's container image moves too, and it is the one that
matters most.  The official golang images set GOTOOLCHAIN=local, so a
golang:1.25 container refuses a go 1.26 module outright rather than
fetching a newer toolchain, and that job owns the ~205 GB checkpoint.

Closes #265

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 17:00:47 -04:00
yonluandClaude Opus 5.5 bb26d5f289 feat(explore): arrows on every sideways row, and one album card size
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 4m25s
CI / e2e (pull_request) Successful in 13m43s
The shelves, the top results and the artist page's discography and
similar-artists rows scrolled sideways only by a horizontal wheel or a
trackpad, so a plain mouse could not reach anything past the fold.
<scroll-row> wraps each of them with previous/next arrows that are
hidden at the end they cannot move from, revealed on hover where there
is hover, and always shown where there is not.

The album card is defined once, in albumCardStyles, instead of twice.
explore-view clamped its cards to 130-150px, and with square artwork
that made cards in one row different heights as well as widths.  The
width is fixed now and each line under the art reserves its own space.
Covers are inset rather than cropped (contain, not cover).

The ownership badge sits over the artwork for owned and unowned alike,
and catalog cards no longer dim: a grid of dimmed covers read as a page
that had failed to load.  The album page's tracklist still dims unowned
rows, which says something different about something different.

Every MusicBrainz link now goes through openMusicBrainz, which pins the
origin, including the one on the album page that still called
window.open directly.  solid/chevron-left is bundled for the left arrow;
without it the arrow drew the missing-icon fallback.

Closes #264

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 16:59:28 -04:00
yonluandClaude Opus 5.5 7b42b9ce56 feat(explore): sample the one-album-owned shelf instead of ranking it
"More from artists you own one album by" drew its artists and their
albums most-popular first, so it was a second leaderboard: the same
handful of big names every time the page opened, which is not what the
shelf is saying.  The pool is still bounded, but which artists and
which albums fill it is RANDOM(), so each visit is a different sample.
The test asserts the set rather than the order.

Refs #264

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 16:58:57 -04:00
yonluandClaude Opus 5.5 0a33b9d653 fix(download): try the next acceptable copy when a transfer fails
A failed transfer failed the whole download.  On Soulseek the usual
failure is one peer being offline or refusing, and a popular album has
several other peers offering the same folder; the ranked list that
names them was already held in m.results and nothing walked it.

grab now loops: when a candidate's transfer fails, or delivers too
little of the album to import, the next candidate is tried in its
place, up to three in all.  Three rules keep that honest:

- Only a candidate auto-pick would itself have accepted is offered, so
  a second choice clears the same match, quality and guardrail gates
  as the first.
- On Soulseek the failure is the peer's, so every folder that peer
  offered is skipped with it; elsewhere only the failed release is.
- A candidate the user picked by hand does not fall back.  They chose
  that copy, and quietly substituting another is a decision they did
  not make.

The same change fixes auto-pick grabbing the wrong candidate.
AutoPickVeto judges the best candidate inside the user's guardrails,
but Start and Attempt then grabbed ranked[0] -- so when the overall best
was over the size ceiling, the veto passed on the strength of the
second and the first was downloaded anyway: the one copy the user had
said not to take unattended.  autoPick returns the candidate the veto
actually judged.

Closes #263

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 16:12:06 -04:00
yonluandClaude Opus 5.5 fc0121228e fix(download): give up on a stalled slskd peer and cancel its transfers
awaitTransfers waited on every requested file reaching a terminal state
with no bound but the caller's six-hour context.  slskd's transfer
limit is one, so a peer that queued us and never sent a byte held every
other Soulseek download behind it for the whole six hours.  A grab now
gives up after ten minutes with no bytes moving; a folder that stalls
on its last tracks goes forward with what arrived, as a partial failure
always has.

Three smaller faults on the same path:

- A file slskd never lists (refused at enqueue) could never reach a
  terminal state, so the wait could not end.  It counts as failed after
  a short grace period.
- A terminal record left by an earlier attempt at the same file from the
  same peer was read as this attempt's answer on the first poll.  The
  ids already terminal before enqueue are ignored.
- Giving up, for any reason, left slskd downloading for a request nobody
  was waiting on.  The live transfers are cancelled and removed there,
  on a context of their own so a cancelled caller still sends it.

Usernames are now path-escaped; they may carry spaces and slashes.

Refs #263

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 16:11:52 -04:00
49 changed files with 5617 additions and 883 deletions
+1 -1
View File
@@ -68,7 +68,7 @@ jobs:
SHA: ${{ github.sha }} SHA: ${{ github.sha }}
REF_NAME: ${{ github.ref_name }} REF_NAME: ${{ github.ref_name }}
DEBIAN_FRONTEND: noninteractive DEBIAN_FRONTEND: noninteractive
GO_VERSION: '1.25.0' GO_VERSION: '1.26.0'
npm_config_store_dir: /cache/pnpm-store npm_config_store_dir: /cache/pnpm-store
# The Go half wants the NDK; the Gradle half wants a platform. # The Go half wants the NDK; the Gradle half wants a platform.
ANDROID_HOME: /cache/android-sdk ANDROID_HOME: /cache/android-sdk
+2 -2
View File
@@ -36,7 +36,7 @@ concurrency:
cancel-in-progress: true cancel-in-progress: true
env: env:
GO_VERSION: '1.25.0' GO_VERSION: '1.26.0'
# Shared by all three Playwright consumers (@playwright/cli, e2e/'s # Shared by all three Playwright consumers (@playwright/cli, e2e/'s
# @playwright/test, frontend/'s Vitest provider). See the browsers # @playwright/test, frontend/'s Vitest provider). See the browsers
# step in job 2 for why that is not the whole story. # step in job 2 for why that is not the whole story.
@@ -53,7 +53,7 @@ jobs:
check: check:
runs-on: ubuntu-latest runs-on: ubuntu-latest
container: container:
# Not golang:1.25 — this job runs `make ui-test`, which is Vitest # Not golang:1.26 — this job runs `make ui-test`, which is Vitest
# *browser* mode and needs a Chromium and its system libraries # *browser* mode and needs a Chromium and its system libraries
# anyway, so the "fast job needs no browser" split does not hold. # anyway, so the "fast job needs no browser" split does not hold.
# Not the Playwright image either: e2e/ pins @playwright/test # Not the Playwright image either: e2e/ pins @playwright/test
+1 -1
View File
@@ -48,7 +48,7 @@ jobs:
SHA: ${{ github.sha }} SHA: ${{ github.sha }}
REF_NAME: ${{ github.ref_name }} REF_NAME: ${{ github.ref_name }}
DEBIAN_FRONTEND: noninteractive DEBIAN_FRONTEND: noninteractive
GO_VERSION: '1.25.0' GO_VERSION: '1.26.0'
npm_config_store_dir: /cache/pnpm-store npm_config_store_dir: /cache/pnpm-store
steps: steps:
# The same set ci.yml's check job installs: the app is cgo, and # The same set ci.yml's check job installs: the app is cgo, and
+33
View File
@@ -5097,3 +5097,36 @@ beside what they explain. These three did not:
The drawer-style gutter would buy the affordance by taking width off a The drawer-style gutter would buy the affordance by taking width off a
full-screen surface on a 424px viewport; back and a 44px close button full-screen surface on a 424px viewport; back and a 44px close button
answer it instead. answer it instead.
## slskd's API, read from its source rather than a live daemon (2026-09-26)
`backend/download/provider_slskd.go` had never run against a real slskd
when #263–#272 shipped, so its assumptions were checked against slskd
0.26.0's source. One was wrong, and one design was only safe by luck.
These are properties of someone else's server; re-check on an upgrade.
`TestSlskdLive` (env-gated, see its comment) is the way to confirm them
against a running one.
- **`searchTimeout` is seconds, from the last response**, minimum 5
(`SearchRequest.cs`). We sent milliseconds (#274). The other search
options — `responseLimit`, `fileLimit`, `filterResponses`,
`minimumResponseFileCount`, `maximumPeerQueueLength` — are named as we
send them; slskd's defaults are 100 responses, 10 000 files, queue
1 000 000.
- **`GET /searches/{id}/responses` exists**, and `DELETE
/transfers/downloads/{user}/{id}?remove=true` cancels and removes.
- **A finished download is moved to `<downloads>/<Subdirectory>/`**,
where `Destination.Subdirectory` defaults to `${SOURCE_DIRECTORY}`
(the remote leaf folder) and is user-configurable. A taken name is
written as `name_<ticks>.ext` (`Destination.Exists = rename`, the
default). No transfer record says where the file went.
- **Batch enqueue (`POST /transfers/downloads/batches`) is new in 0.26.0**
and is the only way to choose where a file lands: `options.destination`
overrides the subdirectory pattern. A batch's files land flat in it,
so one batch per disc. On an older daemon that path is routed to the
per-user enqueue as username "batches" and the object body is
rejected with 400 — which is why 400 means "no batches" here.
- **`GET .../downloads/{user}/{id}/position` asks the peer** and returns
a bare integer. slskd's own comment on `PlaceInQueue` is "may be
wildly innacurate to the point of uselessness", which is why #275 acts
only on two readings in a row.
+1 -1
View File
@@ -13,7 +13,7 @@ frontend, bridged by [Wails v3](https://wails.io/).
| Tool | Version | | Tool | Version |
|------|---------| |------|---------|
| Go | 1.25+ | | Go | 1.26+ |
| Node.js | 22+ | | Node.js | 22+ |
| pnpm | 10+ | | pnpm | 10+ |
| Wails CLI | v3 — vendored, no install needed (`go tool wails3`) | | Wails CLI | v3 — vendored, no install needed (`go tool wails3`) |
+26 -6
View File
@@ -4,6 +4,7 @@ import (
"bytes" "bytes"
"context" "context"
"encoding/json" "encoding/json"
"errors"
"fmt" "fmt"
"io" "io"
"net/http" "net/http"
@@ -144,17 +145,36 @@ func (c *apiClient) checkStatus(resp *http.Response) error {
case resp.StatusCode >= 400: case resp.StatusCode >= 400:
snippet, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) snippet, _ := io.ReadAll(io.LimitReader(resp.Body, 512))
return fmt.Errorf( return fmt.Errorf("%w: %w", c.errUnreachable, &httpStatusError{
"%w: HTTP %d: %s", code: resp.StatusCode,
c.errUnreachable, body: strings.TrimSpace(string(snippet)),
resp.StatusCode, })
strings.TrimSpace(string(snippet)),
)
default: default:
return nil return nil
} }
} }
// httpStatusError is a non-2xx answer, kept typed so a caller can tell
// a missing endpoint from a daemon that is down.
type httpStatusError struct {
code int
body string
}
func (e *httpStatusError) Error() string {
return fmt.Sprintf("HTTP %d: %s", e.code, e.body)
}
// statusCode returns the HTTP status an error carries, or 0.
func statusCode(err error) int {
var se *httpStatusError
if errors.As(err, &se) {
return se.code
}
return 0
}
// decodeJSON decodes a JSON string into out. Providers whose auth or // decodeJSON decodes a JSON string into out. Providers whose auth or
// response handling does not fit apiClient still parse bodies the same // response handling does not fit apiClient still parse bodies the same
// way, so the helper lives here rather than being repeated. // way, so the helper lives here rather than being repeated.
+248
View File
@@ -0,0 +1,248 @@
package download
import (
"context"
"os"
"path/filepath"
"testing"
)
// Multi-disc rips, single-track results and coverage counted in tracks
// rather than files (#270).
func TestParsePathReadsTheDiscFromItsFolder(t *testing.T) {
t.Parallel()
cases := []struct {
path string
disc int
track int
folder string
}{
{`\share\Pink Floyd - The Wall (1979)\CD2\03 Hey You.flac`, 2, 3, "The Wall"},
{`\share\The Wall\Disc 1\01 In The Flesh.flac`, 1, 1, "The Wall"},
{`\share\The Wall\[Disk-2]\01 Hey You.flac`, 2, 1, "The Wall"},
{`\share\The Wall\CD1 - Live\04 Mother.flac`, 1, 4, "The Wall"},
// The filename's own disc number is more specific than the folder.
{`\share\The Wall\CD1\2-05 Comfortably Numb.flac`, 2, 5, "The Wall"},
// Not a disc folder: a number is required.
{`\share\CDs\The Wall\01 In The Flesh.flac`, 0, 1, "The Wall"},
}
for _, tc := range cases {
t.Run(tc.path, func(t *testing.T) {
t.Parallel()
got := ParsePath(tc.path)
if got.Disc != tc.disc || got.Track != tc.track || got.Folder != tc.folder {
t.Errorf(
"ParsePath = disc %d track %d folder %q, want %d %d %q",
got.Disc, got.Track, got.Folder, tc.disc, tc.track, tc.folder,
)
}
})
}
}
func TestAlbumDir(t *testing.T) {
t.Parallel()
cases := map[string]string{
`\share\Album\CD1\01 A.flac`: "/share/Album",
`\share\Album\01 A.flac`: "/share/Album",
`CD1\01 A.flac`: "CD1",
`\share\CD Collection\01.mp3`: "/share/CD Collection",
}
for in, want := range cases {
if got := AlbumDir(in); got != want {
t.Errorf("AlbumDir(%q) = %q, want %q", in, got, want)
}
}
}
// One album shared as CD1/CD2 is one candidate, named after the album.
func TestSlskdGroupsDiscFoldersIntoOneCandidate(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.responses = []slskdResponse{{
Username: "peer",
Files: []slskdFile{
{Filename: `\m\The Wall\CD1\01 In The Flesh.flac`, Size: 1},
{Filename: `\m\The Wall\CD1\02 The Thin Ice.flac`, Size: 1},
{Filename: `\m\The Wall\CD2\01 Hey You.flac`, Size: 1},
{Filename: `\m\The Wall\CD2\02 Is There Anybody Out There.flac`, Size: 1},
},
}}
s, _ := newStubSlskd(t, stub)
got, err := s.Search(context.Background(), Download{Query: "the wall"})
if err != nil {
t.Fatalf("Search: %v", err)
}
if len(got) != 1 {
t.Fatalf("got %d candidates, want the two discs as one", len(got))
}
if got[0].Title != "The Wall" || len(got[0].Files) != 4 {
t.Errorf(
"candidate = %q with %d files, want \"The Wall\" with 4",
got[0].Title, len(got[0].Files),
)
}
}
// A track search matches one file per folder, so a single-track request
// must accept a one-file folder that an album request rightly drops.
func TestSlskdKeepsASingleFileForATrackRequest(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.responses = []slskdResponse{{
Username: "peer",
Files: []slskdFile{
{Filename: `\m\OK Computer\02 Paranoid Android.flac`, Size: 1},
},
}}
s, _ := newStubSlskd(t, stub)
track, err := s.Search(context.Background(), Download{
RecordingMBID: "rec-1", Artist: "Radiohead", Album: "Paranoid Android",
})
if err != nil {
t.Fatalf("Search: %v", err)
}
if len(track) != 1 {
t.Errorf("track request: got %d candidates, want 1", len(track))
}
album, err := s.Search(context.Background(), Download{
ReleaseMBID: "rel-1", Artist: "Radiohead", Album: "OK Computer",
})
if err != nil {
t.Fatalf("Search: %v", err)
}
if len(album) != 0 {
t.Errorf("album request: got %d candidates, want the one-file folder dropped", len(album))
}
}
// Two discs with a file of the same name both reach staging, each under
// its disc folder, where the importer reads the disc number from.
func TestSlskdCollectKeepsDiscFolders(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
s, downloads := newStubSlskd(t, stub)
for _, disc := range []string{"CD1", "CD2"} {
dir := filepath.Join(downloads, disc)
if err := os.MkdirAll(dir, 0o750); err != nil {
t.Fatalf("mkdir: %v", err)
}
if err := os.WriteFile(
filepath.Join(dir, "01 Intro.flac"), []byte(disc), 0o600,
); err != nil {
t.Fatalf("write: %v", err)
}
}
dst := t.TempDir()
got, err := s.collect(Candidate{Files: []CandidateFile{
{Path: `\m\Album\CD1\01 Intro.flac`, IsAudio: true},
{Path: `\m\Album\CD2\01 Intro.flac`, IsAudio: true},
}}, dst, "", nil)
if err != nil {
t.Fatalf("collect: %v", err)
}
if len(got.Files) != 2 {
t.Fatalf("collected %d files, want 2", len(got.Files))
}
for _, disc := range []string{"CD1", "CD2"} {
data, err := os.ReadFile(filepath.Join(dst, disc, "01 Intro.flac"))
if err != nil || string(data) != disc {
t.Errorf("%s's file missing or overwritten: %q, %v", disc, data, err)
}
if hint := ParsePath(filepath.Join(dst, disc, "01 Intro.flac")); hint.Disc == 0 {
t.Errorf("staged %s file lost its disc number", disc)
}
}
}
// A two-disc release whose discs both number from 01 aligns completely
// once the disc comes from the folder; before, disc 2's 01 collided with
// disc 1's.
func TestMultiDiscCandidateAlignsEveryTrack(t *testing.T) {
t.Parallel()
dl := Download{
ReleaseMBID: "the-wall",
Artist: "Pink Floyd",
Album: "The Wall",
Expected: []ExpectedTrack{
{DiscNumber: 1, Position: 1, Title: "In the Flesh?"},
{DiscNumber: 1, Position: 2, Title: "The Thin Ice"},
{DiscNumber: 2, Position: 1, Title: "Hey You"},
{DiscNumber: 2, Position: 2, Title: "Is There Anybody Out There?"},
},
}
c := Candidate{
Title: "The Wall",
Files: []CandidateFile{
{Path: `\m\Pink Floyd - The Wall\CD1\01 In the Flesh.flac`, Size: 1},
{Path: `\m\Pink Floyd - The Wall\CD1\02 The Thin Ice.flac`, Size: 1},
{Path: `\m\Pink Floyd - The Wall\CD2\01 Hey You.flac`, Size: 1},
{Path: `\m\Pink Floyd - The Wall\CD2\02 Is There Anybody Out There.flac`, Size: 1},
},
}
got := Score(dl, c, 50, AutoDownloadPrefs{})
if got.Match.Completeness != 1 {
t.Errorf("completeness = %f, want 1", got.Match.Completeness)
}
if got.Match.AlbumFit < 0.99 {
t.Errorf("album fit = %f, want the album's own name to match", got.Match.AlbumFit)
}
if got.Match.Overall < minMatch {
t.Errorf("match = %f, want it to clear the auto-pick bar %f", got.Match.Overall, minMatch)
}
}
// Ten files against a ten-track album is not a complete album when only
// three of them are its tracks.
func TestCompletenessCountsTracksNotFiles(t *testing.T) {
t.Parallel()
dl := okComputer()
c := Candidate{Title: "OK Computer", Files: []CandidateFile{
{Path: `\m\Radiohead - OK Computer\Airbag.flac`, Size: 1},
{Path: `\m\Radiohead - OK Computer\Paranoid Android.flac`, Size: 1},
{Path: `\m\Radiohead - OK Computer\Exit Music (For a Film).flac`, Size: 1},
{Path: `\m\Radiohead - OK Computer\Creep.flac`, Size: 1},
}}
got := Score(dl, c, 50, AutoDownloadPrefs{})
if got.Match.Completeness > 0.76 {
t.Errorf(
"completeness = %f with 3 of 4 tracks present, want at most 0.75",
got.Match.Completeness,
)
}
}
+13 -12
View File
@@ -47,7 +47,7 @@ func grabAll(
go func() { go func() {
defer wg.Done() defer wg.Done()
f.manager.grab(ctx, dl, candidate, nil) f.manager.grab(ctx, dl, candidate, nil, false)
}() }()
} }
@@ -79,9 +79,9 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) {
want int want int
}{ }{
{ {
name: "slskd defaults to one", name: "slskd defaults to a few peers",
cfg: Config{Kind: KindSlskd}, cfg: Config{Kind: KindSlskd},
want: 1, want: 3,
}, },
{ {
name: "usenet defaults higher", name: "usenet defaults higher",
@@ -92,9 +92,9 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) {
name: "explicit override wins", name: "explicit override wins",
cfg: Config{ cfg: Config{
Kind: KindSlskd, Kind: KindSlskd,
Settings: map[string]string{concurrencyKey: "3"}, Settings: map[string]string{concurrencyKey: "1"},
}, },
want: 3, want: 1,
}, },
{ {
name: "nonsense override falls back", name: "nonsense override falls back",
@@ -102,7 +102,7 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) {
Kind: KindSlskd, Kind: KindSlskd,
Settings: map[string]string{concurrencyKey: "not a number"}, Settings: map[string]string{concurrencyKey: "not a number"},
}, },
want: 1, want: 3,
}, },
{ {
name: "zero override falls back", name: "zero override falls back",
@@ -110,7 +110,7 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) {
Kind: KindSlskd, Kind: KindSlskd,
Settings: map[string]string{concurrencyKey: "0"}, Settings: map[string]string{concurrencyKey: "0"},
}, },
want: 1, want: 3,
}, },
{ {
name: "unknown kind falls back to the global default", name: "unknown kind falls back to the global default",
@@ -126,9 +126,9 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) {
} }
} }
// The reason the per-provider cap exists: a Soulseek daemon capped at // The reason the per-provider cap exists: a daemon capped at one
// one transfer must serialize, even when the global cap would allow // transfer must serialize, even when the global cap would allow more and
// more and the user has queued several albums at once. // the user has queued several albums at once.
func TestPerProviderCapSerializesTransfers(t *testing.T) { func TestPerProviderCapSerializesTransfers(t *testing.T) {
t.Parallel() t.Parallel()
@@ -142,6 +142,7 @@ func TestPerProviderCapSerializesTransfers(t *testing.T) {
ID: 1, ID: 1,
Kind: KindSlskd, Kind: KindSlskd,
Priority: 50, Priority: 50,
Settings: map[string]string{concurrencyKey: "1"},
}, slow) }, slow)
// Three requests against the same one-at-a-time provider. // Three requests against the same one-at-a-time provider.
@@ -210,8 +211,8 @@ func TestSyncSemaphoresReplacesChangedLimits(t *testing.T) {
f.manager.installProvider(Config{ID: 1, Kind: KindSlskd}, nil) f.manager.installProvider(Config{ID: 1, Kind: KindSlskd}, nil)
first := f.manager.semaphoreFor(1) first := f.manager.semaphoreFor(1)
if cap(first) != 1 { if want := kindConcurrency[KindSlskd]; cap(first) != want {
t.Fatalf("slskd semaphore cap = %d, want 1", cap(first)) t.Fatalf("slskd semaphore cap = %d, want %d", cap(first), want)
} }
// Same limit: the semaphore is kept, so in-flight accounting is not // Same limit: the semaphore is kept, so in-flight accounting is not
+261
View File
@@ -0,0 +1,261 @@
package download
import (
"context"
"errors"
"os"
"testing"
)
// A transfer that fails on one copy of an album is not a failed
// download while another acceptable copy exists. On Soulseek the usual
// failure is one peer being offline, with several others offering the
// same folder.
var errPeerOffline = errors.New("peer went offline")
func TestManagerFallsBackToTheNextCandidate(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
// The failing source ranks first on priority, so the fallback is
// what reaches the one that works.
bad := fakeWithAlbum(1, "offline-peer", ".flac")
bad.GrabErr = errPeerOffline
good := fakeWithAlbum(2, "online-peer", ".flac")
f.manager.installProvider(Config{ID: 1, Priority: 90}, bad)
f.manager.installProvider(Config{ID: 2, Priority: 10}, good)
dl := fourTrackDownload()
if _, err := f.manager.Start(context.Background(), dl); err != nil {
t.Fatalf("Start: %v", err)
}
waitForDownloadState(t, f.store, dl.ID, StateComplete)
if bad.GrabCalls != 1 || good.GrabCalls != 1 {
t.Errorf(
"grabs: failing=%d working=%d, want 1 and 1",
bad.GrabCalls, good.GrabCalls,
)
}
// The abandoned attempt's staging goes with it; only a request that
// fails outright keeps its staging for inspection.
waitFor(t, func() bool {
entries, err := os.ReadDir(f.staging.Root())
return err == nil && len(entries) == 0
}, "the failed attempt's staging was never released")
}
// Falling back must not lower the bar. A second choice outside the
// user's guardrails is not a choice auto-pick may make, first or second.
func TestManagerFallbackRespectsTheGuardrails(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
f.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 50})
bad := fakeWithAlbum(1, "offline-peer", ".flac")
bad.GrabErr = errPeerOffline
bad.Candidates[0].TotalSize = 40 << 20
huge := fakeWithAlbum(2, "oversized", ".flac")
huge.Candidates[0].TotalSize = 900 << 20
f.manager.installProvider(Config{ID: 1, Priority: 90}, bad)
f.manager.installProvider(Config{ID: 2, Priority: 10}, huge)
dl := fourTrackDownload()
if _, err := f.manager.Start(context.Background(), dl); err != nil {
t.Fatalf("Start: %v", err)
}
waitForDownloadState(t, f.store, dl.ID, StateFailed)
if huge.GrabCalls != 0 {
t.Errorf("fell back to a candidate over the size ceiling")
}
}
// A copy the user picked by hand is the copy they asked for. Quietly
// substituting another is a decision they did not make.
func TestManagerPickDoesNotFallBack(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
bad := fakeWithAlbum(1, "offline-peer", ".flac")
bad.GrabErr = errPeerOffline
good := fakeWithAlbum(2, "online-peer", ".flac")
f.manager.installProvider(Config{ID: 1, Priority: 90}, bad)
f.manager.installProvider(Config{ID: 2, Priority: 10}, good)
// A ceiling below both copies parks the result set for the user.
f.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 1})
bad.Candidates[0].TotalSize = 30 << 20
good.Candidates[0].TotalSize = 30 << 20
dl := fourTrackDownload()
if _, err := f.manager.Start(context.Background(), dl); err != nil {
t.Fatalf("Start: %v", err)
}
if err := f.manager.Pick(
context.Background(), dl.ID, "offline-peer-cand",
); err != nil {
t.Fatalf("Pick: %v", err)
}
waitForDownloadState(t, f.store, dl.ID, StateFailed)
if good.GrabCalls != 0 {
t.Errorf("a hand-picked grab fell back to another candidate")
}
}
// Fallback is for surviving an offline peer or two, not for walking a
// forty-peer list for six hours.
func TestManagerFallbackIsBounded(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
var providers []*FakeProvider
for i := int64(1); i <= maxGrabAttempts+2; i++ {
p := fakeWithAlbum(i, "peer-"+itoa(int(i)), ".flac")
p.GrabErr = errPeerOffline
f.manager.installProvider(Config{ID: i, Priority: 50}, p)
providers = append(providers, p)
}
dl := fourTrackDownload()
if _, err := f.manager.Start(context.Background(), dl); err != nil {
t.Fatalf("Start: %v", err)
}
waitForDownloadState(t, f.store, dl.ID, StateFailed)
grabs := 0
for _, p := range providers {
grabs += p.GrabCalls
}
if grabs != maxGrabAttempts {
t.Errorf("grabs = %d, want %d", grabs, maxGrabAttempts)
}
}
// The veto judges the best candidate *inside* the guardrails, so the
// grab has to take that one — not the overall best, which may be the
// very copy the user said not to take unattended.
func TestManagerAutoPickTakesTheBestEligibleCandidate(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
f.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 50})
huge := fakeWithAlbum(1, "oversized", ".flac")
huge.Candidates[0].TotalSize = 900 << 20
fits := fakeWithAlbum(2, "fits", ".flac")
fits.Candidates[0].TotalSize = 40 << 20
f.manager.installProvider(Config{ID: 1, Priority: 90}, huge)
f.manager.installProvider(Config{ID: 2, Priority: 10}, fits)
dl := fourTrackDownload()
ranked, err := f.manager.Start(context.Background(), dl)
if err != nil {
t.Fatalf("Start: %v", err)
}
if ranked[0].ID != "oversized-cand" {
t.Fatalf("fixture: best overall is %s, want the oversized copy", ranked[0].ID)
}
waitForDownloadState(t, f.store, dl.ID, StateComplete)
if huge.GrabCalls != 0 || fits.GrabCalls != 1 {
t.Errorf(
"grabs: oversized=%d fits=%d, want 0 and 1",
huge.GrabCalls, fits.GrabCalls,
)
}
}
// On Soulseek a failure is the peer's, so every folder that peer offered
// goes with it. Elsewhere a failure is the release's, and one indexer's
// other releases are still worth trying.
func TestRuledOutBy(t *testing.T) {
t.Parallel()
failed := []Candidate{
{ID: "slskd:alice:Album", Kind: KindSlskd, ProviderID: 1, Origin: "alice"},
{ID: "tracker-1", Kind: KindProwlarr, ProviderID: 2, Origin: "indexer"},
}
cases := []struct {
name string
c Candidate
want bool
}{
{
name: "the same candidate",
c: Candidate{ID: "tracker-1", Kind: KindProwlarr, ProviderID: 2, Origin: "indexer"},
want: true,
},
{
name: "another folder from a failed peer",
c: Candidate{
ID: "slskd:alice:Album (2)",
Kind: KindSlskd,
ProviderID: 1,
Origin: "alice",
},
want: true,
},
{
name: "another peer",
c: Candidate{ID: "slskd:bob:Album", Kind: KindSlskd, ProviderID: 1, Origin: "bob"},
want: false,
},
{
name: "another release from the same indexer",
c: Candidate{ID: "tracker-2", Kind: KindProwlarr, ProviderID: 2, Origin: "indexer"},
want: false,
},
{
name: "a peer of the same name on a different daemon",
c: Candidate{
ID: "slskd:alice:Album",
Kind: KindSlskd,
ProviderID: 3,
Origin: "alice",
},
want: false,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
if got := ruledOutBy(tc.c, failed); got != tc.want {
t.Errorf("ruledOutBy = %v, want %v", got, tc.want)
}
})
}
}
+190
View File
@@ -0,0 +1,190 @@
package download
import (
"cmp"
"context"
"fmt"
"strings"
"yellowjacket/backend/jobs"
)
// Filling in an almost-complete album (#276).
//
// A grab that delivers nine of twelve tracks clears the completeness
// floor and is imported, and before this the other three were never
// looked for. On Soulseek that is the commonest way an album ends up
// almost right: one peer's folder is missing a track, or one file
// failed. So after an import, each missing track is searched for on
// its own and fetched from somewhere else, into the same album.
// maxFillInTracks bounds how many tracks are fetched one by one. An
// album missing more than a few is a different candidate's job, not a
// dozen single-track grabs.
const maxFillInTracks = 3
// fillIn fetches the tracks a successful import did not deliver. It
// never fails the download: the album is already imported, and a track
// it cannot find is logged and left.
func (m *Manager) fillIn(
ctx context.Context,
dl Download,
main Candidate,
imported ImportResult,
job *jobs.Handle,
) {
missing := missingTracks(dl, imported.Matched)
if len(missing) == 0 {
return
}
for _, t := range missing {
if ctx.Err() != nil {
return
}
paths, err := m.fillInTrack(ctx, dl, main, t, job)
if err != nil {
m.logger.Info(
"could not fill in a missing track",
"download", dl.ID,
"track", t.Title,
"error", err,
)
if job != nil {
job.Logf(jobs.LevelWarn, fmt.Sprintf(
"Could not find %q elsewhere: %v", t.Title, err,
))
}
continue
}
if job != nil {
job.Logf(jobs.LevelInfo, fmt.Sprintf(
"Filled in %q from another source (%d file)", t.Title, len(paths),
))
}
}
}
// missingTracks is what an import left out, when filling it in is
// worth trying: an album with a tracklist, a few tracks short.
func missingTracks(dl Download, matched []ExpectedTrack) []ExpectedTrack {
// A recording request is one track; there is no album to complete.
if dl.RecordingMBID != "" || len(dl.Expected) < 2 {
return nil
}
have := make(map[trackKey]bool, len(matched))
for _, t := range matched {
have[keyOf(t)] = true
}
var missing []ExpectedTrack
for _, t := range dl.Expected {
if !have[keyOf(t)] {
missing = append(missing, t)
}
}
// A half-empty album was a poor copy, not a nearly complete one.
if len(missing) > maxFillInTracks || 2*len(missing) >= len(dl.Expected) {
return nil
}
return missing
}
// fillInTrack searches for one track and grabs the first acceptable
// copy that is not from the source that already failed to supply it.
// One attempt: a fill-in that walks a candidate list per track would
// multiply a download's grabs by the number of gaps.
func (m *Manager) fillInTrack(
ctx context.Context,
dl Download,
main Candidate,
t ExpectedTrack,
job *jobs.Handle,
) ([]string, error) {
want := trackRequest(dl, t)
ranked, err := m.Search(ctx, want)
if err != nil {
return nil, err
}
prefs := m.preferences()
for _, c := range ranked {
if ruledOutBy(c, []Candidate{main}) || !autoAcceptable(want, c, prefs) {
continue
}
// The track is being put into an album this app placed; a
// delegate would put it in its own library instead.
if plan, err := m.planTransfer(want, c); err != nil || plan.delegated() {
continue
}
narrowed, ok := narrowTo(c, t)
if !ok {
continue
}
out := m.attemptGrab(ctx, dl, narrowed, job, []ExpectedTrack{t})
if out.item.StagingDir != "" {
if err := m.staging.Release(out.item.StagingDir); err != nil {
m.logger.Warn("could not release staging dir", "error", err)
}
}
if out.err != nil {
return nil, out.err
}
if err := m.store.SetItemImported(
ctx, out.item.ID, out.imported.Paths,
); err != nil {
m.logger.Warn("could not record imported paths", "error", err)
}
return out.imported.Paths, nil
}
return nil, ErrNoCandidates
}
// trackRequest is the search for one track of an album: the track's
// artist and title as the query, the album kept so a copy from that
// album outranks the same song off a compilation, and one expected
// track so a single file is a complete answer.
func trackRequest(dl Download, t ExpectedTrack) Download {
artist := cmp.Or(t.Artist, dl.Artist)
want := dl
want.Query = strings.TrimSpace(artist + " " + t.Title)
want.Expected = []ExpectedTrack{t}
return want
}
// narrowTo trims a candidate to the one file that aligns to t, so the
// grab fetches a track rather than whatever else the folder offered.
func narrowTo(c Candidate, t ExpectedTrack) (Candidate, bool) {
aligned, _ := matchFiles(c.Files, []ExpectedTrack{t})
for _, f := range aligned {
if f.IsAudio && f.MatchedTo == t.Position {
c.Files = []CandidateFile{f}
c.TotalSize = f.Size
return c, true
}
}
return Candidate{}, false
}
+168
View File
@@ -0,0 +1,168 @@
package download
import (
"context"
"errors"
"os"
"path/filepath"
"testing"
)
// Filling in the tracks an almost-complete album is missing (#276).
func fiveTrackTitles() []string {
return append(allTitles(), "Let Down")
}
func fiveTrackDownload() Download {
dl := fourTrackDownload()
dl.Expected = append(dl.Expected, ExpectedTrack{Position: 5, Title: "Let Down"})
return dl
}
func TestMissingTracks(t *testing.T) {
t.Parallel()
dl := fiveTrackDownload()
got := func(positions ...int) []ExpectedTrack {
out := make([]ExpectedTrack, 0, len(positions))
for _, p := range positions {
out = append(out, dl.Expected[p-1])
}
return out
}
cases := []struct {
name string
dl Download
matched []ExpectedTrack
want int
}{
{"complete", dl, got(1, 2, 3, 4, 5), 0},
{"one short", dl, got(1, 2, 3, 4), 1},
{"two short", dl, got(1, 2, 3), 2},
{"half gone is a poor copy", dl, got(1, 2), 0},
{"a recording is not an album", func() Download {
d := dl
d.RecordingMBID = "rec"
return d
}(), got(1, 2, 3, 4), 0},
}
for _, tc := range cases {
if n := len(missingTracks(tc.dl, tc.matched)); n != tc.want {
t.Errorf("%s: %d missing, want %d", tc.name, n, tc.want)
}
}
big := fiveTrackDownload()
for i := 6; i <= 20; i++ {
big.Expected = append(big.Expected, ExpectedTrack{Position: i, Title: "T" + itoa(i)})
}
if n := len(missingTracks(big, big.Expected[:16])); n != 0 {
t.Errorf("four of twenty missing: %d filled in, want none past %d", n, maxFillInTracks)
}
}
// A fill-in import takes only the file that is the missing track, and
// places it in the album with the album's tags; anything else the grab
// brought is left out.
func TestImportOnlyTakesTheMissingTrack(t *testing.T) {
t.Parallel()
f := newImportFixture(t,
"05 - Let Down.flac",
"02 - Paranoid Android.flac",
)
dl := fiveTrackDownload()
got, err := f.importer.Import(
context.Background(), dl,
Result{Dir: f.dir, Files: f.files},
ImportOptions{LibraryRoot: f.root, WriteTags: true, Only: dl.Expected[4:]},
)
if err != nil {
t.Fatalf("Import: %v", err)
}
want := filepath.Join(f.root, "Radiohead", "OK Computer", "05 Let Down.flac")
if len(got.Paths) != 1 || got.Paths[0] != want {
t.Errorf("imported %q, want only %s", got.Paths, want)
}
// A file that is not the missing track is not imported at all.
g := newImportFixture(t, "02 - Paranoid Android.flac")
if _, err := g.importer.Import(
context.Background(), dl,
Result{Dir: g.dir, Files: g.files},
ImportOptions{LibraryRoot: g.root, WriteTags: true, Only: dl.Expected[4:]},
); !errors.Is(err, ErrTooIncomplete) {
t.Errorf("Import = %v, want nothing matched", err)
}
}
// The album comes from one source missing its fifth track; the fifth is
// then found on its own at another and lands in the same album.
func TestManagerFillsInAMissingTrack(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
titles := fiveTrackTitles()
album := NewFakeProvider(1, "album", Caps{CanSearch: true, CanTransport: true})
ac := candidateFor("album-cand", titles, ".flac", 30_000_000)
ac.ProviderID = 1
album.Candidates = []Candidate{ac}
for i, tt := range titles[:4] {
album.Written[trackToken(i+1)+" - "+tt+".flac"] = []byte("audio-data")
}
single := NewFakeProvider(2, "single", Caps{CanSearch: true, CanTransport: true})
sc := Candidate{
ID: "single-cand",
Protocol: ProtocolDirect,
Title: "Radiohead - OK Computer",
Artist: "Radiohead",
Files: []CandidateFile{{
Path: "Radiohead - OK Computer/05 - Let Down.flac",
Size: 30_000_000,
}},
Health: 0.5,
ProviderID: 2,
}
single.Candidates = []Candidate{sc}
single.Written["05 - Let Down.flac"] = []byte("audio-data")
f.manager.installProvider(Config{ID: 1, Priority: 90}, album)
f.manager.installProvider(Config{ID: 2, Priority: 10}, single)
dl := fiveTrackDownload()
if _, err := f.manager.Start(context.Background(), dl); err != nil {
t.Fatalf("Start: %v", err)
}
waitForDownloadState(t, f.store, dl.ID, StateComplete)
if album.GrabCallCount() != 1 || single.GrabCallCount() != 1 {
t.Errorf(
"grabs: album=%d single=%d, want 1 and 1",
album.GrabCallCount(), single.GrabCallCount(),
)
}
for i, tt := range titles {
p := filepath.Join(f.root, "Radiohead", "OK Computer", trackToken(i+1)+" "+tt+".flac")
if _, err := os.Stat(p); err != nil {
t.Errorf("track %d not in the library: %v", i+1, err)
}
}
}
+61 -2
View File
@@ -79,6 +79,12 @@ type ImportOptions struct {
// them. Off for delegate providers, which have already imported // them. Off for delegate providers, which have already imported
// and tagged the files themselves. // and tagged the files themselves.
WriteTags bool WriteTags bool
// Only, when set, imports just the files that align to these
// tracks of the download and skips the completeness check: it is a
// fill-in for tracks an earlier grab of the same album did not
// deliver (#276), tagged and placed as part of that album.
Only []ExpectedTrack
} }
// DefaultPathTemplate is the layout used when none is configured. // DefaultPathTemplate is the layout used when none is configured.
@@ -118,6 +124,10 @@ type ImportResult struct {
// Skipped counts non-audio files left in staging (logs, cue sheets, // Skipped counts non-audio files left in staging (logs, cue sheets,
// scene .nfo files) — deliberately not imported. // scene .nfo files) — deliberately not imported.
Skipped int Skipped int
// Matched are the expected tracks an imported file was aligned to,
// which is how a caller learns what the grab did not deliver.
Matched []ExpectedTrack
} }
// Import verifies, tags and moves a completed grab into the library. // Import verifies, tags and moves a completed grab into the library.
@@ -140,14 +150,25 @@ func (i *Importer) Import(
return ImportResult{}, ErrNoAudio return ImportResult{}, ErrNoAudio
} }
if err := checkCompleteness(len(audio), dl); err != nil { if len(opts.Only) == 0 {
return ImportResult{}, err if err := checkCompleteness(len(audio), dl); err != nil {
return ImportResult{}, err
}
} }
// Align staged files to the expected tracklist so tags and // Align staged files to the expected tracklist so tags and
// filenames reflect the release, not the uploader's naming. // filenames reflect the release, not the uploader's naming.
plan := i.planFiles(audio, dl) plan := i.planFiles(audio, dl)
if len(opts.Only) > 0 {
plan = onlyTracks(plan, opts.Only)
if len(plan) == 0 {
return ImportResult{}, fmt.Errorf(
"%w: no file matched the missing track", ErrTooIncomplete,
)
}
}
out := ImportResult{ out := ImportResult{
Paths: make([]string, 0, len(plan)), Paths: make([]string, 0, len(plan)),
Skipped: skipped, Skipped: skipped,
@@ -184,11 +205,49 @@ func (i *Importer) Import(
} }
out.Paths = append(out.Paths, dest) out.Paths = append(out.Paths, dest)
if p.Matched {
out.Matched = append(out.Matched, p.Track)
}
} }
return out, nil return out, nil
} }
// trackKey identifies an expected track within a release.
type trackKey struct{ disc, position int }
func keyOf(t ExpectedTrack) trackKey {
return trackKey{disc: t.DiscNumber, position: t.Position}
}
// onlyTracks keeps the planned files aligned to one of want. A fill-in
// grab can bring more than the one file it was after — a folder where
// the title also matched a live take — and anything else would land in
// the album as a duplicate or a stranger.
func onlyTracks(plan []plannedFile, want []ExpectedTrack) []plannedFile {
keys := make(map[trackKey]bool, len(want))
for _, t := range want {
keys[keyOf(t)] = true
}
out := make([]plannedFile, 0, len(want))
seen := map[trackKey]bool{}
for _, p := range plan {
k := keyOf(p.Track)
if !p.Matched || !keys[k] || seen[k] {
continue
}
seen[k] = true
out = append(out, p)
}
return out
}
// plannedFile pairs a staged file with the expected track it matched. // plannedFile pairs a staged file with the expected track it matched.
type plannedFile struct { type plannedFile struct {
Source string Source string
+77
View File
@@ -0,0 +1,77 @@
package download
import (
"context"
"sync"
)
// keyedLock is a set of mutexes created on demand, one per key, that
// honour a context while waiting. An entry lives only while someone
// holds or waits on it, so a key per Soulseek peer or per folder name
// does not accumulate for the life of the process.
type keyedLock[K comparable] struct {
mu sync.Mutex
held map[K]*keyedEntry
}
type keyedEntry struct {
ch chan struct{}
// refs counts holders and waiters; the entry is dropped at zero.
refs int
}
// acquire blocks until k is free or ctx ends, and returns the function
// that frees it.
func (l *keyedLock[K]) acquire(ctx context.Context, k K) (func(), error) {
l.mu.Lock()
if l.held == nil {
l.held = map[K]*keyedEntry{}
}
e, ok := l.held[k]
if !ok {
e = &keyedEntry{ch: make(chan struct{}, 1)}
l.held[k] = e
}
e.refs++
l.mu.Unlock()
select {
case e.ch <- struct{}{}:
case <-ctx.Done():
l.drop(k, e)
return nil, ctx.Err()
}
var once sync.Once
return func() {
once.Do(func() {
<-e.ch
l.drop(k, e)
})
}, nil
}
func (l *keyedLock[K]) drop(k K, e *keyedEntry) {
l.mu.Lock()
defer l.mu.Unlock()
e.refs--
if e.refs == 0 {
delete(l.held, k)
}
}
// size reports how many keys are held or awaited, for tests.
func (l *keyedLock[K]) size() int {
l.mu.Lock()
defer l.mu.Unlock()
return len(l.held)
}
+248 -65
View File
@@ -59,13 +59,15 @@ const concurrencyKey = "maxConcurrent"
// A single global cap is the wrong shape here: usenet and torrent // A single global cap is the wrong shape here: usenet and torrent
// clients are built to run many transfers at once and are throttled by // clients are built to run many transfers at once and are throttled by
// bandwidth, while Soulseek transfers come from one person's home // bandwidth, while Soulseek transfers come from one person's home
// upload slot. Hitting the same peer with parallel requests gets you // upload slot. Politeness there is per *peer* — asking one user for two
// queued behind everyone else at best and banned at worst, so slskd is // folders at once gets you queued behind everyone else at best and
// capped at one — the polite number, and the one that actually // banned at worst — and the manager holds that line separately, one
// completes fastest, because a Soulseek peer serves one file at a time // grab per peer (peerLocks). Two different users do not compete for
// regardless of how many you ask for. // anyone's slot, so the daemon-wide number only bounds how many peers
// are asked at once, and one slow peer no longer serialises every other
// Soulseek download behind it.
var kindConcurrency = map[Kind]int{ var kindConcurrency = map[Kind]int{
KindSlskd: 1, KindSlskd: 3,
KindYtDlp: 2, KindYtDlp: 2,
KindQBittorrent: 4, KindQBittorrent: 4,
KindSABnzbd: 4, KindSABnzbd: 4,
@@ -155,6 +157,11 @@ type Manager struct {
semMu sync.Mutex semMu sync.Mutex
provSem map[int64]chan struct{} provSem map[int64]chan struct{}
// peerLocks holds one grab per Soulseek peer, taken before any
// slot: a grab waiting for a busy peer must not sit on a provider
// slot another peer could be using.
peerLocks keyedLock[peerKey]
// delegatePoll is how often delegating managers are asked for // delegatePoll is how often delegating managers are asked for
// status. A field rather than the constant so tests can drive the // status. A field rather than the constant so tests can drive the
// full delegate flow without sleeping through it. // full delegate flow without sleeping through it.
@@ -577,12 +584,12 @@ func (m *Manager) Start(
)) ))
} }
if m.AutoPickable(dl, ranked) { if pick, ok := autoPick(dl, ranked, m.preferences()); ok {
if job != nil { if job != nil {
job.Logf(jobs.LevelInfo, "Auto-selected best candidate") job.Logf(jobs.LevelInfo, "Auto-selected best candidate")
} }
go m.grab(context.WithoutCancel(ctx), dl, ranked[0], job) go m.grab(context.WithoutCancel(ctx), dl, pick, job, true)
return ranked, nil return ranked, nil
} }
@@ -622,6 +629,13 @@ func (m *Manager) Attempt(
return false, veto, nil return false, veto, nil
} }
pick, ok := autoPick(dl, ranked, m.preferences())
if !ok {
// Unreachable while autoPick and AutoPickVeto agree; kept so a
// future divergence refuses rather than grabbing blind.
return false, "no candidate clears the auto-download bar", nil
}
if err := m.store.CreateDownload(ctx, dl); err != nil { if err := m.store.CreateDownload(ctx, dl); err != nil {
return false, "", err return false, "", err
} }
@@ -643,7 +657,7 @@ func (m *Manager) Attempt(
)) ))
} }
go m.grab(context.WithoutCancel(ctx), dl, ranked[0], job) go m.grab(context.WithoutCancel(ctx), dl, pick, job, true)
return true, "", nil return true, "", nil
} }
@@ -678,7 +692,7 @@ func (m *Manager) Pick(
job := m.startJob(dl) job := m.startJob(dl)
go m.grab(context.WithoutCancel(ctx), dl, *chosen, job) go m.grab(context.WithoutCancel(ctx), dl, *chosen, job, false)
return nil return nil
} }
@@ -702,13 +716,20 @@ func (m *Manager) Cancel(ctx context.Context, downloadID string) error {
return nil return nil
} }
// grab drives one candidate all the way to the library. It runs on its // grab drives one request all the way to the library. It runs on its
// own goroutine and owns the job from here on. // own goroutine and owns the job from here on.
//
// When fallback is set and a candidate's transfer fails, the next
// candidate that auto-pick would itself have accepted is tried in its
// place (see nextCandidate). It is set for the two unattended routes
// and not for a candidate the user picked by hand: they chose that copy,
// and quietly substituting another is a decision they did not make.
func (m *Manager) grab( func (m *Manager) grab(
ctx context.Context, ctx context.Context,
dl Download, dl Download,
c Candidate, c Candidate,
job *jobs.Handle, job *jobs.Handle,
fallback bool,
) { ) {
ctx, cancel := context.WithTimeout(ctx, grabTimeout) ctx, cancel := context.WithTimeout(ctx, grabTimeout)
defer cancel() defer cancel()
@@ -723,6 +744,101 @@ func (m *Manager) grab(
m.actMu.Unlock() m.actMu.Unlock()
}() }()
var failed []Candidate
for {
out := m.attemptGrab(ctx, dl, c, job, nil)
if out.err == nil {
m.fillIn(ctx, dl, c, out.imported, job)
m.finishGrab(ctx, dl, out.item, out.imported, job)
return
}
failed = append(failed, c)
next, ok := m.nextCandidate(ctx, dl, failed, out, fallback)
if !ok {
m.failDownload(ctx, job, dl.ID, out.err)
return
}
m.logger.Info(
"download candidate failed; trying the next",
"download", dl.ID,
"failed", c.ID,
"next", next.ID,
"error", out.err,
)
if job != nil {
job.Logf(jobs.LevelWarn, fmt.Sprintf(
"%s failed (%v); trying %s instead",
describeCandidate(c), out.err, describeCandidate(next),
))
}
// The failed attempt's staging holds at most a partial folder
// nobody is going to import, and the next attempt reserves its
// own. Only the final failure keeps its staging for inspection.
if out.item.StagingDir != "" {
if err := m.staging.Release(out.item.StagingDir); err != nil {
m.logger.Warn("could not release staging dir", "error", err)
}
}
c = next
}
}
// maxGrabAttempts bounds how many candidates one request will try. A
// popular album can have dozens of peers; the point of falling back is
// to survive the ordinary one or two that are offline, not to walk the
// whole list for six hours.
const maxGrabAttempts = 3
// peerKey names one Soulseek user on one daemon. The same username on
// two daemons is two logins and two queues.
type peerKey struct {
provider int64
peer string
}
// peerKeyFor returns the peer a candidate is fetched from, when the
// source is one where asking a peer for two things at once is rude.
func peerKeyFor(c Candidate) (peerKey, bool) {
if c.Kind != KindSlskd || c.Origin == "" {
return peerKey{}, false
}
return peerKey{provider: c.ProviderID, peer: c.Origin}, true
}
// grabOutcome is how one candidate's attempt ended.
type grabOutcome struct {
item DownloadItem
imported ImportResult
err error
// retryable reports whether another candidate might succeed where
// this one failed: the transfer failed, or delivered too little of
// the album. Anything else — no staging space, no library root, a
// tag write failing — would fail the next candidate identically.
retryable bool
}
// attemptGrab takes one candidate through transfer and import. It
// records the item's own failure, but not the download's: whether the
// download has failed is the caller's decision, since another candidate
// may yet succeed.
func (m *Manager) attemptGrab(
ctx context.Context,
dl Download,
c Candidate,
job *jobs.Handle,
only []ExpectedTrack,
) grabOutcome {
// Who will move the bytes is decided before any slot is taken, so // Who will move the bytes is decided before any slot is taken, so
// the transfer waits in its own provider's queue rather than in a // the transfer waits in its own provider's queue rather than in a
// global one. A delegate takes no slot at all: the transfer is // global one. A delegate takes no slot at all: the transfer is
@@ -731,21 +847,26 @@ func (m *Manager) grab(
// work against our budget. // work against our budget.
plan, err := m.planTransfer(dl, c) plan, err := m.planTransfer(dl, c)
if err != nil { if err != nil {
m.failDownload(ctx, job, dl.ID, err) return grabOutcome{err: err}
return
} }
if !plan.delegated() { if !plan.delegated() {
if key, ok := peerKeyFor(c); ok {
release, err := m.peerLocks.acquire(ctx, key)
if err != nil {
return grabOutcome{err: err}
}
defer release()
}
provSem := m.semaphoreFor(plan.transportID) provSem := m.semaphoreFor(plan.transportID)
select { select {
case provSem <- struct{}{}: case provSem <- struct{}{}:
defer func() { <-provSem }() defer func() { <-provSem }()
case <-ctx.Done(): case <-ctx.Done():
m.failDownload(ctx, job, dl.ID, ctx.Err()) return grabOutcome{err: ctx.Err()}
return
} }
globalSem := m.globalSem() globalSem := m.globalSem()
@@ -754,9 +875,7 @@ func (m *Manager) grab(
case globalSem <- struct{}{}: case globalSem <- struct{}{}:
defer func() { <-globalSem }() defer func() { <-globalSem }()
case <-ctx.Done(): case <-ctx.Done():
m.failDownload(ctx, job, dl.ID, ctx.Err()) return grabOutcome{err: ctx.Err()}
return
} }
} }
@@ -771,24 +890,30 @@ func (m *Manager) grab(
dir, err := m.staging.Reserve(item.ID) dir, err := m.staging.Reserve(item.ID)
if err != nil { if err != nil {
m.failDownload(ctx, job, dl.ID, err) return grabOutcome{err: err}
return
} }
item.StagingDir = dir item.StagingDir = dir
if err := m.store.CreateItem(ctx, item); err != nil { if err := m.store.CreateItem(ctx, item); err != nil {
m.failDownload(ctx, job, dl.ID, err) return grabOutcome{item: item, err: err}
}
return fail := func(err error, retryable bool) grabOutcome {
if serr := m.store.SetItemState(
ctx, item.ID, StateFailed, err.Error(),
); serr != nil {
m.logger.Warn("could not record item failure", "error", serr)
}
return grabOutcome{item: item, err: err, retryable: retryable}
} }
result, err := m.transfer(ctx, dl, item, plan, job) result, err := m.transfer(ctx, dl, item, plan, job)
if err != nil { if err != nil {
m.failItem(ctx, job, item, dl.ID, err) // A delegate's failure is the external manager's verdict on the
// whole request, not on one copy of it.
return return fail(err, !plan.delegated())
} }
m.setStates(ctx, dl.ID, item.ID, StateImporting) m.setStates(ctx, dl.ID, item.ID, StateImporting)
@@ -798,42 +923,117 @@ func (m *Manager) grab(
job.SetStages(importStages(2)) job.SetStages(importStages(2))
} }
var imported ImportResult
if result.Delegated { if result.Delegated {
// The external manager already placed and tagged these files in // The external manager already placed and tagged these files in
// its own library. Moving them out from under a system that is // its own library. Moving them out from under a system that is
// still managing them would be worse than useless, so the files // still managing them would be worse than useless, so the files
// are recorded where they are and the library scan picks them // are recorded where they are and the library scan picks them
// up in place. // up in place.
imported = ImportResult{Paths: result.Files}
if job != nil { if job != nil {
job.Logf(jobs.LevelInfo, fmt.Sprintf( job.Logf(jobs.LevelInfo, fmt.Sprintf(
"External manager imported %d files; recording them in place", "External manager imported %d files; recording them in place",
len(result.Files), len(result.Files),
)) ))
} }
} else {
opts := m.importOptions()
opts.WriteTags = true
opts.LibraryRoot, err = m.library.LibraryPath(dl.LibraryID) return grabOutcome{
if err != nil { item: item,
m.failItem(ctx, job, item, dl.ID, imported: ImportResult{Paths: result.Files},
fmt.Errorf("resolve library root: %w", err))
return
}
imported, err = m.importer.Import(ctx, dl, result, opts)
if err != nil {
m.failItem(ctx, job, item, dl.ID, err)
return
} }
} }
opts := m.importOptions()
opts.WriteTags = true
opts.Only = only
opts.LibraryRoot, err = m.library.LibraryPath(dl.LibraryID)
if err != nil {
return fail(fmt.Errorf("resolve library root: %w", err), false)
}
imported, err := m.importer.Import(ctx, dl, result, opts)
if err != nil {
return fail(err, errors.Is(err, ErrTooIncomplete))
}
return grabOutcome{item: item, imported: imported}
}
// nextCandidate picks the candidate to try after the ones in failed.
//
// It only ever offers a candidate auto-pick would have taken on its own
// (autoAcceptable), so falling back cannot lower the bar an unattended
// download is held to: the second choice has to clear the same gates
// the first did.
//
// On Soulseek a failure belongs to the *peer* — offline, refusing, or
// holding us in a queue — so every folder that peer offered is skipped
// with it. Elsewhere a failure belongs to the release, and only that
// candidate is.
func (m *Manager) nextCandidate(
ctx context.Context,
dl Download,
failed []Candidate,
out grabOutcome,
fallback bool,
) (Candidate, bool) {
if !fallback || !out.retryable || ctx.Err() != nil ||
len(failed) >= maxGrabAttempts {
return Candidate{}, false
}
m.resMu.RLock()
ranked := m.results[dl.ID]
m.resMu.RUnlock()
prefs := m.preferences()
for _, c := range ranked {
if ruledOutBy(c, failed) || !autoAcceptable(dl, c, prefs) {
continue
}
return c, true
}
return Candidate{}, false
}
// ruledOutBy reports whether a failure among failed also rules out c.
func ruledOutBy(c Candidate, failed []Candidate) bool {
for _, f := range failed {
if c.ID == f.ID && c.ProviderID == f.ProviderID {
return true
}
if c.Kind == KindSlskd && f.Kind == KindSlskd &&
c.ProviderID == f.ProviderID && c.Origin != "" &&
c.Origin == f.Origin {
return true
}
}
return false
}
// describeCandidate names a candidate for the job log.
func describeCandidate(c Candidate) string {
if c.Origin != "" {
return fmt.Sprintf("%q from %s", c.Title, c.Origin)
}
return fmt.Sprintf("%q", c.Title)
}
// finishGrab records a successful import and retires what the request
// was holding.
func (m *Manager) finishGrab(
ctx context.Context,
dl Download,
item DownloadItem,
imported ImportResult,
job *jobs.Handle,
) {
if err := m.store.SetItemImported( if err := m.store.SetItemImported(
ctx, item.ID, imported.Paths, ctx, item.ID, imported.Paths,
); err != nil { ); err != nil {
@@ -1177,23 +1377,6 @@ func (m *Manager) failDownload(
} }
} }
// failItem records an item-level failure and fails its download.
func (m *Manager) failItem(
ctx context.Context,
job *jobs.Handle,
item DownloadItem,
downloadID string,
err error,
) {
if serr := m.store.SetItemState(
ctx, item.ID, StateFailed, err.Error(),
); serr != nil {
m.logger.Warn("could not record item failure", "error", serr)
}
m.failDownload(ctx, job, downloadID, err)
}
// startJob registers the request in the background jobs panel. // startJob registers the request in the background jobs panel.
func (m *Manager) startJob(dl Download) *jobs.Handle { func (m *Manager) startJob(dl Download) *jobs.Handle {
if m.jobsReg == nil { if m.jobsReg == nil {
+140 -8
View File
@@ -66,6 +66,14 @@ var (
// separatorPattern splits "Artist - Album" style folder names. // separatorPattern splits "Artist - Album" style folder names.
separatorPattern = regexp.MustCompile(`\s+[-–—]\s+`) separatorPattern = regexp.MustCompile(`\s+[-–—]\s+`)
// discFolderPattern matches a directory that holds one disc of an
// album rather than the album: "CD1", "CD 2", "Disc 3", "Disk-1",
// "[Disc 2]", "CD1 - The Early Years". A number is required, so a
// folder merely called "CDs" is not one.
discFolderPattern = regexp.MustCompile(
`(?i)^\s*[\[(]?\s*(?:cd|disc|disk)\s*[-_.#]?\s*(\d{1,2})\b`,
)
) )
// FormatForPath returns the audio format implied by a path's extension, // FormatForPath returns the audio format implied by a path's extension,
@@ -94,16 +102,63 @@ type TrackHint struct {
Folder string Folder string
} }
// discFolder reports whether a directory name is one disc of an album,
// and which.
func discFolder(name string) (int, bool) {
m := discFolderPattern.FindStringSubmatch(name)
if m == nil {
return 0, false
}
n, err := strconv.Atoi(m[1])
if err != nil || n == 0 {
return 0, false
}
return n, true
}
// AlbumDir is the directory that holds a file's *album*: its parent,
// or its grandparent when the parent is a disc folder.
//
// Multi-disc rips are shared as `Album/CD1/…` and `Album/CD2/…`, and
// grouping candidates by the immediate parent split one album into two
// half-albums, each titled "CD1". Neither could clear the completeness
// or album-title bars, so a multi-disc release could not be auto-picked
// at all. A disc folder at the root has no album above it and is
// returned as it is.
func AlbumDir(p string) string {
dir := path.Dir(strings.ReplaceAll(p, `\`, "/"))
if _, ok := discFolder(path.Base(dir)); !ok {
return dir
}
parent := path.Dir(dir)
if parent == "." || parent == "/" || parent == "" {
return dir
}
return parent
}
// ParsePath extracts what it can from one candidate file path. // ParsePath extracts what it can from one candidate file path.
func ParsePath(p string) TrackHint { func ParsePath(p string) TrackHint {
// Soulseek paths are Windows-style; normalize before splitting. // Soulseek paths are Windows-style; normalize before splitting.
norm := strings.ReplaceAll(p, `\`, "/") norm := strings.ReplaceAll(p, `\`, "/")
base := path.Base(norm) base := path.Base(norm)
folder := path.Base(path.Dir(norm))
name := strings.TrimSuffix(base, path.Ext(base)) name := strings.TrimSuffix(base, path.Ext(base))
hint := TrackHint{Folder: cleanAlbumName(folder)} // The album's name is the album directory's, not a disc folder's,
// and the disc folder is where a multi-disc rip says which disc a
// file is on. A disc number in the filename ("2-01 …") is more
// specific and overrides it below.
hint := TrackHint{Folder: cleanAlbumName(path.Base(AlbumDir(norm)))}
if disc, ok := discFolder(path.Base(path.Dir(norm))); ok {
hint.Disc = disc
}
if m := trackNumPattern.FindStringSubmatch(name); m != nil { if m := trackNumPattern.FindStringSubmatch(name); m != nil {
if m[1] != "" { if m[1] != "" {
@@ -203,21 +258,72 @@ func AnnotateFiles(files []CandidateFile) []CandidateFile {
// matchFiles aligns a candidate's audio files to the expected tracklist // matchFiles aligns a candidate's audio files to the expected tracklist
// and returns the per-file assignment plus the mean title similarity of // and returns the per-file assignment plus the mean title similarity of
// the aligned pairs. // the aligned pairs. alignFiles is the same alignment with the
// duration evidence as well.
func matchFiles(
files []CandidateFile,
expected []ExpectedTrack,
) ([]CandidateFile, float64) {
a := alignFiles(files, expected)
return a.files, a.titleFit
}
// alignment is what aligning a candidate to a tracklist found.
type alignment struct {
files []CandidateFile
// titleFit is the mean title similarity over aligned pairs.
titleFit float64
// durationFit is the mean duration agreement over aligned pairs
// where both sides state a length, and timedPairs is how many such
// pairs there were.
durationFit float64
timedPairs int
aligned int
}
// durationAgreement scores how well a file's length matches the
// expected track's, in 0..1. Rips of the same master differ by a
// second or two of silence; a different edit, a live take or a
// truncated file differs by tens of seconds.
func durationAgreement(got, want int64) float64 {
const (
exactMillis = 3_000
wrongMillis = 30_000
)
d := got - want
if d < 0 {
d = -d
}
switch {
case d <= exactMillis:
return 1
case d >= wrongMillis:
return 0
default:
return 1 - float64(d-exactMillis)/float64(wrongMillis-exactMillis)
}
}
// alignFiles aligns a candidate's audio files to the expected tracklist.
// //
// Alignment is greedy by score rather than optimal: candidate folders // Alignment is greedy by score rather than optimal: candidate folders
// are small (a few dozen files at most) and the common cases — correct // are small (a few dozen files at most) and the common cases — correct
// track numbers, or clean "NN Title" names — are unambiguous, so the // track numbers, or clean "NN Title" names — are unambiguous, so the
// extra machinery of Hungarian assignment buys nothing here. // extra machinery of Hungarian assignment buys nothing here.
func matchFiles( func alignFiles(
files []CandidateFile, files []CandidateFile,
expected []ExpectedTrack, expected []ExpectedTrack,
) ([]CandidateFile, float64) { ) alignment {
annotated := make([]CandidateFile, len(files)) annotated := make([]CandidateFile, len(files))
copy(annotated, files) copy(annotated, files)
if len(expected) == 0 { if len(expected) == 0 {
return annotated, 0 return alignment{files: annotated}
} }
hints := make([]TrackHint, len(annotated)) hints := make([]TrackHint, len(annotated))
@@ -230,8 +336,19 @@ func matchFiles(
var ( var (
total float64 total float64
matched int matched int
durTotal float64
timed int
) )
// timing adds a pair's duration evidence when both sides state one.
timing := func(f CandidateFile, e ExpectedTrack) {
if f.LengthMillis > 0 && e.LengthMillis > 0 {
durTotal += durationAgreement(f.LengthMillis, e.LengthMillis)
timed++
}
}
// Pass 1: trust explicit track numbers when they are unique and in // Pass 1: trust explicit track numbers when they are unique and in
// range. A folder that numbers its files correctly is the strong // range. A folder that numbers its files correctly is the strong
// case, and title comparison only adds noise there. // case, and title comparison only adds noise there.
@@ -250,6 +367,8 @@ func matchFiles(
total += autotag.TitleSimilarity(hints[i].Title, expected[idx].Title) total += autotag.TitleSimilarity(hints[i].Title, expected[idx].Title)
matched++ matched++
timing(annotated[i], expected[idx])
} }
// Pass 2: title similarity for whatever is left. // Pass 2: title similarity for whatever is left.
@@ -284,13 +403,26 @@ func matchFiles(
total += bestSim total += bestSim
matched++ matched++
timing(annotated[i], expected[bestIdx])
} }
if matched == 0 { if matched == 0 {
return annotated, 0 return alignment{files: annotated}
} }
return annotated, total / float64(matched) a := alignment{
files: annotated,
titleFit: total / float64(matched),
timedPairs: timed,
aligned: matched,
}
if timed > 0 {
a.durationFit = durTotal / float64(timed)
}
return a
} }
// indexForPosition finds the expected track at a disc/track position. // indexForPosition finds the expected track at a disc/track position.
+224
View File
@@ -0,0 +1,224 @@
package download
import (
"context"
"errors"
"path/filepath"
"sync"
"testing"
"time"
)
// Soulseek politeness is per peer, not per daemon (#272).
func TestKeyedLockSerialisesOneKeyOnly(t *testing.T) {
t.Parallel()
var l keyedLock[string]
ctx := context.Background()
releaseA, err := l.acquire(ctx, "a")
if err != nil {
t.Fatalf("acquire a: %v", err)
}
// Another key is free while "a" is held.
releaseB, err := l.acquire(ctx, "b")
if err != nil {
t.Fatalf("acquire b: %v", err)
}
releaseB()
// The same key waits, and gives up with its context.
short, cancel := context.WithTimeout(ctx, 20*time.Millisecond)
defer cancel()
if _, err := l.acquire(short, "a"); !errors.Is(err, context.DeadlineExceeded) {
t.Fatalf("second acquire of a held key = %v, want the deadline", err)
}
releaseA()
releaseA() // Idempotent: a second call must not free someone else's hold.
if n := l.size(); n != 0 {
t.Errorf("%d keys left behind, want none once nobody holds or waits", n)
}
}
// grabEach runs one grab per candidate and returns a function that waits
// for all of them; grabAll's reasons for waiting apply.
func grabEach(t *testing.T, f managerFixture, cands []Candidate) func() {
t.Helper()
ctx := context.Background()
var wg sync.WaitGroup
for i, c := range cands {
dl := fourTrackDownload()
dl.ID = "dl-" + string(rune('a'+i))
if err := f.store.CreateDownload(ctx, dl); err != nil {
t.Fatalf("CreateDownload: %v", err)
}
wg.Add(1)
go func() {
defer wg.Done()
f.manager.grab(ctx, dl, c, nil, false)
}()
}
return func() {
done := make(chan struct{})
go func() {
wg.Wait()
close(done)
}()
select {
case <-done:
case <-time.After(5 * time.Second):
t.Error("transfers did not finish")
}
}
}
func slskdCandidates(p *FakeProvider, peers ...string) []Candidate {
out := make([]Candidate, 0, len(peers))
for i, peer := range peers {
c := p.Candidates[0]
c.ID = c.ID + "-" + itoa(i)
c.Kind = KindSlskd
c.ProviderID = 1
c.Origin = peer
out = append(out, c)
}
return out
}
// Three albums from one user are asked for one at a time, even though
// the daemon would allow three transfers.
func TestOnePeerIsAskedForOneThingAtATime(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
f.manager.SetMaxConcurrent(4)
p := fakeWithAlbum(1, "slskd", ".flac")
p.GrabGate = make(chan struct{})
f.manager.installProvider(Config{ID: 1, Kind: KindSlskd, Priority: 50}, p)
wait := grabEach(t, f, slskdCandidates(p, "alice", "alice", "alice"))
waitFor(t, func() bool { return p.GrabCallCount() >= 1 }, "no grab started")
time.Sleep(150 * time.Millisecond)
if got := p.MaxParallelGrabs(); got != 1 {
t.Errorf("%d simultaneous grabs from one peer, want 1", got)
}
close(p.GrabGate)
waitFor(t, func() bool { return p.GrabCallCount() == 3 }, "queued grabs never ran")
wait()
if n := f.manager.peerLocks.size(); n != 0 {
t.Errorf("%d peer locks left behind", n)
}
}
// Different users run at once, up to the daemon's cap — the point of
// the change: one slow peer no longer holds up every other.
func TestDifferentPeersRunTogether(t *testing.T) {
t.Parallel()
f := newManagerFixture(t)
f.manager.SetMaxConcurrent(8)
p := fakeWithAlbum(1, "slskd", ".flac")
p.GrabGate = make(chan struct{})
f.manager.installProvider(Config{ID: 1, Kind: KindSlskd, Priority: 50}, p)
wait := grabEach(t, f, slskdCandidates(p, "alice", "bob", "carol", "dave"))
waitFor(
t,
func() bool { return p.MaxParallelGrabs() >= kindConcurrency[KindSlskd] },
"different peers were serialised",
)
time.Sleep(100 * time.Millisecond)
if got := p.MaxParallelGrabs(); got != kindConcurrency[KindSlskd] {
t.Errorf("%d simultaneous grabs, want the daemon cap %d", got, kindConcurrency[KindSlskd])
}
close(p.GrabGate)
wait()
}
func TestSlskdLocalFolders(t *testing.T) {
t.Parallel()
s := &slskd{downloadsPath: "/dl"}
got := s.localFolders(Candidate{Files: []CandidateFile{
{Path: `\m\The Wall\CD2\01 Hey You.flac`},
{Path: `\m\The Wall\CD1\01 In The Flesh.flac`},
{Path: `\m\The Wall\CD1\02 The Thin Ice.flac`},
}})
want := []string{filepath.Join("/dl", "CD1"), filepath.Join("/dl", "CD2")}
if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] {
t.Errorf("localFolders = %q, want %q", got, want)
}
}
// Two peers' "Greatest Hits" land in one slskd directory, so the second
// grab does not enqueue until the first has collected its files.
func TestSlskdSameFolderNameWaits(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
s, downloads := newStubSlskd(t, stub)
c := Candidate{
Payload: map[string]string{"username": "bob"},
Files: []CandidateFile{
{Path: `\music\Greatest Hits\01 Intro.flac`, Size: 1, IsAudio: true},
},
}
release, err := lockSlskdFolders(
context.Background(), []string{filepath.Join(downloads, "Greatest Hits")},
)
if err != nil {
t.Fatalf("lock: %v", err)
}
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancel()
if _, err := s.Grab(ctx, c, t.TempDir(), nil); !errors.Is(err, context.DeadlineExceeded) {
t.Fatalf("Grab = %v, want it to wait on the held folder", err)
}
release()
stub.mu.Lock()
posted := stub.posted
stub.mu.Unlock()
if posted {
t.Error("enqueued transfers into a folder another grab held")
}
}
+8 -7
View File
@@ -236,17 +236,18 @@ func Register(d Descriptor, c Constructor) {
} }
// concurrencyField describes the per-provider transfer limit, with help // concurrencyField describes the per-provider transfer limit, with help
// text explaining why the default is what it is — a user who raises // text explaining what the number means where it means something
// slskd from 1 to 8 and gets themselves queued behind every other // unusual: on slskd it counts peers, since each peer is only ever asked
// Soulseek user deserves to have been warned. // for one folder at a time whatever it is set to.
func concurrencyField(k Kind) Field { func concurrencyField(k Kind) Field {
help := "Maximum simultaneous transfers from this client." help := "Maximum simultaneous transfers from this client."
if k == KindSlskd { if k == KindSlskd {
help = "Maximum simultaneous transfers. Soulseek peers serve " + help = "How many Soulseek users to download from at once. " +
"one file at a time and queue or ban clients that ask for " + "Each user is only ever asked for one album at a time, " +
"more, so 1 is both the polite setting and usually the " + "since peers queue or ban clients that ask for more; " +
"fastest." "this bounds how many different users are asked in " +
"parallel."
} }
return Field{ return Field{
File diff suppressed because it is too large Load Diff
@@ -0,0 +1,130 @@
package download
import (
"context"
"os"
"path/filepath"
"slices"
"testing"
"time"
)
// TestSlskdLive runs the provider against a real slskd daemon. It is
// skipped unless YJ_SLSKD_URL, YJ_SLSKD_API_KEY and YJ_SLSKD_DOWNLOADS
// are set, and it downloads something only when YJ_SLSKD_GRAB=1 — then
// the smallest candidate the search returns, from whichever stranger
// is sharing it.
//
// Everything else here tests the provider against a stub written from
// reading slskd's source. This is where those readings are checked:
// the search options, the responses endpoint, the batch destination,
// the cancel.
//
// YJ_SLSKD_URL=http://localhost:5030 YJ_SLSKD_API_KEY=… \
// YJ_SLSKD_DOWNLOADS=/path/to/slskd/downloads YJ_SLSKD_GRAB=1 \
// go test -run TestSlskdLive -v ./backend/download/
func TestSlskdLive(t *testing.T) {
base, key, downloads := os.Getenv("YJ_SLSKD_URL"),
os.Getenv("YJ_SLSKD_API_KEY"), os.Getenv("YJ_SLSKD_DOWNLOADS")
if base == "" || key == "" || downloads == "" {
t.Skip(
"set YJ_SLSKD_URL, YJ_SLSKD_API_KEY and YJ_SLSKD_DOWNLOADS to run against a real slskd",
)
}
query := os.Getenv("YJ_SLSKD_QUERY")
if query == "" {
query = "Radiohead OK Computer"
}
p, err := newSlskd(
Config{
ID: 1, Kind: KindSlskd, Name: "live", Enabled: true,
Settings: map[string]string{"url": base, "downloadsPath": downloads},
},
func(string) (string, error) { return key, nil },
slogDiscard(),
)
if err != nil {
t.Fatalf("newSlskd: %v", err)
}
s, ok := p.(*slskd)
if !ok {
t.Fatalf("provider is %T", p)
}
ctx := context.Background()
if err := s.Check(ctx); err != nil {
t.Fatalf("Check: %v", err)
}
started := time.Now()
got, err := s.Search(ctx, Download{Query: query})
if err != nil {
t.Fatalf("Search: %v", err)
}
t.Logf(
"search %q: %d candidates in %s",
query,
len(got),
time.Since(started).Round(time.Millisecond),
)
if len(got) == 0 {
t.Fatal("no candidates; try a more common YJ_SLSKD_QUERY")
}
timed := 0
for _, c := range got {
for _, f := range c.Files {
if f.LengthMillis > 0 {
timed++
}
}
}
t.Logf("%d files carry a length", timed)
if os.Getenv("YJ_SLSKD_GRAB") != "1" {
return
}
smallest := slices.MinFunc(got, func(a, b Candidate) int {
return int(a.TotalSize - b.TotalSize)
})
t.Logf("grabbing %q from %s (%d files, %d bytes)",
smallest.Title, smallest.Origin, len(smallest.Files), smallest.TotalSize)
s.stallAfter = 3 * time.Minute
gctx, cancel := context.WithTimeout(ctx, 15*time.Minute)
defer cancel()
res, err := s.Grab(gctx, smallest, t.TempDir(), func(p Progress) {
t.Logf("%s: %d/%d bytes", p.Phase, p.Current, p.Total)
})
if err != nil {
// A stranger going offline is not a defect; what matters is
// that the transfers were cancelled, which slskd's UI shows.
t.Fatalf("Grab: %v", err)
}
t.Logf("batches: %v", s.batches.Load() == batchesSupported)
for _, f := range res.Files {
info, err := os.Stat(f)
if err != nil || info.Size() == 0 {
t.Errorf("collected %s is missing or empty: %v", f, err)
}
}
if entries, _ := os.ReadDir(filepath.Join(downloads, slskdDestRoot)); len(entries) != 0 {
t.Errorf("%d batch folders left in slskd's downloads", len(entries))
}
}
+499 -29
View File
@@ -7,7 +7,9 @@ import (
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"os" "os"
"path"
"path/filepath" "path/filepath"
"strconv"
"strings" "strings"
"sync" "sync"
"testing" "testing"
@@ -31,11 +33,45 @@ type slskdStub struct {
transfers [][]slskdTransfer transfers [][]slskdTransfer
pollCount int pollCount int
// before is what the downloads endpoint reports until something is
// enqueued: records slskd already held from earlier attempts.
before []slskdTransfer
// enqueued records what was requested for download. // enqueued records what was requested for download.
enqueued []map[string]any enqueued []map[string]any
posted bool
// paths records the escaped path of every transfers call, and
// cancelled the escaped request URI of every DELETE.
paths []string
cancelled []string
// unauthorized makes every call return 401. // unauthorized makes every call return 401.
unauthorized bool unauthorized bool
// searches records every search request body, and searchGets the
// request URI of every search GET.
searches []map[string]any
searchGets []string
// noResponsesEndpoint makes /searches/{id}/responses 404, as an
// older daemon would.
noResponsesEndpoint bool
// batches makes the daemon take batch downloads, as 0.26 does.
// Without it the batch endpoint answers 400, which is what an older
// daemon's per-user route does with a batch body. batchBodies
// records each batch, and delivered is written into its destination
// under downloads when it is enqueued, keyed by file base name.
batches bool
batchBodies []map[string]any
// positions is what the queue-position endpoint answers, in order;
// the last repeats. positionAsks counts the calls.
positions []int
positionAsks int
delivered map[string]string
downloads string
} }
func newSlskdStub(t *testing.T) *slskdStub { func newSlskdStub(t *testing.T) *slskdStub {
@@ -57,6 +93,16 @@ func newSlskdStub(t *testing.T) *slskdStub {
return return
} }
var body map[string]any
if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
t.Errorf("decode search body: %v", err)
}
s.mu.Lock()
s.searches = append(s.searches, body)
s.mu.Unlock()
w.WriteHeader(http.StatusCreated) w.WriteHeader(http.StatusCreated)
}) })
@@ -73,8 +119,26 @@ func newSlskdStub(t *testing.T) *slskdStub {
s.mu.Lock() s.mu.Lock()
responses := s.responses responses := s.responses
noEndpoint := s.noResponsesEndpoint
s.searchGets = append(s.searchGets, r.URL.RequestURI())
s.mu.Unlock() s.mu.Unlock()
if strings.HasSuffix(r.URL.Path, "/responses") {
if noEndpoint {
w.WriteHeader(http.StatusNotFound)
return
}
writeJSON(t, w, responses)
return
}
if r.URL.Query().Get("includeResponses") != "true" {
responses = nil
}
writeJSON(t, w, slskdSearch{ writeJSON(t, w, slskdSearch{
ID: "search-1", ID: "search-1",
IsComplete: true, IsComplete: true,
@@ -87,7 +151,35 @@ func newSlskdStub(t *testing.T) *slskdStub {
return return
} }
if r.Method == http.MethodPost { s.mu.Lock()
s.paths = append(s.paths, r.URL.EscapedPath())
s.mu.Unlock()
if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/position") {
s.mu.Lock()
idx := min(s.positionAsks, len(s.positions)-1)
s.positionAsks++
place := 0
if idx >= 0 {
place = s.positions[idx]
}
s.mu.Unlock()
writeJSON(t, w, place)
return
}
if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/batches") {
s.enqueueBatch(t, w, r)
return
}
switch r.Method {
case http.MethodPost:
var body []map[string]any var body []map[string]any
if err := json.NewDecoder(r.Body).Decode(&body); err != nil { if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
@@ -96,15 +188,42 @@ func newSlskdStub(t *testing.T) *slskdStub {
s.mu.Lock() s.mu.Lock()
s.enqueued = body s.enqueued = body
s.posted = true
for _, file := range body {
name, _ := file["filename"].(string)
norm := strings.ReplaceAll(name, `\`, "/")
s.write(t, path.Base(path.Dir(norm)), path.Base(norm))
}
s.mu.Unlock() s.mu.Unlock()
w.WriteHeader(http.StatusCreated) w.WriteHeader(http.StatusCreated)
return
case http.MethodDelete:
s.mu.Lock()
s.cancelled = append(s.cancelled, r.URL.RequestURI())
s.mu.Unlock()
w.WriteHeader(http.StatusNoContent)
return return
} }
s.mu.Lock() s.mu.Lock()
if !s.posted {
before := s.before
s.mu.Unlock()
writeJSON(t, w, map[string]any{
"directories": []map[string]any{{"files": before}},
})
return
}
idx := s.pollCount idx := s.pollCount
if idx >= len(s.transfers) { if idx >= len(s.transfers) {
idx = len(s.transfers) - 1 idx = len(s.transfers) - 1
@@ -130,6 +249,89 @@ func newSlskdStub(t *testing.T) *slskdStub {
return s return s
} }
// enqueueBatch answers the batch endpoint.
func (s *slskdStub) enqueueBatch(t *testing.T, w http.ResponseWriter, r *http.Request) {
t.Helper()
s.mu.Lock()
defer s.mu.Unlock()
if !s.batches {
w.WriteHeader(http.StatusBadRequest)
return
}
var body map[string]any
if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
t.Errorf("decode batch body: %v", err)
}
s.batchBodies = append(s.batchBodies, body)
s.posted = true
files, _ := body["files"].([]any)
options, _ := body["options"].(map[string]any)
dest, _ := options["destination"].(string)
for _, f := range files {
file, _ := f.(map[string]any)
s.enqueued = append(s.enqueued, file)
name, _ := file["filename"].(string)
base := path.Base(strings.ReplaceAll(name, `\`, "/"))
s.write(t, filepath.FromSlash(dest), base)
}
w.WriteHeader(http.StatusCreated)
}
// deliver names the files that arrive once enqueued.
func (s *slskdStub) deliver(names ...string) {
s.mu.Lock()
defer s.mu.Unlock()
if s.delivered == nil {
s.delivered = map[string]string{}
}
for _, n := range names {
s.delivered[n] = "audio"
}
}
// write puts a delivered file where slskd would: under dir in the
// downloads folder, renamed name_<ticks>.ext when the name is taken, as
// slskd's default Destination.Exists does. Callers hold s.mu.
func (s *slskdStub) write(t *testing.T, dir, base string) {
t.Helper()
content, ok := s.delivered[base]
if !ok {
return
}
full := filepath.Join(s.downloads, dir)
if err := os.MkdirAll(full, 0o750); err != nil {
t.Errorf("mkdir: %v", err)
}
target := filepath.Join(full, base)
if _, err := os.Stat(target); err == nil {
ext := filepath.Ext(base)
target = filepath.Join(
full,
strings.TrimSuffix(base, ext)+"_"+strconv.FormatInt(time.Now().UnixNano(), 10)+ext,
)
}
if err := os.WriteFile(target, []byte(content), 0o600); err != nil {
t.Errorf("write: %v", err)
}
}
// reject enforces API-key auth like the real daemon. // reject enforces API-key auth like the real daemon.
func (s *slskdStub) reject(w http.ResponseWriter, r *http.Request) bool { func (s *slskdStub) reject(w http.ResponseWriter, r *http.Request) bool {
s.mu.Lock() s.mu.Lock()
@@ -162,6 +364,10 @@ func newStubSlskd(t *testing.T, stub *slskdStub) (*slskd, string) {
downloads := t.TempDir() downloads := t.TempDir()
stub.mu.Lock()
stub.downloads = downloads
stub.mu.Unlock()
p, err := newSlskd( p, err := newSlskd(
Config{ Config{
ID: 1, ID: 1,
@@ -191,6 +397,13 @@ func newStubSlskd(t *testing.T, stub *slskdStub) (*slskd, string) {
s.searchWait = 200 * time.Millisecond s.searchWait = 200 * time.Millisecond
s.transferPoll = time.Millisecond s.transferPoll = time.Millisecond
// Long enough that no existing test trips them by accident; the
// tests about stalls and absences set their own.
s.stallAfter = time.Minute
s.absentGrace = time.Minute
s.positionPoll = time.Millisecond
s.queueCeiling = time.Hour
return s, downloads return s, downloads
} }
@@ -394,24 +607,9 @@ func TestSlskdGrabCollectsFromDownloadsFolder(t *testing.T) {
}, },
} }
s, downloads := newStubSlskd(t, stub) s, _ := newStubSlskd(t, stub)
// slskd writes into <downloads>/<folder>/<file>. stub.deliver("01 Airbag.flac", "02 Paranoid Android.flac")
folder := filepath.Join(downloads, "OK Computer")
if err := os.MkdirAll(folder, 0o750); err != nil {
t.Fatalf("mkdir: %v", err)
}
for _, name := range []string{
"01 Airbag.flac",
"02 Paranoid Android.flac",
} {
if err := os.WriteFile(
filepath.Join(folder, name), []byte("audio"), 0o600,
); err != nil {
t.Fatalf("write: %v", err)
}
}
c := Candidate{ c := Candidate{
ID: "slskd:peer:OK Computer", ID: "slskd:peer:OK Computer",
@@ -470,18 +668,9 @@ func TestSlskdGrabToleratesPartialFailure(t *testing.T) {
{Filename: `\s\Album\02 B.flac`, State: "Completed, Errored"}, {Filename: `\s\Album\02 B.flac`, State: "Completed, Errored"},
}} }}
s, downloads := newStubSlskd(t, stub) s, _ := newStubSlskd(t, stub)
folder := filepath.Join(downloads, "Album") stub.deliver("01 A.flac")
if err := os.MkdirAll(folder, 0o750); err != nil {
t.Fatalf("mkdir: %v", err)
}
if err := os.WriteFile(
filepath.Join(folder, "01 A.flac"), []byte("audio"), 0o600,
); err != nil {
t.Fatalf("write: %v", err)
}
c := Candidate{ c := Candidate{
Files: []CandidateFile{ Files: []CandidateFile{
@@ -565,3 +754,284 @@ func TestSlskdRequiresConfiguration(t *testing.T) {
}) })
} }
} }
// slskdAlbum is a two-file candidate from peer, whose arrived files
// the stub writes where slskd would once they are enqueued.
func slskdAlbum(t *testing.T, stub *slskdStub, peer string, arrived ...string) Candidate {
t.Helper()
stub.deliver(arrived...)
return Candidate{
Files: []CandidateFile{
{Path: `\s\Album\01 A.flac`, Size: 500, IsAudio: true},
{Path: `\s\Album\02 B.flac`, Size: 500, IsAudio: true},
},
TotalSize: 1000,
Payload: map[string]string{"username": peer},
}
}
// cancelledURIs returns what the stub was asked to cancel.
func (s *slskdStub) cancelledURIs() []string {
s.mu.Lock()
defer s.mu.Unlock()
return append([]string(nil), s.cancelled...)
}
// A peer that queues us and never sends a byte is given up on, and the
// queued transfers are cancelled in slskd rather than left to start
// hours later for a request nobody is waiting on.
func TestSlskdGrabGivesUpOnAStalledPeer(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = [][]slskdTransfer{{
{ID: "t1", Filename: `\s\Album\01 A.flac`, State: "Queued, Remotely"},
{ID: "t2", Filename: `\s\Album\02 B.flac`, State: "Queued, Remotely"},
}}
s, _ := newStubSlskd(t, stub)
s.stallAfter = 30 * time.Millisecond
_, err := s.Grab(
context.Background(), slskdAlbum(t, stub, "peer"), t.TempDir(), nil,
)
if !errors.Is(err, ErrSlskdTimeout) {
t.Fatalf("error = %v, want ErrSlskdTimeout", err)
}
got := stub.cancelledURIs()
if len(got) != 2 {
t.Fatalf("cancelled %v, want both queued transfers", got)
}
for _, uri := range got {
if !strings.HasSuffix(uri, "?remove=true") {
t.Errorf("cancel %s does not remove the record", uri)
}
}
}
// A folder that stalls on its last track goes forward with what
// arrived, the same as one whose last track failed; the importer's
// completeness check decides whether that is enough.
func TestSlskdGrabKeepsWhatArrivedBeforeAStall(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = [][]slskdTransfer{{
{
ID: "t1", Filename: `\s\Album\01 A.flac`,
State: "Completed, Succeeded", BytesTransferred: 500,
},
{ID: "t2", Filename: `\s\Album\02 B.flac`, State: "Queued, Remotely"},
}}
s, _ := newStubSlskd(t, stub)
s.stallAfter = 30 * time.Millisecond
got, err := s.Grab(
context.Background(),
slskdAlbum(t, stub, "peer", "01 A.flac"),
t.TempDir(), nil,
)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 1 {
t.Errorf("collected %d files, want the 1 that arrived", len(got.Files))
}
if cancelled := stub.cancelledURIs(); len(cancelled) != 1 ||
!strings.Contains(cancelled[0], "/t2") {
t.Errorf("cancelled %v, want only the stalled t2", cancelled)
}
}
// Progress is what holds the stall timer off. A transfer that keeps
// moving bytes is never abandoned, however long it takes.
func TestSlskdGrabWaitsOnATransferThatIsMoving(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
for b := int64(1); b <= 100; b++ {
stub.transfers = append(stub.transfers, []slskdTransfer{
{ID: "t1", Filename: `\s\Album\01 A.flac`, State: "InProgress", BytesTransferred: b},
{ID: "t2", Filename: `\s\Album\02 B.flac`, State: "Queued, Remotely"},
})
}
stub.transfers = append(stub.transfers, []slskdTransfer{
{
ID: "t1",
Filename: `\s\Album\01 A.flac`,
State: "Completed, Succeeded",
BytesTransferred: 500,
},
{
ID: "t2",
Filename: `\s\Album\02 B.flac`,
State: "Completed, Succeeded",
BytesTransferred: 500,
},
})
s, _ := newStubSlskd(t, stub)
// A hundred polls take several times the stall window; each one
// moves a byte. The window is kept well above one poll so a
// descheduled test runner does not read as a stall.
s.transferPoll = 5 * time.Millisecond
s.stallAfter = 150 * time.Millisecond
got, err := s.Grab(
context.Background(),
slskdAlbum(t, stub, "peer", "01 A.flac", "02 B.flac"),
t.TempDir(), nil,
)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 2 {
t.Errorf("collected %d files, want 2", len(got.Files))
}
}
// A file slskd never lists was refused at enqueue and will never reach
// a terminal state. It counts as failed once the grace period is up,
// rather than being waited on until the six-hour ceiling.
func TestSlskdGrabCountsAnUnlistedFileAsFailed(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = [][]slskdTransfer{{
{
ID: "t1", Filename: `\s\Album\01 A.flac`,
State: "Completed, Succeeded", BytesTransferred: 500,
},
}}
s, _ := newStubSlskd(t, stub)
s.absentGrace = 20 * time.Millisecond
got, err := s.Grab(
context.Background(),
slskdAlbum(t, stub, "peer", "01 A.flac"),
t.TempDir(), nil,
)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 1 {
t.Errorf("collected %d files, want 1", len(got.Files))
}
}
// A finished record left by an earlier attempt at the same file is not
// this attempt's answer. Without the snapshot it would fail the grab on
// the first poll, before the new transfer had started.
func TestSlskdGrabIgnoresAnEarlierAttemptsRecord(t *testing.T) {
t.Parallel()
stale := slskdTransfer{
ID: "old", Filename: `\s\Album\01 A.flac`, State: "Completed, Errored",
}
stub := newSlskdStub(t)
stub.before = []slskdTransfer{stale}
stub.transfers = [][]slskdTransfer{
{stale},
{
stale,
{
ID: "new", Filename: `\s\Album\01 A.flac`,
State: "Completed, Succeeded", BytesTransferred: 500,
},
},
}
s, _ := newStubSlskd(t, stub)
c := slskdAlbum(t, stub, "peer", "01 A.flac")
c.Files = c.Files[:1]
got, err := s.Grab(context.Background(), c, t.TempDir(), nil)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 1 {
t.Errorf("collected %d files, want 1", len(got.Files))
}
}
// Cancelling the download cancels the transfer in slskd too. The
// cleanup must not inherit the cancelled context, or it is never sent.
func TestSlskdGrabCancelsTransfersWhenTheCallerGivesUp(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = [][]slskdTransfer{{
{ID: "t1", Filename: `\s\Album\01 A.flac`, State: "InProgress", BytesTransferred: 10},
{ID: "t2", Filename: `\s\Album\02 B.flac`, State: "Queued, Remotely"},
}}
s, _ := newStubSlskd(t, stub)
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
defer cancel()
_, err := s.Grab(ctx, slskdAlbum(t, stub, "peer"), t.TempDir(), nil)
if !errors.Is(err, ErrSlskdTimeout) {
t.Fatalf("error = %v, want ErrSlskdTimeout", err)
}
if got := stub.cancelledURIs(); len(got) != 2 {
t.Errorf("cancelled %v, want both live transfers", got)
}
}
// Soulseek usernames carry spaces and punctuation; spliced raw into the
// path, a name with a slash addresses a different endpoint entirely.
func TestSlskdEscapesTheUsername(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = [][]slskdTransfer{{
{
ID: "t1", Filename: `\s\Album\01 A.flac`,
State: "Completed, Succeeded", BytesTransferred: 500,
},
{
ID: "t2", Filename: `\s\Album\02 B.flac`,
State: "Completed, Succeeded", BytesTransferred: 500,
},
}}
s, _ := newStubSlskd(t, stub)
if _, err := s.Grab(
context.Background(),
slskdAlbum(t, stub, "dj a/b", "01 A.flac", "02 B.flac"),
t.TempDir(), nil,
); err != nil {
t.Fatalf("Grab: %v", err)
}
stub.mu.Lock()
paths := append([]string(nil), stub.paths...)
stub.mu.Unlock()
for _, p := range paths {
// The batch endpoint carries the name in its body.
if p != "/api/v0/transfers/downloads/dj%20a%2Fb" &&
p != "/api/v0/transfers/downloads/batches" {
t.Errorf("transfers call went to %s", p)
}
}
}
+104 -18
View File
@@ -35,6 +35,15 @@ const (
weightArtistFit = 0.12 weightArtistFit = 0.12
) )
// Match sub-weights when the candidate's durations are known. Duration
// takes its weight from title fit, the signal it corroborates: a title
// says which song a file claims to be, a length says whether it is that
// recording — the right edit, the whole file, not the live take.
const (
timedWeightTitleFit = 0.25
timedWeightDurationFit = 0.15
)
// Quality sub-weights. Each set sums to 1.0. // Quality sub-weights. Each set sums to 1.0.
// //
// There are two of them because a stated preference changes what the // There are two of them because a stated preference changes what the
@@ -319,13 +328,13 @@ func Score(dl Download, c Candidate, priority int, prefs AutoDownloadPrefs) Cand
audio := c.AudioFiles() audio := c.AudioFiles()
matched, titleFit := matchFiles(audio, dl.Expected) a := alignFiles(audio, dl.Expected)
// Write the alignment back so the picker can show which file maps // Write the alignment back so the picker can show which file maps
// to which track. // to which track.
c.Files = mergeMatched(c.Files, matched) c.Files = mergeMatched(c.Files, a.files)
c.Match = scoreMatch(dl, c, audio, titleFit) c.Match = scoreMatch(dl, c, audio, a)
c.Quality = scoreQuality( c.Quality = scoreQuality(
c, audio, priority, prefs, dl.runtimeMillis(), c, audio, priority, prefs, dl.runtimeMillis(),
) )
@@ -340,14 +349,21 @@ func scoreMatch(
dl Download, dl Download,
c Candidate, c Candidate,
audio []CandidateFile, audio []CandidateFile,
titleFit float64, a alignment,
) MatchScore { ) MatchScore {
m := MatchScore{ m := MatchScore{
Anchored: dl.Anchored(), Anchored: dl.Anchored(),
TitleFit: titleFit, TitleFit: a.titleFit,
DurationFit: a.durationFit,
// Durations count once at least half the aligned pairs state
// one; a single timed pair would be a coin toss carrying 15%.
DurationKnown: a.timedPairs > 0 && a.timedPairs*2 >= a.aligned,
} }
m.Completeness = completeness(len(audio), len(dl.Expected)) m.Completeness = completeness(
alignedCount(c.Files), len(audio), len(dl.Expected),
)
// The candidate's own title, and the folder its files sit in, are // The candidate's own title, and the folder its files sit in, are
// two independent guesses at the album name. Take the better one: // two independent guesses at the album name. Take the better one:
@@ -367,9 +383,16 @@ func scoreMatch(
// With no expected tracklist there is no title signal at all, so // With no expected tracklist there is no title signal at all, so
// redistribute its weight onto the album/artist evidence rather // redistribute its weight onto the album/artist evidence rather
// than scoring every free-text result as half-wrong. // than scoring every free-text result as half-wrong.
if len(dl.Expected) == 0 { switch {
case len(dl.Expected) == 0:
m.Overall = 0.55*m.AlbumFit + 0.45*m.ArtistFit m.Overall = 0.55*m.AlbumFit + 0.45*m.ArtistFit
} else { case m.DurationKnown:
m.Overall = timedWeightTitleFit*m.TitleFit +
timedWeightDurationFit*m.DurationFit +
weightCompleteness*m.Completeness +
weightAlbumFit*m.AlbumFit +
weightArtistFit*m.ArtistFit
default:
m.Overall = weightTitleFit*m.TitleFit + m.Overall = weightTitleFit*m.TitleFit +
weightCompleteness*m.Completeness + weightCompleteness*m.Completeness +
weightAlbumFit*m.AlbumFit + weightAlbumFit*m.AlbumFit +
@@ -415,30 +438,54 @@ func artistFit(want string, c Candidate) float64 {
return best return best
} }
// completeness scores audio file count against the expected track // completeness scores how much of the expected tracklist a candidate
// count. Extra files are penalized far more gently than missing ones: // covers. Extra files are penalized far more gently than missing ones:
// a folder with bonus tracks or a stray intro is still the album, while // a folder with bonus tracks or a stray intro is still the album, while
// a folder missing half the tracks is not. // a folder missing half the tracks is not.
func completeness(got, want int) float64 { //
// **Coverage is counted in aligned tracks, not in files.** It used to
// be the audio file count, so any ten files scored full marks against
// a ten-track album whether or not they were its tracks — and since
// title fit is the mean over the files that *did* align, a folder where
// three titles matched read as a near-perfect candidate on both counts.
// `aligned` is how many files matchFiles assigned to an expected track;
// `audio` still sets the penalty for extras, because a folder of thirty
// files holding the ten wanted is a worse copy than one holding ten.
func completeness(aligned, audio, want int) float64 {
if want == 0 { if want == 0 {
if got > 0 { if audio > 0 {
return 0.5 return 0.5
} }
return 0 return 0
} }
if got == 0 { if aligned == 0 {
return 0 return 0
} }
if got >= want { cover := float64(min(aligned, want)) / float64(want)
extra := float64(got-want) / float64(want)
return math.Max(0.75, 1.0-0.25*extra) if audio > want {
extra := float64(audio-want) / float64(want)
cover *= math.Max(0.75, 1.0-0.25*extra)
} }
return float64(got) / float64(want) return cover
}
// alignedCount is how many audio files were assigned to an expected
// track.
func alignedCount(files []CandidateFile) int {
n := 0
for _, f := range files {
if f.IsAudio && f.MatchedTo != 0 {
n++
}
}
return n
} }
// scoreQuality answers whether this is a good copy. // scoreQuality answers whether this is a good copy.
@@ -735,6 +782,45 @@ func AutoPickVeto(
return "" return ""
} }
// autoAcceptable reports whether auto-pick may take this one candidate
// without asking: the request is anchored to a tracklist, and the
// candidate is inside the user's guardrails and clears the match and
// quality bars. It is AutoPickVeto's test applied to a single
// candidate, which is what falling back to a second choice needs.
func autoAcceptable(dl Download, c Candidate, prefs AutoDownloadPrefs) bool {
return dl.Anchored() &&
len(dl.Expected) > 0 &&
prefs.eligible(c, dl.runtimeMillis()) &&
c.Match.Overall >= minMatch &&
c.Quality.Overall >= minQuality
}
// autoPick returns the candidate auto-pick takes: the best-ranked one
// it may take at all.
//
// That is not `ranked[0]`. AutoPickVeto judges the best candidate
// *inside* the guardrails, so when the overall best is outside them —
// over the size ceiling, say — the veto passes on the strength of the
// second, and grabbing the first would download exactly the copy the
// user said not to take unattended.
func autoPick(
dl Download,
ranked []Candidate,
prefs AutoDownloadPrefs,
) (Candidate, bool) {
if AutoPickVeto(dl, ranked, prefs) != "" {
return Candidate{}, false
}
for _, c := range ranked {
if autoAcceptable(dl, c, prefs) {
return c, true
}
}
return Candidate{}, false
}
// mergeMatched copies MatchedTo assignments from the audio-only slice // mergeMatched copies MatchedTo assignments from the audio-only slice
// back onto the full file list. // back onto the full file list.
func mergeMatched(all, matched []CandidateFile) []CandidateFile { func mergeMatched(all, matched []CandidateFile) []CandidateFile {
+14 -10
View File
@@ -282,28 +282,32 @@ func TestCompleteness(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
got int aligned int
audio int
want int want int
minScore float64 minScore float64
maxScore float64 maxScore float64
}{ }{
{"exact", 10, 10, 1.0, 1.0}, {"exact", 10, 10, 10, 1.0, 1.0},
{"half missing", 5, 10, 0.49, 0.51}, {"half missing", 5, 5, 10, 0.49, 0.51},
{"one bonus track", 11, 10, 0.95, 1.0}, {"one bonus track", 10, 11, 10, 0.95, 1.0},
{"double", 20, 10, 0.74, 0.76}, {"double", 10, 20, 10, 0.74, 0.76},
{"nothing", 0, 10, 0, 0}, {"nothing", 0, 0, 10, 0, 0},
{"no expectation", 5, 0, 0.5, 0.5}, {"no expectation", 0, 5, 0, 0.5, 0.5},
// Ten files are not ten tracks: three that align are three.
{"right count, wrong tracks", 3, 10, 10, 0.29, 0.31},
{"files that align to nothing", 0, 10, 10, 0, 0},
} }
for _, tt := range tests { for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { t.Run(tt.name, func(t *testing.T) {
t.Parallel() t.Parallel()
got := completeness(tt.got, tt.want) got := completeness(tt.aligned, tt.audio, tt.want)
if got < tt.minScore || got > tt.maxScore { if got < tt.minScore || got > tt.maxScore {
t.Errorf( t.Errorf(
"completeness(%d, %d) = %f, want in [%f, %f]", "completeness(%d, %d, %d) = %f, want in [%f, %f]",
tt.got, tt.want, got, tt.minScore, tt.maxScore, tt.aligned, tt.audio, tt.want, got, tt.minScore, tt.maxScore,
) )
} }
}) })
+238
View File
@@ -0,0 +1,238 @@
package download
import (
"context"
"strings"
"testing"
)
// What Soulseek is asked, how, and what is kept from the answer (#271).
func TestSlskdQueries(t *testing.T) {
t.Parallel()
cases := []struct {
name string
dl Download
want []string
}{
{
name: "a plain request is searched once",
dl: Download{Artist: "Radiohead", Album: "OK Computer"},
want: []string{"Radiohead OK Computer"},
},
{
name: "an edition qualifier gets a second query without it",
dl: Download{Artist: "Radiohead", Album: "OK Computer (Collector's Edition)"},
want: []string{
"Radiohead OK Computer (Collector's Edition)",
"Radiohead OK Computer",
},
},
{
name: "a trailing remaster note",
dl: Download{Artist: "Pink Floyd", Album: "Animals - 2018 Remaster"},
want: []string{
"Pink Floyd Animals - 2018 Remaster",
"Pink Floyd Animals",
},
},
{
name: "a leading dash would be an exclusion",
dl: Download{Artist: "Mocky", Album: "-ism"},
want: []string{"Mocky -ism", "Mocky ism"},
},
{
name: "a compilation is not searched by its placeholder artist",
dl: Download{Artist: "Various Artists", Album: "Pulp Fiction"},
want: []string{"Various Artists Pulp Fiction", "Pulp Fiction"},
},
{
name: "what the user typed is searched as written",
dl: Download{Query: "ok computer (deluxe)", Album: "OK Computer (Deluxe)"},
want: []string{"ok computer (deluxe)"},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
got := slskdQueries(tc.dl)
if strings.Join(got, "|") != strings.Join(tc.want, "|") {
t.Errorf("slskdQueries = %q, want %q", got, tc.want)
}
})
}
}
// Both queries run, the options are stated rather than left to the
// daemon's defaults, and a folder both queries found is one candidate.
func TestSlskdSearchRunsBothQueriesAndMerges(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.responses = []slskdResponse{{
Username: "peer",
Files: []slskdFile{
{Filename: `\m\Radiohead - OK Computer\01 Airbag.flac`, Size: 1, Length: 284},
{Filename: `\m\Radiohead - OK Computer\02 Paranoid Android.flac`, Size: 1, Length: 383},
},
}}
s, _ := newStubSlskd(t, stub)
got, err := s.Search(context.Background(), Download{
ReleaseMBID: "rel", Artist: "Radiohead", Album: "OK Computer (Deluxe Edition)",
})
if err != nil {
t.Fatalf("Search: %v", err)
}
if len(got) != 1 {
t.Fatalf("got %d candidates, want the one folder once", len(got))
}
if got[0].Files[0].LengthMillis != 284_000 {
t.Errorf("length = %d ms, want 284000 from slskd's seconds", got[0].Files[0].LengthMillis)
}
stub.mu.Lock()
searches := append([]map[string]any(nil), stub.searches...)
gets := append([]string(nil), stub.searchGets...)
stub.mu.Unlock()
if len(searches) != 2 {
t.Fatalf("ran %d searches, want 2", len(searches))
}
for _, body := range searches {
for _, key := range []string{
"searchTimeout", "responseLimit", "fileLimit",
"minimumResponseFileCount", "maximumPeerQueueLength",
} {
if _, ok := body[key]; !ok {
t.Errorf("search %q does not state %s", body["searchText"], key)
}
}
}
// The responses are fetched once at the end, not with every poll.
for _, uri := range gets {
if strings.Contains(uri, "includeResponses") {
t.Errorf("poll %s asked for every response", uri)
}
}
}
// A daemon without the responses endpoint still returns results.
func TestSlskdSearchFallsBackForAnOlderDaemon(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.noResponsesEndpoint = true
stub.responses = []slskdResponse{{
Username: "peer",
Files: []slskdFile{
{Filename: `\m\Album\01 A.flac`, Size: 1},
{Filename: `\m\Album\02 B.flac`, Size: 1},
},
}}
s, _ := newStubSlskd(t, stub)
got, err := s.Search(context.Background(), Download{Query: "album"})
if err != nil {
t.Fatalf("Search: %v", err)
}
if len(got) != 1 {
t.Errorf("got %d candidates, want 1 through the fallback", len(got))
}
}
func TestDurationAgreement(t *testing.T) {
t.Parallel()
cases := []struct {
got, want int64
score float64
}{
{300_000, 300_000, 1},
{301_500, 300_000, 1}, // a second of silence
{300_000, 316_500, 0.5},
{300_000, 345_000, 0}, // a different edit
}
for _, tc := range cases {
if got := durationAgreement(tc.got, tc.want); got < tc.score-0.01 || got > tc.score+0.01 {
t.Errorf("durationAgreement(%d, %d) = %f, want %f", tc.got, tc.want, got, tc.score)
}
}
}
// Two folders with the same track names are told apart by their
// lengths: one is the album, the other a live record of the same songs.
func TestDurationsSeparateTheRightRecording(t *testing.T) {
t.Parallel()
dl := okComputer()
timed := func(id string, lengths ...int64) Candidate {
c := candidateFor(id, allTitles(), ".flac", 30_000_000)
for i := range c.Files {
c.Files[i].LengthMillis = lengths[i]
}
return c
}
studio := timed("studio", trackMillis, trackMillis+1_000, trackMillis, trackMillis-500)
live := timed(
"live",
trackMillis+60_000,
trackMillis+75_000,
trackMillis+50_000,
trackMillis+90_000,
)
ranked := Rank(dl, []Candidate{live, studio}, nil, AutoDownloadPrefs{})
if ranked[0].ID != "studio" {
t.Fatalf("winner = %s, want the recording whose lengths match", ranked[0].ID)
}
if !ranked[0].Match.DurationKnown || ranked[0].Match.DurationFit < 0.99 {
t.Errorf(
"studio duration fit = %f known=%v",
ranked[0].Match.DurationFit,
ranked[0].Match.DurationKnown,
)
}
if ranked[1].Match.DurationFit != 0 {
t.Errorf("live duration fit = %f, want 0", ranked[1].Match.DurationFit)
}
}
// Without lengths the score is exactly what it was before durations
// were read, so a provider that reports none is not penalised.
func TestUnknownDurationsLeaveTheScoreAlone(t *testing.T) {
t.Parallel()
dl := okComputer()
c := Score(dl, candidateFor("c", allTitles(), ".flac", 30_000_000), 50, AutoDownloadPrefs{})
if c.Match.DurationKnown {
t.Fatal("no file states a length, yet durations are known")
}
want := weightTitleFit*c.Match.TitleFit +
weightCompleteness*c.Match.Completeness +
weightAlbumFit*c.Match.AlbumFit +
weightArtistFit*c.Match.ArtistFit
if c.Match.Overall != want {
t.Errorf("match = %f, want the untimed formula's %f", c.Match.Overall, want)
}
}
+328
View File
@@ -0,0 +1,328 @@
package download
import (
"context"
"os"
"path/filepath"
"strings"
"testing"
"time"
)
// Where slskd writes a grab's files, and how collect finds them (#274).
// slskd reads searchTimeout in whole seconds, from the last response.
func TestSlskdSearchTimeoutIsInSeconds(t *testing.T) {
t.Parallel()
cases := []struct {
wait time.Duration
want int
}{
{20 * time.Second, 18},
{200 * time.Millisecond, slskdMinSearchTimeout},
}
for _, tc := range cases {
s := &slskd{searchWait: tc.wait}
got, ok := s.searchRequest("id", "text", 2)["searchTimeout"].(int)
if !ok || got != tc.want {
t.Errorf("wait %s: searchTimeout = %v, want %d seconds", tc.wait, got, tc.want)
}
}
}
func TestIsRenamedCopy(t *testing.T) {
t.Parallel()
cases := map[string]bool{
"01 A_638912345678901234.flac": true,
"01 A.flac": false,
"01 A_.flac": false,
"01 A_v2.flac": false,
"01 A_123.mp3": false,
"01 AB_123.flac": false,
}
for name, want := range cases {
if got := isRenamedCopy(name, "01 A", ".flac"); got != want {
t.Errorf("isRenamedCopy(%q) = %v, want %v", name, got, want)
}
}
}
func succeeded(names ...string) [][]slskdTransfer {
out := make([]slskdTransfer, 0, len(names))
for i, n := range names {
out = append(out, slskdTransfer{
ID: "t" + itoa(i),
Filename: n,
State: "Completed, Succeeded",
BytesTransferred: 500,
})
}
return [][]slskdTransfer{out}
}
// On a daemon with batches, each grab writes into a folder of its own,
// collect reads from exactly there, and the folder is gone afterwards.
func TestSlskdBatchGrabUsesItsOwnFolder(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.batches = true
stub.transfers = succeeded(`\s\Album\01 A.flac`, `\s\Album\02 B.flac`)
s, downloads := newStubSlskd(t, stub)
// A same-named file in the folder a per-user enqueue would use is
// someone else's, and must not be touched.
other := filepath.Join(downloads, "Album", "01 A.flac")
if err := os.MkdirAll(filepath.Dir(other), 0o750); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(other, []byte("the user's"), 0o600); err != nil {
t.Fatal(err)
}
dst := t.TempDir()
got, err := s.Grab(
context.Background(), slskdAlbum(t, stub, "peer", "01 A.flac", "02 B.flac"), dst, nil,
)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 2 {
t.Fatalf("collected %d files, want 2", len(got.Files))
}
stub.mu.Lock()
bodies := append([]map[string]any(nil), stub.batchBodies...)
stub.mu.Unlock()
if len(bodies) != 1 {
t.Fatalf("%d batches, want 1", len(bodies))
}
if bodies[0]["username"] != "peer" {
t.Errorf("batch username = %v", bodies[0]["username"])
}
dest, _ := bodies[0]["options"].(map[string]any)["destination"].(string)
if !strings.HasPrefix(dest, slskdDestRoot+"/") {
t.Errorf("destination %q is not under %s", dest, slskdDestRoot)
}
if _, err := os.Stat(filepath.Join(downloads, filepath.FromSlash(dest))); !os.IsNotExist(err) {
t.Errorf("batch folder left behind: %v", err)
}
if data, _ := os.ReadFile(other); string(data) != "the user's" {
t.Errorf("the user's own file was taken or changed: %q", data)
}
}
// A batch's files land flat in its destination, so a two-disc rip is
// two batches, one per disc, or disc 2's "01" is renamed out of the way
// of disc 1's.
func TestSlskdBatchSplitsDiscs(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.batches = true
stub.transfers = succeeded(`\s\Wall\CD1\01 In.flac`, `\s\Wall\CD2\01 Hey You.flac`)
stub.deliver("01 In.flac", "01 Hey You.flac")
s, _ := newStubSlskd(t, stub)
dst := t.TempDir()
got, err := s.Grab(context.Background(), Candidate{
Files: []CandidateFile{
{Path: `\s\Wall\CD1\01 In.flac`, Size: 500, IsAudio: true},
{Path: `\s\Wall\CD2\01 Hey You.flac`, Size: 500, IsAudio: true},
},
Payload: map[string]string{"username": "peer"},
}, dst, nil)
if err != nil {
t.Fatalf("Grab: %v", err)
}
stub.mu.Lock()
n := len(stub.batchBodies)
stub.mu.Unlock()
if n != 2 {
t.Errorf("%d batches, want one per disc", n)
}
for _, want := range []string{
filepath.Join(dst, "CD1", "01 In.flac"),
filepath.Join(dst, "CD2", "01 Hey You.flac"),
} {
found := false
for _, f := range got.Files {
found = found || f == want
}
if !found {
t.Errorf("%s not collected; got %q", want, got.Files)
}
}
}
// An abandoned batch grab leaves nothing in slskd's folder either.
func TestSlskdBatchFailureDiscardsItsFolder(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.batches = true
stub.transfers = [][]slskdTransfer{{
{ID: "t0", Filename: `\s\Album\01 A.flac`, State: "Completed, Errored"},
{ID: "t1", Filename: `\s\Album\02 B.flac`, State: "Completed, Errored"},
}}
s, downloads := newStubSlskd(t, stub)
// The stub delivers the file, as a partial slskd left behind would.
if _, err := s.Grab(
context.Background(), slskdAlbum(t, stub, "peer", "01 A.flac"), t.TempDir(), nil,
); err == nil {
t.Fatal("Grab succeeded with every transfer failed")
}
entries, _ := os.ReadDir(filepath.Join(downloads, slskdDestRoot))
if len(entries) != 0 {
t.Errorf("%d batch folders left behind", len(entries))
}
}
// An older daemon answers the batch endpoint with 400; the grab falls
// back to the per-user enqueue, and later grabs do not ask again.
func TestSlskdFallsBackWithoutBatches(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = succeeded(`\s\Album\01 A.flac`, `\s\Album\02 B.flac`)
s, _ := newStubSlskd(t, stub)
for range 2 {
stub.mu.Lock()
stub.posted = false
stub.pollCount = 0
stub.mu.Unlock()
if _, err := s.Grab(
context.Background(), slskdAlbum(t, stub, "peer", "01 A.flac"), t.TempDir(), nil,
); err != nil {
t.Fatalf("Grab: %v", err)
}
}
stub.mu.Lock()
paths := append([]string(nil), stub.paths...)
stub.mu.Unlock()
batchCalls := 0
for _, p := range paths {
if strings.HasSuffix(p, "/batches") {
batchCalls++
}
}
if batchCalls != 1 {
t.Errorf("asked for a batch %d times, want once", batchCalls)
}
}
// Without batches, a file of the same name already in slskd's folder is
// not this grab's: slskd wrote ours beside it as name_<ticks>.ext, and
// that is the one collected.
func TestSlskdCollectsTheRenamedCopyNotTheOldFile(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = succeeded(`\s\Album\01 A.flac`, `\s\Album\02 B.flac`)
s, downloads := newStubSlskd(t, stub)
old := filepath.Join(downloads, "Album", "01 A.flac")
if err := os.MkdirAll(filepath.Dir(old), 0o750); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(old, []byte("left by an earlier attempt"), 0o600); err != nil {
t.Fatal(err)
}
dst := t.TempDir()
got, err := s.Grab(
context.Background(), slskdAlbum(t, stub, "peer", "01 A.flac", "02 B.flac"), dst, nil,
)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 2 {
t.Fatalf("collected %d files, want 2", len(got.Files))
}
data, err := os.ReadFile(filepath.Join(dst, "01 A.flac"))
if err != nil || string(data) != "audio" {
t.Errorf("collected %q, want this grab's file", data)
}
if data, _ := os.ReadFile(old); string(data) != "left by an earlier attempt" {
t.Error("the file that was already there was moved")
}
}
// And when this grab's copy never arrived, the old one is not taken in
// its place.
func TestSlskdDoesNotCollectAFileThatWasAlreadyThere(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = [][]slskdTransfer{
{
{ID: "t0", Filename: `\s\Album\01 A.flac`, State: "Completed, Errored"},
{
ID: "t1",
Filename: `\s\Album\02 B.flac`,
State: "Completed, Succeeded",
BytesTransferred: 500,
},
},
}
s, downloads := newStubSlskd(t, stub)
old := filepath.Join(downloads, "Album", "01 A.flac")
if err := os.MkdirAll(filepath.Dir(old), 0o750); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(old, []byte("stale"), 0o600); err != nil {
t.Fatal(err)
}
got, err := s.Grab(
context.Background(), slskdAlbum(t, stub, "peer", "02 B.flac"), t.TempDir(), nil,
)
if err != nil {
t.Fatalf("Grab: %v", err)
}
if len(got.Files) != 1 || filepath.Base(got.Files[0]) != "02 B.flac" {
t.Errorf("collected %q, want only 02 B.flac", got.Files)
}
}
+132
View File
@@ -0,0 +1,132 @@
package download
import (
"context"
"errors"
"strings"
"testing"
"time"
)
// A peer that has queued us is judged by where we are in its queue, not
// only by a timer (#275).
func queuedThen(polls int, final string) [][]slskdTransfer {
queued := []slskdTransfer{
{ID: "t0", Filename: `\s\Album\01 A.flac`, State: "Queued, Remotely"},
{ID: "t1", Filename: `\s\Album\02 B.flac`, State: "Queued, Remotely"},
}
out := make([][]slskdTransfer, 0, polls+1)
for range polls {
out = append(out, queued)
}
return append(out, []slskdTransfer{
{ID: "t0", Filename: `\s\Album\01 A.flac`, State: final, BytesTransferred: 500},
{ID: "t1", Filename: `\s\Album\02 B.flac`, State: final, BytesTransferred: 500},
})
}
func descending(from int) []int {
out := make([]int, 0, from)
for p := from; p >= 1; p-- {
out = append(out, p)
}
return out
}
// Two readings far back in the queue give the peer up at once, rather
// than after the stall timer.
func TestSlskdGivesUpOnALongQueue(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = queuedThen(100_000, "Completed, Succeeded")
stub.positions = []int{400}
s, _ := newStubSlskd(t, stub)
s.stallAfter = time.Hour
started := time.Now()
_, err := s.Grab(context.Background(), slskdAlbum(t, stub, "peer"), t.TempDir(), nil)
if !errors.Is(err, ErrSlskdTimeout) || !strings.Contains(err.Error(), "position 400") {
t.Fatalf("Grab = %v, want a queue-position give-up", err)
}
if time.Since(started) > 5*time.Second {
t.Error("the give-up waited on something other than the position")
}
if len(stub.cancelledURIs()) == 0 {
t.Error("the queued transfers were not cancelled")
}
}
// One far reading is not enough: slskd says the figure can be wildly
// wrong.
func TestSlskdOneBadPositionIsNotEnough(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = queuedThen(20, "Completed, Succeeded")
stub.positions = []int{400, 3}
s, _ := newStubSlskd(t, stub)
s.positionPoll = 0
if _, err := s.Grab(
context.Background(),
slskdAlbum(t, stub, "peer", "01 A.flac", "02 B.flac"),
t.TempDir(), nil,
); err != nil {
t.Fatalf("Grab: %v", err)
}
}
// A queue that is moving is progress: the grab outlives the stall timer
// while its position improves.
func TestSlskdAMovingQueueIsProgress(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
// Near enough to wait for, with more improving readings (45) than
// there are queued polls (30), so the queue outlives the stall timer
// (30 polls of at least 2 ms against 40 ms) while still improving,
// however slowly the machine runs the loop.
stub.transfers = queuedThen(30, "Completed, Succeeded")
stub.positions = descending(slskdMaxQueuePosition - 5)
s, _ := newStubSlskd(t, stub)
s.transferPoll = 2 * time.Millisecond
s.positionPoll = 0
s.stallAfter = 40 * time.Millisecond
if _, err := s.Grab(
context.Background(),
slskdAlbum(t, stub, "peer", "01 A.flac", "02 B.flac"),
t.TempDir(), nil,
); err != nil {
t.Fatalf("Grab: %v; a moving queue was treated as a stall", err)
}
}
// However steadily the queue moves, waiting in it has a ceiling.
func TestSlskdQueueHasACeiling(t *testing.T) {
t.Parallel()
stub := newSlskdStub(t)
stub.transfers = queuedThen(100_000, "Completed, Succeeded")
stub.positions = descending(100_000)
s, _ := newStubSlskd(t, stub)
s.stallAfter = time.Hour
s.queueCeiling = 50 * time.Millisecond
_, err := s.Grab(context.Background(), slskdAlbum(t, stub, "peer"), t.TempDir(), nil)
if !errors.Is(err, ErrSlskdTimeout) {
t.Fatalf("Grab = %v, want the queue ceiling", err)
}
}
+19 -7
View File
@@ -232,12 +232,17 @@ type Candidate struct {
// results give paths and sizes but no tags, so Format and duration are // results give paths and sizes but no tags, so Format and duration are
// inferred from the path and size where possible. // inferred from the path and size where possible.
type CandidateFile struct { type CandidateFile struct {
Path string `json:"path"` Path string `json:"path"`
Size int64 `json:"size"` Size int64 `json:"size"`
Format Format `json:"format"` Format Format `json:"format"`
Bitrate int `json:"bitrate,omitempty"` // kbps, 0 when unknown Bitrate int `json:"bitrate,omitempty"` // kbps, 0 when unknown
IsAudio bool `json:"isAudio"` IsAudio bool `json:"isAudio"`
MatchedTo int `json:"matchedTo,omitempty"` // expected track position
// LengthMillis is the file's duration as the source reports it, or
// 0 when it does not. Soulseek reports it for most audio files.
LengthMillis int64 `json:"lengthMillis,omitempty"`
MatchedTo int `json:"matchedTo,omitempty"` // expected track position
} }
// Format is a normalized audio container/codec name. // Format is a normalized audio container/codec name.
@@ -286,7 +291,14 @@ type MatchScore struct {
TitleFit float64 `json:"titleFit"` // filenames vs expected titles TitleFit float64 `json:"titleFit"` // filenames vs expected titles
ArtistFit float64 `json:"artistFit"` // path/origin vs expected artist ArtistFit float64 `json:"artistFit"` // path/origin vs expected artist
AlbumFit float64 `json:"albumFit"` // folder name vs album title AlbumFit float64 `json:"albumFit"` // folder name vs album title
Completeness float64 `json:"completeness"` // audio files vs expected count Completeness float64 `json:"completeness"` // aligned tracks vs expected count
// DurationFit is how well the aligned files' lengths agree with the
// expected tracks', and DurationKnown whether enough of them stated
// a length for that to count. When it does not, the score is the
// four text signals alone, exactly as before durations were read.
DurationFit float64 `json:"durationFit"`
DurationKnown bool `json:"durationKnown"`
// Anchored records whether an MBID drove this score. Unanchored // Anchored records whether an MBID drove this score. Unanchored
// matches are capped, because there is nothing to be right about. // matches are capped, because there is nothing to be right about.
+17 -11
View File
@@ -383,9 +383,13 @@ func (si *SearchIndex) topByPopularity(
// MusicBrainz IDs, so this never touches the library tables and asks // MusicBrainz IDs, so this never touches the library tables and asks
// one query rather than one per artist. // one query rather than one per artist.
// //
// The artists are drawn most-popular-owned-album first, so a large // **The row is drawn at random, and that is the whole point of it.**
// library's pool is the part of it the user is likeliest to recognise // Ordered by popularity it was a second leaderboard: the same handful of
// rather than whichever artists sort first. // big names appeared every time the page opened, which is not what "you
// own one album by these artists" is saying. The pool is still bounded
// to `pool` artists — a 4 000-artist library does not need all of them
// ranked — but which of them, and which of their albums, is `RANDOM()`,
// so the shelf is a different sample each visit.
func (si *SearchIndex) unownedAlbumsBySinglyOwnedArtists( func (si *SearchIndex) unownedAlbumsBySinglyOwnedArtists(
ctx context.Context, ctx context.Context,
pool, limit int, pool, limit int,
@@ -396,15 +400,17 @@ func (si *SearchIndex) unownedAlbumsBySinglyOwnedArtists(
WHERE entity_type = 2 /* release_group */ WHERE entity_type = 2 /* release_group */
AND in_library = 0 AND in_library = 0
AND artist_mbid IN ( AND artist_mbid IN (
SELECT artist_mbid FROM explore_index SELECT artist_mbid FROM (
WHERE entity_type = 2 /* release_group */ SELECT artist_mbid FROM explore_index
AND in_library = 1 WHERE entity_type = 2 /* release_group */
AND artist_mbid != x'' AND in_library = 1
GROUP BY artist_mbid AND artist_mbid != x''
HAVING COUNT(*) = 1 GROUP BY artist_mbid
ORDER BY MAX(popularity) DESC HAVING COUNT(*) = 1
)
ORDER BY RANDOM()
LIMIT ?) LIMIT ?)
ORDER BY popularity DESC ORDER BY RANDOM()
LIMIT ?`, LIMIT ?`,
pool, limit, pool, limit,
)) ))
+6
View File
@@ -3,6 +3,7 @@ package explore
import ( import (
"context" "context"
"log/slog" "log/slog"
"sort"
"testing" "testing"
"yellowjacket/backend/database" "yellowjacket/backend/database"
@@ -202,6 +203,11 @@ func TestShelves_MoreFromOwnedNeedsExactlyOneOwnedAlbum(t *testing.T) {
titles = append(titles, album.Title) titles = append(titles, album.Title)
} }
// The row is a random sample, so the *set* is what is asserted and
// not the order — see `unownedAlbumsBySinglyOwnedArtists` for why
// the ordering was given up.
sort.Strings(titles)
if len(titles) != 2 || titles[0] != "Second" || titles[1] != "Third" { if len(titles) != 2 || titles[0] != "Second" || titles[1] != "Third" {
t.Fatalf("albums = %v, want [Second Third]", titles) t.Fatalf("albums = %v, want [Second Third]", titles)
} }
+4 -4
View File
@@ -8,6 +8,7 @@ import (
"yellowjacket/backend/coverart" "yellowjacket/backend/coverart"
"yellowjacket/backend/database/sql/sqlcgen" "yellowjacket/backend/database/sql/sqlcgen"
"yellowjacket/internal/testfixtures"
) )
// TestScan_StoresOnlyCoverTiers pins the size decision: a scan writes // TestScan_StoresOnlyCoverTiers pins the size decision: a scan writes
@@ -26,10 +27,9 @@ func TestScan_StoresOnlyCoverTiers(t *testing.T) {
lib, db := setupTestLibrary(t) lib, db := setupTestLibrary(t)
root, err := filepath.Abs("../../test_data/music_library_test") // Load skips when the fixture library has not been generated, as
if err != nil { // every other fixture test does.
t.Fatalf("resolve fixture path: %v", err) root := testfixtures.Load(t).Root()
}
library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{ library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{
Name: "Fixtures", Name: "Fixtures",
@@ -121,6 +121,12 @@ export interface CandidateFile {
"bitrate"?: number; "bitrate"?: number;
"isAudio": boolean; "isAudio": boolean;
/**
* LengthMillis is the file's duration as the source reports it, or
* 0 when it does not. Soulseek reports it for most audio files.
*/
"lengthMillis"?: number;
/** /**
* expected track position * expected track position
*/ */
@@ -453,10 +459,19 @@ export interface MatchScore {
"albumFit": number; "albumFit": number;
/** /**
* audio files vs expected count * aligned tracks vs expected count
*/ */
"completeness": number; "completeness": number;
/**
* DurationFit is how well the aligned files' lengths agree with the
* expected tracks', and DurationKnown whether enough of them stated
* a length for that to count. When it does not, the score is the
* four text signals alone, exactly as before durations were read.
*/
"durationFit": number;
"durationKnown": boolean;
/** /**
* Anchored records whether an MBID drove this score. Unanchored * Anchored records whether an MBID drove this score. Unanchored
* matches are capped, because there is nothing to be right about. * matches are capped, because there is nothing to be right about.
@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 320 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="M9.4 233.4c-12.5 12.5-12.5 32.8 0 45.3l192 192c12.5 12.5 32.8 12.5 45.3 0s12.5-32.8 0-45.3L77.3 256 246.6 86.6c12.5-12.5 12.5-32.8 0-45.3s-32.8-12.5-45.3 0l-192 192z"/></svg>

After

Width:  |  Height:  |  Size: 476 B

@@ -60,6 +60,7 @@ import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
import { dictByName } from '@utils/binding'; import { dictByName } from '@utils/binding';
import type { TrackDetails } from '@components/track-details/track-details.js'; import type { TrackDetails } from '@components/track-details/track-details.js';
import { showTrackDetailsForPath } from '@utils/track-details-opener.js'; import { showTrackDetailsForPath } from '@utils/track-details-opener.js';
import { openMusicBrainz } from '@utils/external-link';
import '@components/playlist-picker/playlist-picker.js'; import '@components/playlist-picker/playlist-picker.js';
import { import {
ICON_CAN_REQUEST, ICON_CAN_REQUEST,
@@ -2942,7 +2943,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
if (!track?.mbid) return; if (!track?.mbid) return;
window.open(`https://musicbrainz.org/recording/${track.mbid}`, '_blank', 'noopener'); openMusicBrainz(`/recording/${track.mbid}`);
} }
/** /**
@@ -4,6 +4,8 @@ import { customElement, property, state, query } from 'lit/decorators.js';
import { classMap } from 'lit/directives/class-map.js'; import { classMap } from 'lit/directives/class-map.js';
import { designTokens } from '../../styles/tokens.css'; import { designTokens } from '../../styles/tokens.css';
import { backButton } from '../../styles/back-button.css'; import { backButton } from '../../styles/back-button.css';
import { albumCardStyles } from '../../styles/album-card.css';
import '../scroll-row/scroll-row.js';
import { import {
LookupArtist, LookupArtist,
BrowseReleaseGroups, BrowseReleaseGroups,
@@ -46,11 +48,8 @@ import {
libraryStatusFor, libraryStatusFor,
toggleRequest, toggleRequest,
} from '@utils/library-status'; } from '@utils/library-status';
import { import { isOwned, ownershipLabel } from '@utils/ownership';
isOwned, import { openMusicBrainz } from '@utils/external-link';
ownershipLabel,
unownedStyles,
} from '@utils/ownership';
import { completenessStore } from '@store/completeness-store'; import { completenessStore } from '@store/completeness-store';
import '../catalog-scope-notice/catalog-scope-notice.js'; import '../catalog-scope-notice/catalog-scope-notice.js';
import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js'; import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js';
@@ -62,6 +61,7 @@ import {
ContextMenuController, ContextMenuController,
contextMenuStyles, contextMenuStyles,
isContextMenuKey, isContextMenuKey,
MenuKeyboard,
} from '@utils/context-menu-controller.js'; } from '@utils/context-menu-controller.js';
import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js';
import '@awesome.me/webawesome/dist/components/popup/popup.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js';
@@ -187,11 +187,11 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
@state() private topReleasesExpanded = false; @state() private topReleasesExpanded = false;
private topSectionStacked = false; private topSectionStacked = false;
private topSectionObserver?: ResizeObserver; private topSectionObserver?: ResizeObserver;
@state() private expandedDiscoGroups = new Set<string>();
/** Number of album cards that fit in one row of the discography grid. */ /** Whether the Play button's Shuffle dropdown is up. */
@state() private discoRowSize = 5; @state() private playMenuOpen = false;
private discoObserver?: ResizeObserver; private playMenuKeyboard = new MenuKeyboard(() => this.closePlayMenu());
@state() private similarExpanded = false; private playOutsideAttached = false;
/* ── Release prefetch ── */ /* ── Release prefetch ── */
@@ -221,6 +221,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
@query('#context-menu') @query('#context-menu')
private contextMenuPopup!: MenuSurface; private contextMenuPopup!: MenuSurface;
@query('.play-menu-button')
private playMenuButton?: HTMLButtonElement;
@query('#artist-play-menu')
private playMenuPanel?: HTMLElement;
@query('#playlist-submenu') @query('#playlist-submenu')
private playlistSubmenuPopup?: WaPopup; private playlistSubmenuPopup?: WaPopup;
@@ -271,7 +277,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
backButton, backButton,
exploreLinkStyles, exploreLinkStyles,
contextMenuStyles, contextMenuStyles,
unownedStyles, albumCardStyles,
css` css`
:host { :host {
display: flex; display: flex;
@@ -319,10 +325,45 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
object-fit: cover; object-fit: cover;
} }
.artist-follow { .artist-actions {
display: flex;
align-items: center;
gap: 8px;
flex-wrap: wrap;
margin-top: 10px; margin-top: 10px;
} }
/* The Play button and its caret are one control, so they
are one box: no gap between them, and the caret carries
the same filled appearance as the button it extends. */
.play-split {
display: inline-flex;
align-items: stretch;
}
.play-menu-button {
display: inline-flex;
align-items: center;
justify-content: center;
width: 28px;
padding: 0;
border: none;
border-left: 1px solid rgba(0, 0, 0, 0.25);
border-radius: 0 6px 6px 0;
background: var(--yj-accent, #ffd43b);
color: var(--yj-accent-fg, #000);
cursor: pointer;
}
.play-menu-button:hover {
filter: brightness(1.1);
}
.play-menu-button:focus-visible {
outline: 2px solid var(--yj-accent-text, #ffd43b);
outline-offset: 2px;
}
.artist-info { .artist-info {
display: flex; display: flex;
flex-direction: column; flex-direction: column;
@@ -331,7 +372,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
} }
.artist-title { .artist-title {
font-size: 24px; font-size: 28px;
font-weight: 700; font-weight: 700;
color: var(--yj-text-primary, #fff); color: var(--yj-text-primary, #fff);
white-space: nowrap; white-space: nowrap;
@@ -359,6 +400,13 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
flex-wrap: wrap; flex-wrap: wrap;
} }
/* The listen count is a headline number, not metadata, so
it sits a size above the type/country line. */
.artist-listens {
font-size: var(--yj-text-lg);
color: var(--yj-text-secondary, #b3b3b3);
}
.meta-separator { .meta-separator {
opacity: 0.4; opacity: 0.4;
} }
@@ -446,23 +494,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
outline-offset: -2px; outline-offset: -2px;
} }
.artist-play-actions {
margin-top: 10px;
display: flex;
gap: 8px;
align-items: center;
flex-wrap: wrap;
}
.track-rank {
width: 24px;
text-align: right;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-md);
font-variant-numeric: tabular-nums;
flex-shrink: 0;
}
.track-art { .track-art {
width: 32px; width: 32px;
height: 32px; height: 32px;
@@ -489,6 +520,52 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
opacity: 0.5; opacity: 0.5;
} }
/* Play where you own the track, the request badge where you
do not — over the artwork rather than at the end of the
row, where it was a badge beside a row you can already
double-click. */
.track-art-overlay {
position: absolute;
inset: 0;
display: flex;
align-items: center;
justify-content: center;
border-radius: 4px;
background: rgba(0, 0, 0, 0.55);
visibility: hidden;
opacity: 0;
transition: opacity 0.15s ease, visibility 0.15s ease;
}
.track-art-play {
display: flex;
align-items: center;
justify-content: center;
padding: 0;
border: none;
background: none;
color: #fff;
font-size: 14px;
cursor: pointer;
}
@media (hover: hover) and (pointer: fine) {
.track-item:hover .track-art-overlay,
.track-item:focus-within .track-art-overlay {
visibility: visible;
opacity: 1;
}
}
/* No hover means no double-click either, so the overlay is
the only route to playing a top track and must be there. */
@media not all and (hover: hover) {
.track-art-overlay {
visibility: visible;
opacity: 1;
}
}
.track-info { .track-info {
flex: 1; flex: 1;
min-width: 0; min-width: 0;
@@ -522,7 +599,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
.track-item library-status-indicator { .track-item library-status-indicator {
flex-shrink: 0; flex-shrink: 0;
} }
/* ── Top section (tracks + releases side-by-side) ── */ /* ── Top section (tracks + releases side-by-side) ── */
.top-section-wrapper { .top-section-wrapper {
container-type: inline-size; container-type: inline-size;
@@ -754,8 +830,30 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
white-space: nowrap; white-space: nowrap;
} }
.top-release-meta library-status-indicator { .top-release-art .album-card-badge {
flex-shrink: 0; position: absolute;
top: 4px;
left: 4px;
z-index: 1;
display: flex;
visibility: hidden;
opacity: 0;
transition: opacity 0.15s ease, visibility 0.15s ease;
}
@media (hover: hover) and (pointer: fine) {
.top-release-card:hover .album-card-badge,
.top-release-card:focus-within .album-card-badge {
visibility: visible;
opacity: 1;
}
}
@media not all and (hover: hover) {
.top-release-art .album-card-badge {
visibility: visible;
opacity: 1;
}
} }
@@ -773,150 +871,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
margin: 0; margin: 0;
} }
.album-grid {
display: grid;
grid-template-columns: repeat(auto-fill, 140px);
gap: 16px;
}
.album-grid.collapsed {
grid-template-rows: 1fr;
overflow: hidden;
}
.disco-toggle {
display: flex;
align-items: center;
justify-content: center;
gap: 6px;
padding: 4px 10px;
margin-top: 4px;
border: none;
border-radius: 6px;
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
color: var(--yj-text-secondary, #b3b3b3);
font-size: var(--yj-text-xs);
cursor: pointer;
transition: background 0.15s ease, color 0.15s ease;
width: 100%;
}
.disco-toggle:hover {
background: var(--yj-bg-hover, rgba(255, 255, 255, 0.1));
color: var(--yj-text-primary, #fff);
}
.disco-toggle wa-icon {
font-size: 11px;
transition: transform 0.2s ease;
}
.disco-toggle[aria-expanded='true'] wa-icon {
transform: rotate(180deg);
}
.album-card {
display: flex;
flex-direction: column;
gap: 6px;
padding: 8px;
border-radius: 8px;
cursor: pointer;
transition: background 0.15s ease;
}
.album-card:hover {
background: var(
--yj-bg-overlay,
rgba(255, 255, 255, 0.06)
);
}
.album-card:active {
transform: scale(0.97);
}
.album-art-container {
width: 100%;
aspect-ratio: 1;
border-radius: 4px;
overflow: hidden;
flex-shrink: 0;
position: relative;
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
}
.album-art-container img {
width: 100%;
height: 100%;
object-fit: cover;
display: block;
border-radius: 4px;
}
.album-art-fallback {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
position: absolute;
inset: 0;
}
.album-art-fallback wa-icon {
color: var(--yj-text-tertiary, #888);
font-size: 24px;
opacity: 0.5;
}
.album-title {
font-weight: 500;
color: var(--yj-text-primary, #fff);
font-size: var(--yj-text-sm);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-meta {
display: flex;
align-items: center;
justify-content: space-between;
gap: 6px;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
min-height: 20px;
}
.album-meta-text {
display: flex;
align-items: center;
gap: 6px;
min-width: 0;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
.album-meta library-status-indicator {
flex-shrink: 0;
margin-left: auto;
}
/* ── Similar artists ── */ /* ── Similar artists ── */
.similar-row {
display: grid;
grid-template-columns: repeat(auto-fill, 140px);
gap: 16px;
overflow: hidden;
}
.similar-row.collapsed {
grid-template-rows: 1fr;
overflow: hidden;
}
.similar-artist-card { .similar-artist-card {
display: flex; display: flex;
flex-direction: column; flex-direction: column;
@@ -926,6 +881,9 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
border-radius: 8px; border-radius: 8px;
cursor: pointer; cursor: pointer;
text-align: center; text-align: center;
width: 120px;
box-sizing: border-box;
flex-shrink: 0;
transition: background 0.15s ease; transition: background 0.15s ease;
} }
@@ -1056,7 +1014,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
this.unsubSimilarReady?.(); this.unsubSimilarReady?.();
if (this.discogFallbackTimer) clearTimeout(this.discogFallbackTimer); if (this.discogFallbackTimer) clearTimeout(this.discogFallbackTimer);
this.topSectionObserver?.disconnect(); this.topSectionObserver?.disconnect();
this.discoObserver?.disconnect(); this.detachPlayOutsideClose();
} }
/** /**
@@ -1083,17 +1041,14 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
protected override firstUpdated() { protected override firstUpdated() {
this.observeTopSectionWidth(); this.observeTopSectionWidth();
this.observeDiscoWidth();
} }
protected override updated() { protected override updated() {
// Re-attach observers if elements appeared after initial render. // Re-attach the observer if the section appeared after initial
// render.
if (!this.topSectionObserver) { if (!this.topSectionObserver) {
this.observeTopSectionWidth(); this.observeTopSectionWidth();
} }
if (!this.discoObserver) {
this.observeDiscoWidth();
}
} }
/** /**
@@ -1126,32 +1081,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
this.topSectionObserver.observe(wrapper); this.topSectionObserver.observe(wrapper);
} }
/**
* Watch the .content width and compute how many album cards
* fit in one row of the discography grid.
* Grid uses: repeat(auto-fill, minmax(140px, 1fr)) with 16px gap
* and album-card has 8px padding on each side.
*/
private observeDiscoWidth() {
const content = this.renderRoot.querySelector('.content');
if (!content) return;
const CARD_MIN = 140;
const GAP = 16;
this.discoObserver = new ResizeObserver((entries) => {
for (const entry of entries) {
const width = entry.contentBoxSize?.[0]?.inlineSize ?? entry.contentRect.width;
const cols = Math.max(1, Math.floor((width + GAP) / (CARD_MIN + GAP)));
if (cols !== this.discoRowSize) {
this.discoRowSize = cols;
}
}
});
this.discoObserver.observe(content);
}
/* ── Data Loading ── */ /* ── Data Loading ── */
private async loadAllData() { private async loadAllData() {
@@ -1862,6 +1791,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
} catch { } catch {
// No image — letter avatar stays. // No image — letter avatar stays.
} }
return undefined;
}), }),
); );
} }
@@ -1999,6 +1930,65 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
} }
} }
/* ── Play / Shuffle split button ── */
/**
* Open the Play button's Shuffle dropdown.
*
* `page-header`'s overflow menu one control over: the same
* `MenuKeyboard`, the same document-level outside-close, and the
* same `menu-surface`, so the phone gets the bottom sheet rather
* than a popup that Chrome 113 clips.
*/
private togglePlayMenu = (): void => {
if (this.playMenuOpen) {
this.closePlayMenu();
return;
}
this.playMenuOpen = true;
void this.updateComplete.then(() => {
if (!this.playMenuOpen) return;
this.playMenuKeyboard.open(
this.playMenuPanel ?? null,
this.playMenuButton ?? null,
);
this.attachPlayOutsideClose();
});
};
private closePlayMenu = (): void => {
if (!this.playMenuOpen) return;
this.detachPlayOutsideClose();
this.playMenuKeyboard.close();
this.playMenuOpen = false;
};
private onPlayOutsideDown = (e: Event): void => {
if (e.composedPath().includes(this.playMenuPanel as EventTarget)) return;
if (e.composedPath().includes(this.playMenuButton as EventTarget)) return;
this.closePlayMenu();
};
private attachPlayOutsideClose(): void {
if (this.playOutsideAttached) return;
this.playOutsideAttached = true;
document.addEventListener('mousedown', this.onPlayOutsideDown, true);
}
private detachPlayOutsideClose(): void {
if (!this.playOutsideAttached) return;
this.playOutsideAttached = false;
document.removeEventListener('mousedown', this.onPlayOutsideDown, true);
}
/** /**
* File path for one top track, resolved by recording MBID — the * File path for one top track, resolved by recording MBID — the
* same key `localId` was set from. Works whether or not the * same key `localId` was set from. Works whether or not the
@@ -2240,11 +2230,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
if (!release?.mbid) return; if (!release?.mbid) return;
window.open( openMusicBrainz(`/release-group/${release.mbid}`);
`https://musicbrainz.org/release-group/${release.mbid}`,
'_blank',
'noopener',
);
} }
private onContextMenuAction( private onContextMenuAction(
@@ -2358,7 +2344,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
if (!track?.recordingMbid) return; if (!track?.recordingMbid) return;
window.open(`https://musicbrainz.org/recording/${track.recordingMbid}`, '_blank', 'noopener'); openMusicBrainz(`/recording/${track.recordingMbid}`);
} }
/* ── Navigation ── */ /* ── Navigation ── */
@@ -2538,10 +2524,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
: nothing} : nothing}
${this.renderArtistMeta()} ${this.renderArtistMeta()}
${this.artist?.popularity && this.artist.popularity > 0 ${this.artist?.popularity && this.artist.popularity > 0
? html`<span class="artist-meta">${formatListenCount(this.artist.popularity)} plays on ListenBrainz</span>` ? html`<span class="artist-listens">${formatListenCount(this.artist.popularity)} plays on ListenBrainz</span>`
: nothing} : nothing}
${this.renderPlayLibraryAction()} <div class="artist-actions">
${this.renderFollowAction()} ${this.renderPlayLibraryAction()}
${this.renderFollowAction()}
</div>
</div> </div>
</div> </div>
<div class="content"> <div class="content">
@@ -2572,25 +2560,53 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
if (this.ownedLocalAlbumIds().length === 0) return nothing; if (this.ownedLocalAlbumIds().length === 0) return nothing;
return html` return html`
<div class="artist-play-actions"> <div class="play-split">
<wa-button <wa-button
size="small" size="small"
appearance="filled" appearance="filled"
data-testid="artist-play-library" data-testid="artist-play-library"
title="Play library tracks"
@click=${() => void this.playLibraryTracks(false)} @click=${() => void this.playLibraryTracks(false)}
> >
<wa-icon slot="start" name="play"></wa-icon> <wa-icon slot="start" name="play"></wa-icon>
Play library tracks Play
</wa-button> </wa-button>
<wa-button <menu-surface
size="small" placement="bottom-start"
appearance="outlined" .active=${this.playMenuOpen}
data-testid="artist-shuffle-library" @menu-dismiss=${this.closePlayMenu}
@click=${() => void this.playLibraryTracks(true)}
> >
<wa-icon slot="start" name="shuffle"></wa-icon> <button
Shuffle slot="anchor"
</wa-button> class="play-menu-button"
type="button"
data-testid="artist-play-menu"
aria-label="More play options"
aria-haspopup="menu"
aria-expanded=${this.playMenuOpen ? 'true' : 'false'}
aria-controls="artist-play-menu"
@click=${this.togglePlayMenu}
>
<wa-icon name="chevron-down"></wa-icon>
</button>
<div
id="artist-play-menu"
class="context-menu-panel"
role="menu"
aria-label="Play options"
>
<wa-dropdown-item
data-testid="artist-shuffle-library"
@click=${() => {
this.closePlayMenu();
void this.playLibraryTracks(true);
}}
>
<wa-icon slot="icon" name="shuffle"></wa-icon>
Shuffle
</wa-dropdown-item>
</div>
</menu-surface>
</div> </div>
`; `;
} }
@@ -2786,25 +2802,27 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
const request = downloadStore.requestFor(this.artistMBID); const request = downloadStore.requestFor(this.artistMBID);
return html` return html`
<div class="artist-follow"> <wa-button
<wa-button size="small"
size="small" appearance=${request ? 'filled' : 'outlined'}
appearance=${request ? 'filled' : 'outlined'} data-testid="artist-follow"
@click=${() => void this.toggleFollow(request?.id)} title=${request
> ? 'Following this artist'
<!-- This was bookmark-check, which is not in : 'Follow this artist for new releases'}
names.txt and so has rendered the missing-icon @click=${() => void this.toggleFollow(request?.id)}
fallback — a circled question mark — on every >
followed artist since it was written. A <!-- This was bookmark-check, which is not in
backtick around that name would end this names.txt and so has rendered the missing-icon
template literal, which is why there is none. --> fallback — a circled question mark — on every
<wa-icon followed artist since it was written. A
slot="start" backtick around that name would end this
name=${request ? ICON_REQUESTED : ICON_CAN_REQUEST} template literal, which is why there is none. -->
></wa-icon> <wa-icon
${request ? 'Following' : 'Follow for new releases'} slot="start"
</wa-button> name=${request ? ICON_REQUESTED : ICON_CAN_REQUEST}
</div> ></wa-icon>
${request ? 'Following' : 'Follow'}
</wa-button>
`; `;
} }
@@ -2888,16 +2906,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
this.topReleasesExpanded = !this.topReleasesExpanded; this.topReleasesExpanded = !this.topReleasesExpanded;
} }
private toggleDiscoGroup(type: string) {
const next = new Set(this.expandedDiscoGroups);
if (next.has(type)) {
next.delete(type);
} else {
next.add(type);
}
this.expandedDiscoGroups = next;
}
private renderTopSection() { private renderTopSection() {
const hasTracks = !this.loadingTracks && this.topTracks.length > 0; const hasTracks = !this.loadingTracks && this.topTracks.length > 0;
const hasReleases = !this.loadingTopReleases && this.topReleaseGroups.length > 0; const hasReleases = !this.loadingTopReleases && this.topReleaseGroups.length > 0;
@@ -2971,6 +2979,31 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
}} />` }} />`
: html`<wa-icon name="compact-disc"></wa-icon>`; : html`<wa-icon name="compact-disc"></wa-icon>`;
})()} })()}
<!-- Over the artwork, not beside the
row: play where you own it, the
request badge where you do not. -->
<div class="track-art-overlay">
${owned
? html`<button
class="track-art-play"
type="button"
aria-label=${`Play ${t.trackName}`}
@click=${(e: Event) => {
e.stopPropagation();
void this.playTrack(t);
}}
>
<wa-icon name="play"></wa-icon>
</button>`
: html`<library-status-indicator
status=${libraryStatusFor(false, t.recordingMbid)}
entity-type="track"
label=${t.trackName}
request-mbid=${t.recordingMbid}
request-artist=${t.artistName ?? ''}
size="18"
></library-status-indicator>`}
</div>
</div> </div>
<div class="track-info"> <div class="track-info">
<div class="track-title">${trackLink(t.trackName, t.releaseName, t.releaseGroupMbid ?? '', t.recordingMbid)}</div> <div class="track-title">${trackLink(t.trackName, t.releaseName, t.releaseGroupMbid ?? '', t.recordingMbid)}</div>
@@ -2979,15 +3012,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<span class="track-listens"> <span class="track-listens">
${formatListenCount(t.totalListenCount)} plays ${formatListenCount(t.totalListenCount)} plays
</span> </span>
${owned
? nothing
: html`<library-status-indicator
status=${libraryStatusFor(false, t.recordingMbid)}
entity-type="track"
label=${t.trackName}
request-mbid=${t.recordingMbid}
request-artist=${t.artistName ?? ''}
></library-status-indicator>`}
</div> </div>
`; `;
})} })}
@@ -3093,6 +3117,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<div class="album-art-fallback" style="${artURL ? 'display: none' : ''}"> <div class="album-art-fallback" style="${artURL ? 'display: none' : ''}">
<wa-icon name="compact-disc"></wa-icon> <wa-icon name="compact-disc"></wa-icon>
</div> </div>
<div class="album-card-badge">
<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.releaseGroupMbid}
request-artist=${this.artist?.name ?? ''}
size="21"
></library-status-indicator>
</div>
</div> </div>
<div class="top-release-text"> <div class="top-release-text">
<div class="top-release-title" title="${rg.title}"> <div class="top-release-title" title="${rg.title}">
@@ -3102,18 +3138,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<div class="top-release-meta-text"> <div class="top-release-meta-text">
${rg.date ? html`<span>${extractYear(rg.date)}</span>` : nothing} ${rg.date ? html`<span>${extractYear(rg.date)}</span>` : nothing}
</div> </div>
${badge.status === 'in-library'
? nothing
: html`<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.releaseGroupMbid}
request-artist=${this.artist?.name ?? ''}
size="18"
></library-status-indicator>`}
</div> </div>
</div> </div>
</div> </div>
@@ -3164,37 +3188,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<section> <section>
<h3 class="section-header">Discography</h3> <h3 class="section-header">Discography</h3>
${groups.map( ${groups.map(
(g) => { (g) => html`
const isExpanded = this.expandedDiscoGroups.has(g.type); <div class="disco-group">
const rowSize = this.discoRowSize; <h4 class="disco-type-header">
const showToggle = g.items.length > rowSize; ${g.type === 'Other' ? 'Other Releases' : g.type.endsWith('s') ? g.type : `${g.type}s`}
const visibleItems = isExpanded ? g.items : g.items.slice(0, rowSize); </h4>
<scroll-row>
return html` ${g.items.map((rg) => this.renderAlbumCard(rg))}
<div class="disco-group"> </scroll-row>
<h4 class="disco-type-header"> </div>
${g.type === 'Other' ? 'Other Releases' : g.type.endsWith('s') ? g.type : `${g.type}s`} `,
</h4>
<div class="album-grid">
${visibleItems.map((rg) => this.renderAlbumCard(rg))}
</div>
${showToggle
? html`
<button
class="disco-toggle"
aria-expanded="${isExpanded}"
@click=${() => this.toggleDiscoGroup(g.type)}
>
${isExpanded
? 'Show less'
: `Show all ${g.items.length}`}
<wa-icon name="chevron-down"></wa-icon>
</button>
`
: nothing}
</div>
`;
},
)} )}
</section> </section>
`; `;
@@ -3234,23 +3237,25 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<div class="album-art-fallback" style="${artURL ? 'display: none' : ''}"> <div class="album-art-fallback" style="${artURL ? 'display: none' : ''}">
<wa-icon name="compact-disc"></wa-icon> <wa-icon name="compact-disc"></wa-icon>
</div> </div>
<div class="album-card-badge">
<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.mbid}
request-artist=${this.artist?.name ?? ''}
size="23"
></library-status-indicator>
</div>
</div> </div>
<div class="album-title" title="${rg.title}">${rg.title}</div> <div class="album-title" title="${rg.title}">${rg.title}</div>
<div class="album-artist">${rg.artistCredit ?? ''}</div>
<div class="album-meta"> <div class="album-meta">
<div class="album-meta-text"> <div class="album-meta-text">
${year ? html`<span>${year}</span>` : nothing} ${year ? html`<span>${year}</span>` : nothing}
</div> </div>
${badge.status === 'in-library'
? nothing
: html`<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.mbid}
request-artist=${this.artist?.name ?? ''}
></library-status-indicator>`}
</div> </div>
</div> </div>
`; `;
@@ -3266,15 +3271,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
// Cap the similar-artists list at 10 to avoid a very long list. // Cap the similar-artists list at 10 to avoid a very long list.
const maxSimilar = 10; const maxSimilar = 10;
const artists = this.similarArtists.slice(0, maxSimilar); const artists = this.similarArtists.slice(0, maxSimilar);
const showToggle = artists.length > this.discoRowSize;
const collapsed = !this.similarExpanded && showToggle;
const visible = collapsed ? artists.slice(0, this.discoRowSize) : artists;
return html` return html`
<section> <section>
<h3 class="section-header">Similar Artists</h3> <h3 class="section-header">Similar Artists</h3>
<div class="similar-row ${collapsed ? 'collapsed' : ''}"> <scroll-row>
${visible.map((a) => { ${artists.map((a) => {
const imgURL = this.similarImageURLs.get(a.artistMbid); const imgURL = this.similarImageURLs.get(a.artistMbid);
return html` return html`
<div <div
@@ -3313,21 +3315,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
</div> </div>
`; `;
})} })}
</div> </scroll-row>
${showToggle
? html`
<button
class="disco-toggle"
aria-expanded="${this.similarExpanded}"
@click=${() => { this.similarExpanded = !this.similarExpanded; }}
>
${this.similarExpanded
? 'Show less'
: `Show all ${artists.length}`}
<wa-icon name="chevron-down"></wa-icon>
</button>
`
: nothing}
</section> </section>
`; `;
} }
@@ -1,10 +1,7 @@
import { avatarBackground } from '@utils/avatar-color'; import { avatarBackground } from '@utils/avatar-color';
import { albumBadgeFor, libraryStatusFor } from '@utils/library-status'; import { albumBadgeFor, libraryStatusFor } from '@utils/library-status';
import { import { isOwned, ownershipLabel } from '@utils/ownership';
isOwned, import { openMusicBrainz } from '@utils/external-link';
ownershipLabel,
unownedStyles,
} from '@utils/ownership';
import { completenessStore } from '@store/completeness-store'; import { completenessStore } from '@store/completeness-store';
import { downloadStore } from '@store/download-store'; import { downloadStore } from '@store/download-store';
import { LitElement, html, css, nothing } from 'lit'; import { LitElement, html, css, nothing } from 'lit';
@@ -13,6 +10,8 @@ import { classMap } from 'lit/directives/class-map.js';
import '@components/page-header/page-header'; import '@components/page-header/page-header';
import { designTokens } from '../../styles/tokens.css'; import { designTokens } from '../../styles/tokens.css';
import { srOnly } from '../../styles/sr-only.css'; import { srOnly } from '../../styles/sr-only.css';
import { albumCardStyles } from '../../styles/album-card.css';
import '../scroll-row/scroll-row.js';
import { SearchLocal, SearchLyrics, GetThumbnail, GetThumbnails, GetArtistImageURL, GetArtistImagesCachedPaths, GetExploreShelves, RecordSearchClick } from '@go/explore/service.js'; import { SearchLocal, SearchLyrics, GetThumbnail, GetThumbnails, GetArtistImageURL, GetArtistImagesCachedPaths, GetExploreShelves, RecordSearchClick } from '@go/explore/service.js';
import { GetFilePathsByAlbums, GetFilePathsByRecordingMBIDs } from '@go/library/library.js'; import { GetFilePathsByAlbums, GetFilePathsByRecordingMBIDs } from '@go/library/library.js';
import { EventsOn } from '@runtime/runtime'; import { EventsOn } from '@runtime/runtime';
@@ -253,7 +252,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
srOnly, srOnly,
exploreLinkStyles, exploreLinkStyles,
contextMenuStyles, contextMenuStyles,
unownedStyles, albumCardStyles,
css` css`
:host { :host {
display: block; display: block;
@@ -529,21 +528,9 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
line-height: 1.5; line-height: 1.5;
} }
/* ── Horizontal scroll rows ── */
.horizontal-row {
display: flex;
gap: 12px;
overflow-x: auto;
padding-bottom: 4px;
scrollbar-width: none;
}
.horizontal-row::-webkit-scrollbar {
display: none;
}
/* ── Top result cards ── */
/* ── Artist cards ── */ /* ── Artist cards ── */
/* Fixed width, for the reason the album card is: a range
means two cards in one row are different sizes. */
.artist-card { .artist-card {
display: flex; display: flex;
flex-direction: column; flex-direction: column;
@@ -552,8 +539,8 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
padding: 10px; padding: 10px;
border-radius: 8px; border-radius: 8px;
cursor: pointer; cursor: pointer;
min-width: 100px; width: 120px;
max-width: 120px; box-sizing: border-box;
flex-shrink: 0; flex-shrink: 0;
text-align: center; text-align: center;
transition: background 0.15s ease; transition: background 0.15s ease;
@@ -624,115 +611,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
font-size: var(--yj-text-xs); font-size: var(--yj-text-xs);
} }
/* ── Album cards ── */
.album-card {
display: flex;
flex-direction: column;
gap: 6px;
padding: 8px;
border-radius: 8px;
cursor: pointer;
min-width: 130px;
max-width: 150px;
flex-shrink: 0;
transition: background 0.15s ease;
}
.album-card:hover {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
}
.album-card:active {
transform: scale(0.97);
}
.album-art-container {
width: 100%;
aspect-ratio: 1;
border-radius: 4px;
overflow: hidden;
background: linear-gradient(
135deg,
var(--yj-bg-overlay, #404040) 0%,
var(--yj-bg-surface, #282828) 100%
);
display: flex;
align-items: center;
justify-content: center;
position: relative;
}
.album-art-container img {
width: 100%;
height: 100%;
object-fit: cover;
display: block;
}
.album-art-fallback {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
position: absolute;
inset: 0;
}
.album-art-fallback wa-icon {
color: var(--yj-text-tertiary, #888);
font-size: 24px;
opacity: 0.5;
}
.album-title {
font-weight: 500;
color: var(--yj-text-primary, #fff);
font-size: var(--yj-text-sm);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-artist {
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-meta {
display: flex;
align-items: center;
justify-content: space-between;
gap: 6px;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
min-height: 20px;
}
.album-meta-text {
display: flex;
align-items: center;
gap: 6px;
min-width: 0;
overflow: hidden;
}
.album-meta library-status-indicator {
flex-shrink: 0;
margin-left: auto;
}
.type-badge {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.08));
padding: 1px 6px;
border-radius: 3px;
font-size: 10px;
white-space: nowrap;
}
/* ── Track list ── */ /* ── Track list ── */
.track-list { .track-list {
display: flex; display: flex;
@@ -753,7 +631,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
cursor: pointer; cursor: pointer;
} }
.album-card:focus-visible,
.track-item:focus-visible { .track-item:focus-visible {
outline: 2px solid var(--yj-accent-text, #ffd43b); outline: 2px solid var(--yj-accent-text, #ffd43b);
outline-offset: -2px; outline-offset: -2px;
@@ -1371,7 +1248,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
const entity = target.kind === 'album' ? 'release-group' : 'recording'; const entity = target.kind === 'album' ? 'release-group' : 'recording';
window.open(`https://musicbrainz.org/${entity}/${target.mbid}`, '_blank', 'noopener'); openMusicBrainz(`/${entity}/${target.mbid}`);
} }
private renderExploreContextMenu() { private renderExploreContextMenu() {
@@ -1662,6 +1539,8 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
} catch { } catch {
// No image — leave empty string. // No image — leave empty string.
} }
return undefined;
}), }),
); );
@@ -2122,7 +2001,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
${subtitle ${subtitle
? html`<p class="section-reason">${subtitle}</p>` ? html`<p class="section-reason">${subtitle}</p>`
: nothing} : nothing}
<div class="horizontal-row"> <scroll-row>
${artists.map((a) => { ${artists.map((a) => {
const owned = isOwned(a); const owned = isOwned(a);
const name = a.englishName || a.name; const name = a.englishName || a.name;
@@ -2171,7 +2050,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
</div> </div>
`; `;
})} })}
</div> </scroll-row>
</section> </section>
`; `;
} }
@@ -2187,7 +2066,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
${subtitle ${subtitle
? html`<p class="section-reason">${subtitle}</p>` ? html`<p class="section-reason">${subtitle}</p>`
: nothing} : nothing}
<div class="horizontal-row"> <scroll-row>
${releaseGroups.map((rg) => { ${releaseGroups.map((rg) => {
const artURL = this.thumbnailCache.get(rg.mbid) || ''; const artURL = this.thumbnailCache.get(rg.mbid) || '';
const year = extractYear(rg.firstReleaseDate); const year = extractYear(rg.firstReleaseDate);
@@ -2249,6 +2128,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
> >
<wa-icon name="compact-disc"></wa-icon> <wa-icon name="compact-disc"></wa-icon>
</div> </div>
<div class="album-card-badge">
<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.mbid}
request-artist=${rg.artistCredit ?? ''}
size="23"
></library-status-indicator>
</div>
</div> </div>
<div class="album-title" title="${rg.title}"> <div class="album-title" title="${rg.title}">
${rg.title} ${rg.title}
@@ -2256,29 +2147,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
<div class="album-artist">${creditLink(creditStore.credits(rg.mbid), rg.artistCredit, rg.artistMbid ?? '')}</div> <div class="album-artist">${creditLink(creditStore.credits(rg.mbid), rg.artistCredit, rg.artistMbid ?? '')}</div>
<div class="album-meta"> <div class="album-meta">
<div class="album-meta-text"> <div class="album-meta-text">
${year ? html`<span>${year}</span>` : nothing}
${rg.primaryType ${rg.primaryType
? html`<span class="type-badge" ? html`<span class="type-badge"
>${rg.primaryType}</span >${rg.primaryType}</span
>` >`
: nothing} : nothing}
${year ? html`<span>${year}</span>` : nothing}
</div> </div>
${badge.status === 'in-library'
? nothing
: html`<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.mbid}
request-artist=${rg.artistCredit ?? ''}
></library-status-indicator>`}
</div> </div>
</div> </div>
`; `;
})} })}
</div> </scroll-row>
</section> </section>
`; `;
} }
+5 -12
View File
@@ -12,6 +12,7 @@ import { libraryStore } from '@store/library-store';
import { EventsOn } from '@runtime/runtime'; import { EventsOn } from '@runtime/runtime';
import { Events } from '../../events'; import { Events } from '../../events';
import '@components/page-header/page-header'; import '@components/page-header/page-header';
import '../scroll-row/scroll-row.js';
import { designTokens } from '../../styles/tokens.css'; import { designTokens } from '../../styles/tokens.css';
import { ViewLifecycleMixin } from '../../utils/view-lifecycle'; import { ViewLifecycleMixin } from '../../utils/view-lifecycle';
@@ -99,16 +100,6 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
color: var(--yj-text-tertiary, #888); color: var(--yj-text-tertiary, #888);
} }
.row {
display: grid;
grid-auto-flow: column;
grid-auto-columns: 160px;
gap: 14px;
overflow-x: auto;
padding-bottom: 6px;
scrollbar-width: thin;
}
.card { .card {
background: none; background: none;
border: none; border: none;
@@ -117,6 +108,8 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
cursor: pointer; cursor: pointer;
color: inherit; color: inherit;
display: block; display: block;
width: 160px;
flex-shrink: 0;
} }
.art { .art {
@@ -336,9 +329,9 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
<span class="shelf-title">${shelf.title}</span> <span class="shelf-title">${shelf.title}</span>
</div> </div>
<p class="shelf-sub">${shelf.subtitle}</p> <p class="shelf-sub">${shelf.subtitle}</p>
<div class="row"> <scroll-row>
${(shelf.albums ?? []).map((album) => this.renderCard(album))} ${(shelf.albums ?? []).map((album) => this.renderCard(album))}
</div> </scroll-row>
</section> </section>
`; `;
} }
@@ -0,0 +1,213 @@
import { LitElement, css, html } from 'lit';
import { customElement, query, state } from 'lit/decorators.js';
import '@awesome.me/webawesome/dist/components/icon/icon.js';
/** How far one press moves the row — most of a screenful, not all of
* it, so the card that was at the edge stays as an anchor. */
const SCROLL_FRACTION = 0.8;
/**
* A horizontally scrolling row with arrow buttons.
*
* The shelves, the search results and (now) the artist page's
* discography and similar-artists rows are all "more than fits, scroll
* sideways". Until this existed the only way to see the rest was a
* mousewheel or a trackpad gesture, which is not an affordance — a
* mouse with no horizontal wheel simply could not reach the cards past
* the fold.
*
* It is a component rather than a rule on `.horizontal-row` for two
* reasons. The arrows are *state* — which way the row can still move —
* and that state has to be recomputed when the viewport resizes or a
* card arrives with its cover art; a stylesheet cannot do that. And
* every caller then gets the same arrows, the same reveal and the same
* keyboard labels without writing them again.
*
* **The arrows are `hidden`, not merely transparent, at the end they
* cannot move from** — a control that cannot act is worse than none,
* and an invisible one still holds a hit area and a tab stop. On a
* pointer device the pair fades in with the row's hover; where there is
* no hover they are always visible, because there is no other route to
* them there (a swipe is not an affordance a mouse-less keyboard user
* has either).
*
* The cards are light DOM children and stay in the *host's* shadow
* root, so the host's own `.album-card` / `.artist-card` styles apply
* unchanged — this component only owns the box they scroll inside.
*/
@customElement('scroll-row')
export class ScrollRow extends LitElement {
@query('.viewport') private viewport?: HTMLElement;
@state() private atStart = true;
@state() private atEnd = true;
@state() private overflowing = false;
private observer?: ResizeObserver;
static override styles = css`
:host {
display: block;
position: relative;
}
.viewport {
overflow-x: auto;
overflow-y: hidden;
scrollbar-width: none;
/* A swipe that reaches the row's end should not drag the
whole page sideways with it. */
overscroll-behavior-x: contain;
}
.viewport::-webkit-scrollbar {
display: none;
}
.track {
display: flex;
gap: 12px;
}
.arrow {
position: absolute;
top: 50%;
transform: translateY(-50%);
z-index: 2;
display: flex;
align-items: center;
justify-content: center;
width: 36px;
height: 36px;
padding: 0;
border-radius: 50%;
border: 1px solid var(--yj-border-subtle, rgba(255, 255, 255, 0.1));
background: var(--yj-bg-elevated, #343a40);
color: var(--yj-text-primary, #fff);
cursor: pointer;
opacity: 0;
transition: opacity 0.15s ease;
}
.arrow[hidden] {
display: none;
}
.arrow.prev {
left: 4px;
}
.arrow.next {
right: 4px;
}
.arrow:hover {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.12));
}
.arrow:focus-visible {
outline: 2px solid var(--yj-accent, #ffd43b);
outline-offset: 2px;
}
@media (hover: hover) and (pointer: fine) {
:host(:hover) .arrow,
.arrow:focus-visible {
opacity: 1;
}
}
@media not all and (hover: hover) {
.arrow {
opacity: 1;
}
}
`;
override firstUpdated(): void {
const viewport = this.viewport;
if (!viewport) return;
this.observer = new ResizeObserver(() => this.measure());
this.observer.observe(viewport);
// The track's own size is what changes when a card arrives with
// its cover art, and a ResizeObserver on the viewport alone
// never fires for that.
const track = viewport.firstElementChild;
if (track) this.observer.observe(track);
this.measure();
}
override disconnectedCallback(): void {
super.disconnectedCallback();
this.observer?.disconnect();
this.observer = undefined;
}
private measure(): void {
const viewport = this.viewport;
if (!viewport) return;
this.overflowing = viewport.scrollWidth > viewport.clientWidth + 1;
this.atStart = viewport.scrollLeft <= 1;
this.atEnd =
viewport.scrollLeft + viewport.clientWidth >=
viewport.scrollWidth - 1;
}
private onScroll = (): void => this.measure();
private scrollStep(direction: -1 | 1): void {
const viewport = this.viewport;
if (!viewport) return;
viewport.scrollBy({
left: direction * viewport.clientWidth * SCROLL_FRACTION,
behavior: 'smooth',
});
}
override render() {
const showPrev = this.overflowing && !this.atStart;
const showNext = this.overflowing && !this.atEnd;
return html`
<button
class="arrow prev"
type="button"
aria-label="Scroll left"
?hidden=${!showPrev}
@click=${() => this.scrollStep(-1)}
>
<wa-icon name="chevron-left"></wa-icon>
</button>
<div class="viewport" @scroll=${this.onScroll}>
<div class="track"><slot></slot></div>
</div>
<button
class="arrow next"
type="button"
aria-label="Scroll right"
?hidden=${!showNext}
@click=${() => this.scrollStep(1)}
>
<wa-icon name="chevron-right"></wa-icon>
</button>
`;
}
}
declare global {
interface HTMLElementTagNameMap {
'scroll-row': ScrollRow;
}
}
@@ -1,6 +1,7 @@
import { LitElement, html, css, nothing } from 'lit'; import { LitElement, html, css, nothing } from 'lit';
import { customElement, property } from 'lit/decorators.js'; import { customElement, property } from 'lit/decorators.js';
import { designTokens } from '../../styles/tokens.css'; import { designTokens } from '../../styles/tokens.css';
import '../scroll-row/scroll-row.js';
import type * as explore from '@go/explore/models.js'; import type * as explore from '@go/explore/models.js';
import { import {
GetArtistImageURL, GetArtistImageURL,
@@ -15,7 +16,6 @@ import { albumBadgeFor, libraryStatusFor } from '../../utils/library-status';
import { import {
isOwned, isOwned,
ownershipLabel, ownershipLabel,
unownedStyles,
type OwnableKind, type OwnableKind,
} from '../../utils/ownership'; } from '../../utils/ownership';
import { completenessStore } from '../../store/completeness-store'; import { completenessStore } from '../../store/completeness-store';
@@ -103,20 +103,12 @@ export class TopResultsRow extends LitElement {
static override styles = [ static override styles = [
designTokens, designTokens,
exploreLinkStyles, exploreLinkStyles,
unownedStyles,
css` css`
:host { :host {
display: block; display: block;
margin-bottom: 16px; margin-bottom: 16px;
} }
.row {
display: flex;
gap: 12px;
overflow-x: auto;
padding-bottom: 4px;
}
.card { .card {
flex: 0 0 auto; flex: 0 0 auto;
width: 200px; width: 200px;
@@ -285,9 +277,9 @@ export class TopResultsRow extends LitElement {
return html` return html`
<div class="section-label">Top Results</div> <div class="section-label">Top Results</div>
<div class="row"> <scroll-row>
${this.results.map((r) => this.renderCard(r))} ${this.results.map((r) => this.renderCard(r))}
</div> </scroll-row>
`; `;
} }
+1
View File
@@ -31,6 +31,7 @@ solid/bookmark
solid/box-open solid/box-open
solid/check solid/check
solid/chevron-down solid/chevron-down
solid/chevron-left
solid/chevron-right solid/chevron-right
solid/circle-check solid/circle-check
solid/circle-exclamation solid/circle-exclamation
+184
View File
@@ -0,0 +1,184 @@
import { css } from 'lit';
/**
* The Explore album card, once.
*
* Two components draw one — `explore-view`'s shelves and search
* results, and `explore-artist-details`'s discography — and they had
* grown two copies of the same rules. That is how the size came apart:
* `explore-view` clamped its cards to a 130–150px range so two cards in
* one row could be different widths, and since the artwork is square
* that made them different *heights* as well. A row of covers with
* ragged bottoms is the whole complaint.
*
* So the width is a fixed `--yj-album-card-width` and the lines below
* the art each reserve their own space, which is what makes every card
* the same size no matter what a given album happens to carry —
* `album-card-size.test.ts` measures that rather than trusting it.
*
* Three rules here are the parts that changed rather than moved.
*
* **The artwork is inset in the square, not cropped to it.** The
* container was already `aspect-ratio: 1` but the image was
* `object-fit: cover`, so a non-square cover lost its edges. It is
* `contain` now and the container's own background is transparent, so
* a tall or wide cover sits in the middle of the square with the page
* showing through beside it.
*
* **The badge lives on the artwork, top-left, and only under the
* pointer.** It used to sit in the metadata line and only for the
* unowned case. It draws for every card now — an owned album's tick is
* the answer to the same question — and it is revealed by hover on a
* pointer device. Where there is no hover it is *always* visible rather
* than never, because on those devices it is the only route to its
* action: `explore-view`'s card menu carries no request item, so a
* phone with the badge hidden could not ask for an album at all.
*
* **Nothing dims an unowned card.** `unownedStyles` was removed from
* the catalog surfaces on the rule that the badge is the mark; the
* album page's *tracklist* still dims unowned rows, which is a
* different statement about a different thing.
*/
export const albumCardStyles = css`
.album-card {
width: var(--yj-album-card-width, 150px);
display: flex;
flex-direction: column;
gap: 6px;
padding: 8px;
border-radius: 8px;
box-sizing: border-box;
flex-shrink: 0;
cursor: pointer;
transition: background 0.15s ease;
}
.album-card:hover {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
}
.album-card:active {
transform: scale(0.97);
}
.album-card:focus-visible {
outline: 2px solid var(--yj-accent-text, #ffd43b);
outline-offset: -2px;
}
.album-art-container {
position: relative;
width: 100%;
aspect-ratio: 1;
border-radius: 4px;
overflow: hidden;
background: transparent;
display: flex;
align-items: center;
justify-content: center;
}
.album-art-container img {
width: 100%;
height: 100%;
object-fit: contain;
display: block;
}
/* The placeholder is the one case that *is* a full square, so it
carries the background the container gave up. */
.album-art-fallback {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
position: absolute;
inset: 0;
background: linear-gradient(
135deg,
var(--yj-bg-overlay, #404040) 0%,
var(--yj-bg-surface, #282828) 100%
);
}
.album-art-fallback wa-icon {
color: var(--yj-text-tertiary, #888);
font-size: 24px;
opacity: 0.5;
}
.album-card-badge {
position: absolute;
top: 6px;
left: 6px;
z-index: 1;
display: flex;
visibility: hidden;
opacity: 0;
transition: opacity 0.15s ease, visibility 0.15s ease;
}
@media (hover: hover) and (pointer: fine) {
.album-card:hover .album-card-badge,
.album-card:focus-within .album-card-badge {
visibility: visible;
opacity: 1;
}
}
@media not all and (hover: hover) {
.album-card-badge {
visibility: visible;
opacity: 1;
}
}
.album-title {
font-weight: 500;
color: var(--yj-text-primary, #fff);
font-size: var(--yj-text-sm);
line-height: 1.3;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
/* Reserved even where a surface has no artist to draw, so a card
in a row is never shorter than its neighbour. */
.album-artist {
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
line-height: 1.3;
min-height: 1.3em;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-meta {
display: flex;
align-items: center;
justify-content: space-between;
gap: 6px;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
height: 20px;
}
.album-meta-text {
display: flex;
align-items: center;
gap: 6px;
min-width: 0;
overflow: hidden;
}
.type-badge {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.08));
padding: 1px 6px;
border-radius: 3px;
font-size: 10px;
white-space: nowrap;
}
`;
+29
View File
@@ -0,0 +1,29 @@
/**
* Opening an external page, with the destination pinned.
*
* Every external link this app opens is a MusicBrainz entity page built
* from an MBID that came from the catalog. Constructing the URL by
* string concatenation leaves the destination to whatever is in that
* string, so this parses it against the one origin the app means and
* refuses anything else — an MBID cannot change the host, and if it
* somehow did, nothing would open.
*
* It navigates through a real anchor rather than `window.open`: the
* same top-level `_blank` navigation with `noopener`, and it keeps the
* destination an ordinary link rather than an argument to a function
* whose first parameter is a URL.
*/
const MUSICBRAINZ_ORIGIN = 'https://musicbrainz.org';
export function openMusicBrainz(path: string): void {
const url = new URL(path, MUSICBRAINZ_ORIGIN);
if (url.origin !== MUSICBRAINZ_ORIGIN) return;
const link = document.createElement('a');
link.href = url.toString();
link.target = '_blank';
link.rel = 'noopener noreferrer';
link.click();
}
+12 -1
View File
@@ -10,6 +10,16 @@
* badge as the only difference. This is that rule, written once, so * badge as the only difference. This is that rule, written once, so
* eight surfaces cannot each keep their own version of it. * eight surfaces cannot each keep their own version of it.
* *
* **The catalog's *cards* no longer dim.** A grid of dimmed covers read
* as a page that had failed to load rather than as a page of things you
* could ask for, so on Explore the mark is the badge alone — over the
* artwork, on hover, drawn for owned and unowned alike. The album
* page's *tracklist* still dims unowned rows: that is a different
* statement ("this one is not here") about a different thing, and the
* `aria-disabled` row that cannot be played is what it is for. So
* `unownedStyles` survives for that one surface and the cards simply do
* not include it.
*
* ## Ownership is a file, and `localId` is the flag that says so * ## Ownership is a file, and `localId` is the flag that says so
* *
* The album page answers "do I own this row" with `filePaths`, a map * The album page answers "do I own this row" with `filePaths`, a map
@@ -96,7 +106,8 @@ export function ownershipLabel(
} }
/** /**
* The dimming, shared so it cannot drift across surfaces. * The dimming, shared so it cannot drift across surfaces — and now
* used by exactly one of them.
* *
* Two things about it are load-bearing. * Two things about it are load-bearing.
* *
@@ -0,0 +1,174 @@
/**
* Every album card is the same size, and its artwork is a square.
*
* The size came apart because `explore-view` clamped its cards to a
* 130–150px range, so two cards in one row could be different widths —
* and since the artwork is square, different *heights* as well. A row
* of covers with ragged bottoms is what that looks like.
*
* What makes the fix hold is that the lines below the art each reserve
* their own space (`album-card.css.ts`), so an album with no year, no
* release type or a one-character title is not shorter than its
* neighbour. This measures that rather than trusting it, because the
* next component to format a card is the way it comes back.
*
* The artwork half is the other change: the container was already
* square but the image was `object-fit: cover`, so a non-square cover
* was cropped to it. It is `contain` now, and the container has no
* background of its own, so a tall cover is inset with the page
* showing through beside it.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import type { LitElement } from 'lit';
import '@components/explore-view/explore-view';
import { flush, stub, resetHarness } from '@test/support/harness';
import { fixture, shadow, shadowAll, update } from '@test/support/render';
import { completenessStore } from '@store/completeness-store';
const SEARCH = 'explore.Service.SearchLocal';
const SHELVES = 'explore.Service.GetExploreShelves';
/** A 1x1 transparent gif, so the `<img>` branch renders. */
const TINY_IMAGE =
'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
/** Release groups chosen so every optional line is present on one and
* absent on another — that is what a size regression hides behind. */
const ALBUMS = [
{
mbid: 'rg-1',
title: 'A',
artistCredit: '',
artistMbid: 'ar-1',
primaryType: '',
firstReleaseDate: '',
popularity: 1,
listenerCount: 1,
secondaryTypes: [],
inLibrary: false,
localId: 0,
},
{
mbid: 'rg-2',
title: 'A Very Long Album Name That Will Certainly Be Truncated By The Card',
artistCredit: 'An Artist With A Long Name',
artistMbid: 'ar-2',
primaryType: 'Album',
firstReleaseDate: '1994-05-01',
popularity: 1,
listenerCount: 1,
secondaryTypes: [],
inLibrary: false,
localId: 0,
},
{
mbid: 'rg-3',
title: 'Three',
artistCredit: 'Another',
artistMbid: 'ar-3',
primaryType: 'EP',
firstReleaseDate: '2001-01-01',
popularity: 1,
listenerCount: 1,
secondaryTypes: [],
inLibrary: false,
localId: 0,
},
];
async function exploreWithAlbums(): Promise<LitElement> {
stub(SHELVES, { shelves: [], state: 'ready' });
stub(SEARCH, {
artists: [],
releaseGroups: ALBUMS,
recordings: [],
});
stub('explore.Service.GetThumbnails', Object.fromEntries(
ALBUMS.map((a) => [a.mbid, TINY_IMAGE]),
));
stub('explore.Service.GetThumbnail', TINY_IMAGE);
const el = await fixture<LitElement>('explore-view');
(el as unknown as { onViewActivate: () => void }).onViewActivate?.();
await update(el, {
results: { artists: [], releaseGroups: ALBUMS, recordings: [] },
});
await flush();
await el.updateComplete;
return el;
}
beforeEach(() => {
resetHarness();
stub('library.Library.GetAlbumsCompleteness', {});
completenessStore.invalidate();
});
describe('the album card size', () => {
it('is the same width and height for every card in a row', async () => {
const el = await exploreWithAlbums();
const cards = shadowAll(el, '.album-card');
expect(cards.length).toBe(ALBUMS.length);
const boxes = cards.map((c) => c.getBoundingClientRect());
// The first card is the reference; every other one must match it.
for (const box of boxes) {
expect(box.width).toBe(boxes[0]!.width);
expect(box.height).toBe(boxes[0]!.height);
}
// …and the reference is a real box, or the loop above is vacuous.
expect(boxes[0]!.width).toBeGreaterThan(0);
expect(boxes[0]!.height).toBeGreaterThan(0);
});
it('keeps the artwork square', async () => {
const el = await exploreWithAlbums();
for (const art of shadowAll(el, '.album-art-container')) {
const box = art.getBoundingClientRect();
expect(Math.round(box.width)).toBe(Math.round(box.height));
}
});
it('insets a non-square cover rather than cropping it', async () => {
const el = await exploreWithAlbums();
// Read from the parsed stylesheet rather than from a rendered
// `<img>`: the search path is what calls `loadThumbnails`, and
// setting `results` directly skips it, so there is no image to
// measure. The regression worth catching is the rule going back to
// `cover`, which is a stylesheet fact.
const rules = (el.shadowRoot?.adoptedStyleSheets ?? []).flatMap((sheet) =>
Array.from(sheet.cssRules).map((rule) => rule.cssText),
);
const art = rules.find(
(text) =>
text.startsWith('.album-art-container img') &&
text.includes('object-fit'),
);
expect(art, 'no object-fit rule for the cover image').toBeDefined();
expect(art).toContain('object-fit: contain');
});
it('draws the badge over the artwork, and not in the metadata line', async () => {
const el = await exploreWithAlbums();
const card = shadow(el, '.album-card')!;
const badge = card.querySelector('.album-art-container .album-card-badge');
expect(badge).not.toBeNull();
// The badge is positioned inside the art box, so its parent is the
// square rather than the row underneath it.
expect(badge?.parentElement?.classList.contains('album-art-container')).toBe(
true,
);
});
});
@@ -0,0 +1,168 @@
/**
* The artist page's header and its top tracks.
*
* Two cleanups, asserted together because they are one screen:
*
* - the Play/Shuffle pair became one split button ("Play" with the
* words on its title, Shuffle behind the caret), the Follow button
* moved onto the same line, and the name and listen count went up a
* size;
* - a top track's play/request affordance moved onto its artwork,
* where a hover reveals it, instead of a badge at the end of the
* row beside a row that already plays on a double-click.
*/
import { describe, expect, it, beforeEach } from 'vitest';
import type { LitElement } from 'lit';
import '@components/explore-artist-details/explore-artist-details';
import { stub, flush, emit, resetHarness } from '@test/support/harness';
import { Events } from '../../src/events';
import { fixture, shadow, shadowAll } from '@test/support/render';
const ARTIST = 'artist-0001';
const track = (name: string, localId = 0) => ({
recordingMbid: `rec-${name}`,
artistName: 'Tideline',
trackName: name,
totalListenCount: 100,
caaReleaseMbid: '',
releaseName: 'Foreshore',
releaseGroupMbid: 'rg-owned',
length: 200000,
inLibrary: localId > 0,
localId,
});
beforeEach(() => {
resetHarness();
stub('explore.Service.LookupArtist', {
mbid: ARTIST,
name: 'Tideline',
popularity: 1200,
type: 'Group',
country: 'GB',
});
stub('explore.Service.TopReleaseGroupsForArtist', []);
stub('explore.Service.TopRecordingsForArtist', [
track('Owned Song', 7),
track('Absent Song'),
]);
stub('explore.Service.SimilarArtists', []);
stub('explore.Service.PrefetchReleases', undefined);
stub('explore.Service.BrowseReleaseGroups', [
{
mbid: 'rg-owned',
title: 'Foreshore',
artistCredit: 'Tideline',
primaryType: 'Album',
inLibrary: true,
localId: 7,
},
]);
stub('library.Library.GetAlbumsCompleteness', {});
stub('download.Service.ListRequests', []);
});
async function mount(): Promise<LitElement> {
const el = await fixture<LitElement>('explore-artist-details', {
artistMBID: ARTIST,
artistName: 'Tideline',
});
await flush();
return el;
}
describe('the artist header', () => {
it('offers Play, with Shuffle behind its caret', async () => {
const el = await mount();
const play = shadow<HTMLElement>(el, '[data-testid="artist-play-library"]')!;
// The words moved to the title, which is where "Play library
// tracks" can still be read without taking the width of a button.
expect(play.textContent?.trim()).toBe('Play');
expect(play.getAttribute('title')).toBe('Play library tracks');
const menuButton = shadow(el, '[data-testid="artist-play-menu"]');
expect(menuButton).not.toBeNull();
const menu = shadow(el, '#artist-play-menu');
expect(menu?.textContent).toContain('Shuffle');
});
it('puts Follow on the same line as Play', async () => {
const el = await mount();
const actions = shadow(el, '.artist-actions')!;
expect(actions.querySelector('[data-testid="artist-play-library"]')).not.toBeNull();
const follow = actions.querySelector('[data-testid="artist-follow"]') as HTMLElement;
expect(follow).not.toBeNull();
expect(follow.textContent?.trim()).toBe('Follow');
});
it('says Following once the artist is on the request list', async () => {
const el = await mount();
// The store is a singleton and caches its list, so the change is
// announced the way the backend announces one.
stub('download.Service.ListRequests', [
{ id: 3, mbid: ARTIST, state: 'queued' },
]);
emit(Events.RequestsChanged);
await flush();
await el.updateComplete;
const follow = shadow<HTMLElement>(el, '[data-testid="artist-follow"]')!;
expect(follow.textContent?.trim()).toBe('Following');
});
it('sizes the name and the listen count above the metadata line', async () => {
const el = await mount();
const title = shadow<HTMLElement>(el, '.artist-title')!;
const listens = shadow<HTMLElement>(el, '.artist-listens')!;
const meta = shadow<HTMLElement>(el, '.artist-meta')!;
expect(listens.textContent).toContain('plays on ListenBrainz');
const titleSize = parseFloat(getComputedStyle(title).fontSize);
const listensSize = parseFloat(getComputedStyle(listens).fontSize);
const metaSize = parseFloat(getComputedStyle(meta).fontSize);
expect(titleSize).toBeGreaterThan(24);
expect(listensSize).toBeGreaterThan(metaSize);
});
});
describe('a top track’s affordance', () => {
it('plays from the artwork when it is owned', async () => {
const el = await mount();
const rows = shadowAll<HTMLElement>(el, '.track-item');
const owned = rows.find((r) => r.textContent?.includes('Owned Song'))!;
expect(owned.querySelector('.track-art-overlay .track-art-play')).not.toBeNull();
// Nothing beside the row any more.
expect(owned.querySelector(':scope > library-status-indicator')).toBeNull();
});
it('requests from the artwork when it is not', async () => {
const el = await mount();
const rows = shadowAll<HTMLElement>(el, '.track-item');
const absent = rows.find((r) => r.textContent?.includes('Absent Song'))!;
expect(
absent.querySelector('.track-art-overlay library-status-indicator'),
).not.toBeNull();
expect(absent.querySelector('.track-art-overlay .track-art-play')).toBeNull();
});
});
@@ -25,7 +25,9 @@ const ARTIST = 'artist-0001';
/** The labels of the open menu's items, trimmed. */ /** The labels of the open menu's items, trimmed. */
function menuItems(el: LitElement): string[] { function menuItems(el: LitElement): string[] {
const panel = shadow(el, '.context-menu-panel'); // Scoped to the context menu: the artist page also has a Play/Shuffle
// dropdown, and its panel carries the same class.
const panel = shadow(el, '#context-menu .context-menu-panel');
if (!panel) return []; if (!panel) return [];
@@ -100,7 +102,7 @@ describe('the context menu on an artist page release', () => {
await openMenuOnAlbum(el, 0); await openMenuOnAlbum(el, 0);
const panel = shadow(el, '.context-menu-panel'); const panel = shadow(el, '#context-menu .context-menu-panel');
expect(panel).toBeTruthy(); expect(panel).toBeTruthy();
// The panel is shared with the track menu, so a label that does not // The panel is shared with the track menu, so a label that does not
+131
View File
@@ -0,0 +1,131 @@
/**
* A horizontally scrolling row can be moved without a wheel.
*
* Until this existed the only way to see the cards past the fold on the
* shelves, the search results and the artist page's discography was a
* mousewheel or a trackpad gesture — which is not an affordance. A
* mouse with no horizontal wheel simply could not reach them.
*
* What is asserted here is the state that makes the arrows honest: an
* arrow is `hidden` at the end it cannot move from, because a control
* that cannot act is worse than none, and an invisible one still holds
* a hit area and a tab stop.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import type { LitElement } from 'lit';
import '@components/scroll-row/scroll-row';
import { fixture } from '@test/support/render';
/** Six 100px cards in a 320px row — comfortably overflowing. */
function content(el: Element): void {
for (let i = 0; i < 6; i += 1) {
const card = document.createElement('div');
card.style.cssText = 'flex: 0 0 100px; height: 40px';
card.textContent = String(i);
el.append(card);
}
}
function arrows(el: LitElement): { prev: HTMLButtonElement; next: HTMLButtonElement } {
const root = el.shadowRoot!;
return {
prev: root.querySelector('.arrow.prev') as HTMLButtonElement,
next: root.querySelector('.arrow.next') as HTMLButtonElement,
};
}
function viewport(el: LitElement): HTMLElement {
return el.shadowRoot!.querySelector('.viewport') as HTMLElement;
}
async function row(): Promise<LitElement> {
const el = await fixture<LitElement>('scroll-row');
el.style.display = 'block';
el.style.width = '320px';
content(el);
await el.updateComplete;
// The observer reports on a later frame than a microtask drain.
await new Promise((r) => setTimeout(r, 60));
await el.updateComplete;
return el;
}
describe('<scroll-row>', () => {
beforeEach(() => {
document.body.style.margin = '0';
});
it('draws an arrow for each direction it can still move', async () => {
const el = await row();
const { prev, next } = arrows(el);
expect(prev).not.toBeNull();
expect(next).not.toBeNull();
// At the start there is nothing behind, so only the forward arrow is
// offered.
expect(prev.hasAttribute('hidden')).toBe(true);
expect(next.hasAttribute('hidden')).toBe(false);
});
it('offers the way back once the row has moved', async () => {
const el = await row();
const vp = viewport(el);
vp.scrollLeft = 120;
vp.dispatchEvent(new Event('scroll'));
await el.updateComplete;
expect(arrows(el).prev.hasAttribute('hidden')).toBe(false);
});
it('stands the forward arrow down at the end', async () => {
const el = await row();
const vp = viewport(el);
vp.scrollLeft = vp.scrollWidth;
vp.dispatchEvent(new Event('scroll'));
await el.updateComplete;
expect(arrows(el).next.hasAttribute('hidden')).toBe(true);
expect(arrows(el).prev.hasAttribute('hidden')).toBe(false);
});
it('moves the row when the arrow is pressed', async () => {
const el = await row();
const vp = viewport(el);
expect(vp.scrollLeft).toBe(0);
arrows(el).next.click();
await expect.poll(() => vp.scrollLeft).toBeGreaterThan(0);
});
it('shows nothing to scroll when the content fits', async () => {
const el = await fixture<LitElement>('scroll-row');
el.style.cssText = 'display: block; width: 320px';
const only = document.createElement('div');
only.style.cssText = 'flex: 0 0 100px; height: 40px';
only.textContent = 'one';
el.append(only);
await el.updateComplete;
await new Promise((r) => setTimeout(r, 60));
await el.updateComplete;
await new Promise((r) => requestAnimationFrame(() => r(null)));
await el.updateComplete;
const { prev, next } = arrows(el);
expect(prev.hasAttribute('hidden')).toBe(true);
expect(next.hasAttribute('hidden')).toBe(true);
});
});
@@ -1,24 +1,28 @@
/** /**
* Owned is plain; unowned is what gets marked. * The catalog's cards are not dimmed; the badge is the mark.
* *
* `explore-album-details` had this right for one tracklist and nothing * The rule this replaced had every unowned card dimmed *and* badged,
* else did: Explore's cards, the top-results row and the artist page's * which on a shelf of mostly-unowned covers read as a page that had
* three card shapes all mixed owned and unowned with a small badge as * failed to load rather than a page of things you could ask for. So the
* the only difference — and drew a green tick on the *common* case, * dimming is gone from the catalog surfaces and the badge carries the
* which is the treatment the album page's own green ticks were removed * whole statement — over the artwork, on hover, drawn for owned and
* for. * unowned alike.
* *
* What is pinned here is the rule rather than any one surface, because * What is still pinned here is the half that was never about dimming:
* the fault this replaced was eight call sites each holding their own
* version of it:
* *
* - an owned thing draws **no badge at all**;
* - an unowned one is dimmed *and* says so in its accessible name,
* because dimming is a colour and cannot be the only signal;
* - ownership is a **file** (`localId`), never the catalog's * - ownership is a **file** (`localId`), never the catalog's
* `inLibrary` ratchet, which is a flag that happens to agree; * `inLibrary` ratchet, which is a flag that happens to agree;
* - and a partly-held album says *how* partly, which is the one thing * - a row that cannot be played is `aria-disabled`, while a card that
* a tick cannot. * still navigates is not;
* - a partly-held album says *how* partly, which is the one thing a
* tick cannot;
* - and an unowned thing still says so in its accessible name, because
* with the dimming gone that name is the whole signal for anyone not
* seeing the badge.
*
* The album page's *tracklist* still dims unowned rows — a different
* statement about a different thing — and is covered by
* `album-request-badge-visibility.test.ts`.
*/ */
import { beforeEach, describe, expect, it } from 'vitest'; import { beforeEach, describe, expect, it } from 'vitest';
import { page } from 'vitest/browser'; import { page } from 'vitest/browser';
@@ -26,7 +30,7 @@ import { page } from 'vitest/browser';
import '@components/explore-view/explore-view'; import '@components/explore-view/explore-view';
import '@components/top-results-row/top-results-row'; import '@components/top-results-row/top-results-row';
import { flush, stub, resetHarness } from '@test/support/harness'; import { flush, stub, resetHarness } from '@test/support/harness';
import { fixture, shadow, shadowAll, update } from '@test/support/render'; import { fixture, shadow, update } from '@test/support/render';
import { completenessStore } from '@store/completeness-store'; import { completenessStore } from '@store/completeness-store';
const SEARCH = 'explore.Service.SearchLocal'; const SEARCH = 'explore.Service.SearchLocal';
@@ -105,27 +109,44 @@ beforeEach(() => {
// absent one — which is the point, or 87% of a grid re-asks forever. // absent one — which is the point, or 87% of a grid re-asks forever.
// Two tests in one file are two sessions as far as it is concerned, // Two tests in one file are two sessions as far as it is concerned,
// so a stale entry from the test above would otherwise decide the // so a stale entry from the test above would otherwise decide the
// one below. Found by writing the assertion the wrong way round. // one below.
completenessStore.invalidate(); completenessStore.invalidate();
}); });
describe('an owned thing is plain', () => { describe('an unowned card is marked by its badge alone', () => {
it('draws no badge on an album card it has files for', async () => { it('does not dim the artwork', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
const art = shadow(el, '.album-card .album-art-container')!;
// The dimming was an opacity on this box. With it gone the cover is
// at full strength, and the badge is what says the card is not
// yours.
expect(getComputedStyle(art).opacity).toBe('1');
expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull();
});
it('still says so in the name the browser computes', async () => {
await exploreShowing({ releaseGroups: [album('Absent', {})] });
await expect
.element(page.getByRole('button', { name: /Absent — not in your library/ }))
.toBeInTheDocument();
});
});
describe('an owned card is plain except for its badge', () => {
it('draws the in-library badge rather than nothing', async () => {
const el = await exploreShowing({ const el = await exploreShowing({
releaseGroups: [album('Held', { localId: 7 })], releaseGroups: [album('Held', { localId: 7 })],
}); });
expect(shadowAll(el, '.album-card')).toHaveLength(1); const badge = shadow(el, '.album-card library-status-indicator');
expect(shadow(el, '.album-card library-status-indicator')).toBeNull();
});
it('draws no badge on a track row it has a file for', async () => { expect(badge).not.toBeNull();
const el = await exploreShowing({ expect(badge?.getAttribute('status')).toBe('in-library');
recordings: [recording('Held', { localId: 9 })],
});
expect(shadowAll(el, '.track-item')).toHaveLength(1);
expect(shadow(el, '.track-item library-status-indicator')).toBeNull();
}); });
it('does not dim it', async () => { it('does not dim it', async () => {
@@ -139,56 +160,6 @@ describe('an owned thing is plain', () => {
}); });
}); });
describe('an unowned thing is marked', () => {
it('dims the card and keeps its request badge', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true);
expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull();
});
/**
* The name is the half of this that reaches anyone not seeing the
* dimming, so it has to be the browser's own answer — a shadow-root
* query cannot compute a name, and this repo has shipped a nameless
* control three times.
*/
it('says so in the name the browser computes', async () => {
await exploreShowing({ releaseGroups: [album('Absent', {})] });
await expect
.element(page.getByRole('button', { name: /Absent — not in your library/ }))
.toBeInTheDocument();
});
/**
* A track row is `aria-disabled` and a card is not, and the
* difference is not cosmetic: activating an unowned row does nothing
* (`onRecordingRowDblClick` returns early), while a card navigates to
* the catalog page for it, which is a perfectly good thing to do with
* something you do not own.
*/
it('marks a row that cannot be played as disabled', async () => {
const el = await exploreShowing({
recordings: [recording('Absent', {})],
});
expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe(
'true',
);
});
it('leaves a card that still navigates enabled', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
expect(shadow(el, '.album-card')?.getAttribute('aria-disabled')).toBeNull();
});
});
/** /**
* The decision this issue turned on. * The decision this issue turned on.
* *
@@ -206,7 +177,9 @@ describe('ownership is a file, not a flag', () => {
}); });
expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true); expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true);
expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull(); expect(
shadow(el, '.album-card library-status-indicator')?.getAttribute('status'),
).not.toBe('in-library');
}); });
it('does the same for a track row', async () => { it('does the same for a track row', async () => {
@@ -220,6 +193,26 @@ describe('ownership is a file, not a flag', () => {
}); });
}); });
describe('a track row that cannot be played is disabled', () => {
it('marks an unowned row', async () => {
const el = await exploreShowing({
recordings: [recording('Absent', {})],
});
expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe(
'true',
);
});
it('leaves a card that still navigates enabled', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
expect(shadow(el, '.album-card')?.getAttribute('aria-disabled')).toBeNull();
});
});
/** /**
* The count, which is what `#16`'s deferred third step asked for: an * The count, which is what `#16`'s deferred third step asked for: an
* album held 2 tracks of 10 wore the same green tick as one held whole, * album held 2 tracks of 10 wore the same green tick as one held whole,
@@ -248,9 +241,13 @@ describe('a partly-held album says how partly', () => {
// A partly-held album is *actionable* — it has three tracks left to // A partly-held album is *actionable* — it has three tracks left to
// ask for — so the badge is a button, and the name has to carry the // ask for — so the badge is a button, and the name has to carry the
// action and the count. Naming it after the action alone left the // action and the count. The badge is revealed by the card's focus
// one state the ring exists for as the one state whose name did not // (`:focus-within`), and `visibility: hidden` is what takes it out
// mention it. // of the accessibility tree until then, so the card is focused
// first — which is exactly the route a keyboard user takes.
shadow<HTMLElement>(el, '.album-card')?.focus();
await el.updateComplete;
await expect await expect
.element( .element(
page.getByRole('button', { page.getByRole('button', {
@@ -266,7 +263,7 @@ describe('a partly-held album says how partly', () => {
* state, and a ring drawn from its absence would mark all of it * state, and a ring drawn from its absence would mark all of it
* incomplete on no evidence. That is the rule `Known` exists for. * incomplete on no evidence. That is the rule `Known` exists for.
*/ */
it('says nothing when the total was never declared', async () => { it('falls back to the plain in-library badge when the total was never declared', async () => {
stub(COMPLETENESS, { stub(COMPLETENESS, {
'7': { owned: 3, expected: 0, known: false, complete: false }, '7': { owned: 3, expected: 0, known: false, complete: false },
}); });
@@ -279,7 +276,9 @@ describe('a partly-held album says how partly', () => {
await flush(); await flush();
await el.updateComplete; await el.updateComplete;
expect(shadow(el, '.album-card library-status-indicator')).toBeNull(); expect(
shadow(el, '.album-card library-status-indicator')?.getAttribute('status'),
).toBe('in-library');
}); });
it('asks about the owned albums only, in one call', async () => { it('asks about the owned albums only, in one call', async () => {
@@ -329,11 +328,13 @@ describe('the top-results row follows the same rule', () => {
query: 'held', query: 'held',
}); });
// A top-result card is a mixed bag — artist, album or track — and
// its badge is a corner mark rather than the cover overlay the
// album cards grew, so an owned one stays plain.
expect(shadow(el, '.card library-status-indicator')).toBeNull(); expect(shadow(el, '.card library-status-indicator')).toBeNull();
expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(false);
}); });
it('dims and names something it does not', async () => { it('names something it does not own', async () => {
const el = await fixture('top-results-row', { const el = await fixture('top-results-row', {
results: [result('Absent', 'release_group')], results: [result('Absent', 'release_group')],
query: 'absent', query: 'absent',
@@ -349,7 +350,7 @@ describe('the top-results row follows the same rule', () => {
/** /**
* An artist card has never had a badge — a discography subscription * An artist card has never had a badge — a discography subscription
* is the artist page's Follow button, which can say what it commits * is the artist page's Follow button, which can say what it commits
* to — so the dimming and the name are the whole signal there. * to — so the name is the whole signal there.
*/ */
it('marks an unowned artist without offering a request', async () => { it('marks an unowned artist without offering a request', async () => {
const el = await fixture('top-results-row', { const el = await fixture('top-results-row', {
+2 -2
View File
@@ -1,6 +1,6 @@
module yellowjacket module yellowjacket
go 1.25.0 go 1.26
require ( require (
github.com/BurntSushi/toml v1.6.0 github.com/BurntSushi/toml v1.6.0
@@ -144,7 +144,7 @@ require (
github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect
github.com/go-git/go-billy/v5 v5.9.0 // indirect github.com/go-git/go-billy/v5 v5.9.0 // indirect
github.com/go-git/go-git/v5 v5.19.2 // indirect github.com/go-git/go-git/v5 v5.19.2 // indirect
github.com/go-json-experiment/json v0.0.0-20251027170946-4849db3c2f7e // indirect github.com/go-json-experiment/json v0.0.0-20260820222146-c27c302e5fc3 // indirect
github.com/go-ole/go-ole v1.3.0 // indirect github.com/go-ole/go-ole v1.3.0 // indirect
github.com/go-resty/resty/v2 v2.17.1 // indirect github.com/go-resty/resty/v2 v2.17.1 // indirect
github.com/go-sql-driver/mysql v1.9.3 // indirect github.com/go-sql-driver/mysql v1.9.3 // indirect
+2 -2
View File
@@ -362,8 +362,8 @@ github.com/go-git/go-git/v5 v5.19.2/go.mod h1:QqCBE1EFN5ddFmrliLQ3/ntRCUjZU3EJuw
github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU=
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
github.com/go-json-experiment/json v0.0.0-20251027170946-4849db3c2f7e h1:Lf/gRkoycfOBPa42vU2bbgPurFong6zXeFtPoxholzU= github.com/go-json-experiment/json v0.0.0-20260820222146-c27c302e5fc3 h1:UADEEmDKgfXbtnGJZ97beY5XLo9ZechG1nlU4KnRrkE=
github.com/go-json-experiment/json v0.0.0-20251027170946-4849db3c2f7e/go.mod h1:uNVvRXArCGbZ508SxYYTC5v1JWoz2voff5pm25jU1Ok= github.com/go-json-experiment/json v0.0.0-20260820222146-c27c302e5fc3/go.mod h1:tphK2c80bpPhMOI4v6bIc2xWywPfbqi1Z06+RcrMkDg=
github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as=
github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as=
github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY=
+1 -1
View File
@@ -18,7 +18,7 @@ arch=('x86_64')
url="https://git.ljones.me/yonlu/yellowjacket" url="https://git.ljones.me/yonlu/yellowjacket"
license=('custom') license=('custom')
depends=('webkitgtk-6.0' 'gtk4' 'alsa-lib' 'hicolor-icon-theme') depends=('webkitgtk-6.0' 'gtk4' 'alsa-lib' 'hicolor-icon-theme')
makedepends=('go>=1.25' 'nodejs>=22' 'pnpm' 'git') makedepends=('go>=1.26' 'nodejs>=22' 'pnpm' 'git')
options=('!lto') options=('!lto')
# Source is overridable so the same PKGBUILD works two ways: # Source is overridable so the same PKGBUILD works two ways: