diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index 5ba4b56..6359806 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -24,6 +24,8 @@ import { EventsOn } from '@runtime/runtime'; import { Events } from '../../events'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '../library-status-indicator/library-status-indicator.js'; +import { libraryStatusFor } from '@utils/library-status'; +import type { LibraryStatus } from '../library-status-indicator/library-status-indicator'; import '../catalog-scope-notice/catalog-scope-notice.js'; import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js'; import '@awesome.me/webawesome/dist/components/button/button.js'; @@ -1443,7 +1445,7 @@ export class ExploreAlbumDetails extends LitElement { * - cachedAlbums has MBID match → owned * - any selected version has → owned (covers local-only albums * a track marked inLibrary where releaseGroup may be null) - * - else → not owned + * - else → whatever the request list says * * Four different claims of decreasing confidence, OR'd together and * reported as one tick — the last of which fires when a *single* @@ -1452,10 +1454,11 @@ export class ExploreAlbumDetails extends LitElement { * actions key off; this stays as it was, because the indicator's * job is "is any of this yours" and that is what it answers. * - * No queued state for now — that's reserved for future - * download-client integration. + * When none of them hold the answer is not automatically "no": + * the album may be on the request list, which the button directly + * below this badge has reported as "Wanted" all along. */ - private albumLibraryStatus(): 'in-library' | 'not-in-library' { + private albumLibraryStatus(): LibraryStatus { if (this.localAlbumId > 0) return 'in-library'; if (this.releaseGroup?.inLibrary) return 'in-library'; @@ -1477,7 +1480,11 @@ export class ExploreAlbumDetails extends LitElement { } } - return 'not-in-library'; + // None of the five ownership claims held, so the badge falls + // through to the one thing this page already knew and never + // said: whether the album is on the request list. The button + // below it has read "Wanted" all along. + return libraryStatusFor(false, this.releaseGroupMBID); } /** @@ -2280,7 +2287,7 @@ export class ExploreAlbumDetails extends LitElement { )} diff --git a/frontend/src/components/explore-artist-details/explore-artist-details.ts b/frontend/src/components/explore-artist-details/explore-artist-details.ts index 877f5e6..5b3ada4 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -34,6 +34,7 @@ import { EventsOn } from '@runtime/runtime'; import { Events } from '../../events'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '../library-status-indicator/library-status-indicator.js'; +import { libraryStatusFor } from '@utils/library-status'; import '../catalog-scope-notice/catalog-scope-notice.js'; import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js'; @@ -2113,7 +2114,7 @@ export class ExploreArtistDetails extends LitElement { ${formatListenCount(t.totalListenCount)} plays @@ -2225,7 +2226,7 @@ export class ExploreArtistDetails extends LitElement { ${rg.date ? html`${extractYear(rg.date)}` : nothing} { if (this.shelves?.state !== 'ready') void this.loadShelves(); }); + + // The badges on every result card say whether something is + // already requested, which a background reconcile pass changes + // without this page doing anything. Registered `whileActive` + // rather than on connect: this view is cached and never + // unmounts, so a connect-time subscription would run for the + // life of the session from pages it is not on. + // + // `init()` is four fetches, and it happens on arrival for the + // same reason `loadShelves()` does — a user who never opens + // Explore should not pay for it. + this.whileActive(downloadStore.subscribe(() => this.requestUpdate())); + void downloadStore.init().then(() => this.requestUpdate()); } /** A debounced search that lands after the user has left the page is @@ -1836,7 +1851,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) { ${year ? html`${year}` : nothing} @@ -1874,7 +1889,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) { : nothing} diff --git a/frontend/src/components/top-results-row/top-results-row.ts b/frontend/src/components/top-results-row/top-results-row.ts index a43825e..3d97b10 100644 --- a/frontend/src/components/top-results-row/top-results-row.ts +++ b/frontend/src/components/top-results-row/top-results-row.ts @@ -10,6 +10,8 @@ import { import '../library-status-indicator/library-status-indicator.js'; import type { LibraryStatus } from '../library-status-indicator/library-status-indicator.js'; import { artistLink, exploreLinkStyles } from '../../utils/explore-link'; +import { libraryStatusFor } from '../../utils/library-status'; +import { downloadStore } from '../../store/download-store'; /** Format milliseconds as m:ss. */ function formatDuration(ms: number | undefined): string { @@ -50,6 +52,28 @@ export class TopResultsRow extends LitElement { // Per-card state: cover images. private images = new Map(); + private unsubRequests?: () => void; + + /** + * The badges here say whether something is already requested, and + * this row will not hear about a change from its host: `explore-view` + * re-rendering sets the same `results` array back, so Lit stops at + * the property and never updates this element. One subscription for + * the row, not one per card. + */ + override connectedCallback(): void { + super.connectedCallback(); + this.unsubRequests = downloadStore.subscribe(() => + this.requestUpdate(), + ); + } + + override disconnectedCallback(): void { + this.unsubRequests?.(); + this.unsubRequests = undefined; + super.disconnectedCallback(); + } + static override styles = [ designTokens, exploreLinkStyles, @@ -255,7 +279,10 @@ export class TopResultsRow extends LitElement { ? r.year || '' : formatDuration(r.length) || ''; - const status: LibraryStatus = r.inLibrary ? 'in-library' : 'not-in-library'; + const status: LibraryStatus = libraryStatusFor( + Boolean(r.inLibrary), + r.mbid, + ); const entityType: 'artist' | 'album' | 'track' = r.entityType === 'artist' ? 'artist' diff --git a/frontend/src/utils/library-status.ts b/frontend/src/utils/library-status.ts new file mode 100644 index 0000000..8c6326e --- /dev/null +++ b/frontend/src/utils/library-status.ts @@ -0,0 +1,45 @@ +import { downloadStore } from '@store/download-store'; +import type { LibraryStatus } from '../components/library-status-indicator/library-status-indicator'; + +/** + * What the tick/hourglass/plus badge should say about one entity. + * + * This exists because the rule was written at all eight call sites and + * so none of them had the whole of it: every one was a two-way ternary + * between `in-library` and `not-in-library`, and the badge's third + * state — `queued`, styled and labelled since it was written — was + * produced by nothing. An album the user had already asked for through + * the "Want this" button showed a plus and said it was not in their + * library, on the same page, forty pixels from a filled button reading + * "Wanted". + * + * Two rules decide the answer, and both are about honesty rather than + * precedence for its own sake: + * + * - **Owning outranks wanting.** A request that has been satisfied by + * any route — downloaded here, ripped, bought elsewhere — is not + * news; what the user has is. + * - **A request is by MBID, and the badge answers about the entity it + * is on.** A track inside a requested album is not itself requested, + * so it stays a plus. Saying otherwise would promise that clicking + * it later would find *that* recording. + * + * A `satisfied` request is deliberately not `queued`: nothing is coming. + * A `paused` one is, because the user did ask for it and it is still on + * the list — "queued" is a slight overstatement of a paused request and + * a much smaller one than "not in your library". + */ +export function libraryStatusFor( + owned: boolean, + mbid?: string | null, +): LibraryStatus { + if (owned) return 'in-library'; + + if (!mbid) return 'not-in-library'; + + const request = downloadStore.requestFor(mbid); + + if (request && request.state !== 'satisfied') return 'queued'; + + return 'not-in-library'; +} diff --git a/frontend/test/components/library-status.test.ts b/frontend/test/components/library-status.test.ts new file mode 100644 index 0000000..c567b5a --- /dev/null +++ b/frontend/test/components/library-status.test.ts @@ -0,0 +1,185 @@ +/** + * Plan 009 phase 1: the badge tells the truth. + * + * `library-status-indicator` has had three states since it was written + * — a tick, an hourglass and a plus — and the hourglass was produced by + * nothing. All eight call sites were a two-way ternary, so an album the + * user had already asked for through "Want this" showed a plus and said + * it was not in their library, on the same page as a filled button + * reading "Wanted". + * + * Two tiers of assertion here, and the second is the one that would + * have failed: + * + * - the rule itself, which is now written once, and + * - a rendered Explore result whose release group is requested, + * because a helper nobody calls is a rule nobody follows. + */ +import { beforeEach, describe, expect, it } from 'vitest'; + +import '@components/explore-view/explore-view'; +import type { Request } from '@store/download-store'; +import { libraryStatusFor } from '@utils/library-status'; +import { Events } from '../../src/events'; +import { emit, flush, stub } from '@test/support/harness'; +import { fixture, shadow, shadowAll } from '@test/support/render'; + +const SEARCH = 'explore.Service.SearchLocal'; + +function request(overrides: Partial): Request { + return { + id: 1, + mbid: 'rg-wanted', + entity: 'release-group', + libraryId: 1, + artist: 'An Artist', + title: 'Wanted Album', + scope: 'future', + secondary: false, + state: 'wanted', + attempts: 0, + ...overrides, + } as Request; +} + +/** Put a request list into the store the way the backend does. */ +async function withRequests(rows: Request[]): Promise { + stub('download.Service.ListRequests', rows); + emit(Events.RequestsChanged); + await flush(); +} + +const releaseGroup = (mbid: string, title: string) => ({ + mbid, + title, + artistCredit: 'An Artist', + artistMbid: 'ar-1', + primaryType: 'Album', + firstReleaseDate: '1994-05-01', + popularity: 100, + listenerCount: 10, + inLibrary: false, + secondaryTypes: [], +}); + +describe('libraryStatusFor', () => { + beforeEach(async () => { + await withRequests([]); + }); + + it('says nothing about an entity with no MBID to ask about', () => { + expect(libraryStatusFor(false, '')).toBe('not-in-library'); + expect(libraryStatusFor(false, undefined)).toBe('not-in-library'); + }); + + it('reports a request as queued', async () => { + await withRequests([request({ mbid: 'rg-wanted' })]); + + expect(libraryStatusFor(false, 'rg-wanted')).toBe('queued'); + }); + + it('lets owning outrank wanting', async () => { + // Both are true of an album that has arrived but whose request has + // not been retired yet. What the user has is not news; what they + // have is. + await withRequests([request({ mbid: 'rg-wanted' })]); + + expect(libraryStatusFor(true, 'rg-wanted')).toBe('in-library'); + }); + + it('does not call a satisfied request queued', async () => { + // Nothing is coming: the request is history. An unowned entity with + // a satisfied request is a stale row, not a download in flight. + await withRequests([request({ mbid: 'rg-done', state: 'satisfied' })]); + + expect(libraryStatusFor(false, 'rg-done')).toBe('not-in-library'); + }); + + it('does count a paused request, which the user did ask for', async () => { + await withRequests([request({ mbid: 'rg-paused', state: 'paused' })]); + + expect(libraryStatusFor(false, 'rg-paused')).toBe('queued'); + }); + + it('answers about the entity it is on, not the one containing it', () => { + // A request is by MBID. A track inside a requested album is not + // itself requested, and saying otherwise promises that clicking it + // would find that recording. + expect(libraryStatusFor(false, 'recording-inside-rg-wanted')).toBe( + 'not-in-library', + ); + }); +}); + +describe(' badges', () => { + beforeEach(async () => { + stub('explore.Service.GetThumbnails', []); + stub('explore.Service.GetThumbnail', ''); + stub('explore.Service.GetArtistImageURL', ''); + stub('explore.Service.GetExploreShelves', { shelves: [], state: 'ready' }); + stub('library.Library.GetAllAlbums', []); + stub('library.Library.GetAllTracks', []); + await withRequests([]); + }); + + async function searchFor(rows: ReturnType[]) { + stub(SEARCH, { + artists: [], + releaseGroups: rows, + recordings: [], + topResults: [], + }); + + const el = await fixture('explore-view'); + (el as unknown as { viewActivated(): void }).viewActivated(); + await flush(); + + const input = shadow(el, 'input'); + if (input) { + input.value = 'anything'; + input.dispatchEvent(new Event('input', { bubbles: true })); + } + + // The search box debounces, so waiting a frame measures the input + // echoing its own character. + await new Promise((resolve) => setTimeout(resolve, 300)); + await flush(); + await el.updateComplete; + + return el; + } + + it('shows a requested album as queued rather than as absent', async () => { + await withRequests([request({ mbid: 'rg-wanted' })]); + + const el = await searchFor([ + releaseGroup('rg-wanted', 'Wanted Album'), + releaseGroup('rg-other', 'Some Other Album'), + ]); + + const badges = shadowAll(el, 'library-status-indicator'); + + expect(badges.length).toBeGreaterThanOrEqual(2); + expect(badges.map((b) => b.getAttribute('status'))).toEqual([ + 'queued', + 'not-in-library', + ]); + }); + + it('re-renders when the request list changes underneath it', async () => { + // A background reconcile pass expands an artist or retires a want + // without this page doing anything, so the badge has to be told. + const el = await searchFor([releaseGroup('rg-wanted', 'Wanted Album')]); + + expect(shadow(el, 'library-status-indicator')?.getAttribute('status')).toBe( + 'not-in-library', + ); + + await withRequests([request({ mbid: 'rg-wanted' })]); + await el.updateComplete; + + expect(shadow(el, 'library-status-indicator')?.getAttribute('status')).toBe( + 'queued', + ); + }); +});