diff --git a/backend/database/sql/queries/download.sql b/backend/database/sql/queries/download.sql index 22df883..2cb4405 100644 --- a/backend/database/sql/queries/download.sql +++ b/backend/database/sql/queries/download.sql @@ -176,6 +176,16 @@ WHERE state = 'wanted' ORDER BY attempts, created_at LIMIT ?; +-- name: ListWantedDownloadRequests :many +-- The same set ignoring the backoff, for a pass the user asked for by +-- hand: "check now" that respected a six-hour retry schedule looked +-- like a button that did nothing. +SELECT * FROM download_requests +WHERE state = 'wanted' + AND entity <> 'artist' +ORDER BY attempts, created_at +LIMIT ?; + -- name: ListChildDownloadRequests :many SELECT * FROM download_requests WHERE parent_id = ? ORDER BY id; diff --git a/backend/database/sql/sqlcgen/download.sql.go b/backend/database/sql/sqlcgen/download.sql.go index 2601074..bb9da3a 100644 --- a/backend/database/sql/sqlcgen/download.sql.go +++ b/backend/database/sql/sqlcgen/download.sql.go @@ -739,6 +739,58 @@ func (q *Queries) ListLiveDownloads(ctx context.Context) ([]DownloadDownload, er return items, nil } +const listWantedDownloadRequests = `-- name: ListWantedDownloadRequests :many +SELECT id, mbid, entity, library_id, artist, title, scope, secondary, state, parent_id, attempts, last_error, last_tried_at, next_try_at, external_ids, created_at, updated_at FROM download_requests +WHERE state = 'wanted' + AND entity <> 'artist' +ORDER BY attempts, created_at +LIMIT ? +` + +// The same set ignoring the backoff, for a pass the user asked for by +// hand: "check now" that respected a six-hour retry schedule looked +// like a button that did nothing. +func (q *Queries) ListWantedDownloadRequests(ctx context.Context, limit int64) ([]DownloadRequest, error) { + rows, err := q.db.QueryContext(ctx, listWantedDownloadRequests, limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []DownloadRequest + for rows.Next() { + var i DownloadRequest + if err := rows.Scan( + &i.ID, + &i.Mbid, + &i.Entity, + &i.LibraryID, + &i.Artist, + &i.Title, + &i.Scope, + &i.Secondary, + &i.State, + &i.ParentID, + &i.Attempts, + &i.LastError, + &i.LastTriedAt, + &i.NextTryAt, + &i.ExternalIds, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const recordDownloadRequestAttempt = `-- name: RecordDownloadRequestAttempt :exec UPDATE download_requests SET attempts = attempts + 1, diff --git a/backend/download/reconcile.go b/backend/download/reconcile.go index 79c2b9a..1e209c3 100644 --- a/backend/download/reconcile.go +++ b/backend/download/reconcile.go @@ -251,6 +251,16 @@ type Summary struct { // Synced is how many requests were pushed to an external list. Synced int `json:"synced"` + + // Waiting is how many requests are on the list and still being + // looked for. A pass that did nothing is the normal case, and the + // UI can only say so honestly if it knows the list was not empty. + Waiting int `json:"waiting"` + + // NoProviders reports that nothing could be searched because no + // download client is enabled — the one "nothing happened" the user + // can actually fix. + NoProviders bool `json:"noProviders"` } // changed reports whether the pass altered anything worth refreshing @@ -259,9 +269,22 @@ func (s Summary) changed() bool { return s.Expanded > 0 || s.Satisfied > 0 || s.Started > 0 } -// RunOnce works the request list once. It is safe to call directly, and -// the "search now" button does. +// RunOnce works the request list once, honouring each request's +// backoff. This is what the loop calls. func (r *Reconciler) RunOnce(ctx context.Context) (Summary, error) { + return r.run(ctx, false) +} + +// RunNow works the request list ignoring backoff. This is what the +// "check now" button calls: a scheduled retry is a promise to the +// provider, not to the user, and a person who presses a button expects +// their list to actually be searched rather than to be told it is not +// due yet. +func (r *Reconciler) RunNow(ctx context.Context) (Summary, error) { + return r.run(ctx, true) +} + +func (r *Reconciler) run(ctx context.Context, force bool) (Summary, error) { r.runMu.Lock() defer r.runMu.Unlock() @@ -287,13 +310,15 @@ func (r *Reconciler) RunOnce(ctx context.Context) (Summary, error) { summary.Synced = r.syncExternalLists(ctx) - attempted, started, err := r.attemptDue(ctx) + attempted, started, err := r.attemptDue(ctx, force) if err != nil { return summary, err } summary.Attempted = attempted summary.Started = started + summary.Waiting = r.countWaiting(ctx) + summary.NoProviders = len(r.manager.enabledProviders()) == 0 r.logger.Info( "reconciled request list", @@ -506,10 +531,19 @@ func (r *Reconciler) retireOwned(ctx context.Context) (int, error) { // Attempting downloads // --------------------------------------------------------------------------- -// attemptDue searches for a bounded batch of due requests and grabs the -// ones with a clear winner. -func (r *Reconciler) attemptDue(ctx context.Context) (attempted, started int, err error) { - due, err := r.store.ListDueRequests(ctx, r.batch) +// attemptDue searches for a bounded batch of requests and grabs the +// ones with a clear winner. force takes requests whose backoff has not +// elapsed as well. +func (r *Reconciler) attemptDue( + ctx context.Context, + force bool, +) (attempted, started int, err error) { + list := r.store.ListDueRequests + if force { + list = r.store.ListWantedRequests + } + + due, err := list(ctx, r.batch) if err != nil { return 0, 0, err } @@ -615,6 +649,26 @@ func (r *Reconciler) attempt(ctx context.Context, req Request) (bool, string) { return started, reason } +// countWaiting reports how many non-artist requests are still being +// looked for, so "nothing happened" can be reported as "nothing new +// for the twelve things on your list" rather than as silence. +func (r *Reconciler) countWaiting(ctx context.Context) int { + requests, err := r.store.ListRequests(ctx) + if err != nil { + return 0 + } + + waiting := 0 + + for _, req := range requests { + if req.State == RequestStateWanted && !req.Entity.Expands() { + waiting++ + } + } + + return waiting +} + // --------------------------------------------------------------------------- // External list sync // --------------------------------------------------------------------------- diff --git a/backend/download/reconcile_test.go b/backend/download/reconcile_test.go index e87c22a..23eef8c 100644 --- a/backend/download/reconcile_test.go +++ b/backend/download/reconcile_test.go @@ -495,3 +495,101 @@ func waitFor(t *testing.T, cond func() bool, msg string) { t.Fatal(msg) } + +// "Check now" is the user overriding the retry schedule, so it must +// search a request whose backoff has not elapsed. The scheduled pass +// must not: the backoff exists to keep a fruitless search off the +// providers, and a loop that ignored it would hammer them. +func TestRunNowIgnoresBackoffAndRunOnceDoesNot(t *testing.T) { + t.Parallel() + + f := newReconcileFixture(t) + ctx := context.Background() + + provider := NewFakeProvider(1, "weak", Caps{CanSearch: true, CanTransport: true}) + provider.Candidates = []Candidate{candidateFor( + "weak-1", []string{"Something Else Entirely"}, ".mp3", 3_000_000, + )} + + f.manager.installProvider(Config{ID: 1, Priority: 50}, provider) + + id, err := f.store.AddRequest(ctx, Request{ + MBID: "rg-1", + Entity: EntityReleaseGroup, + LibraryID: 1, + Artist: "Radiohead", + Title: "OK Computer", + }) + if err != nil { + t.Fatalf("AddRequest: %v", err) + } + + f.catalog.tracklists["rg-1"] = fourTrackDownload().Expected + + // First pass: attempted, found nothing, backoff armed. + if _, err := f.reconciler.RunOnce(ctx); err != nil { + t.Fatalf("RunOnce: %v", err) + } + + scheduled, err := f.reconciler.RunOnce(ctx) + if err != nil { + t.Fatalf("RunOnce (second): %v", err) + } + + if scheduled.Attempted != 0 { + t.Errorf("scheduled pass attempted %d, want 0 while backed off", + scheduled.Attempted) + } + + forced, err := f.reconciler.RunNow(ctx) + if err != nil { + t.Fatalf("RunNow: %v", err) + } + + if forced.Attempted != 1 { + t.Errorf("forced pass attempted %d, want 1", forced.Attempted) + } + + if forced.Waiting != 1 { + t.Errorf("summary reported %d waiting, want 1 so the UI can say "+ + "what was searched", forced.Waiting) + } + + req, err := f.store.GetRequest(ctx, id) + if err != nil { + t.Fatalf("GetRequest: %v", err) + } + + if req.Attempts != 2 { + t.Errorf("attempts = %d, want 2 after a forced re-check", req.Attempts) + } +} + +// A pass with no providers says so, because "nothing happened" with no +// reason is the one outcome the user cannot act on. +func TestSummaryReportsNoProviders(t *testing.T) { + t.Parallel() + + f := newReconcileFixture(t) + ctx := context.Background() + + if _, err := f.store.AddRequest(ctx, Request{ + MBID: "rg-1", + Entity: EntityReleaseGroup, + LibraryID: 1, + Title: "OK Computer", + }); err != nil { + t.Fatalf("AddRequest: %v", err) + } + + f.catalog.tracklists["rg-1"] = fourTrackDownload().Expected + + summary, err := f.reconciler.RunNow(ctx) + if err != nil { + t.Fatalf("RunNow: %v", err) + } + + if !summary.NoProviders { + t.Error("summary did not report that no download client is enabled") + } +} diff --git a/backend/download/requeststore.go b/backend/download/requeststore.go index a61ad93..6763e92 100644 --- a/backend/download/requeststore.go +++ b/backend/download/requeststore.go @@ -152,6 +152,23 @@ func (s *Store) ListDueRequests(ctx context.Context, limit int) ([]Request, erro return requestRowsToRequests(rows), nil } +// ListWantedRequests returns downloadable requests regardless of their +// backoff, least-attempted first. Only a user-initiated pass uses +// this: the loop honours the schedule, a person pressing "check now" +// is the schedule. +func (s *Store) ListWantedRequests(ctx context.Context, limit int) ([]Request, error) { + if limit <= 0 { + limit = defaultDueBatch + } + + rows, err := s.db.ReadQueries.ListWantedDownloadRequests(ctx, int64(limit)) + if err != nil { + return nil, fmt.Errorf("list wanted download requests: %w", err) + } + + return requestRowsToRequests(rows), nil +} + // ListChildRequests returns the requests an artist subscription // produced. func (s *Store) ListChildRequests( diff --git a/backend/download/service.go b/backend/download/service.go index db2e04a..a350be8 100644 --- a/backend/download/service.go +++ b/backend/download/service.go @@ -609,7 +609,7 @@ func (s *Service) ReconcileRequests() (Summary, error) { ) } - summary, err := s.reconciler.RunOnce(context.Background()) + summary, err := s.reconciler.RunNow(context.Background()) if err != nil { return summary, err } diff --git a/frontend/src/components/downloads-view/downloads-view.ts b/frontend/src/components/downloads-view/downloads-view.ts index 971392d..7cfd315 100644 --- a/frontend/src/components/downloads-view/downloads-view.ts +++ b/frontend/src/components/downloads-view/downloads-view.ts @@ -33,6 +33,14 @@ export class DownloadsView extends LitElement { @state() private lastSummary: RequestSummary | null = null; + /** True when at least one download client is enabled. */ + @state() private canDownload = false; + + /** Ticks so "next check in …" ages while the page is open. */ + @state() private nowMs = Date.now(); + + private clockTimer?: ReturnType; + private unsubscribe: (() => void) | null = null; static override styles = [ @@ -167,6 +175,24 @@ export class DownloadsView extends LitElement { color: var(--yj-text-secondary, #b3b3b3); margin: 8px 0 0; } + + .notice { + display: flex; + align-items: center; + gap: 8px; + padding: 10px 12px; + margin-bottom: 12px; + border-radius: 6px; + font-size: 12px; + background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06)); + color: var(--yj-text-secondary, #b3b3b3); + } + + .section-hint { + margin: 0 0 8px; + font-size: 12px; + color: var(--yj-text-tertiary, #888); + } `, ]; @@ -176,12 +202,20 @@ export class DownloadsView extends LitElement { this.unsubscribe = downloadStore.subscribe(() => { this.requests = downloadStore.requests; this.downloads = downloadStore.downloads; + this.canDownload = downloadStore.available; }); void downloadStore.init().then(() => { this.requests = downloadStore.requests; this.downloads = downloadStore.downloads; + this.canDownload = downloadStore.available; }); + + // A "next check" that never moves reads as a stuck page, so the + // relative times re-render on their own. + this.clockTimer = setInterval(() => { + this.nowMs = Date.now(); + }, 30_000); } override disconnectedCallback(): void { @@ -189,6 +223,7 @@ export class DownloadsView extends LitElement { this.unsubscribe?.(); this.unsubscribe = null; + clearInterval(this.clockTimer); } override render() { @@ -201,10 +236,11 @@ export class DownloadsView extends LitElement { size="small" appearance="outlined" ?disabled=${this.checking} + title="Search every download client for everything on this list right now, instead of waiting for the next scheduled check" @click=${() => void this.checkNow()} > - ${this.checking ? 'Checking…' : 'Check now'} + ${this.checking ? 'Searching…' : 'Check now'} ` : nothing} @@ -213,7 +249,10 @@ export class DownloadsView extends LitElement {

Music you have requested, and the download attempts that have run for it. A request that cannot be found today stays - on the list and is looked for again later. + on the list and is looked for again later — roughly every + six hours at first, then less often the longer it goes + unfound. “Check now” skips that wait and searches + everything on the list immediately.

@@ -248,6 +287,7 @@ export class DownloadsView extends LitElement { const satisfied = this.requests.filter((r) => r.state === 'satisfied'); return html` + ${this.renderProviderNotice()} ${this.renderSummary()} ${satisfied.length > 0 ? html` @@ -269,7 +309,18 @@ export class DownloadsView extends LitElement { subscriptions, (r) => this.renderSubscription(r), )} - ${this.renderRequestSection('Looking for', wanted, (r) => this.renderRequest(r))} + ${wanted.length > 0 + ? html` +

Looking for

+

+ Requested, not found yet. Nothing is wrong — each + of these is searched again on the schedule below, + and moves to “Found” the moment it lands in your + library, however it got there. +

+ ${wanted.map((r) => this.renderRequest(r))} + ` + : nothing} ${this.renderRequestSection('Paused', paused, (r) => this.renderRequest(r))} ${this.renderRequestSection('Found', satisfied, (r) => this.renderRequest(r))} `; @@ -284,6 +335,26 @@ export class DownloadsView extends LitElement { `; } + /** + * A request list with no download client behind it is a list that + * can never move, and that is the single most likely reason “check + * now” appears to do nothing. Say so where the button is. + */ + private renderProviderNotice() { + if (this.canDownload) return nothing; + + return html` +
+ + + No download client is enabled, so nothing on this list + can be searched for. Requests are still kept — add a + client under Settings → Downloads and they start moving. + +
+ `; + } + private renderSummary() { if (!this.lastSummary) return nothing; @@ -293,14 +364,23 @@ export class DownloadsView extends LitElement { s.expanded > 0 ? `${s.expanded} new album${s.expanded === 1 ? '' : 's'} found` : '', s.satisfied > 0 ? `${s.satisfied} already owned` : '', s.started > 0 ? `${s.started} downloading` : '', - s.attempted > 0 ? `${s.attempted} searched for` : '', + s.attempted > 0 + ? `${s.attempted} searched, no clear match yet` + : '', ].filter(Boolean); - return html` -

- ${parts.length > 0 ? parts.join(' · ') : 'Nothing new this time.'} -

- `; + if (parts.length > 0) { + return html`

${parts.join(' · ')}

`; + } + + // "Nothing happened" needs a reason, or the button looks broken. + const idle = s.noProviders + ? 'Nothing was searched: no download client is enabled.' + : s.waiting > 0 + ? `Searched all ${s.waiting} request${s.waiting === 1 ? '' : 's'} — no source has anything new yet.` + : 'Nothing on the list to search for.'; + + return html`

${idle}

`; } private renderRequestSection( @@ -361,7 +441,7 @@ export class DownloadsView extends LitElement { ${request.artist ? `${request.artist} — ` : ''}${request.title || request.mbid}
-
${requestDetail(request)}
+
${requestDetail(request, this.nowMs)}
${request.state === 'satisfied' @@ -495,15 +575,54 @@ export class DownloadsView extends LitElement { * looked for rather than as an error, because that is what it is — the * retry is already scheduled and there is nothing for the user to do. */ -function requestDetail(request: Request): string { +function requestDetail(request: Request, nowMs: number): string { if (request.state === 'satisfied') return 'In your library'; - if (request.state === 'paused') return 'Paused'; + if (request.state === 'paused') return 'Paused — not being looked for'; - if (request.attempts === 0) return 'Not looked for yet'; + if (request.attempts === 0) return 'Queued — not searched for yet'; - const reason = request.lastError ? ` — ${request.lastError}` : ''; + const tries = `Searched ${request.attempts} time${request.attempts === 1 ? '' : 's'}`; + const reason = request.lastError ? `, ${request.lastError}` : ''; + // Wails types a Go time.Time as an opaque class; over the wire it + // is the RFC 3339 string JSON marshalled it as. + const next = nextCheckPhrase( + request.nextTryAt as unknown as string | undefined, + nowMs, + ); - return `Looked for ${request.attempts} time${request.attempts === 1 ? '' : 's'}${reason}`; + return `${tries}${reason}${next}`; +} + +/** + * "Next check" as a phrase, because the retry schedule is the part of + * this feature nothing in the UI used to admit existed — a row that + * says only "searched 3 times" gives the user no way to tell a waiting + * request from an abandoned one. + */ +function nextCheckPhrase(nextTryAt: string | undefined, nowMs: number): string { + if (!nextTryAt) return ''; + + const due = new Date(nextTryAt).getTime(); + if (Number.isNaN(due)) return ''; + + const deltaMs = due - nowMs; + if (deltaMs <= 0) return ' · due for another search'; + + return ` · next check ${relativeFuture(deltaMs)}`; +} + +/** Coarse "in 3 hours" phrasing; minutes are noise on a 6-hour cycle. */ +function relativeFuture(ms: number): string { + const minutes = Math.round(ms / 60_000); + + if (minutes < 60) return `in ${Math.max(1, minutes)} min`; + + const hours = Math.round(minutes / 60); + if (hours < 48) return `in ${hours} hour${hours === 1 ? '' : 's'}`; + + const days = Math.round(hours / 24); + + return `in ${days} day${days === 1 ? '' : 's'}`; } declare global { diff --git a/frontend/wailsjs/go/models.ts b/frontend/wailsjs/go/models.ts index 9d35b18..163dd59 100755 --- a/frontend/wailsjs/go/models.ts +++ b/frontend/wailsjs/go/models.ts @@ -868,6 +868,8 @@ export namespace download { attempted: number; started: number; synced: number; + waiting: number; + noProviders: boolean; static createFrom(source: any = {}) { return new Summary(source); @@ -880,6 +882,8 @@ export namespace download { this.attempted = source["attempted"]; this.started = source["started"]; this.synced = source["synced"]; + this.waiting = source["waiting"]; + this.noProviders = source["noProviders"]; } }