fix(qt): playlist row removal and remote-source cover art
- Remove from playlist did nothing for a Qobuz row. The membership-id split made a row's display id (the catalog id QML sends) differ from its playlist_track_id (the membership id), but remove_track still matched the open document and called playlist/deleteTracks by the catalog id, so the lookup found nothing and the endpoint got a non-membership id. Resolve the catalog id to the row's membership id, remove exactly that row, and delete by membership; same-catalog duplicates keep their other copy. The local and mixed-sidecar arms were already correct. Verified by smoke test. (#764) - Media-server rows (Jellyfin, Subsonic/Navidrome and Plex) showed no cover in the playlist TrackRow while the queue and NowPlayingBar did. The row fed cached_path the raw artwork token, which classify cannot resolve, whereas the queue namespaces it through portable_artwork_ref. Build the row cover from that same ref, and warm uncached covers on the local-playlist load (the Qobuz loader already did) so the cover appears without playing the track. cc
This commit is contained in:
3 files changed
+121
-51
No files matched your search
@@ -972,11 +972,15 @@ Rectangle {
|
||||
onMoveDownRequested: QbzBridge.playlistMoveRow(String(item.id), 1)
|
||||
onPlayRequested: QbzBridge.playlistPlayTrack(item.id)
|
||||
onEnqueueRequested: function (m) { QbzBridge.playlistEnqueueTrack(item.id, m) }
|
||||
// The DISPLAY row id, as a string. For a Qobuz row it IS
|
||||
// the membership id (`playlist_qt.rs:363-364` sets both
|
||||
// from `track.id`); for a LOCAL row `playlistTrackId` is a
|
||||
// queue id — or 0 on an unresolved one — and the removal
|
||||
// has to be keyed on the id the position map knows.
|
||||
// The DISPLAY row id, as a string — for a Qobuz row the
|
||||
// CATALOG id, for a LOCAL row its library rowid / plex key.
|
||||
// Each Rust arm resolves it to the target it needs: the
|
||||
// Qobuz arm (`playlist_qt::remove_track`) looks up the row's
|
||||
// MEMBERSHIP id from it (the two differ since the
|
||||
// membership-id split), the local/sidecar arms key their
|
||||
// repo removal on it directly. Do NOT send `playlistTrackId`
|
||||
// here: on a LOCAL row it is a queue id (or 0 unresolved),
|
||||
// which the position map does not know.
|
||||
onRemoveRequested: QbzBridge.playlistRemoveTrack(String(item.id))
|
||||
// "Find available version" (contract §6.1) — the ONE
|
||||
// surface that offers it, and the gate is the reference's:
|
||||
|
||||
@@ -866,6 +866,19 @@ pub(crate) fn row_to_display(item: &RowItem) -> (PlaylistTrackRow, Option<QueueT
|
||||
_ => "local",
|
||||
}
|
||||
};
|
||||
// The SAME artwork ref the queue/NPB build (`local_queue_track` →
|
||||
// `portable_artwork_ref`): a media-server row is namespaced
|
||||
// (`jellyfin:<id>` / `subsonic:<id>`) so `classify` can hand it to
|
||||
// the source registry to fetch; a local file / Plex thumb stays its
|
||||
// own on-disk path. The old code fed the RAW `artwork_path` — an
|
||||
// opaque token `cached_path` could not classify — which is why
|
||||
// media rows were art-less in the TrackRow while the queue and NPB,
|
||||
// going through this same ref, showed the cover fine.
|
||||
let art_ref = crate::local_rows::portable_artwork_ref(
|
||||
t,
|
||||
crate::local_rows::ArtworkScope::Track,
|
||||
)
|
||||
.unwrap_or_default();
|
||||
let row = PlaylistTrackRow {
|
||||
id: queue.id.to_string(),
|
||||
playlist_track_id: queue.id,
|
||||
@@ -894,16 +907,14 @@ pub(crate) fn row_to_display(item: &RowItem) -> (PlaylistTrackRow, Option<QueueT
|
||||
} else {
|
||||
t.sample_rate
|
||||
}),
|
||||
art_url: t.artwork_path.clone().unwrap_or_default(),
|
||||
// `TrackRow.qml` renders `artPath`, never `artUrl` — the Qobuz
|
||||
// arm fills it from the download cache, and a local row's
|
||||
// cover is ALREADY on disk (or is a Plex thumb path), which
|
||||
// `cached_path` classifies and turns into the `file://` url
|
||||
// QML can decode. Without this the rows rendered art-less
|
||||
// while the sidebar collage for the same playlist did not.
|
||||
art_path: crate::artwork_qt::cached_path(
|
||||
t.artwork_path.as_deref().unwrap_or_default(),
|
||||
),
|
||||
art_url: art_ref.clone(),
|
||||
// `TrackRow.qml` renders `artPath`, never `artUrl`: a disk hit
|
||||
// (local file / Plex thumb) resolves inline here; a media cover
|
||||
// whose bytes are not cached yet fills in on the load's
|
||||
// artwork-warm republish (`cached_path` re-run after
|
||||
// `download_missing`). Without a resolvable ref the rows
|
||||
// rendered art-less while the sidebar collage did not.
|
||||
art_path: crate::artwork_qt::cached_path(&art_ref),
|
||||
source: source.to_string(),
|
||||
..Default::default()
|
||||
};
|
||||
@@ -1311,7 +1322,26 @@ pub async fn load(runtime: &Runtime, playlist_id: &str) -> bool {
|
||||
source: "local",
|
||||
},
|
||||
);
|
||||
// Media-server sidecar covers (jellyfin/subsonic) are namespaced refs
|
||||
// whose bytes may not be cached yet. Unlike the Qobuz loader this path has
|
||||
// no inline artwork-warm, so a row keyed on such a ref rendered art-less
|
||||
// until the track was played (the queue/NPB fetch on their own). Collect
|
||||
// the unresolved refs BEFORE the doc is adopted, then fetch + republish;
|
||||
// local files and Plex thumbs resolved inline in `row_to_display` and
|
||||
// never reach this list, so a pure-local playlist spawns nothing.
|
||||
let missing_art: Vec<String> = doc
|
||||
.tracks
|
||||
.iter()
|
||||
.filter(|r| r.art_path.is_empty() && !r.art_url.is_empty())
|
||||
.map(|r| r.art_url.clone())
|
||||
.collect();
|
||||
crate::playlist_qt::adopt_doc(doc);
|
||||
if !missing_art.is_empty() {
|
||||
crate::spawn(async move {
|
||||
crate::artwork_qt::download_missing(missing_art).await;
|
||||
crate::playlist_qt::rewarm_row_art();
|
||||
});
|
||||
}
|
||||
true
|
||||
}
|
||||
|
||||
|
||||
@@ -781,6 +781,29 @@ pub(crate) fn adopt_doc(doc: PlaylistDoc) {
|
||||
*PAGE.lock().unwrap() = Some(PageState { doc });
|
||||
}
|
||||
|
||||
/// Re-resolve any row (or header) artwork that was NOT on disk when the page
|
||||
/// was built, then republish. The LOCAL loader spawns this after
|
||||
/// `download_missing` lands a media-server sidecar cover (a `jellyfin:` /
|
||||
/// `subsonic:` ref), which mirrors the warm-then-republish the Qobuz loader
|
||||
/// does inline. It reads whatever page is open now, so it is a safe no-op if
|
||||
/// the user has navigated away.
|
||||
pub(crate) fn rewarm_row_art() {
|
||||
let doc = with_doc(|d| {
|
||||
if !d.cover_url.is_empty() && d.cover_path.is_empty() {
|
||||
d.cover_path = crate::artwork_qt::cached_path(&d.cover_url);
|
||||
}
|
||||
for row in d.tracks.iter_mut() {
|
||||
if row.art_path.is_empty() && !row.art_url.is_empty() {
|
||||
row.art_path = crate::artwork_qt::cached_path(&row.art_url);
|
||||
}
|
||||
}
|
||||
d.clone()
|
||||
});
|
||||
if let Some(doc) = doc {
|
||||
publish(&doc);
|
||||
}
|
||||
}
|
||||
|
||||
fn with_doc<R>(f: impl FnOnce(&mut PlaylistDoc) -> R) -> Option<R> {
|
||||
let mut guard = PAGE.lock().unwrap();
|
||||
guard.as_mut().map(|page| f(&mut page.doc))
|
||||
@@ -1914,59 +1937,72 @@ pub async fn delete_playlist(runtime: &Arc<AppRuntime<LoggingAdapter>>) -> Resul
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// Per-row "Remove from playlist" (owner-gated, spec §1.6.1 — the Qobuz
|
||||
/// API is owner-only). playlist_track_ids = the membership row ids.
|
||||
pub async fn remove_track(runtime: &Arc<AppRuntime<LoggingAdapter>>, playlist_track_id: u64) {
|
||||
let Some((pid, playlist_track_id)) = with_doc(|d| {
|
||||
/// API is owner-only).
|
||||
///
|
||||
/// `catalog_id` is the DISPLAY row id QML sends (`PlaylistTrackRow::id`, which
|
||||
/// is the catalog track id — the same value the local/sidecar arms of
|
||||
/// `crate::playlist_remove_track` route on). Qobuz's `playlist/deleteTracks`
|
||||
/// addresses the MEMBERSHIP id (`playlist_track_id`) instead, so it is resolved
|
||||
/// from the open document's matching row here.
|
||||
///
|
||||
/// This resolution IS the fix: the membership-id split (row build above) made
|
||||
/// `id` and `playlist_track_id` two different numbers, but the delete kept
|
||||
/// keying on the value QML sends — the catalog one — so `find`/`retain` matched
|
||||
/// nothing and the endpoint got a non-membership id, i.e. the click rendered
|
||||
/// and no-opped. Same-catalog duplicates resolve to their first occurrence:
|
||||
/// QML sends the same display id for both copies, so that is the most this key
|
||||
/// can disambiguate, and it stays offline-safe (a failed API call reloads).
|
||||
pub async fn remove_track(runtime: &Arc<AppRuntime<LoggingAdapter>>, catalog_id: u64) {
|
||||
let Some((pid, membership_id)) = with_doc(|d| {
|
||||
if !d.is_owner {
|
||||
return (None, 0);
|
||||
return None;
|
||||
}
|
||||
(d.id.parse::<u64>().ok(), playlist_track_id)
|
||||
}) else {
|
||||
return;
|
||||
};
|
||||
let Some(pid) = pid else { return };
|
||||
// Optimistic removal; the API is the source of truth on failure. The
|
||||
// row's CATALOG id is captured before it goes — the membership snapshot
|
||||
// speaks track ids, not membership-row ids.
|
||||
let removed_track_id = with_doc(|d| {
|
||||
let track_id = d
|
||||
let pid = d.id.parse::<u64>().ok()?;
|
||||
let membership_id = d
|
||||
.tracks
|
||||
.iter()
|
||||
.find(|t| t.playlist_track_id == playlist_track_id)
|
||||
.and_then(|t| t.id.parse::<u64>().ok());
|
||||
d.tracks
|
||||
.retain(|t| t.playlist_track_id != playlist_track_id);
|
||||
.find(|t| t.id.parse::<u64>().ok() == Some(catalog_id))
|
||||
.map(|t| t.playlist_track_id)?;
|
||||
Some((pid, membership_id))
|
||||
})
|
||||
.flatten() else {
|
||||
return;
|
||||
};
|
||||
// Optimistic removal of exactly the matched row, keyed on the membership
|
||||
// id so a same-catalog duplicate keeps its other copy. The API is the
|
||||
// source of truth on failure.
|
||||
with_doc(|d| {
|
||||
d.tracks.retain(|t| t.playlist_track_id != membership_id);
|
||||
d.track_count = d.tracks.len() as i32;
|
||||
d.total_duration = total_duration_label(&d.tracks);
|
||||
let doc = d.clone();
|
||||
publish(&doc);
|
||||
track_id
|
||||
})
|
||||
.flatten();
|
||||
});
|
||||
match runtime
|
||||
.core()
|
||||
.remove_tracks_from_playlist(pid, &[playlist_track_id])
|
||||
.remove_tracks_from_playlist(pid, &[membership_id])
|
||||
.await
|
||||
{
|
||||
Ok(()) => {
|
||||
if let Some(track_id) = removed_track_id {
|
||||
let _ = tokio::task::spawn_blocking(move || {
|
||||
crate::library_db_qt::with_db(true, |db| {
|
||||
Ok(db.with_connection(|conn| {
|
||||
qbz_library::qobuz_playlist_snapshot::apply_removed_tracks(
|
||||
conn,
|
||||
pid,
|
||||
&[track_id],
|
||||
)
|
||||
}))
|
||||
})
|
||||
// The membership snapshot speaks catalog track ids, not
|
||||
// membership-row ids.
|
||||
let _ = tokio::task::spawn_blocking(move || {
|
||||
crate::library_db_qt::with_db(true, |db| {
|
||||
Ok(db.with_connection(|conn| {
|
||||
qbz_library::qobuz_playlist_snapshot::apply_removed_tracks(
|
||||
conn,
|
||||
pid,
|
||||
&[catalog_id],
|
||||
)
|
||||
}))
|
||||
})
|
||||
.await;
|
||||
}
|
||||
})
|
||||
.await;
|
||||
}
|
||||
Err(e) => {
|
||||
log::error!(
|
||||
"[qbz-qt] remove track {playlist_track_id} from playlist {pid} failed: {e}"
|
||||
"[qbz-qt] remove membership {membership_id} (catalog {catalog_id}) \
|
||||
from playlist {pid} failed: {e}"
|
||||
);
|
||||
// Reload to reconcile (bounded-retry equivalent — the Slint
|
||||
// reconciles the same way after failed playlist ops).
|
||||
|
||||
Reference in new issue
Block a user