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 e55352b..a730b58 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -3,6 +3,7 @@ import { customElement, property, state, query } from 'lit/decorators.js'; import { classMap } from 'lit/directives/class-map.js'; import { designTokens } from '../../styles/tokens.css'; import { srOnly } from '../../styles/sr-only.css'; +import { unownedLabel, unownedStyles } from '@utils/ownership'; import { LookupReleaseGroup, BrowseReleases, @@ -316,6 +317,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { exploreLinkStyles, contextMenuStyles, srOnly, + unownedStyles, css` :host { display: flex; @@ -687,20 +689,14 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { white-space: nowrap; } - /* A track the library does not have, on the pattern a - * streaming service uses for something it cannot play: the - * row stays, dimmed, so the album reads as the album rather - * than as the subset that happens to be here. - * - * The dimming is a colour, so it cannot be the only signal - * — the row also carries aria-disabled, which is what - * reaches anyone not seeing it. Secondary rather than - * tertiary because the row's hover background is - * bgOverlay, which tertiary does not clear. */ - .track-row.unowned .track-title { - color: var(--yj-text-secondary, #b3b3b3); - font-weight: 400; - } + /* The dimming itself is unownedStyles, from + * utils/ownership.ts, imported above. It was written here + * first — this tracklist is where the treatment came from — + * and moved out when seven other surfaces had to draw the + * same thing, because two of them would otherwise have + * ended up drawing it slightly differently. (No backticks or + * apostrophes-as-quotes here: this is inside a tagged + * template literal.) */ /* The request control is offered on every row that has * something to request, and is not revealed on hover. @@ -3273,7 +3269,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { aria-disabled=${owned ? 'false' : 'true'} aria-label=${owned ? `Play “${track.title}”` - : `${track.title} — not in your library`} + : unownedLabel(track.title, 'track')} @dblclick=${() => this.onTrackRowDblClick(track)} @contextmenu=${(e: MouseEvent) => this.onTrackContextMenu(e, track)} @keydown=${(e: KeyboardEvent) => this.onTrackRowKeydown(e, track)} 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 9c2389e..d7f11a8 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -39,7 +39,17 @@ 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, toggleRequest } from '@utils/library-status'; +import { + albumBadgeFor, + libraryStatusFor, + toggleRequest, +} from '@utils/library-status'; +import { + isOwned, + ownershipLabel, + unownedStyles, +} from '@utils/ownership'; +import { completenessStore } from '@store/completeness-store'; import '../catalog-scope-notice/catalog-scope-notice.js'; import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js'; import { queueStore } from '../../store/queue-store'; @@ -178,7 +188,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost @state() private discoRowSize = 5; private discoObserver?: ResizeObserver; @state() private similarExpanded = false; - private libraryMBIDs = new Set(); /* ── Release prefetch ── */ @@ -257,6 +266,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost designTokens, exploreLinkStyles, contextMenuStyles, + unownedStyles, css` :host { display: flex; @@ -995,6 +1005,9 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost /** Unsubscribe handle for the requests list. */ private unsubRequests: (() => void) | null = null; + /** Unsubscribes the "how much of this album is here" repaint. */ + private unsubCompleteness: (() => void) | null = null; + override connectedCallback() { super.connectedCallback(); if (this.artistMBID || this.localArtistId) { @@ -1007,6 +1020,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost this.unsubRequests = downloadStore.subscribe(() => this.requestUpdate()); void downloadStore.init().then(() => this.requestUpdate()); + // The count behind a partly-held album lands a frame after the + // cards do, since the store batches a screenful into one query. + this.unsubCompleteness = completenessStore.subscribe(() => + this.requestUpdate(), + ); + // A background discography fetch (top tracks / top releases for an // artist that wasn't indexed yet) finished — re-fetch those two // sections, once per artist, so they fill in without the initial @@ -1045,6 +1064,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost super.disconnectedCallback(); this.unsubRequests?.(); this.unsubRequests = null; + this.unsubCompleteness?.(); + this.unsubCompleteness = null; this.unsubDiscogReady?.(); this.unsubSimilarReady?.(); if (this.discogFallbackTimer) clearTimeout(this.discogFallbackTimer); @@ -1573,10 +1594,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost this.catalogPending = false; } - // Populate libraryMBIDs from the inLibrary flag (already - // set by the backend via local_release_group_id cross-ref). - this.checkLibrary(); - // Batch-resolve cover art for discography (lower priority — loaded after top sections). void this.batchResolveThumbnails( rgs?.map((r) => ({ mbid: r.mbid, albumName: r.title, artistName: r.artistCredit })) @@ -1903,23 +1920,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost } } - private checkLibrary() { - // Backend now populates `inLibrary` directly on each MBReleaseGroup - // via the local_release_group_id cross-reference column. Just read it. - let updated = false; - - for (const rg of this.releaseGroups) { - if (rg.mbid && rg.inLibrary && !this.libraryMBIDs.has(rg.mbid)) { - this.libraryMBIDs.add(rg.mbid); - updated = true; - } - } - - if (updated) { - this.requestUpdate(); - } - } - /* ── Playback ── */ /** @@ -2063,7 +2063,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost } private isTrackOwned(track: LBTopRecording): boolean { - return Boolean(track.inLibrary || track.localId); + return isOwned(track); } private onTrackRowDblClick(track: LBTopRecording): void { @@ -2106,7 +2106,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost mbid: rg.releaseGroupMbid || '', localId: rg.localId ?? 0, title: rg.title, - owned: Boolean(rg.inLibrary || rg.localId), + owned: isOwned(rg), }; } @@ -2126,10 +2126,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost mbid: isLocal ? '' : rg.mbid || '', localId: Number.isFinite(localId) ? localId : 0, title: rg.title, - owned: - this.libraryMBIDs.has(rg.mbid) || - Boolean(rg.inLibrary) || - localId > 0, + // The same answer the menu gates Play on, which is the + // point: this used to be `inLibrary` too, so a card could + // report itself owned, be offered no Play (that item is + // gated on the local id) and be offered no request either + // (that one is gated on *not* owned). + owned: localId > 0, }; } @@ -2944,15 +2946,20 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost

Top Tracks

- ${tracks.map( - (t, i) => html` + ${tracks.map((t, i) => { + const owned = this.isTrackOwned(t); + + return html`
this.onTrackRowDblClick(t)} @contextmenu=${(e: MouseEvent) => this.onTrackContextMenu(e, t)} @keydown=${(e: KeyboardEvent) => this.onTrackRowKeydown(e, t)} @@ -2977,16 +2984,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost ${formatListenCount(t.totalListenCount)} plays - + ${owned + ? nothing + : html``}
- `, - )} + `; + })}
${canExpandTracks ? html` @@ -3057,10 +3066,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost private renderTopReleaseCard(rg: LBTopReleaseGroup) { const artURL = this.thumbnailURLs.get(rg.releaseGroupMbid) || ''; const target = this.topReleaseTarget(rg); + const owned = target.owned; + const badge = albumBadgeFor( + { localId: target.localId }, + rg.releaseGroupMbid, + ); return html`
this.navigateToTopRelease(rg)} role="button" tabindex="0" @@ -3092,14 +3107,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
${rg.date ? html`${extractYear(rg.date)}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
@@ -3189,14 +3208,15 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost private renderAlbumCard(rg: MBReleaseGroup) { const artURL = this.thumbnailURLs.get(rg.mbid) || ''; const year = extractYear(rg.firstReleaseDate); - const inLibrary = this.libraryMBIDs.has(rg.mbid) || Boolean(rg.inLibrary); - const status = libraryStatusFor(inLibrary, rg.mbid); const target = this.albumTarget(rg); + const owned = target.owned; + const badge = albumBadgeFor({ localId: target.localId }, target.mbid); return html`
this.navigateToAlbum(rg)} role="button" tabindex="0" @@ -3225,13 +3245,17 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
${year ? html`${year}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
`; diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index b67e003..0925d04 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -1,5 +1,11 @@ import { avatarBackground } from '@utils/avatar-color'; -import { libraryStatusFor } from '@utils/library-status'; +import { albumBadgeFor, libraryStatusFor } from '@utils/library-status'; +import { + isOwned, + ownershipLabel, + unownedStyles, +} from '@utils/ownership'; +import { completenessStore } from '@store/completeness-store'; import { downloadStore } from '@store/download-store'; import { LitElement, html, css, nothing } from 'lit'; import { customElement, state, query as litQuery } from 'lit/decorators.js'; @@ -171,7 +177,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte private searchDebounceTimer?: ReturnType; private thumbnailCache = new LRUMap(THUMBNAIL_CACHE_LIMIT); private artistImageCache = new LRUMap(ARTIST_IMAGE_CACHE_LIMIT); - private libraryMBIDs = new Set(); constructor() { super(); @@ -226,6 +231,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte srOnly, exploreLinkStyles, contextMenuStyles, + unownedStyles, css` :host { display: block; @@ -812,6 +818,13 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte // Explore should not pay for it. this.whileActive(downloadStore.subscribe(() => this.requestUpdate())); void downloadStore.init().then(() => this.requestUpdate()); + + // How much of an owned album is here arrives a frame after the + // cards do — the store coalesces a screenful into one query — + // so a card that turns out to be 9 of 12 repaints when the + // answer lands rather than showing a plain tick until something + // else happens to re-render the grid. + this.whileActive(completenessStore.subscribe(() => this.requestUpdate())); } /** A debounced search that lands after the user has left the page is @@ -1083,7 +1096,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte this.results?.artists ?? [], this.results?.releaseGroups ?? [], ); - this.checkLibrary(); } catch (err) { if (version !== this.searchVersion) return; @@ -1665,42 +1677,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte } } - /** - * Check which result MBIDs exist in the local library. - */ - private checkLibrary() { - if (!this.results) return; - - // Backend now populates `inLibrary` directly on each MB result - // via the local_*_id cross-reference columns. Just read those. - let updated = false; - - for (const a of this.results.artists ?? []) { - if (a.mbid && a.inLibrary && !this.libraryMBIDs.has(a.mbid)) { - this.libraryMBIDs.add(a.mbid); - updated = true; - } - } - - for (const rg of this.results.releaseGroups ?? []) { - if (rg.mbid && rg.inLibrary && !this.libraryMBIDs.has(rg.mbid)) { - this.libraryMBIDs.add(rg.mbid); - updated = true; - } - } - - for (const r of this.results.recordings ?? []) { - if (r.mbid && r.inLibrary && !this.libraryMBIDs.has(r.mbid)) { - this.libraryMBIDs.add(r.mbid); - updated = true; - } - } - - if (updated) { - this.requestUpdate(); - } - } - /* ── Navigation ── */ private navigateToArtist(artist: MBArtist) { @@ -2107,12 +2083,16 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte : nothing}
${artists.map((a) => { + const owned = isOwned(a); + const name = a.englishName || a.name; + return html`
this.navigateToArtist(a)} role="button" tabindex="0" + aria-label=${ownershipLabel(owned, 'Artist', name, 'artist')} @keydown=${(e: KeyboardEvent) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); @@ -2171,11 +2151,17 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte const artURL = this.thumbnailCache.get(rg.mbid) || ''; const year = extractYear(rg.firstReleaseDate); - const owned = Boolean(rg.localId); + const owned = isOwned(rg); + const badge = albumBadgeFor(rg, rg.mbid); return html`
this.navigateToAlbum(rg)} @dblclick=${() => this.onAlbumCardDblClick(rg)} @contextmenu=${(e: MouseEvent) => @@ -2228,13 +2214,17 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte : nothing} ${year ? html`${year}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
`; @@ -2249,12 +2239,20 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte

Tracks

- ${recordings.map( - (r) => html` + ${recordings.map((r) => { + const owned = isOwned(r); + + return html`
this.onRecordingRowDblClick(r)} @contextmenu=${(e: MouseEvent) => this.onExploreContextMenu(e, { @@ -2291,16 +2289,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte ? html`${formatDuration(r.length)}` : nothing}
- + ${owned + ? nothing + : html``}
- `, - )} + `; + })}
`; 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 50ed673..f25e6ad 100644 --- a/frontend/src/components/top-results-row/top-results-row.ts +++ b/frontend/src/components/top-results-row/top-results-row.ts @@ -11,8 +11,16 @@ import '../library-status-indicator/library-status-indicator.js'; import type { LibraryStatus } from '../library-status-indicator/library-status-indicator.js'; import { creditLink, exploreLinkStyles } from '../../utils/explore-link'; import { creditStore } from '@store/credit-store'; -import { libraryStatusFor } from '../../utils/library-status'; +import { albumBadgeFor, libraryStatusFor } from '../../utils/library-status'; +import { + isOwned, + ownershipLabel, + unownedStyles, + type OwnableKind, +} from '../../utils/ownership'; +import { completenessStore } from '../../store/completeness-store'; import { downloadStore } from '../../store/download-store'; +import { classMap } from 'lit/directives/class-map.js'; /** Format milliseconds as m:ss. */ function formatDuration(ms: number | undefined): string { @@ -65,6 +73,9 @@ export class TopResultsRow extends LitElement { /** Unsubscribes the credit-arrival repaint. */ private creditsUnsub?: () => void; + /** Unsubscribes the "how much of this album is here" repaint. */ + private unsubCompleteness?: () => void; + override connectedCallback(): void { super.connectedCallback(); @@ -74,6 +85,9 @@ export class TopResultsRow extends LitElement { this.unsubRequests = downloadStore.subscribe(() => this.requestUpdate(), ); + this.unsubCompleteness = completenessStore.subscribe(() => + this.requestUpdate(), + ); } override disconnectedCallback(): void { @@ -81,12 +95,15 @@ export class TopResultsRow extends LitElement { this.creditsUnsub = undefined; this.unsubRequests?.(); this.unsubRequests = undefined; + this.unsubCompleteness?.(); + this.unsubCompleteness = undefined; super.disconnectedCallback(); } static override styles = [ designTokens, exploreLinkStyles, + unownedStyles, css` :host { display: block; @@ -289,23 +306,41 @@ export class TopResultsRow extends LitElement { ? r.year || '' : formatDuration(r.length) || ''; - const status: LibraryStatus = libraryStatusFor( - Boolean(r.inLibrary), - r.mbid, - ); - const entityType: 'artist' | 'album' | 'track' = + const entityType: OwnableKind = r.entityType === 'artist' ? 'artist' : r.entityType === 'release_group' ? 'album' : 'track'; + // Ownership is the local row, not the catalog's flag — see + // `utils/ownership.ts`. An album additionally says *how much* + // of it is here, which is the one thing a tick cannot. + const owned = isOwned(r); + const badge = + entityType === 'album' + ? albumBadgeFor(r, r.mbid) + : { + status: libraryStatusFor(owned, r.mbid) as LibraryStatus, + owned: 0, + expected: 0, + }; + + // A card navigates whether or not the entity is owned, so it is + // not `aria-disabled` the way an unplayable track row is — the + // name is what carries the state to anyone not seeing the + // dimming. return html`
this.handleClick(r)} @keydown=${(e: KeyboardEvent) => { if (e.key !== 'Enter' && e.key !== ' ') return; @@ -345,10 +380,12 @@ export class TopResultsRow extends LitElement { : nothing}
- ${isArtist + ${isArtist || badge.status === 'in-library' ? nothing : html`(COMPLETENESS_CACHE_LIMIT); + + /** Ids with a request in flight, so a re-render does not refetch. */ + private inFlight = new Set(); + + private listeners = new Set<() => void>(); + + /** Collected by request(), flushed as one batch on the next frame. */ + private pending = new Set(); + + private flushHandle: number | null = null; + + constructor() { + registerCacheProbe('albumCompleteness', () => ({ + entries: this.cache.size, + chars: this.cache.size * 4, + limit: COMPLETENESS_CACHE_LIMIT, + })); + + EventsOn(Events.LibraryScanComplete, () => this.invalidate()); + EventsOn(Events.TrackMetadataChanged, () => this.invalidate()); + EventsOn(Events.TracksRemovedFromLibrary, () => this.invalidate()); + } + + /** + * Subscribe to "some answers arrived". + * + * Deliberately not per-album, for `credit-store`'s reason: a grid + * fetches its cards in one call and re-renders once, so a + * fine-grained signal would buy nothing and cost a listener a card. + */ + subscribe(fn: () => void): () => void { + this.listeners.add(fn); + + return () => this.listeners.delete(fn); + } + + /** The answer for one album, or undefined until it has been asked. */ + get(albumID: number | undefined | null): Completeness | undefined { + if (!albumID || albumID <= 0) return undefined; + + return this.cache.get(albumID); + } + + /** + * Ask about one album, joining whatever batch is forming. + * + * Safe from inside a render: a set insert and a scheduled flush, + * with anything cached or in flight dropped. It does not loop — + * after a flush every id asked for is cached, so the re-render's + * requests are all dropped and nothing notifies again. + */ + request(albumID: number | undefined | null): void { + if (!albumID || albumID <= 0) return; + if (this.cache.has(albumID)) return; + if (this.inFlight.has(albumID)) return; + if (this.pending.has(albumID)) return; + + this.pending.add(albumID); + + if (this.flushHandle !== null) return; + + this.flushHandle = requestAnimationFrame(() => { + this.flushHandle = null; + + const batch = [...this.pending]; + + this.pending.clear(); + + void this.ensure(batch); + }); + } + + /** + * Ask and read in one call, for use inside a template. + * + * A getter with a side effect, deliberately — `credit-store` makes + * the same trade and for the same reason: the alternative is every + * call site writing `request(x)` beside `get(x)` and one of them + * eventually forgetting, which renders a permanently unknown + * completeness that looks exactly like an album with no totals. + */ + completeness(albumID: number | undefined | null): Completeness | undefined { + this.request(albumID); + + return this.get(albumID); + } + + /** Fetch for a list, skipping anything known or already in flight. */ + async ensure(albumIDs: readonly number[]): Promise { + const wanted = new Set(); + + for (const id of albumIDs) { + if (!id || id <= 0) continue; + // `has` rather than `get`: probing must not mark an entry + // recently-used, or scrolling past a card would keep it + // alive ahead of one actually being rendered. + if (this.cache.has(id)) continue; + if (this.inFlight.has(id)) continue; + + wanted.add(id); + } + + if (wanted.size === 0) return; + + const batch = [...wanted]; + + for (const id of batch) this.inFlight.add(id); + + try { + const found = compact(await GetAlbumsCompleteness(batch)); + + for (const id of batch) { + this.cache.set(id, found[String(id)] ?? NOTHING_HERE); + } + + this.notify(); + } catch (err) { + // A count is an enrichment: without it a card shows the + // plain "you have this", which is what it showed before and + // is a weaker answer rather than a broken one. + console.error('Failed to load album completeness', err); + } finally { + for (const id of batch) this.inFlight.delete(id); + } + } + + /** Drop everything: the files on disk changed. */ + invalidate(): void { + this.cache = new LRUMap( + COMPLETENESS_CACHE_LIMIT, + ); + this.notify(); + } + + private notify(): void { + for (const fn of this.listeners) fn(); + } +} + +export const completenessStore = new CompletenessStore(); diff --git a/frontend/src/utils/library-status.ts b/frontend/src/utils/library-status.ts index d943d29..18233f5 100644 --- a/frontend/src/utils/library-status.ts +++ b/frontend/src/utils/library-status.ts @@ -1,7 +1,9 @@ +import { completenessStore } from '@store/completeness-store'; import { downloadStore } from '@store/download-store'; import { libraryStore } from '@store/library-store'; import type * as download from '@go/download/models.js'; import type { LibraryStatus } from '../components/library-status-indicator/library-status-indicator'; +import { isOwned, type Ownable } from './ownership'; /** * What the tick/hourglass/plus badge should say about one entity. @@ -46,6 +48,61 @@ export function libraryStatusFor( return 'not-in-library'; } +/** + * Everything a badge needs about one entity, decided in one place. + * + * `status` is the state; `owned`/`expected` are the counts behind + * `partial` and are zero for every other state, which is what the badge + * requires — it documents that a caller with no total must not pass a + * ring at 0%. + */ +export interface BadgeState { + status: LibraryStatus; + owned: number; + expected: number; +} + +/** + * What the badge on an album card should say. + * + * Three rules, and the middle one is the whole point of this issue. + * + * **Ownership is the local album id**, per `utils/ownership.ts` — a + * file, not the catalog's `inLibrary` ratchet. + * + * **A partly-held album says how partly.** The count comes from + * `completenessStore`, which batches a screenful into one query; + * reading it is what asks for it. Before this, an album held 2 tracks + * of 10 wore the same green tick as one held whole on every grid in + * the app. + * + * **A total that was never declared is not a total of zero.** Where + * `known` is false — most of an untagged library, and every album until + * a rescan repopulates `audio_files.total_tracks` — this is a plain + * `in-library` and says nothing, which is the rule the badge's own + * documentation states and the reason `Known` exists at all. + */ +export function albumBadgeFor( + album: Ownable | null | undefined, + mbid?: string | null, +): BadgeState { + if (!isOwned(album)) { + return { status: libraryStatusFor(false, mbid), owned: 0, expected: 0 }; + } + + const held = completenessStore.completeness(album?.localId); + + if (held?.known && !held.complete) { + return { + status: 'partial', + owned: held.owned, + expected: held.expected, + }; + } + + return { status: 'in-library', owned: 0, expected: 0 }; +} + /** What a badge can ask for. Artists are deliberately absent: a * discography subscription is `explore-artist-details`'s Follow * button, which can say what it is committing to. */ diff --git a/frontend/src/utils/ownership.ts b/frontend/src/utils/ownership.ts new file mode 100644 index 0000000..04b4ff9 --- /dev/null +++ b/frontend/src/utils/ownership.ts @@ -0,0 +1,134 @@ +/** + * What "I do not own this" looks like, and how the app decides it. + * + * The rule the user asked for, in their words: *owned content is the + * default, normal, unadorned presentation; unowned content is what gets + * marked*. `explore-album-details` implemented it for one tracklist — + * dimmed in place, `aria-disabled` because dimming is a colour and + * cannot be the only signal, and nothing at all drawn on the owned rows + * — and every other catalog surface still mixed the two with a small + * badge as the only difference. This is that rule, written once, so + * eight surfaces cannot each keep their own version of it. + * + * ## 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 + * from a displayed track to a real file. A card grid cannot afford a + * lookup per card — and does not need one, because the answer is + * already on every model. + * + * `explore_index.local_artist_id` / `local_release_group_id` / + * `local_recording_id` are built by `collectLibraryEntities` from + * queries that every one join `audio_files`, and cleared by + * `pruneStaleLocalCrossReferences` whose existence test is a file test + * in all three cases. That is the same "ownership is a file" rule, + * computed once per scan instead of once per screenful. + * + * **`inLibrary` is the weaker one and is deliberately not consulted.** + * It is written by the same pass, so today the two agree — but it is a + * one-way ratchet (`in_library = MAX(in_library, excluded.in_library)`) + * whose only clearing pass is gated on a non-null `local_*_id`, so it + * cannot be un-set on its own. One of the two is a fact with an owner; + * the other is a flag that happens to agree with it. + * + * The divergence was observable before this: both `explore-view` and + * `explore-artist-details` kept a `libraryMBIDs` set that accumulated + * every MBID ever seen with `inLibrary` and cleared it never, in a view + * that never unmounts. And on one artist-detail card the two answers + * were used side by side — the context menu gated Play on + * `localId > 0` while the badge said "in your library" from + * `inLibrary`, so a card could claim to be owned, offer no Play, and + * (the request item being gated on *not* owned) offer no way to ask for + * it either. + */ + +import { css } from 'lit'; + +/** Anything a card or row can be drawn from, as far as this is concerned. */ +export interface Ownable { + /** The local row id behind this entity: an album, a file, an artist. */ + localId?: number | null; +} + +/** + * Whether there is something of the user's behind this entity. + * + * Deliberately narrow: a local id and nothing else. Passing the model + * straight in is the point — a call site that has to remember which of + * two fields to read is a call site that will eventually read the other + * one, which is exactly how the two answers came to sit on one card. + */ +export function isOwned(entity: Ownable | null | undefined): boolean { + return (entity?.localId ?? 0) > 0; +} + +/** The kinds of thing a catalog surface can draw. */ +export type OwnableKind = 'album' | 'track' | 'artist'; + +/** + * The sentence an unowned thing says, once. + * + * It reaches whoever is not seeing the dimming, so it has to name the + * thing as well as the state — "not in your library" alone, repeated + * down a grid, identifies nothing. The em dash matches the album + * tracklist's existing phrasing, which is where this came from. + */ +export function unownedLabel(name: string, kind: OwnableKind): string { + return `${name} — not in your library, ${ + kind === 'artist' ? 'browsing the catalog' : 'available to request' + }`; +} + +/** + * The accessible name for a card or row, owned or not. + * + * `activates` is what the thing does when it is yours: "Play", "Album", + * whatever the surface's own verb is. An unowned one does not get that + * verb, because it cannot do it. + */ +export function ownershipLabel( + owned: boolean, + activates: string, + name: string, + kind: OwnableKind, +): string { + return owned ? `${activates} ${name}` : unownedLabel(name, kind); +} + +/** + * The dimming, shared so it cannot drift across surfaces. + * + * Two things about it are load-bearing. + * + * **The text dims to a token, not with `opacity`.** `theme-store`'s + * ramps are checked by `theme-contrast.test.ts` against every surface + * text can sit on; an opacity multiplier is outside that check and + * would quietly drop a dimmed title under 4.5:1 on the light ramps. + * Secondary rather than tertiary for the reason the album tracklist + * gives: these rows and cards have a `bgOverlay` hover background, + * which tertiary does not clear. + * + * **Only the artwork takes an `opacity`.** A cover is not text, so it + * is outside the contrast rule entirely, and it is the part of a card + * that carries the most weight — dimming it is what makes a grid read + * as catalog at a glance rather than needing the badge to be found. + */ +export const unownedStyles = css` + .unowned .album-title, + .unowned .track-title, + .unowned .card-name, + .unowned .top-release-title, + .unowned .artist-name { + color: var(--yj-text-secondary, #b3b3b3); + font-weight: 400; + } + + .unowned .album-art-container, + .unowned .top-release-art, + .unowned .track-art, + .unowned .card-image, + .unowned .card-image-placeholder, + .unowned .artist-avatar { + opacity: 0.55; + } +`; diff --git a/frontend/test/components/unowned-everywhere.test.ts b/frontend/test/components/unowned-everywhere.test.ts new file mode 100644 index 0000000..5bee2de --- /dev/null +++ b/frontend/test/components/unowned-everywhere.test.ts @@ -0,0 +1,363 @@ +/** + * Owned is plain; unowned is what gets marked. + * + * `explore-album-details` had this right for one tracklist and nothing + * else did: Explore's cards, the top-results row and the artist page's + * three card shapes all mixed owned and unowned with a small badge as + * the only difference — and drew a green tick on the *common* case, + * which is the treatment the album page's own green ticks were removed + * for. + * + * What is pinned here is the rule rather than any one surface, because + * 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 + * `inLibrary` ratchet, which is a flag that happens to agree; + * - and a partly-held album says *how* partly, which is the one thing + * a tick cannot. + */ +import { beforeEach, describe, expect, it } from 'vitest'; +import { page } from 'vitest/browser'; + +import '@components/explore-view/explore-view'; +import '@components/top-results-row/top-results-row'; +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'; +const COMPLETENESS = 'library.Library.GetAlbumsCompleteness'; + +/** A release group as the backend projects one. */ +function album( + title: string, + { localId = 0, inLibrary = false }: { localId?: number; inLibrary?: boolean }, +) { + return { + mbid: `rg-${title}`, + title, + artistCredit: 'An Artist', + artistMbid: 'ar-1', + primaryType: 'Album', + firstReleaseDate: '1994-05-01', + popularity: 100, + listenerCount: 10, + secondaryTypes: [], + inLibrary, + localId, + }; +} + +/** A recording as the backend projects one. */ +function recording( + title: string, + { localId = 0, inLibrary = false }: { localId?: number; inLibrary?: boolean }, +) { + return { + mbid: `rec-${title}`, + title, + artistCredit: 'An Artist', + artistMbid: 'ar-1', + length: 200000, + popularity: 0, + listenerCount: 0, + inLibrary, + localId, + }; +} + +/** Mount Explore showing one page of results. */ +async function exploreShowing(results: { + releaseGroups?: unknown[]; + recordings?: unknown[]; + artists?: unknown[]; +}) { + stub(SHELVES, { shelves: [], state: 'ready' }); + stub(SEARCH, { + artists: [], + releaseGroups: [], + recordings: [], + ...results, + }); + + const el = await fixture('explore-view'); + + // A cached primary view only fetches on arrival, and the search is + // what these cards come from. + (el as unknown as { onViewActivate: () => void }).onViewActivate?.(); + await update(el, { results: { artists: [], releaseGroups: [], recordings: [], ...results } }); + await flush(); + await el.updateComplete; + + return el; +} + +beforeEach(() => { + resetHarness(); + stub(COMPLETENESS, {}); + + // The store is a singleton and caches an *answer*, including the + // 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, + // so a stale entry from the test above would otherwise decide the + // one below. Found by writing the assertion the wrong way round. + completenessStore.invalidate(); +}); + +describe('an owned thing is plain', () => { + it('draws no badge on an album card it has files for', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Held', { localId: 7 })], + }); + + expect(shadowAll(el, '.album-card')).toHaveLength(1); + expect(shadow(el, '.album-card library-status-indicator')).toBeNull(); + }); + + it('draws no badge on a track row it has a file for', async () => { + const el = await exploreShowing({ + 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 () => { + const el = await exploreShowing({ + releaseGroups: [album('Held', { localId: 7 })], + }); + + expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe( + false, + ); + }); +}); + +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. + * + * `inLibrary` is written by the same pass that writes the local ids, so + * the two agree in a healthy database — but it is a one-way ratchet + * (`MAX(in_library, excluded.in_library)`) whose only clearing pass is + * gated on a non-null `local_*_id`, so it cannot be un-set on its own. + * A row carrying it with no local id behind it is a claim of ownership + * with no file, which is exactly what the album page refuses to trust. + */ +describe('ownership is a file, not a flag', () => { + it('treats a card flagged inLibrary with no local row as unowned', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Phantom', { inLibrary: true, localId: 0 })], + }); + + expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true); + expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull(); + }); + + it('does the same for a track row', async () => { + const el = await exploreShowing({ + recordings: [recording('Phantom', { inLibrary: true, localId: 0 })], + }); + + expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe( + 'true', + ); + }); +}); + +/** + * 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, + * on every grid in the app. + */ +describe('a partly-held album says how partly', () => { + it('draws the ring and puts the count in the badge name', async () => { + stub(COMPLETENESS, { + '7': { owned: 9, expected: 12, known: true, complete: false }, + }); + + const el = await exploreShowing({ + releaseGroups: [album('Partly', { localId: 7 })], + }); + + // The store batches into the next frame, so the answer lands one + // repaint after the cards do — which is the thing the subscription + // exists for. + await new Promise((r) => requestAnimationFrame(() => r(null))); + await flush(); + await el.updateComplete; + + const badge = shadow(el, '.album-card library-status-indicator'); + + expect(badge?.getAttribute('status')).toBe('partial'); + + // 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 + // action and the count. Naming it after the action alone left the + // one state the ring exists for as the one state whose name did not + // mention it. + await expect + .element( + page.getByRole('button', { + name: /Request the rest of album .*Partly.* — 9 of 12 tracks/, + }), + ) + .toBeInTheDocument(); + }); + + /** + * Where the tags never declared a total, `known` is false and the + * card must say nothing — most of an untagged library is in that + * state, and a ring drawn from its absence would mark all of it + * incomplete on no evidence. That is the rule `Known` exists for. + */ + it('says nothing when the total was never declared', async () => { + stub(COMPLETENESS, { + '7': { owned: 3, expected: 0, known: false, complete: false }, + }); + + const el = await exploreShowing({ + releaseGroups: [album('Untotalled', { localId: 7 })], + }); + + await new Promise((r) => requestAnimationFrame(() => r(null))); + await flush(); + await el.updateComplete; + + expect(shadow(el, '.album-card library-status-indicator')).toBeNull(); + }); + + it('asks about the owned albums only, in one call', async () => { + const seen: unknown[][] = []; + + stub(COMPLETENESS, (...args: unknown[]) => { + seen.push(args); + + return {}; + }); + + await exploreShowing({ + releaseGroups: [ + album('Held', { localId: 7 }), + album('Also held', { localId: 8 }), + album('Absent', {}), + album('Phantom', { inLibrary: true, localId: 0 }), + ], + }); + + await new Promise((r) => requestAnimationFrame(() => r(null))); + await flush(); + + expect(seen).toHaveLength(1); + expect(seen[0]?.[0]).toEqual([7, 8]); + }); +}); + +describe('the top-results row follows the same rule', () => { + const result = ( + name: string, + entityType: string, + extra: Record = {}, + ) => ({ + entityType, + mbid: `top-${name}`, + name, + artistCredit: 'An Artist', + intentScore: 1, + inLibrary: false, + ...extra, + }); + + it('draws no badge on something it owns', async () => { + const el = await fixture('top-results-row', { + results: [result('Held', 'release_group', { localId: 7 })], + query: 'held', + }); + + 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 () => { + const el = await fixture('top-results-row', { + results: [result('Absent', 'release_group')], + query: 'absent', + }); + + expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(true); + + await expect + .element(page.getByRole('button', { name: /Absent — not in your library/ })) + .toBeInTheDocument(); + }); + + /** + * An artist card has never had a badge — a discography subscription + * 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. + */ + it('marks an unowned artist without offering a request', async () => { + const el = await fixture('top-results-row', { + results: [result('An Artist', 'artist')], + query: 'an artist', + }); + + expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(true); + expect(shadow(el, '.card library-status-indicator')).toBeNull(); + }); +});