mirror of
https://github.com/Psychotoxical/psysonic.git
synced 2026-07-21 23:05:46 +00:00
* fix(library): show album artist in album grids (#1056) Prefer album-artist tags over track artist when building album rows from the local index, and align grid cards with OpenSubsonic displayArtist. * fix(library): align album artist in FTS, search, and offline paths (#1056) Apply album-artist preference in FTS album dedupe and live search, fix offline pin hydration order, and use albumArtistDisplayName in remaining cheap UI/export/download call sites. * docs(changelog): album artist grid fix for compilations (PR #1057) * fix(library): parity guard and live-search album artist helper (#1056) Align SQL ELSE branch with pick_album_group_artist trimming, add parity test, and use albumArtistDisplayName in LiveSearch and MobileSearchOverlay.
This commit is contained in:
@@ -49,6 +49,7 @@ type AlbumBrowseTrackRow = (
|
||||
String,
|
||||
Option<String>,
|
||||
Option<String>,
|
||||
Option<String>,
|
||||
Option<i64>,
|
||||
Option<String>,
|
||||
Option<String>,
|
||||
@@ -648,8 +649,8 @@ fn build_album_from_fts(
|
||||
let where_sql = w.where_sql();
|
||||
store.with_read_conn(|conn| {
|
||||
let sql = format!(
|
||||
"SELECT t.server_id, t.album_id, t.album, t.artist, t.artist_id, t.year, \
|
||||
t.genre, t.cover_art_id, t.starred_at, t.synced_at \
|
||||
"SELECT t.server_id, t.album_id, t.album, t.artist, t.album_artist, t.artist_id, \
|
||||
t.year, t.genre, t.cover_art_id, t.starred_at, t.synced_at \
|
||||
FROM track t \
|
||||
WHERE {where_sql}"
|
||||
);
|
||||
@@ -668,13 +669,27 @@ fn build_album_from_fts(
|
||||
r.get(7)?,
|
||||
r.get(8)?,
|
||||
r.get(9)?,
|
||||
r.get(10)?,
|
||||
))
|
||||
})?
|
||||
.collect::<rusqlite::Result<Vec<_>>>()?;
|
||||
|
||||
let mut seen = HashSet::new();
|
||||
let mut deduped: Vec<LibraryAlbumDto> = Vec::new();
|
||||
for (server_id, album_id, album, artist, artist_id, year, genre, cover_art_id, starred_at, synced_at) in rows {
|
||||
for (
|
||||
server_id,
|
||||
album_id,
|
||||
album,
|
||||
track_artist,
|
||||
album_artist,
|
||||
artist_id,
|
||||
year,
|
||||
genre,
|
||||
cover_art_id,
|
||||
starred_at,
|
||||
synced_at,
|
||||
) in rows
|
||||
{
|
||||
if !seen.insert(album_id.clone()) {
|
||||
continue;
|
||||
}
|
||||
@@ -682,7 +697,10 @@ fn build_album_from_fts(
|
||||
server_id,
|
||||
id: album_id,
|
||||
name: album,
|
||||
artist,
|
||||
artist: crate::album_compilation_filter::pick_album_group_artist(
|
||||
track_artist,
|
||||
album_artist,
|
||||
),
|
||||
artist_id,
|
||||
song_count: None,
|
||||
duration_sec: None,
|
||||
@@ -2067,6 +2085,24 @@ mod tests {
|
||||
assert_eq!(resp.albums[0].artist.as_deref(), Some("Various Artists"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn track_grouped_album_browse_prefers_album_artist_over_track_artist() {
|
||||
let store = LibraryStore::open_in_memory();
|
||||
let mut t1 = track("s1", "t1", "Anthem", "Groove Armada", "Back to Mine");
|
||||
t1.album_id = Some("al_mix".into());
|
||||
t1.album_artist = Some("Underworld".into());
|
||||
let mut t2 = track("s1", "t2", "Zebra", "UNKLE", "Back to Mine");
|
||||
t2.album_id = Some("al_mix".into());
|
||||
t2.album_artist = Some("Underworld".into());
|
||||
TrackRepository::new(&store)
|
||||
.upsert_batch(&[t1, t2])
|
||||
.unwrap();
|
||||
let r = req("s1", &[EntityKind::Album]);
|
||||
let resp = run_advanced_search(&store, &r).unwrap();
|
||||
assert_eq!(resp.albums.len(), 1);
|
||||
assert_eq!(resp.albums[0].artist.as_deref(), Some("Underworld"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn compilation_filter_on_track_grouped_album_browse() {
|
||||
let store = LibraryStore::open_in_memory();
|
||||
|
||||
@@ -49,16 +49,30 @@ pub fn various_artists_label(s: &str) -> bool {
|
||||
s.trim().to_ascii_lowercase().contains("various artists")
|
||||
}
|
||||
|
||||
/// Track-grouped album rows: prefer album artist when it marks a VA compilation.
|
||||
/// SQL mirror of [`pick_album_group_artist`] for track-grouped browse subqueries
|
||||
/// (`la`). Used where `ORDER BY` / `COALESCE(a.artist, …)` must stay in SQL;
|
||||
/// keep both implementations in sync.
|
||||
pub fn sql_track_group_display_artist(alias: &str) -> String {
|
||||
format!(
|
||||
"CASE WHEN trim(coalesce({a}.album_artist, '')) != '' \
|
||||
THEN trim({a}.album_artist) \
|
||||
ELSE NULLIF(trim(coalesce({a}.artist, '')), '') END",
|
||||
a = alias
|
||||
)
|
||||
}
|
||||
|
||||
/// Row-mapper form of the album-artist display rule — mirror of
|
||||
/// [`sql_track_group_display_artist`]. Prefer a non-empty album-artist tag;
|
||||
/// fall back to track artist only when album artist is absent (solo albums without TALB).
|
||||
pub fn pick_album_group_artist(
|
||||
track_artist: Option<String>,
|
||||
album_artist: Option<String>,
|
||||
) -> Option<String> {
|
||||
let aa = album_artist.as_deref().unwrap_or("").trim();
|
||||
if various_artists_label(aa) {
|
||||
if !aa.is_empty() {
|
||||
return Some(aa.to_string());
|
||||
}
|
||||
track_artist
|
||||
track_artist.filter(|s| !s.trim().is_empty())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -81,14 +95,70 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pick_album_group_artist_prefers_va_album_artist() {
|
||||
fn pick_album_group_artist_prefers_nonempty_album_artist() {
|
||||
assert_eq!(
|
||||
pick_album_group_artist(Some("Alice".into()), Some("Various Artists".into())),
|
||||
Some("Various Artists".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
pick_album_group_artist(Some("Groove Armada".into()), Some("Underworld".into())),
|
||||
Some("Underworld".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
pick_album_group_artist(Some("Alice".into()), Some("Bob".into())),
|
||||
Some("Alice".to_string())
|
||||
Some("Bob".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pick_album_group_artist_falls_back_to_track_artist() {
|
||||
assert_eq!(
|
||||
pick_album_group_artist(Some("Alice".into()), None),
|
||||
Some("Alice".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
pick_album_group_artist(Some("Alice".into()), Some("".into())),
|
||||
Some("Alice".to_string())
|
||||
);
|
||||
assert_eq!(pick_album_group_artist(None, None), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sql_track_group_display_artist_matches_pick_album_group_artist() {
|
||||
let conn = rusqlite::Connection::open_in_memory().unwrap();
|
||||
conn.execute(
|
||||
"CREATE TABLE la (artist TEXT, album_artist TEXT)",
|
||||
[],
|
||||
)
|
||||
.unwrap();
|
||||
let sql = format!("SELECT {} FROM la", sql_track_group_display_artist("la"));
|
||||
|
||||
let cases: [(&str, &str); 7] = [
|
||||
("Groove Armada", "Underworld"),
|
||||
("Alice", ""),
|
||||
("", "Various Artists"),
|
||||
("Alice", "Bob"),
|
||||
(" ", "Bob"),
|
||||
("Alice", " "),
|
||||
("", ""),
|
||||
];
|
||||
|
||||
for (track, album) in cases {
|
||||
conn.execute("DELETE FROM la", []).unwrap();
|
||||
conn.execute(
|
||||
"INSERT INTO la (artist, album_artist) VALUES (?1, ?2)",
|
||||
rusqlite::params![track, album],
|
||||
)
|
||||
.unwrap();
|
||||
let sql_out: Option<String> = conn.query_row(&sql, [], |r| r.get(0)).ok();
|
||||
let rust_out = pick_album_group_artist(
|
||||
(!track.is_empty()).then(|| track.to_string()),
|
||||
(!album.is_empty()).then(|| album.to_string()),
|
||||
);
|
||||
assert_eq!(
|
||||
sql_out, rust_out,
|
||||
"track={track:?} album={album:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -81,12 +81,13 @@ pub fn get_artist_lossless_browse(
|
||||
}
|
||||
let album_where_sql = album_where.join(" AND ");
|
||||
|
||||
let la_artist = crate::album_compilation_filter::sql_track_group_display_artist("la");
|
||||
let albums_sql = format!(
|
||||
"SELECT \
|
||||
la.server_id, \
|
||||
la.album_id, \
|
||||
COALESCE(a.name, la.album_name), \
|
||||
COALESCE(a.artist, la.artist), \
|
||||
COALESCE(a.artist, {la_artist}), \
|
||||
COALESCE(a.artist_id, la.artist_id), \
|
||||
COALESCE(a.song_count, la.track_count), \
|
||||
COALESCE(a.duration_sec, la.duration_sec), \
|
||||
@@ -102,6 +103,7 @@ pub fn get_artist_lossless_browse(
|
||||
t.album_id, \
|
||||
MAX(t.album) AS album_name, \
|
||||
MAX(t.artist) AS artist, \
|
||||
MAX(t.album_artist) AS album_artist, \
|
||||
MAX(t.artist_id) AS artist_id, \
|
||||
MAX(t.year) AS year, \
|
||||
MAX(t.genre) AS genre, \
|
||||
|
||||
@@ -19,19 +19,20 @@ fn trimmed_nonempty(s: Option<&str>) -> Option<String> {
|
||||
}
|
||||
|
||||
fn genre_album_order_sql(sort: &[LibrarySortClause]) -> String {
|
||||
let la_artist = crate::album_compilation_filter::sql_track_group_display_artist("la");
|
||||
let mut keys: Vec<String> = Vec::new();
|
||||
for s in sort {
|
||||
let col = match s.field.as_str() {
|
||||
"name" => "COALESCE(a.name, la.album_name) COLLATE NOCASE",
|
||||
"artist" => "COALESCE(a.artist, la.artist) COLLATE NOCASE",
|
||||
"year" => "COALESCE(a.year, la.year)",
|
||||
"name" => "COALESCE(a.name, la.album_name) COLLATE NOCASE".to_string(),
|
||||
"artist" => format!("COALESCE(a.artist, {la_artist}) COLLATE NOCASE"),
|
||||
"year" => "COALESCE(a.year, la.year)".to_string(),
|
||||
_ => continue,
|
||||
};
|
||||
let dir = match s.dir {
|
||||
SortDir::Asc => "ASC",
|
||||
SortDir::Desc => "DESC",
|
||||
};
|
||||
keys.push(format!("{col} {dir}"));
|
||||
keys.push(format!("{col} {dir}", col = col));
|
||||
}
|
||||
if keys.is_empty() {
|
||||
keys.push("COALESCE(a.name, la.album_name) COLLATE NOCASE ASC".to_string());
|
||||
@@ -123,12 +124,13 @@ pub fn list_albums_by_genre(
|
||||
}
|
||||
|
||||
let where_sql = where_clauses.join(" AND ");
|
||||
let la_artist = crate::album_compilation_filter::sql_track_group_display_artist("la");
|
||||
let sql = format!(
|
||||
"SELECT \
|
||||
la.server_id, \
|
||||
la.album_id, \
|
||||
COALESCE(a.name, la.album_name), \
|
||||
COALESCE(a.artist, la.artist), \
|
||||
COALESCE(a.artist, {la_artist}), \
|
||||
COALESCE(a.artist_id, la.artist_id), \
|
||||
COALESCE(a.song_count, la.track_count), \
|
||||
COALESCE(a.duration_sec, la.duration_sec), \
|
||||
@@ -144,6 +146,7 @@ pub fn list_albums_by_genre(
|
||||
t.album_id, \
|
||||
MAX(t.album) AS album_name, \
|
||||
MAX(t.artist) AS artist, \
|
||||
MAX(t.album_artist) AS album_artist, \
|
||||
MAX(t.artist_id) AS artist_id, \
|
||||
MAX(t.year) AS year, \
|
||||
MAX(t.genre) AS genre, \
|
||||
|
||||
@@ -244,8 +244,9 @@ fn query_albums(
|
||||
ORDER BY rank \
|
||||
LIMIT ?\
|
||||
) \
|
||||
SELECT t.server_id, t.album_id, t.album, t.artist, t.artist_id, t.year, \
|
||||
t.genre, t.cover_art_id, t.starred_at, t.synced_at, MIN(h.rank) AS best_rank \
|
||||
SELECT t.server_id, t.album_id, MAX(t.album), MAX(t.artist), MAX(t.album_artist), \
|
||||
MAX(t.artist_id), MAX(t.year), MAX(t.genre), MAX(t.cover_art_id), \
|
||||
MAX(t.starred_at), MAX(t.synced_at), MIN(h.rank) AS best_rank \
|
||||
FROM fts_hits h \
|
||||
JOIN track t ON t.rowid = h.rowid \
|
||||
WHERE t.server_id = ? \
|
||||
@@ -261,24 +262,29 @@ fn query_albums(
|
||||
params.push(rusqlite::types::Value::Integer(LIVE_SEARCH_FTS_CANDIDATE_CAP));
|
||||
params.push(rusqlite::types::Value::Text(server_id.to_string()));
|
||||
append_library_scope(&mut sql, &mut params, library_scope);
|
||||
sql.push_str(" GROUP BY t.album_id ORDER BY best_rank LIMIT ?");
|
||||
sql.push_str(" GROUP BY t.server_id, t.album_id ORDER BY best_rank LIMIT ?");
|
||||
params.push(rusqlite::types::Value::Integer(i64::from(limit)));
|
||||
let mut stmt = conn.prepare(&sql)?;
|
||||
let mut out = Vec::new();
|
||||
for row in stmt.query_map(rusqlite::params_from_iter(params.iter()), |r| {
|
||||
let track_artist: Option<String> = r.get(3)?;
|
||||
let album_artist: Option<String> = r.get(4)?;
|
||||
Ok(LibraryAlbumDto {
|
||||
server_id: r.get(0)?,
|
||||
id: r.get(1)?,
|
||||
name: r.get(2)?,
|
||||
artist: r.get(3)?,
|
||||
artist_id: r.get(4)?,
|
||||
artist: crate::album_compilation_filter::pick_album_group_artist(
|
||||
track_artist,
|
||||
album_artist,
|
||||
),
|
||||
artist_id: r.get(5)?,
|
||||
song_count: None,
|
||||
duration_sec: None,
|
||||
year: r.get(5)?,
|
||||
genre: r.get(6)?,
|
||||
cover_art_id: r.get(7)?,
|
||||
starred_at: r.get(8)?,
|
||||
synced_at: r.get(9)?,
|
||||
year: r.get(6)?,
|
||||
genre: r.get(7)?,
|
||||
cover_art_id: r.get(8)?,
|
||||
starred_at: r.get(9)?,
|
||||
synced_at: r.get(10)?,
|
||||
raw_json: serde_json::Value::Null,
|
||||
})
|
||||
})? {
|
||||
|
||||
@@ -44,12 +44,13 @@ pub fn list_lossless_albums(
|
||||
}
|
||||
|
||||
let where_sql = where_clauses.join(" AND ");
|
||||
let la_artist = crate::album_compilation_filter::sql_track_group_display_artist("la");
|
||||
let sql = format!(
|
||||
"SELECT \
|
||||
la.server_id, \
|
||||
la.album_id, \
|
||||
COALESCE(a.name, la.album_name), \
|
||||
COALESCE(a.artist, la.artist), \
|
||||
COALESCE(a.artist, {la_artist}), \
|
||||
COALESCE(a.artist_id, la.artist_id), \
|
||||
COALESCE(a.song_count, la.track_count), \
|
||||
COALESCE(a.duration_sec, la.duration_sec), \
|
||||
@@ -65,6 +66,7 @@ pub fn list_lossless_albums(
|
||||
t.album_id, \
|
||||
MAX(t.album) AS album_name, \
|
||||
MAX(t.artist) AS artist, \
|
||||
MAX(t.album_artist) AS album_artist, \
|
||||
MAX(t.artist_id) AS artist_id, \
|
||||
MAX(t.year) AS year, \
|
||||
MAX(t.genre) AS genre, \
|
||||
|
||||
Reference in New Issue
Block a user