diff --git a/backend/database/sql/queries/audio_files.sql b/backend/database/sql/queries/audio_files.sql index ded9537..2bb789c 100644 --- a/backend/database/sql/queries/audio_files.sql +++ b/backend/database/sql/queries/audio_files.sql @@ -318,3 +318,28 @@ JOIN audio_files af ON af.recording_id = r.id WHERE rgr.release_group_id IN (sqlc.slice('release_group_ids')) AND af.library_id = ? ORDER BY rgr.disc_number, rgr.track_number; + +-- Same shape again, keyed on recording MBID, for the catalog side. +-- An Explore album page knows which of its tracks the user owns only +-- as a set of recording MBIDs -- that is exactly how the backend +-- decides `inLibrary` (markReleasesInLibrary -> CheckMBIDs) -- and +-- MBTrack.LocalID is declared but never written by anything, so there +-- is no id to ask by. Grouped by MBID because a recording can have +-- more than one file (the duplicate fixtures are precisely that) and +-- because the caller owns the order: the tracklist's, not the +-- database's. + +-- name: GetFilePathsByRecordingMBIDs :many +SELECT r.mbid AS recording_mbid, af.file_path +FROM recordings r +JOIN audio_files af ON af.recording_id = r.id +WHERE r.mbid IN (sqlc.slice('mbids')) +ORDER BY af.file_path; + +-- name: GetFilePathsByRecordingMBIDsByLibrary :many +SELECT r.mbid AS recording_mbid, af.file_path +FROM recordings r +JOIN audio_files af ON af.recording_id = r.id +WHERE r.mbid IN (sqlc.slice('mbids')) + AND af.library_id = ? +ORDER BY af.file_path; diff --git a/backend/database/sql/sqlcgen/audio_files.sql.go b/backend/database/sql/sqlcgen/audio_files.sql.go index 96c47b1..7062065 100644 --- a/backend/database/sql/sqlcgen/audio_files.sql.go +++ b/backend/database/sql/sqlcgen/audio_files.sql.go @@ -907,6 +907,115 @@ func (q *Queries) GetAudioFilesNeedingMetadata(ctx context.Context) ([]AudioFile return items, nil } +const getFilePathsByRecordingMBIDs = `-- name: GetFilePathsByRecordingMBIDs :many + +SELECT r.mbid AS recording_mbid, af.file_path +FROM recordings r +JOIN audio_files af ON af.recording_id = r.id +WHERE r.mbid IN (/*SLICE:mbids*/?) +ORDER BY af.file_path +` + +type GetFilePathsByRecordingMBIDsRow struct { + RecordingMbid sql.NullString + FilePath string +} + +// Same shape again, keyed on recording MBID, for the catalog side. +// An Explore album page knows which of its tracks the user owns only +// as a set of recording MBIDs -- that is exactly how the backend +// decides `inLibrary` (markReleasesInLibrary -> CheckMBIDs) -- and +// MBTrack.LocalID is declared but never written by anything, so there +// is no id to ask by. Grouped by MBID because a recording can have +// more than one file (the duplicate fixtures are precisely that) and +// because the caller owns the order: the tracklist's, not the +// database's. +func (q *Queries) GetFilePathsByRecordingMBIDs(ctx context.Context, mbids []sql.NullString) ([]GetFilePathsByRecordingMBIDsRow, error) { + query := getFilePathsByRecordingMBIDs + var queryParams []interface{} + if len(mbids) > 0 { + for _, v := range mbids { + queryParams = append(queryParams, v) + } + query = strings.Replace(query, "/*SLICE:mbids*/?", strings.Repeat(",?", len(mbids))[1:], 1) + } else { + query = strings.Replace(query, "/*SLICE:mbids*/?", "NULL", 1) + } + rows, err := q.db.QueryContext(ctx, query, queryParams...) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetFilePathsByRecordingMBIDsRow + for rows.Next() { + var i GetFilePathsByRecordingMBIDsRow + if err := rows.Scan(&i.RecordingMbid, &i.FilePath); 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 getFilePathsByRecordingMBIDsByLibrary = `-- name: GetFilePathsByRecordingMBIDsByLibrary :many +SELECT r.mbid AS recording_mbid, af.file_path +FROM recordings r +JOIN audio_files af ON af.recording_id = r.id +WHERE r.mbid IN (/*SLICE:mbids*/?) + AND af.library_id = ? +ORDER BY af.file_path +` + +type GetFilePathsByRecordingMBIDsByLibraryParams struct { + Mbids []sql.NullString + LibraryID int64 +} + +type GetFilePathsByRecordingMBIDsByLibraryRow struct { + RecordingMbid sql.NullString + FilePath string +} + +func (q *Queries) GetFilePathsByRecordingMBIDsByLibrary(ctx context.Context, arg GetFilePathsByRecordingMBIDsByLibraryParams) ([]GetFilePathsByRecordingMBIDsByLibraryRow, error) { + query := getFilePathsByRecordingMBIDsByLibrary + var queryParams []interface{} + if len(arg.Mbids) > 0 { + for _, v := range arg.Mbids { + queryParams = append(queryParams, v) + } + query = strings.Replace(query, "/*SLICE:mbids*/?", strings.Repeat(",?", len(arg.Mbids))[1:], 1) + } else { + query = strings.Replace(query, "/*SLICE:mbids*/?", "NULL", 1) + } + queryParams = append(queryParams, arg.LibraryID) + rows, err := q.db.QueryContext(ctx, query, queryParams...) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetFilePathsByRecordingMBIDsByLibraryRow + for rows.Next() { + var i GetFilePathsByRecordingMBIDsByLibraryRow + if err := rows.Scan(&i.RecordingMbid, &i.FilePath); 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 getFilePathsByReleaseGroups = `-- name: GetFilePathsByReleaseGroups :many SELECT rgr.release_group_id, af.file_path diff --git a/backend/library/filepaths_test.go b/backend/library/filepaths_test.go index 5b86a56..c239b6f 100644 --- a/backend/library/filepaths_test.go +++ b/backend/library/filepaths_test.go @@ -229,3 +229,104 @@ func TestGetFilePathsByGenres_Empty(t *testing.T) { t.Errorf("got %v, want empty", got) } } + +// seedRecordingMBIDs stamps recording MBIDs onto the tracks seeded by +// seedAlbumsAndGenres, in the shape the catalog side actually meets: two +// tracks tagged, one deliberately left untagged, and one MBID carried by +// two files in different libraries — which is what a duplicate is. +func seedRecordingMBIDs(t *testing.T, lib *Library) (tagged, shared string) { + t.Helper() + + ctx := lib.ctx + q := lib.db.Queries + + tagged = "11111111-1111-1111-1111-111111111111" + shared = "22222222-2222-2222-2222-222222222222" + + byPath := map[string]string{ + "/music/a1.mp3": tagged, + "/music/a2.mp3": shared, + "/other/b1.mp3": shared, + } + + files, err := q.GetAllAudioFiles(ctx) + if err != nil { + t.Fatalf("get audio files: %v", err) + } + + for _, f := range files { + mbid, ok := byPath[f.FilePath] + if !ok { + continue + } + + if err := q.SetRecordingMBID(ctx, sqlcgen.SetRecordingMBIDParams{ + Mbid: sql.NullString{String: mbid, Valid: true}, + ID: f.RecordingID, + }); err != nil { + t.Fatalf("set recording mbid: %v", err) + } + } + + return tagged, shared +} + +// The catalog side of the same finding: an Explore album page knows what +// the user owns only as recording MBIDs, so this is the lookup that +// turns "you own 7 of these 12" into something playable. +func TestGetFilePathsByRecordingMBIDs(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + _, libraryID := seedAlbumsAndGenres(t, lib) + tagged, shared := seedRecordingMBIDs(t, lib) + + got, err := lib.GetFilePathsByRecordingMBIDs([]string{tagged, shared}, 0) + if err != nil { + t.Fatalf("GetFilePathsByRecordingMBIDs: %v", err) + } + + if len(got[tagged]) != 1 || got[tagged][0] != "/music/a1.mp3" { + t.Errorf("tagged recording = %v, want [/music/a1.mp3]", got[tagged]) + } + + // One recording, two files: grouping is what keeps that visible. + // A flattened result could not say which was which. + if len(got[shared]) != 2 { + t.Errorf("shared recording = %v, want two paths", got[shared]) + } + + // Scoping drops the copy in the other library, and nothing else. + scoped, err := lib.GetFilePathsByRecordingMBIDs([]string{tagged, shared}, libraryID) + if err != nil { + t.Fatalf("GetFilePathsByRecordingMBIDs scoped: %v", err) + } + + if len(scoped[shared]) != 1 || scoped[shared][0] != "/music/a2.mp3" { + t.Errorf("scoped shared = %v, want [/music/a2.mp3]", scoped[shared]) + } +} + +// An empty MBID matches every untagged recording in the library, which +// is the opposite of the question being asked — so an unknown track must +// contribute nothing rather than everything. +func TestGetFilePathsByRecordingMBIDs_IgnoresEmpty(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + seedAlbumsAndGenres(t, lib) + seedRecordingMBIDs(t, lib) + + got, err := lib.GetFilePathsByRecordingMBIDs([]string{"", ""}, 0) + if err != nil { + t.Fatalf("GetFilePathsByRecordingMBIDs: %v", err) + } + + if len(got) != 0 { + t.Errorf("empty MBIDs matched %d recordings, want none", len(got)) + } + + if _, err := lib.GetFilePathsByRecordingMBIDs(nil, 0); err != nil { + t.Fatalf("nil MBIDs: %v", err) + } +} diff --git a/backend/library/query.go b/backend/library/query.go index 1217ebe..75f6e2c 100644 --- a/backend/library/query.go +++ b/backend/library/query.go @@ -1204,3 +1204,117 @@ func (l *Library) GetFilePathsByGenres( return paths, nil } + +// GetFilePathsByRecordingMBIDs returns the file paths of every track +// whose recording MBID is in mbids, grouped by MBID. +// +// This is the catalog side of GetFilePathsByAlbums. An Explore album +// page knows what the user owns as a set of recording MBIDs and nothing +// else: that is exactly how the backend decides a track's InLibrary +// flag (markReleasesInLibrary → CheckMBIDs), and MBTrack.LocalID is a +// declared field that nothing writes, so there is no id to ask by. +// +// Grouped rather than flattened for the same two reasons as its +// siblings — the caller owns the order (the tracklist's, not the +// database's), and one recording can have more than one file, which is +// what this app's duplicate detection exists for. +// +// A library id of 0 means "every library". +func (l *Library) GetFilePathsByRecordingMBIDs( + mbids []string, libraryID int64, +) (map[string][]string, error) { + paths := make(map[string][]string, len(mbids)) + + if len(mbids) == 0 { + return paths, nil + } + + // recordings.mbid is nullable, so sqlc asks for NullStrings. An + // empty MBID would match every untagged recording in the library, + // which is the opposite of the question, so those are dropped here + // rather than passed through as NULL. + keys := make([]sql.NullString, 0, len(mbids)) + + for _, mbid := range mbids { + if mbid == "" { + continue + } + + keys = append(keys, sql.NullString{String: mbid, Valid: true}) + } + + if len(keys) == 0 { + return paths, nil + } + + rows, err := l.filePathRowsByMBID(keys, libraryID) + if err != nil { + return nil, err + } + + for _, row := range rows { + if !row.mbid.Valid { + continue + } + + paths[row.mbid.String] = append(paths[row.mbid.String], row.path) + } + + return paths, nil +} + +// filePathRowsByMBID runs the scoped or unscoped query behind +// GetFilePathsByRecordingMBIDs and flattens the two row types into one. +func (l *Library) filePathRowsByMBID( + keys []sql.NullString, libraryID int64, +) ([]mbidFilePath, error) { + if libraryID > 0 { + rows, err := l.db.ReadQueries.GetFilePathsByRecordingMBIDsByLibrary( + l.ctx, sqlcgen.GetFilePathsByRecordingMBIDsByLibraryParams{ + Mbids: keys, + LibraryID: libraryID, + }, + ) + if err != nil { + l.logger.Error( + "could not retrieve recording file paths for library", + "recordings", len(keys), + "libraryID", libraryID, + "error", err, + ) + + return nil, fmt.Errorf("could not get recording file paths: %w", err) + } + + out := make([]mbidFilePath, 0, len(rows)) + for _, row := range rows { + out = append(out, mbidFilePath{mbid: row.RecordingMbid, path: row.FilePath}) + } + + return out, nil + } + + rows, err := l.db.ReadQueries.GetFilePathsByRecordingMBIDs(l.ctx, keys) + if err != nil { + l.logger.Error( + "could not retrieve recording file paths", + "recordings", len(keys), + "error", err, + ) + + return nil, fmt.Errorf("could not get recording file paths: %w", err) + } + + out := make([]mbidFilePath, 0, len(rows)) + for _, row := range rows { + out = append(out, mbidFilePath{mbid: row.RecordingMbid, path: row.FilePath}) + } + + return out, nil +} + +// mbidFilePath is one row of either GetFilePathsByRecordingMBIDs query. +type mbidFilePath struct { + mbid sql.NullString + path string +} diff --git a/e2e/specs/album-actions.spec.ts b/e2e/specs/album-actions.spec.ts new file mode 100644 index 0000000..1cf4d23 --- /dev/null +++ b/e2e/specs/album-actions.spec.ts @@ -0,0 +1,119 @@ +import { test, expect, callBinding } from '../support/fixtures.js'; +import type { Page } from '@playwright/test'; + +/** + * Plan 007 phase 5: `H-13` — the album page can be played from. + * + * The page is `explore-album-details`; there is no library-side album + * detail page at all, so this catalog page is where a Play button has + * to live, and what it can honestly claim depends on how much of the + * album the user owns. + * + * What this tier adds over the component tests is that the paths + * resolve to something the queue accepts. The first version of this + * feature keyed the lookup on recording MBIDs — which is how the + * backend decides a track is `inLibrary` — and a library-only album has + * none, so Play was wired, labelled correctly, clicked cleanly and + * queued **nothing**. Every component test still passed. + */ +test.describe('playing an album from its page', () => { + test.beforeEach(async ({ app }) => { + await openFirstAlbum(app); + }); + + test.afterEach(async ({ app }) => { + // The suite shares one backend process in file order, and a queue + // left full is state the next spec did not ask for. + await callBinding(app, 'queue.Queue.Clear', []); + await app.getByTestId('nav-tracks').click(); + }); + + test('Play queues what the user owns of it', async ({ app }) => { + const play = app + .locator('explore-album-details') + .locator('[data-testid="album-play"]'); + + // The fixture library is untagged, so this album is wholly local + // and the button carries no count. + await expect(play).toContainText('Play'); + + await play.click(); + + await expect.poll(() => queueLength(app)).toBeGreaterThan(0); + }); + + test('Add to queue appends rather than replacing', async ({ app }) => { + const details = app.locator('explore-album-details'); + + await details.locator('[data-testid="album-play"]').click(); + await expect.poll(() => queueLength(app)).toBeGreaterThan(0); + + const before = await queueLength(app); + + await details.locator('[data-testid="album-queue"]').click(); + + await expect.poll(() => queueLength(app)).toBe(before * 2); + }); + + test('the ticks against the tracks have a legend', async ({ app }) => { + // `H-13` calls them unexplained. They were never *unlabelled* — the + // indicator has carried a title and an aria-label reading + // "Track “X” is in your library" all along — but a sighted user + // scanning the page got a column of green circles and no key. + await expect( + app.locator('explore-album-details').locator('.tracklist-legend'), + ).toContainText('in your library'); + }); +}); + +/** Albums → click the second card, which navigates to the album page. */ +async function openFirstAlbum(app: Page): Promise { + await app.getByTestId('nav-albums').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'albums', + ); + + // The cards come from a virtualizer, so they are not there when the + // view is: a click dispatched into an empty grid hits nothing and + // silently leaves the app on Albums, which reads as a broken + // navigation rather than a race. + await expect.poll(() => cardCount(app)).toBeGreaterThan(1); + + // A plain click on a card navigates here; Enter expands the dropdown + // instead. Dispatched rather than clicked because the card lives in a + // virtualizer inside a shadow root. + await app.evaluate(() => { + document + .querySelector('cover-grid') + ?.shadowRoot?.querySelectorAll('.album-card')[1] + ?.dispatchEvent( + new MouseEvent('click', { bubbles: true, composed: true }), + ); + }); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'explore-album-details', + ); + await expect( + app.locator('explore-album-details').locator('[data-testid="album-play"]'), + ).toBeVisible(); +} + +async function cardCount(app: Page): Promise { + return app.evaluate( + () => + document + .querySelector('cover-grid') + ?.shadowRoot?.querySelectorAll('.album-card').length ?? 0, + ); +} + +async function queueLength(app: Page): Promise { + const state = (await callBinding(app, 'queue.Queue.GetState', [])) as { + tracks?: unknown[]; + }; + + return state?.tracks?.length ?? 0; +} 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 60fad2e..5ba4b56 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -6,7 +6,11 @@ import { BrowseReleases, GetThumbnail, } from '@go/explore/Service'; -import { GetAlbumTracks } from '@go/library/Library'; +import { + GetAlbumTracks, + GetFilePathsByAlbums, + GetFilePathsByRecordingMBIDs, +} from '@go/library/Library'; import { library } from '@go/models'; import type { download, explore } from '@go/models'; type MBReleaseGroup = explore.MBReleaseGroup; @@ -25,6 +29,17 @@ import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice. import '@awesome.me/webawesome/dist/components/button/button.js'; import '../download-picker/download-picker'; import { downloadStore } from '../../store/download-store'; +import { queueStore } from '../../store/queue-store'; +import { notificationStore } from '../../store/notification-store'; +import '../notifications/inline-notice'; + +/** + * The region the album header's own failures are rendered in. + * + * "Inline" says *not global*, not *where* — so the region is named + * once, here, rather than spelled at each call site. + */ +export const ExploreAlbumRegion = 'explore-album'; /* ── Utility functions (duplicated per Knowledge Pattern #9 — no cross-component imports) ── */ @@ -269,6 +284,26 @@ export class ExploreAlbumDetails extends LitElement { flex-shrink: 0; } + .album-actions { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: 8px; + margin-top: 8px; + } + + /* + * The sentence under a partial Play button. It repeats the + * count on the button on purpose: the button has to be + * short and the claim has to be unambiguous, and "Play 7 of + * 12" alone does not say whether the other five are missing + * or merely unselected. + */ + .album-owned-note { + font-size: var(--yj-text-sm); + color: var(--yj-text-secondary, #aaa); + } + .album-artist { font-size: var(--yj-text-lg); color: var(--yj-text-secondary, #b3b3b3); @@ -301,6 +336,17 @@ export class ExploreAlbumDetails extends LitElement { } /* ── Section headers ── */ + .tracklist-legend { + display: inline-flex; + align-items: center; + gap: 4px; + margin-left: 10px; + font-weight: 400; + text-transform: none; + letter-spacing: 0; + color: var(--yj-text-tertiary, #888); + } + .section-header { font-size: 11px; font-weight: 600; @@ -1399,6 +1445,13 @@ export class ExploreAlbumDetails extends LitElement { * a track marked inLibrary where releaseGroup may be null) * - else → not owned * + * Four different claims of decreasing confidence, OR'd together and + * reported as one tick — the last of which fires when a *single* + * recording of a forty-track release matches. `ownership()` is the + * honest version of the same question and is what the header's + * 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. */ @@ -1427,6 +1480,56 @@ export class ExploreAlbumDetails extends LitElement { return 'not-in-library'; } + /** + * How much of the shown release the user actually has. + * + * The tick beside the title is a yes/no answer to "is any of this + * mine", and four of its five branches can be true when one track + * of forty matches. That is fine for a badge and useless for a + * button: "Play" that plays one track of a forty-track release is + * worse than no Play button, so the header asks this instead. + * + * It is counted off the *tracklist being displayed*, which is the + * one thing on this page that is not an inference — each track's + * `inLibrary` is set by the backend from its recording MBID + * (`markReleasesInLibrary`), the same key the file paths are + * fetched by. + */ + private ownership(): { owned: number; total: number } { + const tracks = this.currentVersion()?.tracks ?? []; + + return { + owned: tracks.filter((t) => t.inLibrary).length, + total: tracks.length, + }; + } + + /** + * Say what the green tick against a track means. + * + * `H-13` calls the ticks "unexplained". Half of that has aged: the + * indicator carries a `title` *and* an `aria-label` reading + * "Track \u201cX\u201d is in your library", so a screen reader and a hover + * both get a full sentence. What a sighted user scanning the page + * gets is a column of green circles and no key, which is what this + * is — rendered only when at least one track is actually ticked, so + * it never explains a symbol that is not on screen. + */ + private renderTracklistLegend() { + const { owned } = this.ownership(); + + if (owned === 0) return nothing; + + return html` + + in your library + `; + } + /** * Check whether the selected release's tracklist contains any track * with discNumber > 1, indicating a multi-disc release. @@ -1542,6 +1645,7 @@ export class ExploreAlbumDetails extends LitElement { > ${this.renderAlbumMeta()} + ${this.renderPlayActions()} ${this.renderDownloadAction()} @@ -1549,6 +1653,186 @@ export class ExploreAlbumDetails extends LitElement { `; } + /** + * The primary action, and the sentence that says what it will do. + * + * `H-13`: the album page had no Play, no Shuffle and no Add to + * queue. What it could not have is a Play button that means the + * same thing in every case — this is a *catalog* page, and the + * album on it may be entirely yours, partly yours, or not yours at + * all. So the button says which: + * + * - all of it → "Play", and the count is in the meta line + * - some of it → "Play 7 of 12", because playing seven + * tracks under a button that says "Play" is + * the page lying about what you own + * - none of it → no play button at all; the download and + * want actions below are the whole answer + * + * The count is the tracklist's own `inLibrary` flags, which the + * backend sets from each recording's MBID — the same key + * `GetFilePathsByRecordingMBIDs` resolves the files by, so the + * number on the button is the number of tracks that will play. + */ + private renderPlayActions() { + const { owned, total } = this.ownership(); + + if (owned === 0 || total === 0) return nothing; + + const partial = owned < total; + const playLabel = partial ? `Play ${owned} of ${total}` : 'Play'; + + return html` +
+ void this.playOwned(false)} + > + + ${playLabel} + + void this.playOwned(true)} + > + + Shuffle album + + void this.queueOwned()} + > + + Add to queue + + ${partial + ? html` + You have ${owned} of these ${total} tracks. + ` + : nothing} +
+ + `; + } + + /** + * File paths for the tracks of this release the user actually owns, + * in the tracklist's order. + * + * One call. The obvious alternative — ask for the album's tracks + * and read `FilePath` off them — is the shape `perf.m2` was about, + * and it is not even available here: this page's model is + * `MBTrack`, which carries a recording MBID and a `localId` that + * nothing in the backend ever writes. + */ + private async ownedFilePaths(): Promise { + const libraryID = libraryStore.getSelectedLibraryId() ?? 0; + + // The local album id is the better key whenever there is one: + // it needs no MBIDs at all, and a library-only album has none — + // its tracks are synthesised from `GetAlbumTracks` with + // `mbid: RecordingMBID || ''`, so an untagged library resolves + // to an empty set and the Play button silently does nothing. + // That is exactly what the first version of this did. + if (this.localAlbumId > 0) { + const byAlbum = await GetFilePathsByAlbums( + [this.localAlbumId], + libraryID, + ); + + return byAlbum[this.localAlbumId] ?? []; + } + + // Catalog-only: the page knows what is owned as recording MBIDs + // and nothing else — which is how the backend decided each + // track's `inLibrary` in the first place. + const tracks = this.currentVersion()?.tracks ?? []; + const mbids = tracks + .filter((t) => t.inLibrary && t.mbid) + .map((t) => t.mbid); + + if (mbids.length === 0) return []; + + const byMBID = await GetFilePathsByRecordingMBIDs(mbids, libraryID); + + // Walked in tracklist order rather than flattened, because the + // grouping is what lets the caller keep its own order. A + // recording with more than one file is a duplicate; play the + // first and leave the rest to the feature that exists for them. + const paths: string[] = []; + + for (const mbid of mbids) { + const first = byMBID[mbid]?.[0]; + + if (first) paths.push(first); + } + + return paths; + } + + /** Play what the user owns of this release, optionally shuffled. */ + private async playOwned(shuffle: boolean): Promise { + try { + const paths = await this.ownedFilePaths(); + + if (paths.length === 0) { + notificationStore.inline(ExploreAlbumRegion, { + text: 'None of these tracks could be found in your library.', + }); + + return; + } + + // `shuffleStart` only picks a random first track when + // shuffle mode is *already* on — it does not turn it on — + // so the mode has to be set before the queue, not after. + if (shuffle && !queueStore.getState().shuffleMode) { + queueStore.toggleShuffle(); + } + + queueStore.setQueue(paths, 0, shuffle); + } catch (error) { + console.error('Could not play album:', error); + notificationStore.inline(ExploreAlbumRegion, { + text: describeError(error, 'Could not play this album.'), + }); + } + } + + /** Append what the user owns of this release to the queue. */ + private async queueOwned(): Promise { + try { + const paths = await this.ownedFilePaths(); + + if (paths.length === 0) { + notificationStore.inline(ExploreAlbumRegion, { + text: 'None of these tracks could be found in your library.', + }); + + return; + } + + queueStore.addTracksToQueue(paths); + } catch (error) { + console.error('Could not queue album:', error); + notificationStore.inline(ExploreAlbumRegion, { + text: describeError( + error, + 'Could not add this album to the queue.', + ), + }); + } + } + /** * Offers to acquire the album, but only when the user has actually * connected a download client and does not already own it. Showing @@ -1959,7 +2243,9 @@ export class ExploreAlbumDetails extends LitElement { return html`
-

Tracklist

+

+ Tracklist ${this.renderTracklistLegend()} +

${discNumbers.map((discNum) => { const discTracks = discMap.get(discNum) ?? []; diff --git a/frontend/test/components/album-actions.test.ts b/frontend/test/components/album-actions.test.ts new file mode 100644 index 0000000..825d6ce --- /dev/null +++ b/frontend/test/components/album-actions.test.ts @@ -0,0 +1,166 @@ +/** + * An album page you can play from. + * + * `H-13`: no Play, no Shuffle, no Add to queue on the album header, and + * green ticks with no legend. The reason it is not simply "add three + * buttons" is that this is a **catalog** page — the album on it may be + * entirely the user's, partly theirs, or not theirs at all — and a Play + * button that plays 7 of a release's 40 tracks under a label saying + * "Play" is the page lying about what is owned. + * + * The partial case is the interesting one and it is **only reachable + * here**: it needs a catalog release whose tracklist is partly matched + * against the library, which the fixture library (untagged, no MBIDs, + * no network) cannot produce. The whole-album case was driven by hand + * in the running app. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/explore-album-details/explore-album-details'; +import { stub, flush, resetHarness, calls } from '@test/support/harness'; +import { fixture, shadow, text } from '@test/support/render'; + +type Version = { + key: string; + label: string; + sublabel: string; + tracks: Array<{ + position: number; + discNumber: number; + title: string; + length: number; + mbid: string; + inLibrary: boolean; + }>; +}; + +function track(n: number, owned: boolean) { + return { + position: n, + discNumber: 1, + title: `Track ${n}`, + length: 200000, + mbid: `mbid-${n}`, + inLibrary: owned, + }; +} + +/** + * Put a release on the page without the network. + * + * The component builds its versions from fetched releases; this reaches + * past that and sets the state the header actually reads, which is the + * only part under test here. + */ +async function withVersion( + owned: number, + total: number, +): Promise { + const el = await fixture('explore-album-details', { + albumName: 'Glass Harbour', + }); + + const version: Version = { + key: 'v1', + label: '2019', + sublabel: `${total} tracks`, + tracks: Array.from({ length: total }, (_, i) => track(i + 1, i < owned)), + }; + + Object.assign(el, { + versionEntries: [version], + selectedVersionKey: 'v1', + loadingReleases: false, + loadingInfo: false, + }); + el.requestUpdate(); + await flush(); + await el.updateComplete; + + return el; +} + +const playLabel = (el: LitElement) => + text(el, '[data-testid="album-play"]'); + +describe('the album header’s primary action', () => { + beforeEach(() => { + resetHarness(); + stub('library.Library.GetFilePathsByRecordingMBIDs', {}); + stub('library.Library.GetFilePathsByAlbums', {}); + stub('library.Library.GetAlbumTracks', []); + // The download actions resolve a target library on mount; without + // this the store awaits an undefined binding result and the whole + // file dies in an unhandled rejection rather than a failed test. + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + }); + + it('says “Play” when the whole release is owned', async () => { + const el = await withVersion(6, 6); + + expect(playLabel(el)).toBe('Play'); + expect(shadow(el, '[data-testid="album-shuffle"]')).toBeTruthy(); + expect(shadow(el, '[data-testid="album-queue"]')).toBeTruthy(); + // No count sentence: there is nothing to qualify. + expect(shadow(el, '.album-owned-note')).toBeNull(); + }); + + it('counts itself when only some of it is owned', async () => { + const el = await withVersion(7, 12); + + expect(playLabel(el)).toBe('Play 7 of 12'); + expect(text(el, '.album-owned-note')).toBe( + 'You have 7 of these 12 tracks.', + ); + }); + + it('offers no play button at all when none of it is owned', async () => { + // A Play button that plays nothing is worse than no Play button; + // the download and want actions are the whole answer here. + const el = await withVersion(0, 12); + + expect(shadow(el, '[data-testid="album-play"]')).toBeNull(); + expect(shadow(el, '[data-testid="album-shuffle"]')).toBeNull(); + expect(shadow(el, '[data-testid="album-queue"]')).toBeNull(); + }); + + it('asks for the owned tracks’ paths once, by the key it owns them by', async () => { + // `perf.m2`'s rule: ask for what the caller uses, once. The caller + // here uses file paths and knows its tracks only as recording + // MBIDs — `MBTrack.localId` is declared and never written by + // anything in the backend. + const el = await withVersion(7, 12); + + shadow(el, '[data-testid="album-play"]')!.click(); + await flush(); + + const asked = calls('library.Library.GetFilePathsByRecordingMBIDs'); + + expect(asked).toHaveLength(1); + // Only the owned ones, and no empty MBID — an empty string matches + // every untagged recording in the library. + expect(asked[0]!.args[0]).toHaveLength(7); + expect(asked[0]!.args[0]).not.toContain(''); + }); +}); + +describe('the ticks have a legend', () => { + beforeEach(() => { + resetHarness(); + stub('library.Library.GetAlbumTracks', []); + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + }); + + it('names the symbol when at least one track carries it', async () => { + const el = await withVersion(3, 12); + + expect(text(el, '.tracklist-legend')).toContain('in your library'); + }); + + it('does not explain a symbol that is not on screen', async () => { + const el = await withVersion(0, 12); + + expect(shadow(el, '.tracklist-legend')).toBeNull(); + }); +}); diff --git a/frontend/wailsjs/go/library/Library.d.ts b/frontend/wailsjs/go/library/Library.d.ts index e3443f0..dd343d3 100755 --- a/frontend/wailsjs/go/library/Library.d.ts +++ b/frontend/wailsjs/go/library/Library.d.ts @@ -47,6 +47,8 @@ export function GetFilePathsByAlbums(arg1:Array,arg2:number):Promise,arg2:number):Promise>>; +export function GetFilePathsByRecordingMBIDs(arg1:Array,arg2:number):Promise>>; + export function GetRemovalImpact(arg1:number):Promise; export function GetScanQueueLength():Promise; diff --git a/frontend/wailsjs/go/library/Library.js b/frontend/wailsjs/go/library/Library.js index 8c03669..bbfa8f2 100755 --- a/frontend/wailsjs/go/library/Library.js +++ b/frontend/wailsjs/go/library/Library.js @@ -86,6 +86,10 @@ export function GetFilePathsByGenres(arg1, arg2) { return window['go']['library']['Library']['GetFilePathsByGenres'](arg1, arg2); } +export function GetFilePathsByRecordingMBIDs(arg1, arg2) { + return window['go']['library']['Library']['GetFilePathsByRecordingMBIDs'](arg1, arg2); +} + export function GetRemovalImpact(arg1) { return window['go']['library']['Library']['GetRemovalImpact'](arg1); }