Cover Art Filler: fix album art or artist art independently (Pache711)
Pache711: a cover-art finding showed the (correct) found album art next to a (wrong) artist image with one "Apply Art" button — no way to take one and skip the other. Turned out "Apply Art" only ever applied ALBUM art anyway; the artist image was display-only context, so the bundling was an illusion the UI created. Now the finding is genuinely multi-target: - scan (missing_cover_art.py): also searches for an artist image (always, so a WRONG existing one can be replaced — Boulder's call), name-matched exactly. Stored as found_artist_url only when it differs from the current artist thumb, so nothing is offered when there's nothing to change. - apply (_fix_missing_cover_art): honors a target via _fix_action — 'album' (default, unchanged "Apply Art" behavior: DB thumb + embed + cover.jpg), 'artist' (the artist's DB image), or 'both'. New _fix_artist_art sets artists.thumb_url for the album's artist. - UI: each found image gets its own apply button — "Use for album" / "Use for artist". Applying either resolves the finding, so taking the correct one and ignoring the wrong one IS "fix one, dismiss the other". Current artist art shows as "(current)" context with no button. Default stays album-only, so the plain Apply Art button and every existing caller behave exactly as before. Tests: 5 on the apply targets (artist-only / album-only / default / both / missing-url) against a real SQLite DB, plus the existing cover-art suite updated for the new artist search. 107 repair/ cover-art/UI-integrity tests pass.
This commit is contained in:
parent
8b7609cdb2
commit
7207ec61fb
6 changed files with 295 additions and 20 deletions
|
|
@ -203,6 +203,12 @@ class MissingCoverArtJob(RepairJob):
|
|||
log_line=f'Found art: {title or "Unknown"}',
|
||||
log_type='success'
|
||||
)
|
||||
# Also search for an artist image so the finding can offer it as
|
||||
# an independently-applyable target (Pache711). Searched even
|
||||
# when the artist already has art, so a wrong existing image can
|
||||
# be swapped; the UI only surfaces it when it differs from the
|
||||
# current one.
|
||||
found_artist_url = self._find_artist_art(artist_name, source_priority)
|
||||
# Create finding for user to approve
|
||||
if context.create_finding:
|
||||
try:
|
||||
|
|
@ -222,6 +228,13 @@ class MissingCoverArtJob(RepairJob):
|
|||
'found_artwork_url': artwork_url,
|
||||
'spotify_album_id': spotify_album_id,
|
||||
'artist_thumb_url': artist_thumb or None,
|
||||
# Found artist image (None if none matched, or it
|
||||
# equals the current one — nothing to offer then).
|
||||
'found_artist_url': (
|
||||
found_artist_url
|
||||
if found_artist_url and found_artist_url != artist_thumb
|
||||
else None
|
||||
),
|
||||
# Where the files live + what was missing, so the
|
||||
# apply can embed into the audio + write cover.jpg.
|
||||
'album_folder': os.path.dirname(rep_path) if rep_path else None,
|
||||
|
|
@ -287,6 +300,34 @@ class MissingCoverArtJob(RepairJob):
|
|||
logger.debug("%s art lookup failed for '%s': %s", source.capitalize(), title, e)
|
||||
return None
|
||||
|
||||
def _find_artist_art(self, artist_name, source_priority):
|
||||
"""Search the configured sources for an artist image, in priority
|
||||
order. Returns the first confidently name-matched artist image URL,
|
||||
or None. Mirrors _try_source but for artists (Pache711: let the
|
||||
Cover Art Filler offer artist art as its own fixable target)."""
|
||||
if not artist_name:
|
||||
return None
|
||||
for source in source_priority:
|
||||
client = get_client_for_source(source)
|
||||
if not client or not hasattr(client, 'search_artists'):
|
||||
continue
|
||||
try:
|
||||
for res in (client.search_artists(artist_name, limit=5) or []):
|
||||
r_name = getattr(res, 'name', None)
|
||||
if isinstance(res, dict):
|
||||
r_name = res.get('name')
|
||||
# Exact significant-word match — never hang a wrong artist
|
||||
# photo on someone just because the search was fuzzy.
|
||||
if not r_name or _name_tokens(r_name) != _name_tokens(artist_name):
|
||||
continue
|
||||
url = self._extract_artwork_url(res)
|
||||
if url:
|
||||
return url
|
||||
except Exception as e:
|
||||
logger.debug("%s artist-art lookup failed for '%s': %s",
|
||||
source.capitalize(), artist_name, e)
|
||||
return None
|
||||
|
||||
@staticmethod
|
||||
def _result_title_artist(item):
|
||||
"""Pull (title, artist) from a search result that may be a dict or an
|
||||
|
|
|
|||
|
|
@ -1277,17 +1277,58 @@ class RepairWorker:
|
|||
logger.error("Error fixing track number for %s: %s", file_path, e)
|
||||
return {'success': False, 'error': str(e)}
|
||||
|
||||
def _fix_artist_art(self, album_id, details):
|
||||
"""Apply the found ARTIST image to the album's artist (DB thumb only —
|
||||
artist art has no per-file embed). Pache711: independently applyable
|
||||
from the album art on the same finding."""
|
||||
artist_url = details.get('found_artist_url')
|
||||
if not artist_url:
|
||||
return {'success': False, 'error': 'No artist image found in finding details'}
|
||||
conn = None
|
||||
try:
|
||||
conn = self.db._get_connection()
|
||||
cursor = conn.cursor()
|
||||
cursor.execute(
|
||||
"UPDATE artists SET thumb_url = ?, updated_at = CURRENT_TIMESTAMP "
|
||||
"WHERE id = (SELECT artist_id FROM albums WHERE id = ?)",
|
||||
(artist_url, album_id))
|
||||
conn.commit()
|
||||
if cursor.rowcount == 0:
|
||||
return {'success': False, 'error': 'Artist not found for this album'}
|
||||
finally:
|
||||
if conn:
|
||||
conn.close()
|
||||
return {'success': True, 'action': 'applied_artist_art',
|
||||
'message': 'Applied artist image'}
|
||||
|
||||
def _fix_missing_cover_art(self, entity_type, entity_id, file_path, details):
|
||||
"""Apply found artwork: update the DB thumbnail AND embed art into the
|
||||
album's audio files + write cover.jpg (using the post-processing
|
||||
standard, so the user's album_art_order preference is honored)."""
|
||||
artwork_url = details.get('found_artwork_url')
|
||||
if not artwork_url:
|
||||
return {'success': False, 'error': 'No artwork URL found in finding details'}
|
||||
"""Apply found artwork. ``_fix_action`` selects the target (Pache711):
|
||||
'album' (default — DB thumb + embed into files + cover.jpg), 'artist'
|
||||
(the artist's DB image), or 'both'. Defaulting to 'album' keeps the
|
||||
plain "Apply Art" button behaving exactly as before."""
|
||||
target = (details.get('_fix_action') or 'album').strip().lower()
|
||||
if target not in ('album', 'artist', 'both'):
|
||||
target = 'album'
|
||||
|
||||
album_id = details.get('album_id') or entity_id
|
||||
if not album_id:
|
||||
return {'success': False, 'error': 'No album ID associated with this finding'}
|
||||
|
||||
# Artist-only path: nothing to do with album files.
|
||||
if target == 'artist':
|
||||
return self._fix_artist_art(album_id, details)
|
||||
|
||||
artist_result = None
|
||||
if target == 'both':
|
||||
artist_result = self._fix_artist_art(album_id, details)
|
||||
|
||||
artwork_url = details.get('found_artwork_url')
|
||||
if not artwork_url:
|
||||
# 'both' but no album art — report the artist outcome if that ran.
|
||||
if artist_result is not None:
|
||||
return artist_result
|
||||
return {'success': False, 'error': 'No artwork URL found in finding details'}
|
||||
|
||||
conn = None
|
||||
track_paths = []
|
||||
album_title = details.get('album_title')
|
||||
|
|
@ -1332,8 +1373,10 @@ class RepairWorker:
|
|||
|
||||
if not resolved:
|
||||
# Media-server-only album (no local files): DB thumbnail is all we can set.
|
||||
return {'success': True, 'action': 'applied_cover_art',
|
||||
'message': 'Applied cover art to album (database only — no local files found)'}
|
||||
msg = 'Applied cover art to album (database only — no local files found)'
|
||||
if artist_result is not None and artist_result.get('success'):
|
||||
msg += ' + applied artist image'
|
||||
return {'success': True, 'action': 'applied_cover_art', 'message': msg}
|
||||
|
||||
from core.metadata.art_apply import apply_art_to_album_files
|
||||
metadata = {
|
||||
|
|
@ -1367,6 +1410,8 @@ class RepairWorker:
|
|||
if embedded == 0 and not art_result.get('cover_written'):
|
||||
# DB updated but nothing reached disk (e.g. permissions).
|
||||
msg = 'Updated database thumbnail, but could not write art to files (read-only?)'
|
||||
if artist_result is not None and artist_result.get('success'):
|
||||
msg += ' + applied artist image'
|
||||
return {'success': True, 'action': 'applied_cover_art', 'message': msg, 'art_result': art_result}
|
||||
|
||||
def _fix_library_retag(self, entity_type, entity_id, file_path, details):
|
||||
|
|
|
|||
130
tests/test_cover_art_targets.py
Normal file
130
tests/test_cover_art_targets.py
Normal file
|
|
@ -0,0 +1,130 @@
|
|||
"""Per-target cover-art apply (Pache711: 'select one or the other to fix').
|
||||
|
||||
A missing-cover-art finding now offers album art AND artist art as
|
||||
independently applyable targets. _fix_missing_cover_art routes on _fix_action:
|
||||
'album' (default), 'artist', or 'both'. Verified against a real SQLite DB so
|
||||
the UPDATE statements are exercised.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sys
|
||||
import types
|
||||
|
||||
if "spotipy" not in sys.modules:
|
||||
spotipy = types.ModuleType("spotipy")
|
||||
spotipy.Spotify = type("S", (), {})
|
||||
oauth2 = types.ModuleType("spotipy.oauth2")
|
||||
oauth2.SpotifyOAuth = oauth2.SpotifyClientCredentials = type("O", (), {})
|
||||
spotipy.oauth2 = oauth2
|
||||
sys.modules["spotipy"] = spotipy
|
||||
sys.modules["spotipy.oauth2"] = oauth2
|
||||
|
||||
if "config.settings" not in sys.modules:
|
||||
config_pkg = types.ModuleType("config")
|
||||
settings_mod = types.ModuleType("config.settings")
|
||||
|
||||
class _Cfg:
|
||||
def get(self, key, default=None):
|
||||
return default
|
||||
|
||||
def get_active_media_server(self):
|
||||
return "plex"
|
||||
|
||||
settings_mod.config_manager = _Cfg()
|
||||
config_pkg.settings = settings_mod
|
||||
sys.modules["config"] = config_pkg
|
||||
sys.modules["config.settings"] = settings_mod
|
||||
|
||||
import sqlite3
|
||||
|
||||
import pytest
|
||||
|
||||
from core.repair_worker import RepairWorker
|
||||
|
||||
|
||||
class _DB:
|
||||
def __init__(self, path):
|
||||
self.path = str(path)
|
||||
conn = self._get_connection()
|
||||
c = conn.cursor()
|
||||
c.execute("CREATE TABLE artists (id TEXT PRIMARY KEY, name TEXT, thumb_url TEXT, updated_at TEXT)")
|
||||
c.execute("CREATE TABLE albums (id TEXT PRIMARY KEY, title TEXT, artist_id TEXT, thumb_url TEXT, musicbrainz_release_id TEXT, updated_at TEXT)")
|
||||
c.execute("CREATE TABLE tracks (id TEXT PRIMARY KEY, album_id TEXT, file_path TEXT)")
|
||||
c.execute("INSERT INTO artists VALUES ('ar1', 'Forre Sterra', 'http://old/artist.jpg', NULL)")
|
||||
c.execute("INSERT INTO albums VALUES ('al1', 'For You', 'ar1', NULL, NULL, NULL)")
|
||||
conn.commit()
|
||||
conn.close()
|
||||
|
||||
def _get_connection(self):
|
||||
return sqlite3.connect(self.path)
|
||||
|
||||
|
||||
def _worker(tmp_path):
|
||||
w = RepairWorker.__new__(RepairWorker)
|
||||
w.db = _DB(tmp_path / "m.db")
|
||||
w.transfer_folder = str(tmp_path)
|
||||
w._config_manager = None
|
||||
return w
|
||||
|
||||
|
||||
def _thumbs(w):
|
||||
conn = w.db._get_connection()
|
||||
c = conn.cursor()
|
||||
alb = c.execute("SELECT thumb_url FROM albums WHERE id='al1'").fetchone()[0]
|
||||
art = c.execute("SELECT thumb_url FROM artists WHERE id='ar1'").fetchone()[0]
|
||||
conn.close()
|
||||
return alb, art
|
||||
|
||||
|
||||
DETAILS = {
|
||||
'album_id': 'al1', 'album_title': 'For You', 'artist': 'Forre Sterra',
|
||||
'found_artwork_url': 'http://new/album.jpg',
|
||||
'found_artist_url': 'http://new/artist.jpg',
|
||||
}
|
||||
|
||||
|
||||
def test_artist_only_sets_artist_leaves_album(tmp_path):
|
||||
w = _worker(tmp_path)
|
||||
res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'artist'})
|
||||
assert res['success'] and res['action'] == 'applied_artist_art'
|
||||
album_thumb, artist_thumb = _thumbs(w)
|
||||
assert artist_thumb == 'http://new/artist.jpg' # artist updated
|
||||
assert album_thumb is None # album untouched
|
||||
|
||||
|
||||
def test_album_only_sets_album_leaves_artist(tmp_path):
|
||||
w = _worker(tmp_path)
|
||||
res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'})
|
||||
assert res['success']
|
||||
album_thumb, artist_thumb = _thumbs(w)
|
||||
assert album_thumb == 'http://new/album.jpg' # album updated
|
||||
assert artist_thumb == 'http://old/artist.jpg' # artist left as-is
|
||||
|
||||
|
||||
def test_default_action_is_album_only(tmp_path):
|
||||
# No _fix_action → behaves exactly like the old "Apply Art" (album only).
|
||||
w = _worker(tmp_path)
|
||||
w._fix_missing_cover_art('album', 'al1', None, dict(DETAILS))
|
||||
album_thumb, artist_thumb = _thumbs(w)
|
||||
assert album_thumb == 'http://new/album.jpg'
|
||||
assert artist_thumb == 'http://old/artist.jpg'
|
||||
|
||||
|
||||
def test_both_sets_album_and_artist(tmp_path):
|
||||
w = _worker(tmp_path)
|
||||
res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'both'})
|
||||
assert res['success']
|
||||
album_thumb, artist_thumb = _thumbs(w)
|
||||
assert album_thumb == 'http://new/album.jpg'
|
||||
assert artist_thumb == 'http://new/artist.jpg'
|
||||
assert 'artist image' in res['message']
|
||||
|
||||
|
||||
def test_artist_action_without_found_artist_url_fails_cleanly(tmp_path):
|
||||
w = _worker(tmp_path)
|
||||
res = w._fix_missing_cover_art('album', 'al1', None,
|
||||
{**DETAILS, 'found_artist_url': None, '_fix_action': 'artist'})
|
||||
assert res['success'] is False
|
||||
album_thumb, artist_thumb = _thumbs(w)
|
||||
assert artist_thumb == 'http://old/artist.jpg' # nothing changed
|
||||
|
|
@ -189,8 +189,13 @@ def test_missing_cover_art_uses_configured_art_sources(monkeypatch):
|
|||
result = mca.MissingCoverArtJob().scan(context)
|
||||
|
||||
assert result.findings_created == 1
|
||||
# ALBUM art came from the configured order — the album source-priority loop
|
||||
# was skipped (if it had run, the URL would be the fake client's art).
|
||||
assert findings[0]['details']['found_artwork_url'] == 'https://configured/art.jpg'
|
||||
assert consulted == [] # source-priority loop skipped when configured art wins
|
||||
# Artist-art search (Pache711) is a SEPARATE lookup that does consult the
|
||||
# sources; the fake client has no search_artists, so it finds nothing and
|
||||
# no artist target is offered.
|
||||
assert findings[0]['details']['found_artist_url'] is None
|
||||
|
||||
|
||||
def test_missing_cover_art_uses_primary_when_prefer_unset(monkeypatch):
|
||||
|
|
|
|||
|
|
@ -3092,21 +3092,34 @@ function _renderFindingDetail(f) {
|
|||
if (d.album_title) rows.push(['Album', d.album_title]);
|
||||
if (d.spotify_album_id) rows.push(['Spotify ID', d.spotify_album_id]);
|
||||
let artHtml = '';
|
||||
// Show artist image + found artwork side by side
|
||||
if (d.artist_thumb_url || d.found_artwork_url) {
|
||||
// Each found image is independently applyable (Pache711: "fix one,
|
||||
// dismiss the other"). Per-image Apply buttons let the user take the
|
||||
// correct album art and skip a wrong artist image, or vice versa.
|
||||
if (d.artist_thumb_url || d.found_artwork_url || d.found_artist_url) {
|
||||
artHtml += '<div class="repair-finding-media">';
|
||||
if (d.artist_thumb_url) {
|
||||
artHtml += `<div class="repair-finding-media-card">
|
||||
<img class="repair-finding-media-img artist" src="${_escFinding(d.artist_thumb_url)}" alt="Artist"
|
||||
onerror="this.parentElement.style.display='none'" />
|
||||
<span class="repair-finding-media-label">${_escFinding(d.artist || 'Artist')}</span>
|
||||
</div>`;
|
||||
}
|
||||
if (d.found_artwork_url) {
|
||||
artHtml += `<div class="repair-finding-media-card">
|
||||
<img class="repair-finding-media-img" src="${_escFinding(d.found_artwork_url)}" alt="Found artwork"
|
||||
<img class="repair-finding-media-img" src="${_escFinding(d.found_artwork_url)}" alt="Found album art"
|
||||
onerror="this.parentElement.style.display='none'" />
|
||||
<span class="repair-finding-media-label">Found Artwork</span>
|
||||
<span class="repair-finding-media-label">Found Album Art</span>
|
||||
<button class="repair-art-apply-btn" onclick="applyCoverArtTarget(${f.id}, 'album')">Use for album</button>
|
||||
</div>`;
|
||||
}
|
||||
// Current artist image (context) — no apply, it's what's there now.
|
||||
if (d.artist_thumb_url) {
|
||||
artHtml += `<div class="repair-finding-media-card">
|
||||
<img class="repair-finding-media-img artist" src="${_escFinding(d.artist_thumb_url)}" alt="Current artist art"
|
||||
onerror="this.parentElement.style.display='none'" />
|
||||
<span class="repair-finding-media-label">${_escFinding(d.artist || 'Artist')} (current)</span>
|
||||
</div>`;
|
||||
}
|
||||
// Found artist image — the applyable replacement.
|
||||
if (d.found_artist_url) {
|
||||
artHtml += `<div class="repair-finding-media-card">
|
||||
<img class="repair-finding-media-img artist" src="${_escFinding(d.found_artist_url)}" alt="Found artist art"
|
||||
onerror="this.parentElement.style.display='none'" />
|
||||
<span class="repair-finding-media-label">Found Artist Art</span>
|
||||
<button class="repair-art-apply-btn" onclick="applyCoverArtTarget(${f.id}, 'artist')">Use for artist</button>
|
||||
</div>`;
|
||||
}
|
||||
artHtml += '</div>';
|
||||
|
|
@ -3382,6 +3395,31 @@ async function selectDuplicateToKeep(findingId, keepTrackId) {
|
|||
}
|
||||
}
|
||||
|
||||
async function applyCoverArtTarget(id, target) {
|
||||
// Per-image apply for a cover-art finding (Pache711). target: 'album' |
|
||||
// 'artist'. Applying either resolves the finding, so picking the correct
|
||||
// image and ignoring the wrong one = "fix one, dismiss the other".
|
||||
try {
|
||||
const response = await fetch(`/api/repair/findings/${id}/fix`, {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({ fix_action: target }),
|
||||
});
|
||||
const result = await response.json();
|
||||
if (result.success) {
|
||||
showToast(result.message || `Applied ${target} art`, 'success');
|
||||
} else {
|
||||
showToast(result.error || `Failed to apply ${target} art`, 'error');
|
||||
}
|
||||
loadRepairFindingsDashboard();
|
||||
loadRepairFindings();
|
||||
updateRepairStatus();
|
||||
} catch (error) {
|
||||
console.error('Error applying cover art target:', error);
|
||||
showToast('Error applying art', 'error');
|
||||
}
|
||||
}
|
||||
|
||||
async function fixRepairFinding(id, findingType) {
|
||||
// Orphan files require user to choose an action
|
||||
let fixAction = null;
|
||||
|
|
|
|||
|
|
@ -53477,6 +53477,22 @@ tr.tag-diff-same {
|
|||
line-height: 1.3;
|
||||
font-weight: 500;
|
||||
}
|
||||
/* Per-image apply button on cover-art findings (Pache711: fix one or the other) */
|
||||
.repair-art-apply-btn {
|
||||
padding: 5px 14px;
|
||||
font-size: 11px;
|
||||
font-weight: 600;
|
||||
border-radius: 999px;
|
||||
border: 1px solid rgba(74, 222, 128, 0.4);
|
||||
background: rgba(74, 222, 128, 0.12);
|
||||
color: #4ade80;
|
||||
cursor: pointer;
|
||||
transition: background 0.18s ease, transform 0.18s ease;
|
||||
}
|
||||
.repair-art-apply-btn:hover {
|
||||
background: rgba(74, 222, 128, 0.22);
|
||||
transform: translateY(-1px);
|
||||
}
|
||||
|
||||
/* Play button for findings */
|
||||
.repair-finding-play-btn {
|
||||
|
|
|
|||
Loading…
Reference in a new issue