From 43c44e562e9be04952a2d8c49dec3f3c15bb9e66 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 17:27:06 +0000 Subject: [PATCH] Download synced songs' covers Clementine sends a cover with each synced song that has one, but the phone never fetched it, so kept songs played with no cover unless their file had one inside. Sync now downloads each cover once, however many songs share it, into a hidden .covers folder beside the songs, records it in the store's art table, and deletes covers no song uses any more. A song kept before this gets its cover on the next sync without being downloaded again, and a cover that fails to download is tried again next time without failing the song. The phone player shows Clementine's cover, or else the one in the song's file, as before. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R2bAq6hMmUmb79VSMAxLPp --- .../ClementineKit/Sync/PhonePlayer.swift | 19 +++- .../ClementineKit/Sync/SyncEngine.swift | 46 ++++++++-- .../ClementineKit/Sync/SyncFiles.swift | 8 ++ .../ClementineKitTests/SyncEngineTests.swift | 90 ++++++++++++++++++- 4 files changed, 153 insertions(+), 10 deletions(-) diff --git a/Packages/ClementineKit/Sources/ClementineKit/Sync/PhonePlayer.swift b/Packages/ClementineKit/Sources/ClementineKit/Sync/PhonePlayer.swift index db0f51d..086dd2f 100644 --- a/Packages/ClementineKit/Sources/ClementineKit/Sync/PhonePlayer.swift +++ b/Packages/ClementineKit/Sources/ClementineKit/Sync/PhonePlayer.swift @@ -11,6 +11,9 @@ public struct KeptSong: Sendable, Hashable, Identifiable { /// As Clementine writes it, such as "5:36"; empty if it didn't say. public var length: String public var file: URL + /// The cover Clementine sent with the song, if it did. Otherwise the cover is the one in + /// [file], if it has one. + public var artFile: URL? public var id: String { itemKey } @@ -24,6 +27,7 @@ public struct KeptSong: Sendable, Hashable, Identifiable { album = song.album length = song.prettyLength self.file = file + artFile = item.artKey.flatMap(SyncFiles.artPath(key:)).map { folder.appending(path: $0) } } /// [song], downloaded to this phone, to play with what's kept; its file is its key. @@ -50,7 +54,7 @@ public final class PhonePlayer { public private(set) var isShuffled = false /// Nothing (.off), all the songs (.playlist) or the one playing (.track). public private(set) var repeatMode = RepeatMode.off - /// The cover in the song's file, once read, if it has one. + /// The song's cover, once read, if it has one. public private(set) var artwork: Data? @ObservationIgnored public var onUpdate: (() -> Void)? @@ -247,20 +251,29 @@ public final class PhonePlayer { onUpdate?() } - /// Reads the cover in the playing song's file, when the song changes. + /// Reads the playing song's cover, when the song changes: the one Clementine sent, or else the + /// one in its file. private func readArtwork() { guard song?.itemKey != artworkFor else { return } artworkFor = song?.itemKey artwork = nil guard let key = artworkFor, let file = song?.file else { return } + let artFile = song?.artFile Task { - let data = await Self.artwork(in: file) + let data = await Self.artwork(artFile, orIn: file) guard artworkFor == key, let data else { return } artwork = data onUpdate?() } } + nonisolated private static func artwork(_ artFile: URL?, orIn file: URL) async -> Data? { + if let artFile, let data = try? Data(contentsOf: artFile), !data.isEmpty { + return data + } + return await artwork(in: file) + } + nonisolated private static func artwork(in file: URL) async -> Data? { guard let metadata = try? await AVURLAsset(url: file).load(.commonMetadata) else { return nil } for item in AVMetadataItem.metadataItems(from: metadata, filteredByIdentifier: .commonIdentifierArtwork) { diff --git a/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncEngine.swift b/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncEngine.swift index 3bfc2a5..2ba9593 100644 --- a/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncEngine.swift +++ b/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncEngine.swift @@ -32,9 +32,9 @@ public enum RuleResult: Sendable, Equatable { } /// Brings what's on the phone in line with the rules for one Clementine: asks for each rule's -/// manifest, downloads the songs that are new or changed into `folder`, and deletes the songs that -/// no rule wants any more. A song in several rules is downloaded once. Covers and conversions come -/// later. +/// manifest, downloads the songs that are new or changed into `folder`, with their covers, and +/// deletes the songs that no rule wants any more. A song in several rules is downloaded once, and a +/// cover many songs share is too. Conversions come later. /// /// `supportedFormats` are the types the phone plays, as for streaming (`Renderer.formats`). /// `progress` hears how many of a rule's songs to download have been, before the first and after @@ -90,6 +90,10 @@ public struct SyncEngine: Sendable { try await store.removeItem(serverID: serverID, itemKey: item.itemKey) removed += 1 } + for art in try await store.unusedArt() { + try? FileManager.default.removeItem(at: folder.appending(path: art.path)) + try await store.removeArt(key: art.artKey) + } return removed } @@ -117,6 +121,10 @@ public struct SyncEngine: Sendable { try await download(item, serverID: rule.serverID, replacing: have, over: connection) downloaded += 1 await progress(rule.id, SyncProgress(done: downloaded, total: total)) + } else if var have, have.artKey != (item.hasArt ? item.art.key : nil) { + // The same song with another cover, or one kept before covers were. + have.artKey = await art(item, over: connection) + try await store.putItem(have) } keys.append(item.itemKey) } @@ -189,7 +197,35 @@ public struct SyncEngine: Sendable { SyncedItem( serverID: serverID, itemKey: item.itemKey, version: item.version, path: path, mimeType: item.mimeType, sizeBytes: item.sizeBytes, delivery: item.delivery, - artKey: nil, metadata: try song.serializedData(), pinned: have?.pinned ?? false, - added: have?.added ?? now())) + artKey: await art(item, over: connection), metadata: try song.serializedData(), + pinned: have?.pinned ?? false, added: have?.added ?? now())) + } + + /// The key of `item`'s cover, downloading it if the phone doesn't have it yet; nil if it has + /// none, or it couldn't be downloaded, so it's tried again next time. A song without a cover + /// of its own from Clementine shows the one in its file, if any. + private func art(_ item: Pb_Remote_SyncItem, over connection: some SyncConnection) async -> String? { + guard item.hasArt, let path = SyncFiles.artPath(key: item.art.key) else { return nil } + let key = item.art.key + if (try? await store.art(key: key)) != nil { return key } + + let file = folder.appending(path: path) + let files = FileManager.default + let partial = file.deletingLastPathComponent().appending(path: ".\(UUID().uuidString).part") + do { + try files.createDirectory(at: file.deletingLastPathComponent(), withIntermediateDirectories: true) + try await connection.download(item.art.url, to: partial) + if files.fileExists(atPath: file.path) { + _ = try files.replaceItemAt(file, withItemAt: partial) + } else { + try files.moveItem(at: partial, to: file) + } + let size = (try? file.resourceValues(forKeys: [.fileSizeKey]))?.fileSize ?? 0 + try await store.putArt(SyncedArt(artKey: key, path: path, sizeBytes: Int64(size))) + return key + } catch { + try? files.removeItem(at: partial) + return nil + } } } diff --git a/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncFiles.swift b/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncFiles.swift index 5578b05..80f7ee8 100644 --- a/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncFiles.swift +++ b/Packages/ClementineKit/Sources/ClementineKit/Sync/SyncFiles.swift @@ -11,6 +11,14 @@ enum SyncFiles { return part(artist, otherwise: "Unknown artist") + "/" + part(song.album, otherwise: "Unknown album") + "/" } + /// Where the cover with `key` goes, relative to the synced songs' folder: hidden, since it's + /// the app's, not one of the songs. Nil for a key that isn't a hash, so it can't name a file + /// somewhere else. + static func artPath(key: String) -> String? { + guard !key.isEmpty, key.count <= 128, key.allSatisfy(\.isHexDigit) else { return nil } + return ".covers/" + key.lowercased() + } + static func fileName(_ song: SongMetadata, mimeType: String) -> String { let title = part(song.title, otherwise: "Untitled") let name = song.track > 0 ? (song.track < 10 ? "0" : "") + String(song.track) + " " + title : title diff --git a/Packages/ClementineKit/Tests/ClementineKitTests/SyncEngineTests.swift b/Packages/ClementineKit/Tests/ClementineKitTests/SyncEngineTests.swift index 2682539..d997a49 100644 --- a/Packages/ClementineKit/Tests/ClementineKitTests/SyncEngineTests.swift +++ b/Packages/ClementineKit/Tests/ClementineKitTests/SyncEngineTests.swift @@ -84,7 +84,8 @@ struct SyncEngineTests { return item } - /// The files in the synced songs' folder, by path, leaving out hidden ones. + /// The files in the synced songs' folder, by path, leaving out hidden ones and those in hidden + /// folders. func files() throws -> [String: String] { var files: [String: String] = [:] guard let paths = FileManager.default.enumerator(atPath: folder.path) else { return files } @@ -92,7 +93,7 @@ struct SyncEngineTests { let url = folder.appending(path: path) var isDirectory: ObjCBool = false guard FileManager.default.fileExists(atPath: url.path, isDirectory: &isDirectory), !isDirectory.boolValue, - !url.lastPathComponent.hasPrefix(".") else { continue } + !path.split(separator: "/").contains(where: { $0.hasPrefix(".") }) else { continue } files[path] = try String(decoding: Data(contentsOf: url), as: UTF8.self) } return files @@ -308,6 +309,91 @@ struct SyncEngineTests { #expect(await clementine.opened == ["/d/token/c-1"]) } + /// `item` with the cover `key`, which Clementine serves. + func withArt(_ item: Pb_Remote_SyncItem, _ key: String) async -> Pb_Remote_SyncItem { + var item = item + item.art.key = key + item.art.url = "/a/token/\(key)" + await clementine.put(item.art.url, Data("cover \(key)".utf8)) + return item + } + + func cover(_ key: String) throws -> String { + try String(decoding: Data(contentsOf: folder.appending(path: ".covers/\(key)")), as: UTF8.self) + } + + @Test func downloadsEachCoverOnce() async throws { + _ = try await rule("smart/library/0b6e") + await clementine.serve( + "smart/library/0b6e", + [await withArt(await song("a"), "c0ffee"), await withArt(await song("b", title: "Tame"), "c0ffee"), + await song("c", title: "Hey")]) + _ = try await engine.sync(clementine) + + #expect(await clementine.opened.filter { $0.hasPrefix("/a/") } == ["/a/token/c0ffee"]) + #expect(try await store.item(serverID: Self.server, itemKey: "a")?.artKey == "c0ffee") + #expect(try await store.item(serverID: Self.server, itemKey: "b")?.artKey == "c0ffee") + #expect(try await store.item(serverID: Self.server, itemKey: "c")?.artKey == nil) + #expect(try await store.art(key: "c0ffee")?.path == ".covers/c0ffee") + #expect(try cover("c0ffee") == "cover c0ffee") + // Hidden: they're the app's, not songs. + #expect(try files().count == 3) + + let kept = KeptSong(try #require(await store.item(serverID: Self.server, itemKey: "a")), in: folder) + #expect(kept.artFile == folder.appending(path: ".covers/c0ffee")) + } + + @Test func songsKeptBeforeTheirCoverGetIt() async throws { + _ = try await rule("playlist/1") + await clementine.serve("playlist/1", [await song("a")]) + _ = try await engine.sync(clementine) + + // The same song, now with a cover: only the cover's downloaded. + await clementine.serve("playlist/1", [await withArt(await song("a"), "beef")]) + await clementine.clearOpened() + let result = try await engine.sync(clementine) + #expect(await clementine.opened == ["/a/token/beef"]) + #expect(result.rules.values.first == .synced(songs: 1, downloaded: 0, skipped: 0)) + #expect(try await store.item(serverID: Self.server, itemKey: "a")?.artKey == "beef") + } + + @Test func aCoverThatCantBeDownloadedIsTriedAgain() async throws { + _ = try await rule("playlist/1") + let item = await withArt(await song("a"), "beef") + await clementine.remove(item.art.url) + await clementine.serve("playlist/1", [item]) + let result = try await engine.sync(clementine) + // The song's kept all the same. + #expect(result.rules.values.first == .synced(songs: 1, downloaded: 1, skipped: 0)) + #expect(try await store.item(serverID: Self.server, itemKey: "a")?.artKey == nil) + + await clementine.put(item.art.url, Data("cover beef".utf8)) + _ = try await engine.sync(clementine) + #expect(try await store.item(serverID: Self.server, itemKey: "a")?.artKey == "beef") + } + + @Test func coversNoSongUsesAreDeleted() async throws { + _ = try await rule("playlist/1") + await clementine.serve( + "playlist/1", [await withArt(await song("a"), "beef"), await withArt(await song("b", title: "Tame"), "f00d")]) + _ = try await engine.sync(clementine) + + // One song goes, and the other's cover changes. + await clementine.serve("playlist/1", [await withArt(await song("b", title: "Tame"), "cafe")]) + _ = try await engine.sync(clementine) + #expect(try await store.art(key: "beef") == nil) + #expect(try await store.art(key: "f00d") == nil) + #expect(try cover("cafe") == "cover cafe") + #expect(!FileManager.default.fileExists(atPath: folder.appending(path: ".covers/beef").path)) + #expect(!FileManager.default.fileExists(atPath: folder.appending(path: ".covers/f00d").path)) + } + + @Test func coverKeysMustBeHashes() { + #expect(SyncFiles.artPath(key: "C0FFEE") == ".covers/c0ffee") + #expect(SyncFiles.artPath(key: "../songs") == nil) + #expect(SyncFiles.artPath(key: "") == nil) + } + @Test func namesFilesForPeople() { var song = SongMetadata() song.title = "What/Ever?"