From a3b5b43777c9d0a2b876d9bd4c5e3b3f0f555a11 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 9 Sep 2026 09:19:05 -0400 Subject: [PATCH 1/2] fix(database): preserve playlist phantoms across a stale audio_files retire Retiring a stale audio_files dropped every playlist entry to an empty row: ON DELETE SET NULL ran before the phantom_* columns were filled, whereas the manual rescan path populates them first. Run the same phantom population inside the retire transaction, before the drop, only when audio_files is among the tables going, so ResolvePhantomTracksAfterScan can re-link the entries. Closes #183 --- backend/database/playlistphantoms.go | 125 +++++++++++++++++++ backend/database/staleshape.go | 55 +++++++++ backend/database/staleshape_test.go | 177 +++++++++++++++++++++++++++ backend/library/rescan.go | 35 ++---- 4 files changed, 364 insertions(+), 28 deletions(-) create mode 100644 backend/database/playlistphantoms.go diff --git a/backend/database/playlistphantoms.go b/backend/database/playlistphantoms.go new file mode 100644 index 0000000..1d177c5 --- /dev/null +++ b/backend/database/playlistphantoms.go @@ -0,0 +1,125 @@ +package database + +import ( + "context" + "database/sql" + "fmt" + "log/slog" +) + +// Preserving a playlist entry across the loss of its track is two +// statements, not one, and the split is not tidiness -- it is what +// makes the important half work in the situation that needs it most. +// +// `playlist_tracks.audio_file_id` is ON DELETE SET NULL, so an entry +// outlives its file as an id-less row that says nothing about what the +// user put in the playlist. The phantom_* columns carry the answer +// across and ResolvePhantomTracksAfterScan re-links them afterwards -- +// but only if something fills them *before* the rows go. +// +// The two halves are not equally important and are not equally +// available: +// +// - **phantom_file_path is the one that matters.** +// ResolvePhantomTracksAfterScan matches it against +// `audio_files.file_path`, so without it an entry can never be +// re-linked and the playlist is empty for good. It comes straight +// off `audio_files`, whose `file_path` is the table's natural key +// and has been present in every shape it has ever had -- including +// the pre-013 stub of `(id, file_path, recording_id)`. +// - The rest is *display* for a phantom entry before a rescan +// re-links it, and it comes from the `track_metadata` view, which +// is the one definition of a track row and not worth restating. +// +// Reading the view is what cannot be relied on here, and that is the +// whole reason for the split. This runs *before* applySchema, which is +// precisely the moment the schema is inconsistent: the view is whatever +// the last launch's schema declared, while `audio_files` is whatever +// the launch before that left behind. A view over columns the table no +// longer has is not merely empty -- `pragma_table_info` on it *errors*, +// and so does selecting from it. `cmd/indexbuild`'s fixture is exactly +// that shape and is what caught this. +// +// COALESCE keeps an existing phantom value in both halves: an entry +// already phantom is one whose file went missing in an earlier pass, +// and its recorded metadata is the only copy left. Overwriting that +// from a NULL join erases the rows this exists to protect. +const ( + preservePhantomPathSQL = ` + UPDATE playlist_tracks + SET phantom_file_path = COALESCE(phantom_file_path, ( + SELECT af.file_path FROM audio_files af + WHERE af.id = playlist_tracks.audio_file_id + )) + WHERE audio_file_id IS NOT NULL + ` + + preservePhantomDisplaySQL = ` + UPDATE playlist_tracks + SET + phantom_title = COALESCE(phantom_title, ( + SELECT tm.title FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_artist = COALESCE(phantom_artist, ( + SELECT tm.artist_name FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_album = COALESCE(phantom_album, ( + SELECT tm.album FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_duration_ms = COALESCE(phantom_duration_ms, ( + SELECT af.length_milliseconds FROM audio_files af + WHERE af.id = playlist_tracks.audio_file_id + )), + phantom_genre = COALESCE(phantom_genre, ( + SELECT tm.genre FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )), + phantom_cover_art_path = COALESCE(phantom_cover_art_path, ( + SELECT tm.cover_art_path FROM track_metadata tm + WHERE tm.id = playlist_tracks.audio_file_id + )) + WHERE audio_file_id IS NOT NULL + ` +) + +// PreservePlaylistPhantoms records every linked playlist entry's track +// metadata on the entry itself, so the entry survives the rows being +// deleted underneath it. +// +// Every path that empties `audio_files` must call this first, inside +// the same transaction as the delete. There are two such paths and +// they had drifted: the full rescan in backend/library did this and the +// stale-shape retire in this package did not, so the *documented* +// repair ("delete and rescan") preserved playlists while the automatic +// one that exists to spare the user that work silently emptied them. +// +// The display half is skipped, with a warning, when `track_metadata` +// cannot answer -- see the note above. Skipping it costs a phantom +// entry its title until a rescan re-links it; skipping the path half +// would cost the entry outright, so that one is an error. +func PreservePlaylistPhantoms( + ctx context.Context, tx *sql.Tx, logger *slog.Logger, +) error { + if _, err := tx.ExecContext(ctx, preservePhantomPathSQL); err != nil { + return fmt.Errorf( + "could not preserve playlist track file paths: %w", err, + ) + } + + if _, err := tx.ExecContext(ctx, preservePhantomDisplaySQL); err != nil { + // A failed statement does not roll back a SQLite transaction, + // so the path half above stands and the entries remain + // re-linkable. + logger.Warn( + "could not record display metadata for playlist entries; "+ + "they will be re-linked by the next scan but read as "+ + "unknown until then", + "err", err, + ) + } + + return nil +} diff --git a/backend/database/staleshape.go b/backend/database/staleshape.go index 41022ac..1948057 100644 --- a/backend/database/staleshape.go +++ b/backend/database/staleshape.go @@ -245,6 +245,13 @@ func dropDeferred( ctx context.Context, db *sql.DB, logger *slog.Logger, drop map[string]string, ) error { + // Asked before the transaction opens, because the answer is about + // which tables are live and that cannot change underneath us here. + preserve, err := shouldPreservePhantoms(ctx, db, drop) + if err != nil { + return err + } + tx, err := db.BeginTx(ctx, nil) if err != nil { return fmt.Errorf("could not begin the retire transaction: %w", err) @@ -256,6 +263,22 @@ func dropDeferred( return fmt.Errorf("could not defer foreign keys: %w", err) } + // Before any drop, so every entry still has a track to read. It is + // in this transaction rather than beside it because the preservation + // and the delete have to succeed or fail together: a commit that + // dropped the files without the phantoms is the bug, and a commit + // that wrote phantoms without dropping anything is a lie about rows + // that are still there. + if preserve { + logger.Info( + "preserving playlist entries across the retire of audio_files", + ) + + if err := PreservePlaylistPhantoms(ctx, tx, logger); err != nil { + return err + } + } + // Sorted, so a failure is reproducible. Map order is random, and a // bug that depends on which table happens to go first reproduces on // one run in three and passes review on the other two -- which is @@ -283,6 +306,38 @@ func dropDeferred( return nil } +// shouldPreservePhantoms reports whether this retire is about to take +// `audio_files` out from under the playlists. +// +// The `playlist_tracks` check is not defensive padding. This runs +// *before* applySchema, which is the moment the schema is by definition +// mid-repair, and the preservation reads a table it does not drop. A +// database old enough not to have it would otherwise fail here, and +// failing here means the app does not open at all -- while nothing is +// lost by skipping, since an absent `playlist_tracks` holds no +// playlists to save. +// +// It deliberately does *not* ask after `track_metadata`. Whether that +// view can answer is PreservePlaylistPhantoms's own business, because a +// view broken against an older `audio_files` is a state this function +// cannot detect without hitting the same error it is trying to avoid: +// pragma_table_info on such a view errors rather than reporting no +// columns. +func shouldPreservePhantoms( + ctx context.Context, db *sql.DB, drop map[string]string, +) (bool, error) { + if _, going := drop["audio_files"]; !going { + return false, nil + } + + cols, err := liveColumns(ctx, db, "playlist_tracks") + if err != nil { + return false, err + } + + return len(cols) > 0, nil +} + // staleReason reports why a live table disagrees with its declaration, // or "" when it agrees. A column the live table does not have is the // additive case; a column whose declared type changed is the one an diff --git a/backend/database/staleshape_test.go b/backend/database/staleshape_test.go index 990cfe9..b607bff 100644 --- a/backend/database/staleshape_test.go +++ b/backend/database/staleshape_test.go @@ -497,3 +497,180 @@ func TestParseCreateTablesReadsTheRealSchema(t *testing.T) { } } } + +// TestRetiringAudioFilesKeepsPlaylistContents is the symptom this +// repair exists for: a playlist survived the retire as a row count and +// nothing else. +// +// TestRetiringOwnedTablesDoesNotDangle already asserts the entry does +// not keep a stale id, which is the *dangerous* half. It is satisfied +// just as well by an entry that says nothing at all, which is the +// half that quietly emptied every playlist -- so this asserts what the +// entry still knows, and specifically phantom_file_path, because that +// is the column ResolvePhantomTracksAfterScan matches back against +// audio_files.file_path. +// +// Note the seed drops `comment`, not `artist_credit`: the mutation has +// to leave `track_metadata` standing, since a real launch reaches the +// retire with the view the previous launch created. A test that drops +// the view first is testing the skip path, not this one. +func TestRetiringAudioFilesKeepsPlaylistContents(t *testing.T) { + ctx := context.Background() + db := openRaw(t, t.TempDir()) + + if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil { + t.Fatalf("pragma: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + if _, err := db.ExecContext(ctx, ` + INSERT INTO playlists (id, name) VALUES (1, 'keepme'); + INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music'); + INSERT INTO artists (id, name) VALUES (3, 'Aurora Fields'); + INSERT INTO cover_art (id, file_path, mime_type) + VALUES (9, 'covers/7.jpg', 'image/jpeg'); + INSERT INTO genres (id, name) VALUES (5, 'Ambient'); + INSERT INTO albums (id, name, artist_id, cover_art_id) + VALUES (4, 'Tideline', 3, 9); + INSERT INTO audio_files + (id, file_path, file_type_id, length_milliseconds, + title, artist_credit, artist_id, album_id) + VALUES (7, '/music/a.flac', 1, 1000, + 'Slack Water', 'Aurora Fields', 3, 4); + INSERT INTO file_genres (audio_file_id, genre_id) VALUES (7, 5); + INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (1, 7, 0); + ALTER TABLE audio_files DROP COLUMN comment; + `); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf("retire: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + var ( + path, title, artist, album, genre, cover sql.NullString + duration sql.NullInt64 + ) + + if err := db.QueryRowContext(ctx, ` + SELECT phantom_file_path, phantom_title, phantom_artist, + phantom_album, phantom_duration_ms, phantom_genre, + phantom_cover_art_path + FROM playlist_tracks WHERE playlist_id = 1 + `).Scan(&path, &title, &artist, &album, &duration, &genre, &cover); err != nil { + t.Fatalf("read the surviving entry: %v", err) + } + + // The one that matters: without it the entry can never be re-linked + // by the rescan the retire itself provokes. + if path.String != "/music/a.flac" { + t.Fatalf( + "phantom_file_path is %q, want %q -- the playlist entry "+ + "cannot be re-linked and the playlist is empty for good", + path.String, "/music/a.flac", + ) + } + + if title.String != "Slack Water" { + t.Errorf("phantom_title is %q, want %q", title.String, "Slack Water") + } + + if artist.String != "Aurora Fields" { + t.Errorf("phantom_artist is %q, want %q", artist.String, "Aurora Fields") + } + + if album.String != "Tideline" { + t.Errorf("phantom_album is %q, want %q", album.String, "Tideline") + } + + if duration.Int64 != 1000 { + t.Errorf("phantom_duration_ms is %d, want 1000", duration.Int64) + } + + if genre.String != "Ambient" { + t.Errorf("phantom_genre is %q, want %q", genre.String, "Ambient") + } + + if cover.String != "covers/7.jpg" { + t.Errorf("phantom_cover_art_path is %q, want %q", cover.String, "covers/7.jpg") + } +} + +// TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer is the case +// that broke cmd/indexbuild: this repair runs *before* applySchema, so +// `track_metadata` is whatever the last launch declared while +// `audio_files` is whatever the launch before that left behind, and a +// view over columns the table no longer has does not read as empty -- +// it errors. +// +// The pre-013 stub shape below is the real one that fixture carries. +// What must survive is phantom_file_path, because `file_path` is the +// table's natural key and has been in every shape it ever had; the +// display columns are allowed to be absent, and the open must not fail. +func TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer(t *testing.T) { + ctx := context.Background() + db := openRaw(t, t.TempDir()) + + if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil { + t.Fatalf("pragma: %v", err) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + // The rows go in *after* the reshape: dropping audio_files with + // foreign keys on would fire the ON DELETE SET NULL and null the + // entry this test is about, which would pass for the wrong reason. + if _, err := db.ExecContext(ctx, ` + DROP TABLE audio_files; + CREATE TABLE audio_files ( + id INTEGER PRIMARY KEY, + file_path TEXT NOT NULL UNIQUE, + recording_id INTEGER + ); + INSERT INTO playlists (id, name) VALUES (1, 'keepme'); + INSERT INTO audio_files (id, file_path) VALUES (7, '/music/a.flac'); + INSERT INTO playlist_tracks (playlist_id, audio_file_id, position) + VALUES (1, 7, 0); + `); err != nil { + t.Fatalf("seed: %v", err) + } + + // The symptom this guards: the repair must not turn a recoverable + // database into one the app refuses to open. + if err := retireStaleTables(ctx, db, testLogger()); err != nil { + t.Fatalf( + "the retire failed on a view it could not read, so the app "+ + "would not open at all: %v", err, + ) + } + + if err := applySchema(ctx, db); err != nil { + t.Fatalf("applySchema: %v", err) + } + + var path sql.NullString + if err := db.QueryRowContext(ctx, + "SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = 1", + ).Scan(&path); err != nil { + t.Fatalf("read the surviving entry: %v", err) + } + + if path.String != "/music/a.flac" { + t.Fatalf( + "phantom_file_path is %q, want %q -- the display half being "+ + "unavailable must not cost the entry its one re-link key", + path.String, "/music/a.flac", + ) + } +} diff --git a/backend/library/rescan.go b/backend/library/rescan.go index 5fd75b1..456b0f6 100644 --- a/backend/library/rescan.go +++ b/backend/library/rescan.go @@ -8,6 +8,7 @@ import ( "time" "yellowjacket/backend/coverart" + "yellowjacket/backend/database" ) var errNoLibrariesConfigured = errors.New( @@ -137,34 +138,12 @@ func (l *Library) clearLibraryTables() error { // metadata for all linked tracks before audio_files are deleted. // ON DELETE SET NULL will null out audio_file_id, converting them // to phantoms that ResolvePhantomTracksAfterScan can re-link. - if _, err := tx.ExecContext(l.ctx, ` - UPDATE playlist_tracks - SET - phantom_title = COALESCE(phantom_title, ( - SELECT tm.title FROM track_metadata tm - WHERE tm.id = playlist_tracks.audio_file_id - )), - phantom_artist = COALESCE(phantom_artist, ( - SELECT tm.artist_name FROM track_metadata tm - WHERE tm.id = playlist_tracks.audio_file_id - )), - phantom_album = COALESCE(phantom_album, ( - SELECT tm.album FROM track_metadata tm - WHERE tm.id = playlist_tracks.audio_file_id - )), - phantom_duration_ms = COALESCE(phantom_duration_ms, ( - SELECT af.length_milliseconds FROM audio_files af - WHERE af.id = playlist_tracks.audio_file_id - )), - phantom_file_path = COALESCE(phantom_file_path, ( - SELECT af.file_path FROM audio_files af - WHERE af.id = playlist_tracks.audio_file_id - )) - WHERE audio_file_id IS NOT NULL - `); err != nil { - return fmt.Errorf( - "could not preserve playlist track metadata: %w", err, - ) + // + // Shared with the stale-shape retire in backend/database, which is + // the other path that empties this table and which did not do this + // (#183): the statement lives there so the two cannot drift again. + if err := database.PreservePlaylistPhantoms(l.ctx, tx, l.logger); err != nil { + return err } // Phase 2: the files. file_genres cascades with them. -- 2.54.0 From 68e7edb8c9519b65272142e286bccf8f39a16775 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 9 Sep 2026 09:19:36 -0400 Subject: [PATCH 2/2] feat(database): listening-events log with skip counters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace play_history with listening_events — one row per track exit, kind (complete/play/skip) plus raw position/duration — and add skip_count/last_skipped to audio_files beside play_count/last_played. The classifier that writes these lands later (plan 021); this is the schema it records into. Also drop the dead queue.source_playlist_id column and remove the stale references to the squashed migration chain in download_*.sql and tagging_items.sql, declaring the missing download-request indexes inline. --- .../plans/active/021-listening-accounting.md | 263 ++++++++++++++++++ backend/database/database_test.go | 38 ++- backend/database/sql/schemas/audio_files.sql | 6 + .../database/sql/schemas/download_items.sql | 8 +- .../sql/schemas/download_requests.sql | 19 +- .../database/sql/schemas/listening_events.sql | 42 +++ backend/database/sql/schemas/play_history.sql | 9 - backend/database/sql/schemas/queue.sql | 11 +- .../database/sql/schemas/tagging_items.sql | 22 +- .../database/sql/sqlcgen/audio_files.sql.go | 16 +- backend/database/sql/sqlcgen/models.go | 34 ++- backend/datamap/datamap.go | 9 +- backend/datamap/datamap_test.go | 6 +- backend/queue/playhistory.go | 10 +- 14 files changed, 403 insertions(+), 90 deletions(-) create mode 100644 .planning/plans/active/021-listening-accounting.md create mode 100644 backend/database/sql/schemas/listening_events.sql delete mode 100644 backend/database/sql/schemas/play_history.sql diff --git a/.planning/plans/active/021-listening-accounting.md b/.planning/plans/active/021-listening-accounting.md new file mode 100644 index 0000000..b8370e3 --- /dev/null +++ b/.planning/plans/active/021-listening-accounting.md @@ -0,0 +1,263 @@ +# 021 — Listening accounting: smart plays, skips, and a real history + +**Issue:** none yet — open one before the first edit (tracker is the +source of truth; `./scripts/issue.sh search "skip play count"` comes +back empty as of this writing). + +**Status:** plan — not started. + +**Relates:** play-count rendering (`frontend/src/components/track-list/columns.ts`), +smart playlists (`backend/smartplaylist/`), the event contract +(`TrackPlayCountChanged`), and any future Wrapped / "minutes listened" +surface. + +--- + +## What exists now + +Three facts, all load-bearing. + +**A "play" is recorded only on a natural finish.** `recordPlay` +(`backend/queue/playhistory.go:9`) is called from exactly one place — +`OnPlaybackFinished` (`backend/queue/handlers.go:14`), and only when +`srcErr == nil`. A track the user skips past at 90% is *not* a play; +neither is one they pause at 60% and abandon. `play_count` / +`last_played` on `audio_files` reflect "finished to the end," nothing +more. + +**There is no skip concept at all.** Skipping is indistinguishable +from a natural finish, a pause, or a shutdown. Nothing records "the +user rejected this track," so no downstream feature (smart playlists, +shuffle, the revisit shelf, a future skip-rate heuristic) can ask +about it. + +**`play_history` is a write-only log.** It holds +`(audio_file_id, played_at)` and nothing reads it — no sqlc query +touches it, no `PlayHistory` read path exists. Its only recorded +purpose is the timestamps a future "minutes listened over time" +feature would need. It is classified `Authored, Cascade` in +`backend/datamap/datamap.go:272` ("Listening history"). + +So the gaps are: (1) skips are invisible, and (2) "played" is +under-counted — the opposite of the usual over-counting fear. The +scrobble intuition (count a play once `min(50%, 4:00)` has been +*heard*, independent of how it ends) is the fix for both. + +--- + +## What we're building + +A single classification of every track *exit*, plus one row per exit in +a listening log, plus the existing denormalized `play_count` / +`last_played` updated to match the new meaning. Three exit kinds: + +| kind | condition | +|---|---| +| `complete` | reached natural end, **or** abandoned with `remaining <= tail` | +| `play` | heard `>= playThreshold`, abandoned before the tail | +| `skip` | user moved to a *different* track before `playThreshold` | + +Not counted, not any kind: decode failure, pause/stop/shutdown before +the threshold, and tracks shorter than `minTrackLength`. + +### The thresholds — named judgements, one file + +Follow the `PreviousRestartThreshold` precedent (`backend/queue/queue.go:28`, +a bare `const` with a comment). A new `backend/queue/listen.go` (or a +tiny `backend/listencount` package) declares: + +```go +const ( + // A track this short is deliberated jingle / interstitial and is + // never counted, either way. + minTrackLength = 30 * time.Second + // The scrobble rule: half the track, or four minutes, whichever + // comes first (Last.fm / ListenBrainz). + playThresholdMax = 4 * time.Minute + // "Finished enough": within 15s of the end, or the last 10%, + // whichever is larger. A 10:00 ambient track gets a 60s fade + // window; a 2:00 pop song gets 15s. + tailWindowFloor = 15 * time.Second + tailWindowFraction = 0.10 +) + +func playThreshold(d time.Duration) time.Duration { + return min(d/2, playThresholdMax) +} +func tailWindow(d time.Duration) time.Duration { + return max(d/10, tailWindowFloor) +} +``` + +Classification is a pure function of `(reason, position, duration)` and +*therefore unit-testable without a player*: + +```go +func classify(reason ExitReason, pos, dur time.Duration) Kind +``` + +`ExitReason` is `finished | skipped | failed | abandoned`. `skipped` +means the queue moved to a different track by user action (Next, +Previous past the restart threshold, PlayIndex, queue replacement, +select-from-a-list). `failed` is the decode-error path. `abandoned` is +pause/stop/unload/shutdown — and in v1 is a no-op (see open question 3). + +**"Heard" is approximated by the position at exit.** We read +`player.CurrentPositionSeconds()` at the moment of the transition, not +an accumulated listen-time ledger. A user who seeks to 80% and listens +5 seconds reads as "heard 80%." That is deliberately accepted for v1: +it is how most players actually behave, it is drastically simpler, and +the failure mode ("counted a track you skimmed as played") is mild and +exactly what the scrobble threshold already forgives. Written down +because "position is not listen time" is the one assumption that will +look like a bug if it is not. + +**Fires once per listen.** Leaving a track already leaves it; the +`chainID` guard in `player.onPlaybackFinished` (`backend/player/player.go:633`) +already swallows a stale finish callback, and a transition advances +`currentIndex` past the finished track. The classifier needs the same +guard so a Next-then-stale-finish cannot produce two rows. Key it on the +`(audioFileID, chainID)` the transition was about. + +--- + +## Schema — resolved: fresh design, no migration + +The A/B migration agonizing is moot. This app has two users and both +are devs, and play counts are explicitly not worth preserving yet — so +the schema is written as if listening accounting had been designed in +from the start, and the existing two databases rebuild what they need +(see below). There is no migration step and none is re-introduced. + +**`play_history` is renamed to `listening_events`** and grows the three +kinds, plus the raw position/duration the classification was made from: + +```sql +CREATE TABLE IF NOT EXISTS listening_events ( + id INTEGER PRIMARY KEY, + audio_file_id INTEGER NOT NULL, + kind TEXT NOT NULL DEFAULT 'complete' + CHECK (kind IN ('complete','play','skip')), + position_seconds INTEGER NOT NULL DEFAULT 0, + duration_seconds INTEGER NOT NULL DEFAULT 0, + occurred_at DATETIME NOT NULL DEFAULT (datetime('now')), + FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE +); +CREATE INDEX IF NOT EXISTS idx_listening_events_audio_file_id + ON listening_events(audio_file_id); +CREATE INDEX IF NOT EXISTS idx_listening_events_occurred_at + ON listening_events(occurred_at); +``` + +`position_seconds`/`duration_seconds` are kept raw so a future re-tune +of the threshold does not force the events to be re-recorded. `kind` +stays the write-time classification; the raw reading is evidence, not +a second copy of the rule. + +**The counters are denormalized onto `audio_files`** — `skip_count` / +`last_skipped` join the existing `play_count` / `last_played`, because +that is where the hot read path already lives and a log join per track +row is not acceptable. This does grow the MIXED-KIND wart (see the +survey below for the structural answer), but it is the *continuation* of +the existing design, not a new leak: play counts sat on `audio_files` +from before this feature existed. + +**What happens to the two real databases on next launch.** +`listening_events` is a new table, created verbatim. `audio_files` +gains two columns, which `retireStaleTables` treats as a stale Owned +table and rebuilds by rescan — dropping `play_count` / `last_played` / +`tag_status` with it, which is the accepted cost stated in the issue. +`play_history` is gone from the schema and the datamap, so +`obsoleteTables` drops it; its (natural-finish-only) timestamp rows go +with it. Nothing here is wrong on a fresh install, and on the two dev +machines the answer is the documented "delete and rescan." + +--- + +## Wiring: where the classifier is called + +The risk is not the classifier — it is that **every track-replacement +path must classify the outgoing track**, and there are many: `Next`, +`Previous` (past the 3s restart threshold), `PlayIndex`, `playFromStart`, +`SetQueue` / clear-and-play, remove-current, and select-from-a-list. +Miss one and that path silently never records a skip. + +So the classification is centralized in one queue method — + +```go +// leaveCurrent(reason) classifies the track at currentIndex as it is +// about to be replaced, and records exactly one listening event. +// Must be called without q.mu held (it writes to SQLite). +func (q *Queue) leaveCurrent(reason ExitReason) +``` + +— which reads position/duration from the player, calls `classify`, and +emits the play/skip row + `TrackPlayCountChanged` when `kind != skip`. +`OnPlaybackFinished(nil)` routes through `leaveCurrent(finished)`, the +navigation methods route through `leaveCurrent(skipped)` before they +advance, and `recordPlay` becomes the "did a play happen" half of it. + +Because "one path forgot to call it" is the failure mode, a **source +sweep** pins it, on the pattern of `TestNoDirectRuntimeEmits` +(`backend/events/noemit_test.go`) and `TestCatalogCoversSchema`: a test +walks `backend/queue` for assignments to `currentIndex` (and the +`SetQueue` / remove paths) and fails if a mutation site does not sit +adjacent to a `leaveCurrent` call. The sweep is the enforcement; the +central method is the convenience. + +`recordPlay` keeps its existing contract *when a play happens* — +`TrackPlayCountChanged` with `{audioFileId, filePath, playCount, +lastPlayed}` — so the frontend patch path and +`playhistory_test.go` keep passing. A skip emits no per-track event in +v1 (open question 4). + +--- + +## Phases + +1. **The classifier.** `listen.go`: the constants, `playThreshold`, + `tailWindow`, `classify`. Table-driven unit tests covering every + cell of the tristate, the <30s exemption, the tail window on both a + 10:00 and a 2:00 track, and the clip at the 4:00 cap. No I/O. +2. **Schema.** *Done in this session.* `listening_events` replaces + `play_history`; `skip_count` / `last_skipped` added to + `audio_files`; datamap entry and `TestAuthoredCascadesAreDeliberate` + allow-list renamed; `recordPlay` writes `listening_events + ('complete')`. `make generate` run; database / datamap / queue + tests green. +3. **Wiring.** `leaveCurrent`, the navigation/finish/error call sites, + the `fires once per listen` guard, and the source sweep. Extend + `playhistory_test.go` for skip/complete classification through the + queue rather than the pure function. +4. **Smart-playlist field.** `skip_count` (and optionally + `days_since_skipped`) in `smartplaylist.go` field/numeric maps and + the editor's field list, via subquery. A frontend event for skip — + if a UI wants a skip column — follows separately. + +## Verification + +- **Go:** the classifier is pure and exhaustively unit-tested; the + queue wiring is tested in-process with `events.WithSink` + (`backend/queue/emit_test.go` is the model), asserting a Next at 90% + emits a *play*, a Next at 10% emits a *skip and no play*, a natural + finish emits a *complete*. +- **Database:** schema + datamap tests fail-loud on any new or + reclassified table; `database_test.go`'s listening-events round-trip + asserts the new table and the four denormalized counter columns. +- **e2e:** `e2e/specs/play-count.spec.ts` already awaits + `TrackPlayCountChanged`; add the skip case (advance early, assert no + `TrackPlayCountChanged` and a `skip` row via the `__/test/sql` + endpoint if convenient, or via the playlist effect). +- No visual/component tier needed unless a skip column ships (phase 4). + +## Open questions / decisions needed + +1. **Migration mechanism.** Resolved — fresh design, no migration (see the schema section). Play counts are not worth preserving, both users are devs, and `audio_files` / `play_history` rebuild-or-drop on next launch. +2. **"Position is not listen time."** Accept the approximation for v1, + or track accumulated listen seconds (a real ledger on the player) now? +3. **Abandon on shutdown.** A track paused at 70% and then app-killed: + count a `play` (scrobble says heard) or leave it unrecorded? v1 + proposes *unrecorded* — same as today — to keep the write path off + the shutdown critical path. +4. **Skip event to the frontend.** Emit now (parallel to + `TrackPlayCountChanged`) or only when a surface consumes it? diff --git a/backend/database/database_test.go b/backend/database/database_test.go index c8405ec..9339053 100644 --- a/backend/database/database_test.go +++ b/backend/database/database_test.go @@ -662,19 +662,19 @@ func TestSmartPlaylistColumns(t *testing.T) { } // --------------------------------------------------------------------------- -// Migration 10 — play history tracking +// Listening events tracking // --------------------------------------------------------------------------- -func TestPlayHistoryTable(t *testing.T) { +func TestListeningEventsTable(t *testing.T) { t.Parallel() db := NewTestDB(t) - // Verify play_history table exists. + // Verify listening_events table exists. var tableCount int64 tblRows, err := db.QueryContext( - "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='play_history'", + "SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='listening_events'", ) if err != nil { t.Fatalf("query sqlite_master: %v", err) @@ -695,12 +695,14 @@ func TestPlayHistoryTable(t *testing.T) { _ = tblRows.Close() if tableCount != 1 { - t.Errorf("play_history table count = %d, want 1", tableCount) + t.Errorf("listening_events table count = %d, want 1", tableCount) } - // Verify audio_files has play_count and last_played columns. + // Verify audio_files has the denormalized listening counters. hasPlayCount := false hasLastPlayed := false + hasSkipCount := false + hasLastSkipped := false colRows, err := db.QueryContext("PRAGMA table_info(audio_files)") if err != nil { @@ -732,6 +734,14 @@ func TestPlayHistoryTable(t *testing.T) { if name == "last_played" { hasLastPlayed = true } + + if name == "skip_count" { + hasSkipCount = true + } + + if name == "last_skipped" { + hasLastSkipped = true + } } _ = colRows.Close() @@ -744,6 +754,14 @@ func TestPlayHistoryTable(t *testing.T) { t.Error("audio_files missing last_played column") } + if !hasSkipCount { + t.Error("audio_files missing skip_count column") + } + + if !hasLastSkipped { + t.Error("audio_files missing last_skipped column") + } + // Verify track_metadata VIEW includes play_count and last_played. viewCols := map[string]bool{} @@ -783,7 +801,7 @@ func TestPlayHistoryTable(t *testing.T) { t.Error("track_metadata VIEW missing last_played column") } - // Round-trip: insert a play_history row and verify play_count update. + // Round-trip: insert a listening_events row and verify play_count update. // First, set up test data. The test DB already has library id=0. InsertTestTrack(t, db, TestTrack{ FilePath: "/test/play_history.mp3", @@ -821,12 +839,12 @@ func TestPlayHistoryTable(t *testing.T) { t.Errorf("initial play_count = %d, want 0", playCount) } - // Insert a play_history row and update play_count. + // Insert a listening_events row (kind defaults to 'complete'). _, err = db.ExecContext( - "INSERT INTO play_history (audio_file_id) VALUES (1)", + "INSERT INTO listening_events (audio_file_id, kind) VALUES (1, 'complete')", ) if err != nil { - t.Fatalf("insert play_history: %v", err) + t.Fatalf("insert listening_events: %v", err) } _, err = db.ExecContext( diff --git a/backend/database/sql/schemas/audio_files.sql b/backend/database/sql/schemas/audio_files.sql index 876d852..2bdd960 100644 --- a/backend/database/sql/schemas/audio_files.sql +++ b/backend/database/sql/schemas/audio_files.sql @@ -68,8 +68,14 @@ CREATE TABLE IF NOT EXISTS audio_files ( -- compared against the on-disk mtime during a scan to detect files -- another application retagged in place. modified_at INTEGER NOT NULL DEFAULT 0, + -- Listening counts, denormalized from listening_events so the hot + -- read path (track list sort, shelves, smart playlists) never joins + -- a log table. Authored: a rescan cannot rebuild them. This is the + -- "MIXED KIND" half of audio_files the datamap notes. play_count INTEGER NOT NULL DEFAULT 0, last_played DATETIME, + skip_count INTEGER NOT NULL DEFAULT 0, + last_skipped DATETIME, tag_status TEXT NOT NULL DEFAULT 'untagged' CHECK(tag_status IN ( 'untagged', 'auto_matched', 'user_confirmed', 'user_skipped_permanent' diff --git a/backend/database/sql/schemas/download_items.sql b/backend/database/sql/schemas/download_items.sql index 9021993..d7828e4 100644 --- a/backend/database/sql/schemas/download_items.sql +++ b/backend/database/sql/schemas/download_items.sql @@ -45,8 +45,6 @@ CREATE INDEX IF NOT EXISTS idx_download_items_live CREATE INDEX IF NOT EXISTS idx_download_items_state ON download_items(state); --- idx_download_items_download is deliberately NOT declared here: on an --- existing database this table already exists at schema-pass time with --- its old column still named request_id, so an inline CREATE INDEX on --- download_id would fail outright. See ensureDownloadIndexes in --- backend/database/download_rename_migration.go. +-- ListDownloadItemsForDownload filters on the parent download. +CREATE INDEX IF NOT EXISTS idx_download_items_download + ON download_items(download_id); diff --git a/backend/database/sql/schemas/download_requests.sql b/backend/database/sql/schemas/download_requests.sql index f6cd928..c2bde29 100644 --- a/backend/database/sql/schemas/download_requests.sql +++ b/backend/database/sql/schemas/download_requests.sql @@ -66,14 +66,11 @@ CREATE TABLE IF NOT EXISTS download_requests ( FOREIGN KEY(parent_id) REFERENCES download_requests(id) ON DELETE CASCADE ); --- idx_download_requests_{due,entity,parent} are deliberately NOT --- declared here. This table name is reused from the old one-shot --- attempt table (also called download_requests before the Want/Request --- rename), so on an existing database this CREATE TABLE is a no-op --- against a table that, at schema-pass time, is still shaped like the --- OLD attempts table and lacks these columns entirely — an inline --- CREATE INDEX here would fail outright rather than just no-op. See --- migrateDownloadRename/ensureDownloadIndexes in --- backend/database/download_rename_migration.go, which create these --- once the rename has actually happened (or immediately, on a fresh --- database where the columns exist from the start). +CREATE INDEX IF NOT EXISTS idx_download_requests_due + ON download_requests(state, next_try_at); + +CREATE INDEX IF NOT EXISTS idx_download_requests_entity + ON download_requests(entity, state); + +CREATE INDEX IF NOT EXISTS idx_download_requests_parent + ON download_requests(parent_id) WHERE parent_id IS NOT NULL; diff --git a/backend/database/sql/schemas/listening_events.sql b/backend/database/sql/schemas/listening_events.sql new file mode 100644 index 0000000..103610a --- /dev/null +++ b/backend/database/sql/schemas/listening_events.sql @@ -0,0 +1,42 @@ +-- One row per track *exit*, three ways a listen can end: it reached +-- the end, it was heard enough to count and then skipped past, or it +-- was abandoned for another track before anyone had really listened. +-- +-- This is the source of truth for listening behaviour. The +-- denormalized `play_count` / `last_played` / `skip_count` / +-- `last_skipped` on audio_files are materialized from it, because the +-- hot read path (track-list sort, the shelves, smart playlists) must +-- not join a log that grows by one row per song forever. +-- +-- `kind` is the classification, applied at write time: +-- +-- complete the track reached its natural end, or was skipped in +-- its tail window (the last few seconds of a long fade). +-- play the scrobble threshold was heard — half the track or +-- four minutes, whichever is less — and the user moved on +-- before the end. +-- skip the user moved to a different track before that. +-- +-- `position_seconds` / `duration_seconds` are the raw reading the +-- classification was made from, kept so a future re-tune of the +-- threshold does not need the events re-recorded. 0/0 on a row means +-- "not captured for this event" (e.g. a natural finish recorded before +-- these columns existed), not "a zero-second track". +CREATE TABLE IF NOT EXISTS listening_events ( + id INTEGER PRIMARY KEY, + audio_file_id INTEGER NOT NULL, + kind TEXT NOT NULL DEFAULT 'complete' + CHECK (kind IN ('complete', 'play', 'skip')), + position_seconds INTEGER NOT NULL DEFAULT 0, + duration_seconds INTEGER NOT NULL DEFAULT 0, + occurred_at DATETIME NOT NULL DEFAULT (datetime('now')), + FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE +); + +CREATE INDEX IF NOT EXISTS idx_listening_events_audio_file_id + ON listening_events(audio_file_id); + +-- "What did I listen to this month" walks this, rather than the +-- per-track index above. +CREATE INDEX IF NOT EXISTS idx_listening_events_occurred_at + ON listening_events(occurred_at); diff --git a/backend/database/sql/schemas/play_history.sql b/backend/database/sql/schemas/play_history.sql deleted file mode 100644 index 07166a8..0000000 --- a/backend/database/sql/schemas/play_history.sql +++ /dev/null @@ -1,9 +0,0 @@ -CREATE TABLE IF NOT EXISTS play_history ( - id INTEGER PRIMARY KEY, - audio_file_id INTEGER NOT NULL, - played_at DATETIME NOT NULL DEFAULT (datetime('now')), - FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE -); - -CREATE INDEX IF NOT EXISTS idx_play_history_audio_file_id - ON play_history(audio_file_id); diff --git a/backend/database/sql/schemas/queue.sql b/backend/database/sql/schemas/queue.sql index dd7836a..edf9a76 100644 --- a/backend/database/sql/schemas/queue.sql +++ b/backend/database/sql/schemas/queue.sql @@ -1,18 +1,15 @@ CREATE TABLE IF NOT EXISTS queue ( id INTEGER PRIMARY KEY CHECK(id = 1), - source_playlist_id INTEGER, current_position INTEGER NOT NULL DEFAULT 0, shuffle_mode BOOLEAN NOT NULL DEFAULT false, repeat_mode TEXT NOT NULL DEFAULT 'off', shuffle_order TEXT, - -- source_playlist_id above is unused dead weight (nothing has ever - -- written it a nonzero value); source_type/source_id/source_label - -- below are its generalized replacement, covering albums, playlists, - -- smart playlists, genres and artists rather than playlists alone. + -- What the queue was built from ("Playing from: X"): an album, + -- playlist, smart playlist, genre or artist, identified by the id + -- that source_type's namespace gives it. source_type TEXT NOT NULL DEFAULT '', source_id INTEGER NOT NULL DEFAULT 0, - source_label TEXT NOT NULL DEFAULT '', - FOREIGN KEY(source_playlist_id) REFERENCES playlists(id) ON DELETE SET NULL + source_label TEXT NOT NULL DEFAULT '' ); -- Singleton row: there is exactly one playback queue. diff --git a/backend/database/sql/schemas/tagging_items.sql b/backend/database/sql/schemas/tagging_items.sql index 829061c..ff79946 100644 --- a/backend/database/sql/schemas/tagging_items.sql +++ b/backend/database/sql/schemas/tagging_items.sql @@ -26,17 +26,10 @@ CREATE TABLE IF NOT EXISTS tagging_items ( -- complete rip of their own directory. parent_group_key is the -- original folder group they were split from. -- - -- These two columns are declared LAST, after created_at, even - -- though that reads oddly next to the rest of the table: sql/ - -- migrations/0001 brings a pre-existing tagging_items up to date - -- with `ALTER TABLE ADD COLUMN`, which SQLite always appends at - -- the end of the column list. A fresh install (this file) and an - -- upgraded database (this file + the migration) must end up with - -- IDENTICAL column order, because sqlc-generated `SELECT *` scans - -- (e.g. GetTaggingItem) bind columns positionally — see the - -- schema/migration column-order test in database_test.go. Put - -- new columns wherever reads best when adding a table for the - -- first time; append-only from the second migration on. + -- These columns are appended after created_at rather than grouped + -- with the rest of the row: sqlc's `SELECT *` scans (GetTaggingItem) + -- bind column order positionally, so new columns always go at the + -- end. synthetic INTEGER NOT NULL DEFAULT 0, parent_group_key TEXT NOT NULL DEFAULT '', -- album_artist_conflict latches to 1 the first time two tracks @@ -58,10 +51,3 @@ CREATE INDEX IF NOT EXISTS idx_tagging_items_library_status CREATE INDEX IF NOT EXISTS idx_tagging_items_status_pending ON tagging_items(library_id) WHERE status = 'pending'; - --- idx_tagging_items_parent_group_key is NOT declared here on --- purpose: this file runs unconditionally, before migrations, even --- against a database that hasn't run 0001 yet — an index predicate --- referencing parent_group_key would fail on that table. It lives --- solely in sql/migrations/0001_tagging_items_synthetic.sql, which --- runs after the column exists either way (see database.go). diff --git a/backend/database/sql/sqlcgen/audio_files.sql.go b/backend/database/sql/sqlcgen/audio_files.sql.go index 7aeb3ca..df3dabf 100644 --- a/backend/database/sql/sqlcgen/audio_files.sql.go +++ b/backend/database/sql/sqlcgen/audio_files.sql.go @@ -39,7 +39,7 @@ INSERT INTO audio_files ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? ) -RETURNING id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status +RETURNING id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status ` type CreateAudioFileParams struct { @@ -135,6 +135,8 @@ func (q *Queries) CreateAudioFile(ctx context.Context, arg CreateAudioFileParams &i.ModifiedAt, &i.PlayCount, &i.LastPlayed, + &i.SkipCount, + &i.LastSkipped, &i.TagStatus, ) return i, err @@ -192,7 +194,7 @@ func (q *Queries) GetAllAudioFilePaths(ctx context.Context) ([]GetAllAudioFilePa const getAudioFile = `-- name: GetAudioFile :one -SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE id = ? LIMIT 1 +SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE id = ? LIMIT 1 ` // --------------------------------------------------------------------- @@ -228,13 +230,15 @@ func (q *Queries) GetAudioFile(ctx context.Context, id int64) (AudioFile, error) &i.ModifiedAt, &i.PlayCount, &i.LastPlayed, + &i.SkipCount, + &i.LastSkipped, &i.TagStatus, ) return i, err } const getAudioFileByPath = `-- name: GetAudioFileByPath :one -SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE file_path = ? LIMIT 1 +SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE file_path = ? LIMIT 1 ` func (q *Queries) GetAudioFileByPath(ctx context.Context, filePath string) (AudioFile, error) { @@ -267,6 +271,8 @@ func (q *Queries) GetAudioFileByPath(ctx context.Context, filePath string) (Audi &i.ModifiedAt, &i.PlayCount, &i.LastPlayed, + &i.SkipCount, + &i.LastSkipped, &i.TagStatus, ) return i, err @@ -334,7 +340,7 @@ func (q *Queries) GetAudioFilesByPaths(ctx context.Context, paths []string) ([]G } const getAudioFilesInLibrary = `-- name: GetAudioFilesInLibrary :many -SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE library_id = ? +SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE library_id = ? ` func (q *Queries) GetAudioFilesInLibrary(ctx context.Context, libraryID int64) ([]AudioFile, error) { @@ -373,6 +379,8 @@ func (q *Queries) GetAudioFilesInLibrary(ctx context.Context, libraryID int64) ( &i.ModifiedAt, &i.PlayCount, &i.LastPlayed, + &i.SkipCount, + &i.LastSkipped, &i.TagStatus, ); err != nil { return nil, err diff --git a/backend/database/sql/sqlcgen/models.go b/backend/database/sql/sqlcgen/models.go index c210de6..36e4805 100644 --- a/backend/database/sql/sqlcgen/models.go +++ b/backend/database/sql/sqlcgen/models.go @@ -94,6 +94,8 @@ type AudioFile struct { ModifiedAt int64 PlayCount int64 LastPlayed sql.NullTime + SkipCount int64 + LastSkipped sql.NullTime TagStatus string } @@ -261,6 +263,15 @@ type Library struct { AutotagWarningAcked int64 } +type ListeningEvent struct { + ID int64 + AudioFileID int64 + Kind string + PositionSeconds int64 + DurationSeconds int64 + OccurredAt time.Time +} + type Lyric struct { AudioFileID int64 Text string @@ -273,12 +284,6 @@ type LyricsIndex struct { Lyrics string } -type PlayHistory struct { - ID int64 - AudioFileID int64 - PlayedAt time.Time -} - type PlayerState struct { ID int64 Volume int64 @@ -312,15 +317,14 @@ type PlaylistTrack struct { } type Queue struct { - ID int64 - SourcePlaylistID sql.NullInt64 - CurrentPosition int64 - ShuffleMode bool - RepeatMode string - ShuffleOrder sql.NullString - SourceType string - SourceID int64 - SourceLabel string + ID int64 + CurrentPosition int64 + ShuffleMode bool + RepeatMode string + ShuffleOrder sql.NullString + SourceType string + SourceID int64 + SourceLabel string } type QueueTrack struct { diff --git a/backend/datamap/datamap.go b/backend/datamap/datamap.go index d77215a..f0e1282 100644 --- a/backend/datamap/datamap.go +++ b/backend/datamap/datamap.go @@ -269,10 +269,11 @@ var tables = []Table{ "from owned files plus the LRCLIB backfill.", }, { - Name: "play_history", Kind: Authored, Lifetime: Cascade, - Note: "Listening history. Authored, but intentionally cascades " + - "with its track — history for a file no longer in the library " + - "has nothing to point at.", + Name: "listening_events", Kind: Authored, Lifetime: Cascade, + Note: "Listening history, one row per track exit (complete, play " + + "or skip). Authored, but intentionally cascades with its " + + "track — history for a file no longer in the library has " + + "nothing to point at.", }, { Name: "player_state", Kind: Authored, Lifetime: Retained, diff --git a/backend/datamap/datamap_test.go b/backend/datamap/datamap_test.go index ee0425f..76e268d 100644 --- a/backend/datamap/datamap_test.go +++ b/backend/datamap/datamap_test.go @@ -212,14 +212,14 @@ func TestLifetimesMatchSchema(t *testing.T) { // Authored data is unrecoverable, so it must never be removed as a side // effect of deleting owned data. Cascade is allowed only where the -// catalog explains why (play_history, queue_tracks); this test pins the +// catalog explains why (listening_events, queue_tracks); this test pins the // set so a new cascade onto authored data is a deliberate decision. func TestAuthoredCascadesAreDeliberate(t *testing.T) { t.Parallel() allowed := map[string]bool{ - "play_history": true, - "queue_tracks": true, + "listening_events": true, + "queue_tracks": true, // Download history is scoped to the library it imported into. // When that library is removed the files it acquired go with diff --git a/backend/queue/playhistory.go b/backend/queue/playhistory.go index 19985db..73cb12e 100644 --- a/backend/queue/playhistory.go +++ b/backend/queue/playhistory.go @@ -6,7 +6,7 @@ import ( "yellowjacket/backend/events" ) -// recordPlay inserts a play_history row and updates the denormalized +// recordPlay inserts a listening_events row and updates the denormalized // play_count / last_played columns on audio_files. Called from // OnPlaybackFinished for the track that just finished. // @@ -20,10 +20,12 @@ func (q *Queue) recordPlay(audioFileID int64) { now := time.Now().UTC().Format(time.DateTime) - // Insert play_history row. + // Insert the listening event. A natural finish is a 'complete' by + // construction; position/duration are the classifier's to fill once + // skips are recorded (see .planning/plans/active/021). _, err := q.db.ExecContext( - `INSERT INTO play_history (audio_file_id, played_at) - VALUES (?, ?)`, + `INSERT INTO listening_events (audio_file_id, kind, occurred_at) + VALUES (?, 'complete', ?)`, audioFileID, now, ) if err != nil { -- 2.54.0