From c02e7713d98376e8dfda1e4882c53e9c9ed2d8f8 Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Tue, 18 Aug 2026 20:25:22 -0600 Subject: [PATCH] perf(qt): stop republishing the Recommendations document for artwork MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers that resolved after the document was published forced a second `publish_snapshot`, and a republish hands QML a brand-new array: `QQuickItemView::setModel()` then resets each rail's scroll offset and tears down its QQmlDelegateModel. So every landing batch rebuilt all nine rails' delegates, which is what a screen capture of a cold Recommendations load shows — the rows visibly reassembling while covers trickle in, filling out of order both between rows and within a row. The fix is the pattern this port already applies twice: covers travel as a url-keyed PATCH and the document is left alone. `foryou_qt::emit_foryou_art` is the precedent for the For You rails, and `myqbz_bridge` cites it verbatim for MyQBZ; the reco rails were the remaining holdout. The download path is deliberately untouched. This changes only how a resolved path is ANNOUNCED, never what is fetched or cached, so every other consumer of the shared image cache — the now-playing bar, the queue, the `recent` rails — sees exactly what it saw before. Two traps this had to close, both already paid for once on the For You side: * `sectionArtOf()` falls back to `item.artPath`. `recoArt` only ever holds Recommendations urls, so for a Home / Editor's Picks / For You card the lookup misses and the binding returns what it always returned. No surface that reads a baked `artPath` can lose its cover to this — by construction, not by care. The pinned rail, fed by its own store, is untouched. * `anyItemArtPending` now consults the map. A patched cover never gets its `artPath` back through a republish, so the bare "artUrl set and artPath empty" predicate would be true forever and pin the 900ms skeleton pulse Timer on. `refreshRecoArt()` on mount is the third: HomeView is destroyed on every navigation while this document is published once per session, so a fresh mount must be re-handed what already resolved or the covers come back blank. Weekly's two rails still republish their own narrower property; their blast radius is two rails, not nine, and widening this change was not worth it. Verified by owner smoke: the three checks (nothing else lost its covers, the rails stop reassembling, covers survive navigating away and back) all pass. cc --- crates/qbz-qt/qml/views/HomeView.qml | 62 ++++++++++++++++--- crates/qbz-qt/src/home_bridge.rs | 28 +++++++++ crates/qbz-qt/src/recommendations_qt.rs | 79 +++++++++++++++++++++++-- 3 files changed, 158 insertions(+), 11 deletions(-) diff --git a/crates/qbz-qt/qml/views/HomeView.qml b/crates/qbz-qt/qml/views/HomeView.qml index e8b2f61da..31bbc2b98 100644 --- a/crates/qbz-qt/qml/views/HomeView.qml +++ b/crates/qbz-qt/qml/views/HomeView.qml @@ -200,7 +200,49 @@ Rectangle { // never downloads, never republishes a document, and emits nothing at all // when nothing is resolved yet — so the first, cold mount is a no-op and the // in-flight download's own emit still fills the map. - Component.onCompleted: QbzHome.refreshForyouArt() + Component.onCompleted: { QbzHome.refreshForyouArt(); QbzHome.refreshRecoArt() } + // Recommendations' rails used to get their covers by REPUBLISHING + // `recoSectionsJson` once per landing batch, which hands `model:` a new JS + // array — `QQuickItemView::setModel()` then resets the rail's scroll and + // tears down its QQmlDelegateModel, so every batch rebuilt all nine rails' + // delegates (the same defect the For You rails were moved off, :161). Rust + // now emits a url-keyed patch instead and leaves the document alone. + property var recoArt: ({}) + Connections { + target: QbzHome + function onRecoArtReady(patchJson) { + var patch + try { + patch = JSON.parse(patchJson) + } catch (e) { + return + } + var m = root.recoArt + var changed = false + for (var url in patch) { + if (m[url] !== patch[url]) { + m[url] = patch[url] + changed = true + } + } + if (changed) + root.recoArt = Object.assign({}, m) // rebind needs a NEW ref + } + } + + /// Cover for a card that came out of a SECTIONS document: the patch map + /// first, then whatever the document itself carried. + /// + /// The fallback is what makes this safe to put on rails that render the + /// other tabs too. `recoArt` only ever holds Recommendations urls, so for a + /// Home / Editor's Picks / For You card the lookup misses and the result is + /// `item.artPath` — byte for byte what the binding read before. No consumer + /// of a baked `artPath` can lose its cover to this change. + function sectionArtOf(item) { + return (item && item.artUrl && root.recoArt[item.artUrl]) + || (item ? (item.artPath || "") : "") + } + /// The patch map first, then whatever the document happened to carry (a /// full Home reload publishes the paths it already has). function forYouArtOf(item) { @@ -295,9 +337,15 @@ Rectangle { } return false } + // Consults the patch map, NOT a bare `artPath === ""`. A Recommendations + // cover that arrives as a patch never gets its `artPath` back through a + // republish any more, so the bare predicate would stay true forever and + // pin the 900ms skeleton pulse Timer on — the exact trap the For You + // rails hit (see anyForYouArtPending). Unchanged for every other tab, + // where `sectionArtOf` just returns `artPath`. function anyItemArtPending(items) { for (var i = 0; i < items.length; i++) { - if ((items[i].artUrl || "") !== "" && (items[i].artPath || "") === "") + if ((items[i].artUrl || "") !== "" && root.sectionArtOf(items[i]) === "") return true } return false @@ -425,7 +473,7 @@ Rectangle { qualityTier: modelData.qualityTier ribbon: modelData.ribbon ribbonKind: modelData.ribbonKind - artSource: modelData.artPath + artSource: root.sectionArtOf(modelData) isPinned: modelData.isPinned // Snapshot url the pin payload persists (artPath is // a local cache path — see AlbumCard.artworkUrl). @@ -455,7 +503,7 @@ Rectangle { width: 200 height: 200 pending: (modelData.artUrl || "") !== "" - coverSource: modelData.artPath || "" + coverSource: root.sectionArtOf(modelData) phase: root.skelPhase cellIndex: index // A cover whose download fails republishes the @@ -529,7 +577,7 @@ Rectangle { clip: true RoundedImage { anchors.fill: parent - source: card.artPath || "" + source: root.sectionArtOf(card) radius: 4 } } @@ -1184,7 +1232,7 @@ Rectangle { model: plRail.sectionData.items delegate: PlaylistCard { item: modelData - artSource: modelData.artPath || "" + artSource: root.sectionArtOf(modelData) // The card's own default is `false`; the // pin state travels on the row (home_qt // `map_playlist`), so it has to be handed @@ -1236,7 +1284,7 @@ Rectangle { model: arRail.sectionData.items delegate: ArtistCard { item: modelData - artSource: modelData.artPath || "" + artSource: root.sectionArtOf(modelData) // Same hand-over as the playlist rail — // home_qt `map_fav_artist` publishes the // pin state and the card defaults to false. diff --git a/crates/qbz-qt/src/home_bridge.rs b/crates/qbz-qt/src/home_bridge.rs index 00a491761..d5ace9016 100644 --- a/crates/qbz-qt/src/home_bridge.rs +++ b/crates/qbz-qt/src/home_bridge.rs @@ -245,6 +245,14 @@ pub mod qbz_home { #[qinvokable] fn refresh_foryou_art(self: Pin<&mut QbzHome>); + /// Twin of `refresh_foryou_art` for the Recommendations rails. HomeView + /// is destroyed on every navigation and this document is published once + /// per session, so a fresh mount must be re-handed the covers that + /// arrived as a patch — otherwise they come back blank and the pending + /// probe never goes false, pinning the skeleton pulse Timer on. + #[qinvokable] + fn refresh_reco_art(self: Pin<&mut QbzHome>); + // --- Label releases sub-view -------------------------------------- #[qinvokable] fn label_releases_load_more(self: Pin<&mut QbzHome>); @@ -304,6 +312,14 @@ pub mod qbz_home { /// `foryouArtReady`, i.e. the handler is `onForyouArtReady(patchJson)`. #[qsignal] fn foryou_art_ready(self: Pin<&mut QbzHome>, patch_json: QString); + + /// Same contract as `foryouArtReady`, for the Recommendations tab's + /// rails: covers resolved AFTER the document was published travel as a + /// url-keyed patch instead of forcing a republish, which used to tear + /// down every rail's delegate model on each landing batch. QML spelling + /// is `recoArtReady`, so the handler is `onRecoArtReady(patchJson)`. + #[qsignal] + fn reco_art_ready(self: Pin<&mut QbzHome>, patch_json: QString); } impl cxx_qt::Threading for QbzHome {} @@ -616,6 +632,18 @@ impl qbz_home::QbzHome { .foryou_art_ready(QString::from(patch.as_str())); } + /// Emitted SYNCHRONOUSLY for the same reason as `refresh_foryou_art`: this + /// runs on the Qt thread from HomeView's `Component.onCompleted`, and the + /// `ui()` hop no-ops until `boot()` has registered the thread. + pub fn refresh_reco_art(mut self: Pin<&mut Self>) { + let patch = crate::recommendations_qt::resolved_art_patch(); + if patch.is_empty() { + return; + } + self.as_mut() + .reco_art_ready(QString::from(patch.as_str())); + } + // --- Label releases ---------------------------------------------------- pub fn label_releases_load_more(self: Pin<&mut Self>) { diff --git a/crates/qbz-qt/src/recommendations_qt.rs b/crates/qbz-qt/src/recommendations_qt.rs index 9d5a29d7f..23539afa0 100644 --- a/crates/qbz-qt/src/recommendations_qt.rs +++ b/crates/qbz-qt/src/recommendations_qt.rs @@ -48,7 +48,7 @@ //! from artists you know" row exactly as the reference does with its larger //! played-or-favorited set. -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Mutex; @@ -340,13 +340,84 @@ fn publish_snapshot(download: bool) { if download && !missing.is_empty() { crate::spawn(async move { crate::artwork_qt::download_missing(missing).await; - // download=false: the covers are on disk now, so this pass can - // only attach — it can never spawn another download round. - publish_snapshot(false); + // Was a second `publish_snapshot(false)`. Republishing for artwork + // hands QML a brand-new array, and `QQuickItemView::setModel()` + // then resets each rail's scroll offset and tears down its + // QQmlDelegateModel — see the note at line 273 and the identical + // reasoning in `HomeView.qml:161`. Every landing batch therefore + // rebuilt all nine rails' delegates. The covers now travel as a + // url-keyed PATCH, exactly as `foryou_qt::emit_foryou_art` does + // for the For You rails and `myqbz_bridge` does for MyQBZ; the + // document is left alone. + // + // The download itself is untouched: this changes only how a + // resolved path is ANNOUNCED, never what gets fetched or cached, + // so every other consumer of the shared image cache (the now- + // playing bar, the queue, the `recent` rails) sees exactly what it + // saw before. + emit_reco_art(art_patch()); }); } } +/// Every (art_url -> art_path) this tab's rows can resolve from the disk cache +/// right now. Attaches into a THROWAWAY copy of the sections, so it can never +/// mutate or republish the live document, and never downloads. +fn art_patch() -> BTreeMap { + let rows = rows_snapshot(); + let mut secs = sections(&rows); + let _ = crate::artwork_qt::attach_cached(&mut secs); + let mut patch: BTreeMap = BTreeMap::new(); + for section in secs.iter() { + for card in section.items.iter() { + if !card.art_url.is_empty() && !card.art_path.is_empty() { + patch.insert(card.art_url.clone(), card.art_path.clone()); + } + } + } + patch +} + +/// ONE emit per batch — the receiving QML map is a `var`, so it only notifies +/// on a new object reference, and an emit per url would copy the growing map +/// and invalidate every cover binding once per cover. Nothing paints later for +/// it: `download_missing().await` settles the whole batch first. +fn emit_reco_art(patch: BTreeMap) { + if patch.is_empty() { + return; + } + let json = serde_json::to_string(&patch).unwrap_or_else(|_| "{}".to_string()); + crate::home_bridge::ui(move |mut b| { + b.as_mut().reco_art_ready(QString::from(json.as_str())); + }); +} + +/// Re-hand the resolved covers to a freshly mounted HomeView. HomeView is +/// destroyed and rebuilt on every navigation while this document is published +/// once per session, so without this the covers that arrived as a PATCH (i.e. +/// everything that was not already on disk at publish time) would come back +/// blank for the rest of the session — and the "still pending" probe would +/// stay true forever and pin the skeleton pulse Timer on. The exact trap +/// `foryou_qt::resolved_art_patch` exists to close. +/// +/// Never downloads, never republishes; returns "" when nothing is resolved, so +/// a cold first mount emits nothing and cannot clear a map an in-flight +/// download is about to fill. +pub(crate) fn resolved_art_patch() -> String { + if !LOADED.load(Ordering::SeqCst) { + return String::new(); + } + let patch = art_patch(); + if patch.is_empty() { + return String::new(); + } + log::debug!( + "[qbz-qt] reco art re-handed to a fresh HomeView: {} covers", + patch.len() + ); + serde_json::to_string(&patch).unwrap_or_else(|_| "{}".to_string()) +} + // --------------------------------------------------------------------------- // Entry points // ---------------------------------------------------------------------------