diff --git a/src/db.rs b/src/db.rs index d242cbf..9ae5ef1 100644 --- a/src/db.rs +++ b/src/db.rs @@ -205,11 +205,10 @@ fn init_schema(conn: &Connection) -> Result<()> { )?; // Add the source column to existing databases. SQLite returns an error if // the column already exists, which we silently ignore for idempotency. - conn.execute( - "ALTER TABLE scrobbles ADD COLUMN source TEXT", - [], - ).ok(); - conn.execute_batch(" + conn.execute("ALTER TABLE scrobbles ADD COLUMN source TEXT", []) + .ok(); + conn.execute_batch( + " -- Cache table for album metadata (cover art, genres) from MusicBrainz. -- Populated by the 'enrich' command or automatically when generating -- HTML reports. Rows with cover_url = NULL are re-tried on next run. @@ -1124,7 +1123,7 @@ mod tests { track_duration_secs: Some(186), played_duration_secs: 186, scrobbled_at: today_at("10:00:00"), - source: "test".into(), + source: "test".into(), }; // First insert should get row ID 1. let id = insert_scrobble(&conn, &s).unwrap(); @@ -1502,7 +1501,7 @@ mod tests { track_duration_secs: None, played_duration_secs: 240, scrobbled_at: today_at("12:00:00"), - source: "test".into(), + source: "test".into(), }; insert_scrobble(&conn, &s).unwrap(); let recent = recent_scrobbles(&conn, "all", 1).unwrap(); diff --git a/src/enrich.rs b/src/enrich.rs index b5ab577..8af313d 100644 --- a/src/enrich.rs +++ b/src/enrich.rs @@ -927,7 +927,12 @@ pub fn run_enrich(conn: &Connection, force: bool, quiet: bool, no_itunes: bool) } /// Inner enrichment loop shared by `run_enrich` and `run_enrich_targeted`. -fn run_enrich_albums(conn: &Connection, albums: Vec, quiet: bool, no_itunes: bool) { +fn run_enrich_albums( + conn: &Connection, + albums: Vec, + quiet: bool, + no_itunes: bool, +) { if albums.is_empty() { if !quiet { eprintln!("All albums are already cached. Nothing to do."); @@ -969,12 +974,11 @@ fn run_enrich_albums(conn: &Connection, albums: Vec, quiet: b None } else { eprintln!(" Trying iTunes..."); - fetch_itunes_cover_url(&client, &album.artist, &album.album) - .and_then(|art_url| { - let stem = itunes_cover_stem(&album.artist, &album.album); - let dest = covers.join(format!("{}.jpg", stem)); - try_download(&client, &art_url, &dest) - }) + fetch_itunes_cover_url(&client, &album.artist, &album.album).and_then(|art_url| { + let stem = itunes_cover_stem(&album.artist, &album.album); + let dest = covers.join(format!("{}.jpg", stem)); + try_download(&client, &art_url, &dest) + }) }; if cover.is_some() { @@ -1053,8 +1057,7 @@ fn run_enrich_albums(conn: &Connection, albums: Vec, quiet: b ); for alt_mbid in &candidates[1..] { thread::sleep(RATE_LIMIT_DELAY); - let alt_url = - format!("https://coverartarchive.org/release/{}/front", alt_mbid); + let alt_url = format!("https://coverartarchive.org/release/{}/front", alt_mbid); let dest = covers.join(format!("{}.jpg", primary_mbid)); if let Some(path) = try_download(&client, &alt_url, &dest) { eprintln!(" Cover downloaded (from alternate release {}).", alt_mbid); diff --git a/src/main.rs b/src/main.rs index 11d002a..be28c50 100644 --- a/src/main.rs +++ b/src/main.rs @@ -703,7 +703,17 @@ fn main() { host: mpd_host, port: mpd_port, }; - run_report(&period, json, html, output.as_deref(), limit, atl, no_enrich, &mpd_cfg, &path); + run_report( + &period, + json, + html, + output.as_deref(), + limit, + atl, + no_enrich, + &mpd_cfg, + &path, + ); } Commands::Enrich { online, @@ -717,8 +727,12 @@ fn main() { } => { if no_mpd_covers && !online { eprintln!("Nothing to do. Pass --online and/or omit --no-mpd-covers."); - eprintln!(" (default) Extract embedded covers from music files via MPD (offline)"); - eprintln!(" --online Fetch metadata and covers from MusicBrainz / iTunes / CAA"); + eprintln!( + " (default) Extract embedded covers from music files via MPD (offline)" + ); + eprintln!( + " --online Fetch metadata and covers from MusicBrainz / iTunes / CAA" + ); std::process::exit(0); } diff --git a/src/mpd.rs b/src/mpd.rs index b9540a5..3ba25f3 100644 --- a/src/mpd.rs +++ b/src/mpd.rs @@ -909,10 +909,7 @@ fn try_extract_cover( let cover_path = dest.to_string_lossy().to_string(); let locked = db_conn.lock().unwrap(); if db::set_local_cover(&locked, &state.artist, &state.album, &cover_path).is_ok() { - eprintln!( - "[mpd] Cover cached for {} — {}", - state.artist, state.album - ); + eprintln!("[mpd] Cover cached for {} — {}", state.artist, state.album); } } @@ -1103,6 +1100,26 @@ fn run_mpd_cover_enrich_albums( #[cfg(test)] mod tests { use super::*; + use std::cell::RefCell; + use std::rc::Rc; + + fn state( + file: &str, + artist: &str, + album: &str, + title: &str, + duration_secs: u64, + state: &str, + ) -> MpdPlayerState { + MpdPlayerState { + file: file.to_string(), + artist: artist.to_string(), + album: album.to_string(), + title: title.to_string(), + duration_secs: Some(duration_secs), + state: state.to_string(), + } + } // ----------------------------------------------------------------------- // album_cover_stem — filename generation @@ -1198,6 +1215,123 @@ mod tests { assert_eq!(parse_mpd_state("unknown"), PlayerStatus::Stopped); } + // ----------------------------------------------------------------------- + // dispatch_events + // ----------------------------------------------------------------------- + + #[test] + fn test_dispatch_events_pause_resume_same_file_keeps_metadata_marker() { + let scrobbled: Rc>> = Rc::new(RefCell::new(Vec::new())); + let sink = scrobbled.clone(); + let mut tracker = watcher::ScrobbleTracker::new( + move |s: db::NewScrobble| { + sink.borrow_mut().push(s); + }, + "MPD".to_string(), + ); + + let mut last_metadata_file: Option = None; + + let mut prev = MpdPlayerState::default(); + let play_a = state( + "a.flac", + "Deftones", + "White Pony", + "Digital Bath", + 291, + "play", + ); + dispatch_events(&prev, &play_a, &mut tracker, &mut last_metadata_file); + assert_eq!(last_metadata_file.as_deref(), Some("a.flac")); + + let pause_a = state( + "a.flac", + "Deftones", + "White Pony", + "Digital Bath", + 291, + "pause", + ); + dispatch_events(&play_a, &pause_a, &mut tracker, &mut last_metadata_file); + assert_eq!(last_metadata_file.as_deref(), Some("a.flac")); + + prev = pause_a; + let resume_a = state( + "a.flac", + "Deftones", + "White Pony", + "Digital Bath", + 291, + "play", + ); + dispatch_events(&prev, &resume_a, &mut tracker, &mut last_metadata_file); + assert_eq!(last_metadata_file.as_deref(), Some("a.flac")); + + // No track boundary occurred; therefore no scrobble should have been emitted. + assert!(scrobbled.borrow().is_empty()); + } + + #[test] + fn test_dispatch_events_stop_clears_metadata_marker() { + let scrobbled: Rc>> = Rc::new(RefCell::new(Vec::new())); + let sink = scrobbled.clone(); + let mut tracker = watcher::ScrobbleTracker::new( + move |s: db::NewScrobble| { + sink.borrow_mut().push(s); + }, + "MPD".to_string(), + ); + + let mut last_metadata_file: Option = None; + let prev = MpdPlayerState::default(); + let play_a = state("a.flac", "A", "Alb", "Song A", 200, "play"); + dispatch_events(&prev, &play_a, &mut tracker, &mut last_metadata_file); + assert_eq!(last_metadata_file.as_deref(), Some("a.flac")); + + let stop_a = state("a.flac", "A", "Alb", "Song A", 200, "stop"); + dispatch_events(&play_a, &stop_a, &mut tracker, &mut last_metadata_file); + assert_eq!(last_metadata_file, None); + } + + #[test] + fn test_dispatch_events_replay_same_file_after_stop_reinitialises_track() { + let scrobbled: Rc>> = Rc::new(RefCell::new(Vec::new())); + let sink = scrobbled.clone(); + let mut tracker = watcher::ScrobbleTracker::new( + move |s: db::NewScrobble| { + sink.borrow_mut().push(s); + }, + "MPD".to_string(), + ); + + let mut last_metadata_file: Option = None; + let prev = MpdPlayerState::default(); + + // Use 0-second duration so threshold is 0 and each Stop scrobbles + // immediately if the tracker is tracking an active track. + let play_a = state("a.flac", "A", "Alb", "Song A", 0, "play"); + dispatch_events(&prev, &play_a, &mut tracker, &mut last_metadata_file); + + let stop_a = state("a.flac", "A", "Alb", "Song A", 0, "stop"); + dispatch_events(&play_a, &stop_a, &mut tracker, &mut last_metadata_file); + assert_eq!(scrobbled.borrow().len(), 1); + assert_eq!(last_metadata_file, None); + + // Replay the same file. Because Stop cleared last_metadata_file, + // dispatch_events must emit Metadata again and reinitialise tracking. + let replay_a = state("a.flac", "A", "Alb", "Song A", 0, "play"); + dispatch_events(&stop_a, &replay_a, &mut tracker, &mut last_metadata_file); + let stop_again = state("a.flac", "A", "Alb", "Song A", 0, "stop"); + dispatch_events( + &replay_a, + &stop_again, + &mut tracker, + &mut last_metadata_file, + ); + + assert_eq!(scrobbled.borrow().len(), 2); + } + // ----------------------------------------------------------------------- // MpdConfig // ----------------------------------------------------------------------- diff --git a/src/report.rs b/src/report.rs index fc6ebe7..8a764f4 100644 --- a/src/report.rs +++ b/src/report.rs @@ -601,12 +601,7 @@ pub fn print_terminal_report(data: &ReportData) { ] }) .collect(); - print_box_table( - &["Source", "Scrobbles", "Time"], - &rows, - None, - &[0], - ); + print_box_table(&["Source", "Scrobbles", "Time"], &rows, None, &[0]); } // --- Top Artists --- @@ -927,14 +922,13 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) -> .map(|(i, s)| { // Prefix the source name with a colour dot matching the // palette slot assigned to it in the all-time ranking. - let label = if let Some((bg, _)) = - source_colours(&s.source, &ordered_sources) - { + let label = if let Some((bg, _)) = source_colours(&s.source, &ordered_sources) { format!( "{}", - bg, html_escape(&s.source) + bg, + html_escape(&s.source) ) } else { html_escape(&s.source) @@ -986,10 +980,7 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) -> .as_deref() .and_then(|src| source_colours(src, &ordered_sources)) .map(|(bg, border)| { - format!( - " style=\"background:{};border-color:{}\"", - bg, border - ) + format!(" style=\"background:{};border-color:{}\"", bg, border) }) .unwrap_or_default(); h.linef(format_args!("
", card_style)); @@ -1080,11 +1071,7 @@ pub fn render_html_report(conn: &Connection, limit: i64, all_time_limit: i64) -> html_escape(&a.album) }; BarRow { - cells: vec![ - (i + 1).to_string(), - html_escape(&a.artist), - album_cell, - ], + cells: vec![(i + 1).to_string(), html_escape(&a.artist), album_cell], value: a.plays, suffix: format_duration(a.listen_time_secs), cover, @@ -1373,10 +1360,7 @@ const SOURCE_PALETTE: &[(&str, &str)] = &[ /// Look up the card background and border colours for a source name, given /// the ordered list of all-time sources (most played first). -fn source_colours<'a>( - source: &str, - ordered_sources: &[String], -) -> Option<(&'a str, &'a str)> { +fn source_colours<'a>(source: &str, ordered_sources: &[String]) -> Option<(&'a str, &'a str)> { ordered_sources .iter() .position(|s| s == source) @@ -1549,7 +1533,10 @@ fn write_bar_table(h: &mut HtmlWriter, title: &str, headers: &[&str], rows: &[Ba let play_count_idx = headers.len().saturating_sub(2); for (i, hdr) in headers.iter().enumerate() { if i == play_count_idx { - h.linef(format_args!("{}", html_escape(hdr))); + h.linef(format_args!( + "{}", + html_escape(hdr) + )); } else { h.linef(format_args!("{}", html_escape(hdr))); } diff --git a/src/watcher.rs b/src/watcher.rs index 0860a17..6c6fddf 100644 --- a/src/watcher.rs +++ b/src/watcher.rs @@ -128,6 +128,7 @@ impl CurrentTrack { pub struct ScrobbleTracker { current_track: Option, is_playing: bool, + has_seen_status: bool, playing_since: Option, accumulated_secs: f64, scrobble_fn: F, @@ -140,6 +141,7 @@ impl ScrobbleTracker { Self { current_track: None, is_playing: false, + has_seen_status: false, playing_since: None, accumulated_secs: 0.0, scrobble_fn, @@ -151,9 +153,10 @@ impl ScrobbleTracker { /// /// ## Event handling: /// - /// - **Metadata**: A new track started. Evaluate the previous track - /// (scrobble if threshold met), then begin tracking the new one. - /// We assume the new track starts in Playing state. + /// - **Metadata**: A track boundary or metadata refresh. If metadata + /// points to a different track, evaluate the previous one (scrobble if + /// threshold met), then begin tracking the new one. If metadata refers + /// to the same track, update fields in place and keep timing state. /// /// - **Status(Playing)**: Resume accumulating play time. If already /// playing, this is a no-op (avoids double-counting). @@ -178,6 +181,20 @@ impl ScrobbleTracker { title, duration_us, } => { + // Some players emit metadata updates for the same track (for + // example around pause/resume or delayed tag updates). Treat + // those as in-place updates, not as a track boundary. + if let Some(current) = &mut self.current_track + && current.artist == artist + && current.album == album + && current.title == title + { + if duration_us.is_some() { + current.duration_us = duration_us; + } + return; + } + // Evaluate the previous track before switching to the new one. self.evaluate_previous_track(); @@ -190,12 +207,23 @@ impl ScrobbleTracker { }); self.accumulated_secs = 0.0; - // A metadata event means the player is actively playing the new track. - self.is_playing = true; - self.playing_since = Some(Instant::now()); + // Do not blindly assume metadata implies Playing. If we've + // never seen status yet, fall back to assuming playback has + // started so startup still works when metadata arrives first. + if self.has_seen_status { + self.playing_since = if self.is_playing { + Some(Instant::now()) + } else { + None + }; + } else { + self.is_playing = true; + self.playing_since = Some(Instant::now()); + } } Event::Status(status) => match status { PlayerStatus::Playing => { + self.has_seen_status = true; // Only start the clock if we weren't already playing. // This prevents double-counting if we receive redundant Playing events. if !self.is_playing { @@ -204,6 +232,7 @@ impl ScrobbleTracker { } } PlayerStatus::Paused => { + self.has_seen_status = true; // Flush the elapsed time from the current playing stretch // into the accumulator, then stop the clock. self.flush_playing_time(); @@ -211,6 +240,7 @@ impl ScrobbleTracker { self.playing_since = None; } PlayerStatus::Stopped => { + self.has_seen_status = true; // Treat Stop as a final decision: evaluate the current // track now (scrobble if threshold met, discard if not) // and clear all tracking state. This prevents a spurious @@ -219,6 +249,7 @@ impl ScrobbleTracker { self.evaluate_previous_track(); self.accumulated_secs = 0.0; self.is_playing = false; + self.playing_since = None; } }, Event::Eof => { @@ -380,20 +411,26 @@ pub fn create_db_tracker( conn: std::sync::Arc>, source: String, ) -> ScrobbleTracker { - ScrobbleTracker::new(move |scrobble: NewScrobble| { - let conn = conn.lock().unwrap(); - match db::insert_scrobble(&conn, &scrobble) { - Ok(_) => { - eprintln!( - "[scrobbled] {}: {} - {} ({}s)", - scrobble.source, scrobble.artist, scrobble.title, scrobble.played_duration_secs - ); + ScrobbleTracker::new( + move |scrobble: NewScrobble| { + let conn = conn.lock().unwrap(); + match db::insert_scrobble(&conn, &scrobble) { + Ok(_) => { + eprintln!( + "[scrobbled] {}: {} - {} ({}s)", + scrobble.source, + scrobble.artist, + scrobble.title, + scrobble.played_duration_secs + ); + } + Err(e) => { + eprintln!("[error] Failed to insert scrobble: {}", e); + } } - Err(e) => { - eprintln!("[error] Failed to insert scrobble: {}", e); - } - } - }, source) + }, + source, + ) } // =========================================================================== @@ -410,6 +447,7 @@ pub fn create_db_tracker( pub struct TestableTracker { current_track: Option, is_playing: bool, + has_seen_status: bool, /// Simulated timestamp (in seconds) when the current Playing stretch began. playing_since_secs: Option, /// Accumulated play time for the current track (in seconds). @@ -428,6 +466,7 @@ impl TestableTracker { Self { current_track: None, is_playing: false, + has_seen_status: false, playing_since_secs: None, accumulated_secs: 0.0, scrobbled: Vec::new(), @@ -484,6 +523,17 @@ impl TestableTracker { title, duration_us, } => { + if let Some(current) = &mut self.current_track + && current.artist == artist + && current.album == album + && current.title == title + { + if duration_us.is_some() { + current.duration_us = duration_us; + } + return; + } + self.evaluate_previous_track(); self.current_track = Some(CurrentTrack { artist, @@ -492,25 +542,37 @@ impl TestableTracker { duration_us, }); self.accumulated_secs = 0.0; - self.is_playing = true; - self.playing_since_secs = Some(self.clock_secs); + if self.has_seen_status { + self.playing_since_secs = if self.is_playing { + Some(self.clock_secs) + } else { + None + }; + } else { + self.is_playing = true; + self.playing_since_secs = Some(self.clock_secs); + } } Event::Status(status) => match status { PlayerStatus::Playing => { + self.has_seen_status = true; if !self.is_playing { self.is_playing = true; self.playing_since_secs = Some(self.clock_secs); } } PlayerStatus::Paused => { + self.has_seen_status = true; self.flush_playing_time(); self.is_playing = false; self.playing_since_secs = None; } PlayerStatus::Stopped => { + self.has_seen_status = true; self.evaluate_previous_track(); self.accumulated_secs = 0.0; self.is_playing = false; + self.playing_since_secs = None; } }, Event::Eof => { @@ -876,6 +938,82 @@ mod tests { assert_eq!(tracker.scrobbled[1].title, "Digital Bath"); } + #[test] + fn test_duplicate_metadata_same_track_does_not_double_scrobble() { + // Some players emit duplicate metadata lines for the same track. + // They must not split a single listen into multiple scrobbles. + let mut tracker = TestableTracker::new(); + + tracker.handle_event(Event::Metadata { + artist: "††† (Crosses)".into(), + album: "††† (Crosses)".into(), + title: "This Is a Trick".into(), + duration_us: Some(186_000_000), // threshold = 93s + }); + tracker.advance_time(50.0); + + // Duplicate metadata for the same track should be ignored as a boundary. + tracker.handle_event(Event::Metadata { + artist: "††† (Crosses)".into(), + album: "††† (Crosses)".into(), + title: "This Is a Trick".into(), + duration_us: Some(186_000_000), + }); + tracker.advance_time(50.0); + + // Next track triggers evaluation. + tracker.handle_event(Event::Metadata { + artist: "Deftones".into(), + album: "White Pony".into(), + title: "Digital Bath".into(), + duration_us: Some(291_000_000), + }); + + assert_eq!(tracker.scrobbled.len(), 1); + assert_eq!(tracker.scrobbled[0].title, "This Is a Trick"); + assert_eq!(tracker.scrobbled[0].played_duration_secs, 100); + } + + #[test] + fn test_metadata_while_paused_does_not_start_clock() { + // If metadata arrives while paused, we should not treat it as playing. + let mut tracker = TestableTracker::new(); + + tracker.handle_event(Event::Metadata { + artist: "††† (Crosses)".into(), + album: "††† (Crosses)".into(), + title: "Telepathy".into(), + duration_us: Some(215_000_000), // threshold = 107.5s + }); + + tracker.advance_time(30.0); + tracker.handle_event(Event::Status(PlayerStatus::Paused)); + + // Metadata update while paused (same track, perhaps with better tags). + tracker.handle_event(Event::Metadata { + artist: "††† (Crosses)".into(), + album: "††† (Crosses)".into(), + title: "Telepathy".into(), + duration_us: Some(215_000_000), + }); + + // Long paused stretch should not count. + tracker.advance_time(500.0); + + // Resume and play a bit more, still below threshold overall. + tracker.handle_event(Event::Status(PlayerStatus::Playing)); + tracker.advance_time(30.0); + + tracker.handle_event(Event::Metadata { + artist: "Deftones".into(), + album: "White Pony".into(), + title: "Digital Bath".into(), + duration_us: Some(291_000_000), + }); + + assert_eq!(tracker.scrobbled.len(), 0); + } + #[test] fn test_stop_below_threshold_no_scrobble() { // Play a track for 3 seconds then stop — should NOT be scrobbled,