diff --git a/crates/qbz-qt/src/local_album_actions.rs b/crates/qbz-qt/src/local_album_actions.rs index ee62db7e7..148436a70 100644 --- a/crates/qbz-qt/src/local_album_actions.rs +++ b/crates/qbz-qt/src/local_album_actions.rs @@ -185,8 +185,9 @@ pub fn version_doc(id: &str, index: usize) -> Option { .find_map(|t| t.artwork_path.as_ref().filter(|p| !p.is_empty())) { let source = tracks.first().and_then(|t| t.source.as_deref()); - art.entry(row.art_key.clone()) - .or_insert_with(|| crate::local_rows::art_ref(source, p)); + if let Some(t) = crate::local_rows::art_token(source, p) { + art.entry(row.art_key.clone()).or_insert(t); + } } tracks.iter().map(|t| map_track(t, art)).collect::>() }); diff --git a/crates/qbz-qt/src/local_albums.rs b/crates/qbz-qt/src/local_albums.rs index be23adb13..55990e584 100644 --- a/crates/qbz-qt/src/local_albums.rs +++ b/crates/qbz-qt/src/local_albums.rs @@ -229,10 +229,11 @@ pub fn load_artists_blocking() -> Result, String> { .map(|m| { let key = artist_key(&m.name); if !m.image_path.is_empty() { - art.insert( - key.clone(), - crate::local_rows::art_ref(Some(&m.image_source), &m.image_path), - ); + if let Some(t) = + crate::local_rows::art_token(Some(&m.image_source), &m.image_path) + { + art.insert(key.clone(), t); + } } ArtistRow { art_key: key, diff --git a/crates/qbz-qt/src/local_artwork.rs b/crates/qbz-qt/src/local_artwork.rs index dca50df2c..4ac1c12c3 100644 --- a/crates/qbz-qt/src/local_artwork.rs +++ b/crates/qbz-qt/src/local_artwork.rs @@ -70,7 +70,7 @@ use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, LazyLock, Mutex, RwLock}; -use qbz_source::ArtRef; +use qbz_source::{ArtRef, ArtSize}; use crate::artwork_qt; use crate::local_state::state; @@ -129,19 +129,28 @@ pub struct ArtworkWindow { /// Keys with no cover are dropped, so the QML map only ever grows with real /// hits. pub fn resolve_window_blocking(keys: Vec) -> ArtworkWindow { - let sources: Vec<(String, ArtRef)> = state(|s| { + let sources: Vec<(String, qbz_source::SourceId, String)> = state(|s| { keys.iter() - .filter_map(|k| s.art_index.get(k).map(|p| (k.clone(), p.clone()))) + .filter_map(|k| { + s.art_index + .get(k) + .map(|(src, tok)| (k.clone(), *src, tok.clone())) + }) .collect() }); let mut hits = Vec::with_capacity(sources.len()); let mut plex_misses = Vec::new(); let mut cold = Vec::new(); let mut seen: HashSet = HashSet::new(); - for (key, art) in sources { + for (key, src, token) in sources { if !seen.insert(key.clone()) { continue; } + // THE resolution, and it happens HERE — over the keys on screen, not + // over every row of the document. A remote source answers by reading + // its credentials, so doing this per mapped row cost 39-92 µs each on + // a 1703-row grid that rebuilds on every visit. + let art = qbz_source::registry().artwork_token(src, &token, ArtSize::Card); // The taxonomy is READ, not re-derived. `artwork_qt::classify` used to // stand here and sniff a `String` whose provenance had been thrown // away three layers up; the row's own source answered this question at diff --git a/crates/qbz-qt/src/local_ephemeral.rs b/crates/qbz-qt/src/local_ephemeral.rs index 3bd75cf23..bc30010d1 100644 --- a/crates/qbz-qt/src/local_ephemeral.rs +++ b/crates/qbz-qt/src/local_ephemeral.rs @@ -192,10 +192,9 @@ fn build_doc(name: &str, path: &str, tracks: &[LocalTrack]) -> EphemeralDoc { // key of an indexed album that happens to group the same. let art_key = album_key(&format!("eph:{key}")); if let Some(p) = first.artwork_path.as_ref().filter(|p| !p.is_empty()) { - art.insert( - art_key.clone(), - crate::local_rows::art_ref(first.source.as_deref(), p), - ); + if let Some(t) = crate::local_rows::art_token(first.source.as_deref(), p) { + art.insert(art_key.clone(), t); + } } let rows: Vec = group.iter().map(|t| map_track(*t, art)).collect(); EphemeralAlbumBlock { diff --git a/crates/qbz-qt/src/local_plex.rs b/crates/qbz-qt/src/local_plex.rs index 61f4b1ab9..95563cdb6 100644 --- a/crates/qbz-qt/src/local_plex.rs +++ b/crates/qbz-qt/src/local_plex.rs @@ -25,7 +25,7 @@ use std::path::PathBuf; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{LazyLock, Mutex}; +use std::sync::{LazyLock, Mutex, RwLock}; use qbz_app::user_data::UserDataPaths; use qbz_library::LocalTrack; @@ -49,6 +49,43 @@ static STATE: LazyLock = LazyLock::new(PlexSettingsState::new /// The user id the store is currently bound to (None = session-less). static BOUND: Mutex> = Mutex::new(None); +/// The last read of [`settings`], kept so the accessor is not a database hit. +/// +/// # Why this exists — it is a measured regression, not a micro-optimisation +/// +/// `settings()` runs `ensure_bound()` (a `load_last_user_id` file read plus a +/// mutex) and then a SQLite query. That was fine while its callers were gates +/// asked a handful of times per view. +/// +/// Design 02 §9 stage 4 put it on a PER-ROW path: `local_rows::art_ref` asks +/// the owning source what a row's artwork token means, and `PlexSource`'s +/// answer depends on whether Plex is connected — so every Plex row in the grid +/// paid a file read, a mutex and a query. Measured on the owner's library: +/// +/// ```text +/// 29 local rows map 0.06 ms -> 2 µs/row +/// 1271 Plex rows map 50-117 ms -> 39-92 µs/row +/// ``` +/// +/// 25-45x per row, on a document that is rebuilt on every visit to Local +/// Library. The grid had been tuned to a good place and this is what took it +/// away. +/// +/// The cache is INVALIDATED by every writer in this module and by +/// `init_for_user` / `reset`, so a connect, a disconnect or a user switch is +/// still seen immediately — the property `PlexCredsGlue` documents (it reads +/// the live store rather than caching a copy) is preserved, because the +/// authority itself is what caches now. +static CACHE: RwLock> = RwLock::new(None); + +/// Drop the memo. Call from EVERY writer — a stale `enabled` here is a Plex +/// library that will not disappear when the user switches it off. +fn invalidate_cache() { + if let Ok(mut c) = CACHE.write() { + *c = None; + } +} + /// Bind (or re-bind) the store to the ACTIVE user. Called lazily by every /// accessor, so no glue in the login path is required; `init_for_user` is /// provided for the shell to call explicitly on session activation. @@ -78,6 +115,7 @@ fn ensure_bound() { /// Explicit bind on session activation (optional — accessors self-bind). pub fn init_for_user(base_dir: &std::path::Path) { + invalidate_cache(); if let Err(e) = STATE.init_at(base_dir) { log::warn!("[qbz-qt] plex settings init failed: {e}"); return; @@ -87,14 +125,24 @@ pub fn init_for_user(base_dir: &std::path::Path) { /// Forget the binding (logout) so the next read re-binds to the new user. pub fn reset() { + invalidate_cache(); *BOUND.lock().unwrap() = None; let _ = STATE.teardown(); } /// Current persisted settings (defaults when there is no session). pub fn settings() -> PlexSettings { + if let Ok(c) = CACHE.read() { + if let Some(s) = c.as_ref() { + return s.clone(); + } + } ensure_bound(); - STATE.get_settings().unwrap_or_default() + let fresh = STATE.get_settings().unwrap_or_default(); + if let Ok(mut c) = CACHE.write() { + *c = Some(fresh.clone()); + } + fresh } // --------------------------------------------------------------------------- @@ -348,6 +396,7 @@ pub fn cached_sections() -> (Vec, Vec) { // --------------------------------------------------------------------------- pub fn set_enabled(value: bool) { + invalidate_cache(); ensure_bound(); if let Err(e) = STATE.set_enabled(value) { log::error!("[qbz-qt] plex set_enabled failed: {e}"); @@ -370,6 +419,7 @@ pub fn client_id() -> String { /// through a PIN. The PIN path clears it on success so the panel goes back to /// showing Authorize (`plex_auth.rs:531`). pub fn set_manual_token_mode(value: bool) { + invalidate_cache(); ensure_bound(); if let Err(e) = STATE.set_manual_token_mode(value) { log::error!("[qbz-qt] plex set_manual_token_mode failed: {e}"); @@ -383,6 +433,7 @@ pub fn set_manual_token_mode(value: bool) { /// `ping_inner` — so a Qt-only install stamped `server_id = NULL` on every /// cached row. Porting the ping is what closes that. pub fn set_machine_id(value: &str) { + invalidate_cache(); ensure_bound(); if let Err(e) = STATE.set_machine_id(value) { log::error!("[qbz-qt] plex set_machine_id failed: {e}"); @@ -400,6 +451,7 @@ pub fn credentials() -> (String, String) { // That settings surface is not ported yet, so nothing calls this today. #[allow(dead_code)] pub fn set_metadata_write_enabled(value: bool) { + invalidate_cache(); ensure_bound(); if let Err(e) = STATE.set_metadata_write_enabled(value) { log::error!("[qbz-qt] plex set_metadata_write_enabled failed: {e}"); @@ -410,6 +462,7 @@ pub fn set_metadata_write_enabled(value: bool) { /// is RESOLVED (`proto://host:32400`) before it is stored. Returns the /// resolved base url ("" when the input is unusable). pub fn connect_manual(server_url: &str, token: &str) -> String { + invalidate_cache(); ensure_bound(); let base = resolve_base_url(server_url); if base.is_empty() { @@ -424,6 +477,7 @@ pub fn connect_manual(server_url: &str, token: &str) -> String { } pub fn set_selected_sections(keys: &[String]) { + invalidate_cache(); ensure_bound(); if let Err(e) = STATE.set_selected_section_keys(keys) { log::error!("[qbz-qt] plex set_selected_section_keys failed: {e}"); @@ -434,6 +488,7 @@ pub fn set_selected_sections(keys: &[String]) { /// `client_id`, `metadata_write_enabled`) and purge the shared cache DB so /// no Plex rows survive in the browse union. pub fn disconnect() { + invalidate_cache(); ensure_bound(); if let Err(e) = STATE.disconnect() { log::error!("[qbz-qt] plex disconnect failed: {e}"); diff --git a/crates/qbz-qt/src/local_rows.rs b/crates/qbz-qt/src/local_rows.rs index ac7fb7d16..fb30f0f6a 100644 --- a/crates/qbz-qt/src/local_rows.rs +++ b/crates/qbz-qt/src/local_rows.rs @@ -23,7 +23,7 @@ use std::collections::HashMap; use qbz_library::{AudioFormat, LocalAlbum, LocalTrack}; -use qbz_source::{ArtRef, ArtSize}; +use qbz_source::SourceId; use serde::Serialize; // --------------------------------------------------------------------------- @@ -285,43 +285,28 @@ pub fn badge_source_raw(raw: Option<&str>) -> String { } } -/// The [`ArtRef`] for a row's raw artwork token, decided by the row's OWN -/// source (design 02 §9 stage 4). +/// `(owning source, raw token)` for a row's artwork — the pair the artwork +/// window needs in order to resolve it WITHOUT guessing. /// -/// The art index used to hold the token as a bare `String`, and -/// `artwork_qt::classify` sniffed it back apart later — by which point the row -/// was gone and all that was left was the shape of the characters. That is bug -/// 3, and its concrete failure is not hypothetical: `classify` knows a Plex -/// thumb only because `local_plex::is_thumb_path` was bolted onto it, so a -/// Jellyfin token (`/Items//Images/Primary`) matches its `starts_with('/')` -/// arm and the pipeline tries to open it as a file on disk — blank cover, no -/// error, nothing in the log. +/// This is the cheap half of what stage 4 set out to do, and the split matters. +/// Deciding WHOSE token this is costs one vocabulary lookup and happens here, +/// per row, while the row's provenance still exists — that is bug 3's fix. +/// Deciding what the token RESOLVES TO costs a credentials read for a remote +/// source, so it happens in `local_artwork::resolve_window_blocking`, over the +/// ~50 keys actually on screen rather than all 1703 in the document. /// -/// Registering the answer HERE is the fix: this is the last moment the row's -/// provenance exists. `SourceId::from_word` is the one vocabulary table -/// (§3.1), so all six source words land on the right source, and an unknown -/// word gets `None` rather than being guessed into Qobuz. -/// -/// Cheap by contract — `Source::artwork_token` may not touch the network, a DB -/// or the filesystem — so this stays a per-row call on a page load. -pub fn art_ref(source_word: Option<&str>, token: &str) -> ArtRef { - if token.trim().is_empty() { - return ArtRef::None; - } - // The grid's size class. `ArtSize::Card` is `artwork_qt::PLEX_THUMB_PX` - // (256), which is deliberately ONE app-wide Plex transcode size so a cover - // is downloaded once — do not introduce a second size here. - match qbz_source::SourceId::from_word(source_word.unwrap_or("")) { - Some(id) => qbz_source::registry().artwork_token(id, token, ArtSize::Card), - // No word at all is the LOCAL case in practice (`local_tracks.source` - // is empty for a plain scanned file, §3.1's vocabulary table), and it - // is what `classify` did with those rows too. - None => qbz_source::registry().artwork_token( - qbz_source::SourceId::LOCAL, - token, - ArtSize::Card, - ), +/// Resolving here instead was measured at 39-92 µs/row against 2 µs, on a +/// document rebuilt on every visit to Local Library. Same information, wrong +/// place. +pub fn art_token(source_word: Option<&str>, token: &str) -> Option<(SourceId, String)> { + let token = token.trim(); + if token.is_empty() { + return None; } + // No word at all is the LOCAL case in practice (`local_tracks.source` is + // empty for a plain scanned file, §3.1's vocabulary table). + let id = SourceId::from_word(source_word.unwrap_or("")).unwrap_or(SourceId::LOCAL); + Some((id, token.to_string())) } // --------------------------------------------------------------------------- @@ -329,10 +314,12 @@ pub fn art_ref(source_word: Option<&str>, token: &str) -> ArtRef { // see `art_ref`: the row's own source decides what its token means) // --------------------------------------------------------------------------- -pub fn map_album(a: LocalAlbum, art: &mut HashMap) -> AlbumRow { +pub fn map_album(a: LocalAlbum, art: &mut HashMap) -> AlbumRow { let key = album_key(&a.id); if let Some(p) = a.artwork_path.as_ref().filter(|p| !p.is_empty()) { - art.insert(key.clone(), art_ref(Some(a.source.as_str()), p)); + if let Some(t) = art_token(Some(a.source.as_str()), p) { + art.insert(key.clone(), t); + } } let folder_count = a .source_folders @@ -358,10 +345,12 @@ pub fn map_album(a: LocalAlbum, art: &mut HashMap) -> AlbumRow { } } -pub fn map_track(t: &LocalTrack, art: &mut HashMap) -> TrackRow { +pub fn map_track(t: &LocalTrack, art: &mut HashMap) -> TrackRow { let key = track_key(t.id); if let Some(p) = t.artwork_path.as_ref().filter(|p| !p.is_empty()) { - art.insert(key.clone(), art_ref(t.source.as_deref(), p)); + if let Some(tok) = art_token(t.source.as_deref(), p) { + art.insert(key.clone(), tok); + } } TrackRow { id: t.id.to_string(), diff --git a/crates/qbz-qt/src/local_state.rs b/crates/qbz-qt/src/local_state.rs index d5e755bfb..937aa4947 100644 --- a/crates/qbz-qt/src/local_state.rs +++ b/crates/qbz-qt/src/local_state.rs @@ -12,7 +12,7 @@ use std::sync::Mutex; use qbz_app::user_data::UserDataPaths; use qbz_library::{LibraryDatabase, LibraryError, LocalTrack}; -use qbz_source::ArtRef; +use qbz_source::SourceId; use crate::local_rows::{AlbumRow, ArtistRow, LocalCounts, TrackRow, TreeNode}; @@ -97,15 +97,20 @@ pub struct LocalState { /// The FULL flattened tree (visible derivation applies the rail search). pub tree: Vec, pub tree_search: String, - /// artKey -> what the row's OWN source said its artwork token means - /// (design 02 §9 stage 4). The artwork window turns these into 256px - /// thumbnails (`ArtRef::File`) or disk-cached fetches (`ArtRef::Fetch`). + /// artKey -> `(the source that owns the row, its RAW artwork token)`. /// - /// It used to hold the raw token as a `String`, and `artwork_qt::classify` - /// sniffed it back apart at window time — by which point the row was gone - /// and only the shape of the characters was left. That is bug 3; - /// `local_rows::art_ref` resolves it while provenance still exists. - pub art_index: HashMap, + /// It used to hold the token alone, and `artwork_qt::classify` sniffed the + /// provenance back out of the characters at window time — which is bug 3. + /// It briefly held a fully resolved `ArtRef` instead, which fixed that but + /// moved the resolution from ~50 VISIBLE rows to all 1703 rows of the + /// document, and a Plex row's resolution reads its credentials: measured + /// 39-92 µs/row against 2 µs for a row that needs none. + /// + /// Carrying the SOURCE beside the token keeps both properties. Provenance + /// survives — nobody sniffs anything — and the resolution happens where it + /// always did, in `local_artwork::resolve_window_blocking`, over the keys + /// actually on screen. + pub art_index: HashMap, /// Album identity ("folder" | "metadata") — persisted (locallibrary_ui). pub album_mode: String, pub counts: LocalCounts, @@ -139,7 +144,7 @@ pub fn state(f: impl FnOnce(&mut LocalState) -> R) -> R { /// Take the art index out, run `f` with it, put it back — the shape every /// loader uses so mapping can register covers without a second lock. -pub fn with_art(f: impl FnOnce(&mut HashMap) -> R) -> R { +pub fn with_art(f: impl FnOnce(&mut HashMap) -> R) -> R { let mut art = state(|s| std::mem::take(&mut s.art_index)); let out = f(&mut art); state(|s| s.art_index = art); diff --git a/crates/qbz-qt/src/local_tree.rs b/crates/qbz-qt/src/local_tree.rs index acbfd4d56..27678f27d 100644 --- a/crates/qbz-qt/src/local_tree.rs +++ b/crates/qbz-qt/src/local_tree.rs @@ -18,9 +18,9 @@ use std::sync::Mutex; use qbz_library::{FolderTreeEntry, LibraryError, LocalTrack}; use serde::Serialize; -use qbz_source::ArtRef; +use qbz_source::SourceId; -use crate::local_rows::{art_ref, basename, folder_key, map_track, FolderDetail, SubfolderRow, TrackRow, TreeNode}; +use crate::local_rows::{art_token, basename, folder_key, map_track, FolderDetail, SubfolderRow, TrackRow, TreeNode}; use crate::local_state::{state, with_art, with_db}; /// `list_folder_children` + the reference's on-disk cover fallback for the @@ -63,7 +63,7 @@ fn folder_children_with_covers(path: &str) -> Vec { fn entry_to_node( e: &FolderTreeEntry, depth: i32, - art: &mut std::collections::HashMap, + art: &mut std::collections::HashMap, ) -> TreeNode { match e { FolderTreeEntry::Folder { @@ -76,7 +76,9 @@ fn entry_to_node( if let Some(a) = artwork.as_ref().filter(|a| !a.is_empty()) { // The folder tree is `library.db`: every cover here is a // local file. - art.insert(key.clone(), art_ref(Some("local"), a)); + if let Some(t) = art_token(Some("local"), a) { + art.insert(key.clone(), t); + } } TreeNode { path: path.clone(), @@ -412,7 +414,9 @@ pub fn load_folder_detail_blocking(path: &str) -> FolderDetail { if let Some(a) = artwork.as_ref().filter(|a| !a.is_empty()) { // The folder tree is `library.db`: every cover here is a // local file. - art.insert(key.clone(), art_ref(Some("local"), a)); + if let Some(t) = art_token(Some("local"), a) { + art.insert(key.clone(), t); + } } Some(SubfolderRow { path: path.clone(), diff --git a/crates/qbz-qt/src/media_servers_qt.rs b/crates/qbz-qt/src/media_servers_qt.rs index 93f815382..353e3a4d9 100644 --- a/crates/qbz-qt/src/media_servers_qt.rs +++ b/crates/qbz-qt/src/media_servers_qt.rs @@ -17,15 +17,36 @@ //! beside the rest of the registry's injections, so there is one place to look //! when a source answers "not configured". +use std::collections::HashMap; use std::path::PathBuf; -use std::sync::LazyLock; +use std::sync::{LazyLock, RwLock}; use qbz_app::settings::media_servers::{MediaServerKind, MediaServerSettings, MediaServerState}; static STATE: LazyLock = LazyLock::new(MediaServerState::new); +/// The last read per server, so [`get`] is not a database hit. +/// +/// SAME defect as `local_plex`'s, introduced the same way: `local_rows::art_ref` +/// asks the owning source per ROW what an artwork token means, and both media +/// sources answer by looking at whether they are connected. Without this, every +/// remote row in the grid costs a mutex plus a SQLite query — measured at +/// 39-92 µs/row against 2 µs for a row whose source needs no credentials. +/// +/// Invalidated by [`put`], [`disconnect`], [`init_for_user`] and [`reset`], so +/// a connect or a master-toggle flip is still seen on the very next read. +static CACHE: LazyLock>> = + LazyLock::new(|| RwLock::new(HashMap::new())); + +fn invalidate_cache() { + if let Ok(mut c) = CACHE.write() { + c.clear(); + } +} + /// Bind the store to the active user. Called from `auth_qt`. pub fn init_for_user(base_dir: &std::path::Path) { + invalidate_cache(); STATE.init_at(base_dir); // Mint the two stable identifiers ONCE, now, rather than at first use. // Both are load-bearing and both are easy to get wrong lazily: a Jellyfin @@ -35,19 +56,31 @@ pub fn init_for_user(base_dir: &std::path::Path) { } pub fn reset() { + invalidate_cache(); STATE.reset(); } pub fn get(kind: MediaServerKind) -> MediaServerSettings { - STATE.get(kind) + if let Ok(c) = CACHE.read() { + if let Some(s) = c.get(kind.as_str()) { + return s.clone(); + } + } + let fresh = STATE.get(kind); + if let Ok(mut c) = CACHE.write() { + c.insert(kind.as_str(), fresh.clone()); + } + fresh } pub fn put(kind: MediaServerKind, s: &MediaServerSettings) { STATE.put(kind, s); + invalidate_cache(); } pub fn disconnect(kind: MediaServerKind) { STATE.disconnect(kind); + invalidate_cache(); } /// Which remote sources the Local Library union may show. @@ -57,7 +90,13 @@ pub fn disconnect(kind: MediaServerKind) { /// credentials yet would widen the union's `source IN (…)` over rows nothing /// ever swept. pub fn configured_words() -> Vec<&'static str> { - STATE.configured_words() + // Through the CACHED `get`, not `STATE.configured_words()` — this is called + // once per albums-page load and twice more via `remote_cache_path`. + MediaServerKind::ALL + .iter() + .filter(|k| get(**k).is_configured(**k)) + .map(|k| k.as_str()) + .collect() } /// The shared remote mirror for the active user, or `None` when no remote diff --git a/crates/qbz-qt/src/search_local.rs b/crates/qbz-qt/src/search_local.rs index 4e04d4e4e..44b286caf 100644 --- a/crates/qbz-qt/src/search_local.rs +++ b/crates/qbz-qt/src/search_local.rs @@ -103,7 +103,14 @@ pub(crate) fn local_art_split(source: Option<&str>, path: Option<&str>) -> (Stri // The ROW's source decides, not the shape of the string (design 02 §9 // stage 4). `classify` stood here and would read a Jellyfin token as a // filesystem path purely because it starts with `/`. - match crate::local_rows::art_ref(source, p) { + // Resolved here rather than deferred: this list is the cortinilla's + // bounded result set (76 rows), not the whole library, and the split it + // needs — `art_path` for a file, `art_url` for a fetch — is decided per + // row by definition. + let Some((src, token)) = crate::local_rows::art_token(source, p) else { + return (String::new(), String::new()); + }; + match qbz_source::registry().artwork_token(src, &token, qbz_source::ArtSize::Card) { qbz_source::ArtRef::File(f) => ( String::new(), crate::artwork_qt::file_url(&f.to_string_lossy()),