From 68e7edb8c9519b65272142e286bccf8f39a16775 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 9 Sep 2026 09:19:36 -0400 Subject: [PATCH] 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 {