diff --git a/lib/metadata_db.py b/lib/metadata_db.py index 88b9ef1..2ebf3d0 100644 --- a/lib/metadata_db.py +++ b/lib/metadata_db.py @@ -125,6 +125,24 @@ def _put_perspective_value(meta: dict, col: str): # ── SQLite metadata cache ───────────────────────────────────────────────────── +def _arrangements_all_bass(raw) -> bool: + """True when EVERY arrangement on a chart is a bass part (raw ``arrangements`` + JSON, as stored). Mirrors the library grid's card rule: such a chart's tuning + must be scored against bass base pitches, or a 4-string bass tuning read as + guitar can false-match a guitarist. A chart with no arrangements is not bass. + """ + try: + arrs = json.loads(raw) if raw else [] + except (ValueError, TypeError): + return False + if not isinstance(arrs, list) or not arrs: + return False + return all( + isinstance(a, dict) and re.search(r"\bbass\b", str(a.get("name") or ""), re.I) + for a in arrs + ) + + def _ensure_smart_names(arrangements: list[dict]) -> list[dict]: """Fill in missing ``smart_name`` fields and sort arrangements by smart order. @@ -2697,7 +2715,7 @@ class MetadataDB: rows = self.conn.execute( f"""SELECT ps.filename, ps.position, s.title, s.artist, s.tuning_name, ps.arrangement, ps.work_key, s.arrangements, - (s.filename IS NULL) AS dead + (s.filename IS NULL) AS dead, s.tuning_offsets FROM playlist_songs ps LEFT JOIN songs s ON s.filename = ps.filename WHERE ps.playlist_id = ? {dead_filter} ORDER BY ps.position, ps.filename""", @@ -2709,6 +2727,13 @@ class MetadataDB: entry = { "filename": r[0], "position": r[1], "title": r[2] or r[0], "artist": r[3] or "", "tuning_name": r[4] or "", + # Offsets + the bass-only flag let the playlist tuning check score a + # row against the player's working tuning the same way the library + # grid's chips do: a NAME alone can't be scored (two "Custom Tuning" + # rows are different tunings), and coverage needs to know whether to + # measure against bass or guitar base pitches. + "tuning_offsets": r[9] or "", + "bass_only": _arrangements_all_bass(r[7]), "art_url": f"/api/song/{quote(r[0])}/art", } if is_album: @@ -2736,7 +2761,8 @@ class MetadataDB: if work_key: self._ensure_work_display() row = self.conn.execute( - "SELECT wd.filename, s.title, s.artist, s.tuning_name, s.arrangements " + "SELECT wd.filename, s.title, s.artist, s.tuning_name, s.arrangements, " + "s.tuning_offsets " "FROM work_display wd JOIN songs s ON s.filename = wd.filename " "WHERE wd.effective_work_key = ? AND wd.is_group_representative = 1", (work_key,)).fetchone() @@ -2746,8 +2772,12 @@ class MetadataDB: arrs = _ensure_smart_names(json.loads(row[4]) if row[4] else []) except Exception: arrs = [] + # An orphan-resolved slot PLAYS a different chart, so it must report + # that chart's tuning to the check — not the dead pin's. return {"resolved_filename": row[0], "title": row[1] or row[0], "artist": row[2] or "", "tuning_name": row[3] or "", + "tuning_offsets": row[5] or "", + "bass_only": _arrangements_all_bass(row[4]), "arrangements": arrs, "art_url": f"/api/song/{quote(row[0])}/art", "resolved_from_orphan": True} diff --git a/static/v3/playlists.js b/static/v3/playlists.js index 950345f..7c82098 100644 --- a/static/v3/playlists.js +++ b/static/v3/playlists.js @@ -53,12 +53,185 @@ return (m && m.index != null) ? m.index : null; } + // ── Playlist tuning check ──────────────────────────────────────────────── + // Playlists are commonly grouped BY TUNING so a practice run needs no + // retune mid-session (retuning a bass is minutes of settling, and detuning + // far on standard gauges goes floppy). A playlist built before the tuning + // filter knew about your instrument can hold songs you can't actually play + // without stopping. This flags them. It is READ-ONLY: nothing here edits a + // playlist — removal is a separate, explicit, itemised action. + + // ── SEAM — repoint HERE when `feat/v3-tuning-filter-instrument` lands ──── + // The tuning this row should be scored against for the player's instrument. + // main indexes exactly ONE tuning per chart, derived from the guitar + // arrangement, so today a bass player is scored against the guitar chart's + // tuning — the very data source behind the reported bug. The sibling PR adds + // per-instrument tuning columns (`bass_tuning_name` / `bass_tuning_offsets`) + // plus a `libInstrument()` perspective; when it lands, this ONE function + // picks the bass tuning for a bass player and everything downstream (the + // chips, the summary, the filter, the remove list) follows with no other + // change. Kept as a function, not an inline read, for exactly that reason. + function rowTuningForCheck(s) { + return { + offsets: s.tuning_offsets || s.tuning_name, + // Score a bass-only chart against bass base pitches — a 4-string bass + // tuning read as guitar can false-match a guitarist. + isBass: !!s.bass_only, + }; + } + + // A coverage report says "not covered" BOTH for a real mismatch and for + // "I couldn't work it out" (missing settings/tuner data → an all-empty + // report). Only a report carrying an actual reason — named string changes, + // a reference-pitch gap, or too few strings — is a mismatch. An unexplained + // not-covered is UNKNOWN. A false "wrong tuning" on a hand-curated playlist + // costs more trust than saying nothing. + function tuningStateFromReport(rep) { + if (!rep) return 'unknown'; + if (rep.covered) return 'match'; + if (rep.cantCover || rep.reference + || (Array.isArray(rep.retune) && rep.retune.length)) return 'mismatch'; + return 'unknown'; + } + + // Score every row. Returns null when the host exposes no tuning perspective + // at all (no working-tuning capability / no tuner coverage) — the caller + // then renders the playlist exactly as before rather than claiming anything. + async function checkPlaylistTuning(songs) { + const cov = window._tunerAutoOpen && window._tunerAutoOpen.coverageReport; + const hasWT = window.feedBack && window.feedBack.workingTuning + && typeof window.feedBack.workingTuning.get === 'function'; + if (typeof cov !== 'function' || !hasWT) return null; + const parse = window.parseRawTuningOffsets; + const out = []; + for (const s of songs || []) { + const t = rowTuningForCheck(s); + const offs = (typeof parse === 'function') ? parse(t.offsets) : null; + if (!offs || !offs.length || offs.some((n) => !isFinite(n))) { + out.push({ song: s, state: 'unknown' }); + continue; + } + let rep = null; + try { + rep = await cov({ + tuning: offs, stringCount: offs.length, + arrangement: t.isBass ? 'Bass' : 'Lead', + }); + } catch (_) { rep = null; } + out.push({ song: s, state: tuningStateFromReport(rep) }); + } + return out; + } + + // Colour + a TEXT marker per state — unknown is deliberately neutral-and- + // dimmed rather than amber, because "I couldn't check this" is a different + // claim from "this is the wrong tuning" and must not read as the latter. + function paintTuningChip(chip, state) { + if (!chip) return; + if (chip.dataset.baseTitle == null) chip.dataset.baseTitle = chip.getAttribute('title') || ''; + chip.classList.remove('bg-fb-mid', 'bg-emerald-500', 'bg-amber-400', 'opacity-60'); + chip.classList.add(state === 'match' ? 'bg-emerald-500' + : state === 'mismatch' ? 'bg-amber-400' : 'bg-fb-mid'); + if (state === 'unknown') chip.classList.add('opacity-60'); + chip.setAttribute('title', chip.dataset.baseTitle + (state === 'match' + ? ' — matches your tuning' + : state === 'mismatch' ? ' — needs a retune' + : ' — no tuning data, not checked')); + // Never signal by colour alone. + const mark = state === 'mismatch' ? ' ⚠' : state === 'unknown' ? ' ?' : ''; + let m = chip.querySelector('[data-tuning-mark]'); + if (!m) { + m = document.createElement('span'); + m.setAttribute('data-tuning-mark', ''); + chip.appendChild(m); + } + m.textContent = mark; + } + + function tuningSummaryHtml(results) { + const total = results.length; + if (!total) return ''; + const mism = results.filter((r) => r.state === 'mismatch').length; + const unk = results.filter((r) => r.state === 'unknown').length; + const box = 'mb-4 rounded-lg border px-3 py-2 text-sm flex flex-wrap items-center gap-x-3 gap-y-2 '; + if (!mism) { + return '
They stay in your library — only this playlist changes, and you can add them back.
'; + const ok = (typeof window.uiConfirm === 'function') + ? await window.uiConfirm({ + title: 'Remove mismatched songs?', html: msg, + confirmText: 'Remove ' + doomed.length, cancelText: 'Cancel', danger: true, + }) + : window.confirm('Remove ' + doomed.length + ' song(s) from "' + pl.name + '"?\n\n' + + doomed.map((s) => '• ' + (s.title || s.filename)).join('\n') + + '\n\nThey stay in your library.'); + if (!ok) return; + for (const s of doomed) { + await fetch('/api/playlists/' + pid + '/songs/' + encodeURIComponent(s.filename), + { method: 'DELETE' }); + } + rerender(); + }); + } + function songRow(s, opts) { opts = opts || {}; const handle = opts.draggable ? '⠿' : ''; + // The chip carries its own tuning so the post-paint check can colour it + // in place (green = play it now, amber = needs a retune, dimmed ? = + // couldn't tell) without re-rendering the list. const tuning = s.tuning_name - ? '' + esc(s.tuning_name) + '' : ''; + ? '' + esc(s.tuning_name) + '' : ''; // ── Curated-album slot extras (P6) — mixes/saved emit none of this ── // A slot plays its RESOLVED chart (data-play-fn: the pinned file, or // the work's current keeper when the pinned file is gone) with its @@ -272,6 +445,9 @@ '' + '' + meter + + // Filled in after paint by applyTuningCheck (async, feature-detected) + // — stays empty when the host exposes no tuning perspective. + (pl.songs.length ? '' : '') + (pl.songs.length ? 'Empty — add songs from the library' + (isAlbum ? ' (the ⋮ menu or the batch bar\'s "Add to playlist")' : '') + '.
') + @@ -321,6 +497,9 @@ }); const listEl = root.querySelector('#v3-pl-songs'); if (listEl) wireSongRows(listEl, pid, () => renderPlaylistDetail(pid)); + // Post-paint so the list is interactive immediately; a per-song coverage + // call can await the tuner plugin's settings fetch. + if (listEl) applyTuningCheck(root, pl, pid, () => renderPlaylistDetail(pid)); // Album slot editor (▾ per row): pick the slot's chart + arrangement. if (listEl && isAlbum) { listEl.querySelectorAll('li[data-fn]').forEach((li) => { diff --git a/tests/js/v3_playlist_tuning_check.test.js b/tests/js/v3_playlist_tuning_check.test.js new file mode 100644 index 0000000..a66368a --- /dev/null +++ b/tests/js/v3_playlist_tuning_check.test.js @@ -0,0 +1,304 @@ +// The playlist tuning check (static/v3/playlists.js). +// +// A bass-playing tester built playlists grouped BY TUNING so a practice run +// needs no retune, using a library filter that only ever looked at the guitar +// tuning. Those playlists still hold songs he can't play without stopping. The +// check flags them; it must never quietly edit the playlist, and — the part +// that decides whether he trusts it — it must not call a song "wrong tuning" +// when it simply couldn't work the song out. +// +// The real functions are lifted out of playlists.js and run in a vm (the module +// is a browser IIFE with no export surface, and there is no jsdom here). No +// re-implementation: if the source changes, these tests run the changed code. + +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +const PL_JS = path.join(__dirname, '..', '..', 'static', 'v3', 'playlists.js'); +const TUNING_JS = path.join(__dirname, '..', '..', 'static', 'js', 'tuning-display.js'); +const TUNER_JS = path.join(__dirname, '..', '..', 'plugins', 'tuner', 'screen.js'); + +const PL_SRC = fs.readFileSync(PL_JS, 'utf8'); + +function extractBlock(src, startMarker) { + const start = src.indexOf(startMarker); + if (start === -1) throw new Error(`extractBlock: '${startMarker}' not found`); + const openBrace = src.indexOf('{', start); + let depth = 1; + let i = openBrace + 1; + while (i < src.length && depth > 0) { + const ch = src[i]; + if (ch === '{') depth++; + else if (ch === '}') depth--; + i++; + } + if (depth !== 0) throw new Error(`extractBlock: unbalanced braces after '${startMarker}'`); + return src.slice(start, i); +} + +// The REAL offset parser the checker calls through window.parseRawTuningOffsets. +function loadParseRawTuningOffsets() { + const body = fs.readFileSync(TUNING_JS, 'utf8').replace(/^export /gm, ''); + const sandbox = { window: { feedBack: {} }, exports: {} }; + vm.createContext(sandbox); + vm.runInContext(body + '\nexports.parseRawTuningOffsets = parseRawTuningOffsets;', sandbox); + return sandbox.exports.parseRawTuningOffsets; +} + +// Build a sandbox holding the real checker functions, over a caller-supplied +// window (so each test controls the host capabilities and the coverage stub +// boundary). `coverage` stands in for the tuner plugin's coverageReport — a +// genuinely external collaborator, not the subject under test; the contract +// test at the bottom pins its report shape so these fixtures can't drift. +function loadChecker(opts) { + opts = opts || {}; + const calls = []; + const window = { + parseRawTuningOffsets: loadParseRawTuningOffsets(), + feedBack: opts.noWorkingTuning ? {} : { workingTuning: { get: () => ({ instrument: 'bass' }) } }, + _tunerAutoOpen: opts.noCoverage ? undefined : { + coverageReport: async (info) => { + calls.push(info); + if (opts.coverage) return opts.coverage(info); + throw new Error('no coverage fixture supplied'); + }, + }, + }; + const sandbox = { window, exports: {} }; + vm.createContext(sandbox); + vm.runInContext( + extractBlock(PL_SRC, 'function rowTuningForCheck(') + '\n' + + extractBlock(PL_SRC, 'function tuningStateFromReport(') + '\n' + + extractBlock(PL_SRC, 'async function checkPlaylistTuning(') + '\n' + + extractBlock(PL_SRC, 'function tuningSummaryHtml(') + '\n' + + 'exports.rowTuningForCheck = rowTuningForCheck;\n' + + 'exports.tuningStateFromReport = tuningStateFromReport;\n' + + 'exports.checkPlaylistTuning = checkPlaylistTuning;\n' + + 'exports.tuningSummaryHtml = tuningSummaryHtml;\n', + sandbox + ); + return { ...sandbox.exports, calls }; +} + +// Report shapes exactly as plugins/tuner/screen.js documents and returns them. +const REPORT_COVERED = { covered: true, retune: [], reference: false, cantCover: false }; +const REPORT_RETUNE = { covered: false, retune: [{ from: 'E', to: 'D' }], reference: false, cantCover: false }; +const REPORT_REFERENCE = { covered: false, retune: [], reference: true, cantCover: false }; +const REPORT_CANT_COVER = { covered: false, retune: [], reference: false, cantCover: true }; +// The "I couldn't work it out" report — the tuner's `none` bail-out. Byte-for-byte +// a not-covered report with no reason attached. +const REPORT_UNKNOWN = { covered: false, retune: [], reference: false, cantCover: false }; + +// The checker runs inside the vm, so the arrays it returns belong to another +// realm and would fail deepStrictEqual's prototype check. Copy into host arrays. +const plain = (a) => Array.from(a); + +const song = (over) => Object.assign( + { filename: 'a.sloppak', title: 'A', tuning_name: 'E Standard', tuning_offsets: '0 0 0 0 0 0', bass_only: false }, + over +); + +// ── The unknown-vs-mismatch distinction ───────────────────────────────────── + +test('a covered report is a match', () => { + const { tuningStateFromReport } = loadChecker(); + assert.equal(tuningStateFromReport(REPORT_COVERED), 'match'); +}); + +test('a not-covered report WITH a reason is a mismatch', () => { + const { tuningStateFromReport } = loadChecker(); + assert.equal(tuningStateFromReport(REPORT_RETUNE), 'mismatch'); + assert.equal(tuningStateFromReport(REPORT_REFERENCE), 'mismatch'); + assert.equal(tuningStateFromReport(REPORT_CANT_COVER), 'mismatch'); +}); + +test('a not-covered report with NO reason is unknown, not a mismatch', () => { + // This is the whole trust argument. The tuner returns this identical shape + // when settings/tuner data are missing. Treating it as "wrong tuning" (which + // the library grid's chip decorator does) would put a false ⚠ on songs that + // are perfectly playable, on a playlist the user curated by hand. + const { tuningStateFromReport } = loadChecker(); + assert.equal(tuningStateFromReport(REPORT_UNKNOWN), 'unknown'); +}); + +test('a null/absent report is unknown', () => { + const { tuningStateFromReport } = loadChecker(); + assert.equal(tuningStateFromReport(null), 'unknown'); + assert.equal(tuningStateFromReport(undefined), 'unknown'); +}); + +// ── Round-tripping a whole playlist ───────────────────────────────────────── + +test('each song is scored and reported in playlist order', async () => { + const byFile = { + 'match.sloppak': REPORT_COVERED, + 'bad.sloppak': REPORT_RETUNE, + 'huh.sloppak': REPORT_UNKNOWN, + }; + const songs = [ + song({ filename: 'match.sloppak', title: 'Match' }), + song({ filename: 'bad.sloppak', title: 'Bad', tuning_offsets: '-2 -2 -2 -2 -2 -2' }), + song({ filename: 'huh.sloppak', title: 'Huh', tuning_offsets: '-1 0 0 0 0 0' }), + ]; + // Resolve the fixture from the offsets the checker actually passed, so the + // mapping can't silently drift out of playlist order. + const byOffsets = new Map(songs.map((s) => [s.tuning_offsets.replace(/\s+/g, ','), byFile[s.filename]])); + const checker = loadChecker({ coverage: async (info) => byOffsets.get(info.tuning.join(',')) }); + const out = await checker.checkPlaylistTuning(songs); + assert.deepEqual(plain(out.map((r) => r.state)), ['match', 'mismatch', 'unknown']); + assert.deepEqual(plain(out.map((r) => r.song.filename)), songs.map((s) => s.filename)); +}); + +test('a song with no usable tuning data is unknown WITHOUT consulting coverage', async () => { + // Adversarial payloads: empty, whitespace, a non-numeric name with no + // offsets, and a garbage offsets string. None of these can be scored, and + // asking coverage about them would invite a bogus not-covered → false ⚠. + const checker = loadChecker({ coverage: async () => REPORT_RETUNE }); + const out = await checker.checkPlaylistTuning([ + song({ filename: 'a', tuning_offsets: '', tuning_name: '' }), + song({ filename: 'b', tuning_offsets: ' ', tuning_name: ' ' }), + song({ filename: 'c', tuning_offsets: '', tuning_name: 'E Standard' }), + song({ filename: 'd', tuning_offsets: 'not offsets', tuning_name: 'x' }), + song({ filename: 'e', tuning_offsets: null, tuning_name: null }), + ]); + assert.deepEqual(plain(out.map((r) => r.state)), ['unknown', 'unknown', 'unknown', 'unknown', 'unknown']); + assert.equal(checker.calls.length, 0, 'coverage must not be asked about unscoreable rows'); +}); + +test('a coverage call that throws degrades to unknown, not mismatch', async () => { + const checker = loadChecker({ coverage: async () => { throw new Error('tuner exploded'); } }); + const out = await checker.checkPlaylistTuning([song({})]); + assert.deepEqual(plain(out.map((r) => r.state)), ['unknown']); +}); + +test('a bass-only chart is scored against bass base pitches', async () => { + // Otherwise a 4-string bass tuning read as guitar can false-match — the + // cross-instrument confusion this whole feature exists to undo. + const checker = loadChecker({ coverage: async () => REPORT_COVERED }); + await checker.checkPlaylistTuning([ + song({ filename: 'bass', tuning_offsets: '0 0 0 0', bass_only: true }), + song({ filename: 'gtr', tuning_offsets: '0 0 0 0 0 0', bass_only: false }), + ]); + assert.deepEqual(checker.calls.map((c) => c.arrangement), ['Bass', 'Lead']); + assert.deepEqual(checker.calls.map((c) => c.stringCount), [4, 6]); +}); + +test('the check stays silent when the host exposes no tuning perspective', async () => { + // No working-tuning capability, or no tuner coverage → null, and the caller + // renders the playlist exactly as before. Guessing "guitar" here would + // reproduce the original bug in a new place. + for (const opts of [{ noWorkingTuning: true }, { noCoverage: true }]) { + const checker = loadChecker(Object.assign({ coverage: async () => REPORT_COVERED }, opts)); + assert.equal(await checker.checkPlaylistTuning([song({})]), null); + } +}); + +test('an empty playlist yields an empty result, not a crash', async () => { + const checker = loadChecker({ coverage: async () => REPORT_COVERED }); + assert.deepEqual(plain(await checker.checkPlaylistTuning([])), []); + assert.deepEqual(plain(await checker.checkPlaylistTuning(null)), []); +}); + +// ── The summary ───────────────────────────────────────────────────────────── + +test('the summary counts mismatches against the playlist total', () => { + const { tuningSummaryHtml } = loadChecker(); + const results = [ + { state: 'mismatch' }, { state: 'mismatch' }, { state: 'mismatch' }, + ...Array(21).fill({ state: 'match' }), + ]; + const html = tuningSummaryHtml(results); + assert.match(html, /3<\/strong> of 24 songs aren't in your tuning/); +}); + +test('unknowns are reported separately from mismatches and never counted as them', () => { + const { tuningSummaryHtml } = loadChecker(); + const html = tuningSummaryHtml([{ state: 'mismatch' }, { state: 'unknown' }, { state: 'match' }]); + assert.match(html, /1<\/strong> of 3 songs aren't in your tuning/); + assert.match(html, /1 couldn't be checked/); + assert.match(html, /left alone/); +}); + +test('an all-unknown playlist makes no mismatch claim and offers no removal', () => { + const { tuningSummaryHtml } = loadChecker(); + const html = tuningSummaryHtml([{ state: 'unknown' }, { state: 'unknown' }]); + assert.doesNotMatch(html, /aren't in your tuning/); + assert.doesNotMatch(html, /v3-pl-tune-remove/); + assert.match(html, /2 couldn't be checked/); +}); + +test('a clean playlist offers no filter and no removal button', () => { + const { tuningSummaryHtml } = loadChecker(); + const html = tuningSummaryHtml([{ state: 'match' }, { state: 'match' }]); + assert.match(html, /All 2 songs are in your tuning/); + assert.doesNotMatch(html, /v3-pl-tune-only/); + assert.doesNotMatch(html, /v3-pl-tune-remove/); +}); + +test('an empty playlist renders no summary at all', () => { + const { tuningSummaryHtml } = loadChecker(); + assert.equal(tuningSummaryHtml([]), ''); +}); + +// ── Read-only / explicit-action guarantees (source-level) ─────────────────── + +test('the check itself never mutates the playlist', () => { + // checkPlaylistTuning and its helpers must contain no write verbs. The only + // DELETE in the module's tuning path is inside the confirmed removal. + const fns = ['function rowTuningForCheck(', 'function tuningStateFromReport(', + 'async function checkPlaylistTuning(', 'function tuningSummaryHtml(']; + for (const marker of fns) { + const body = extractBlock(PL_SRC, marker); + assert.doesNotMatch(body, /DELETE|jsend\(|method:/, + marker + ' must not mutate the playlist'); + } +}); + +test('bulk removal names every song and is confirmed before any DELETE', () => { + const body = extractBlock(PL_SRC, 'async function applyTuningCheck('); + // The confirm is built from the doomed titles … + assert.match(body, /doomed\.map\(\(s\) => '