From dae9da7084ddf1116c73b2dd2af88d0a69b7d836 Mon Sep 17 00:00:00 2001 From: Sven Date: Mon, 20 Apr 2026 13:02:51 +0200 Subject: [PATCH] Genres fix --- .../ServicesMALibraryManager.swift | 68 ++++++- .../ServicesMAService.swift | 46 ++--- .../ViewsLibraryGenresView.swift | 9 +- .../MALibraryManagerTests.swift | 179 ++++++++++++++++++ 4 files changed, 265 insertions(+), 37 deletions(-) diff --git a/Mobile Music Assistant/ServicesMALibraryManager.swift b/Mobile Music Assistant/ServicesMALibraryManager.swift index 47d0a28..249e342 100644 --- a/Mobile Music Assistant/ServicesMALibraryManager.swift +++ b/Mobile Music Assistant/ServicesMALibraryManager.swift @@ -24,6 +24,8 @@ final class MALibraryManager { private(set) var playlists: [MAPlaylist] = [] private(set) var podcasts: [MAPodcast] = [] private(set) var genres: [MAGenre] = [] + /// Deduplicated, non-empty genres ready for display. Populated after loadGenres completes. + private(set) var displayGenres: [MAGenre] = [] // Pagination private var artistsOffset = 0 @@ -413,12 +415,72 @@ final class MALibraryManager { logger.info("Loading genres") let loaded = try await service.getGenres() genres = loaded.sorted { $0.name < $1.name } - logger.info("Loaded \(loaded.count) genres") + logger.info("Loaded \(loaded.count) genres, filtering empty ones…") + let filtered = await filterNonEmptyGenres(service: service) + displayGenres = filtered + logger.info("Displaying \(filtered.count) non-empty genres") } - func browseGenre(genreUri: String) async throws -> [MAMediaItem] { + /// Returns deduplicated genres that have at least one artist or album. + private func filterNonEmptyGenres(service: MAService) async -> [MAGenre] { + // Deduplicate by name and collect all IDs for each unique name + var seenNames = Set() + var uniqueGenres: [MAGenre] = [] + for genre in genres where seenNames.insert(genre.name.lowercased()).inserted { + uniqueGenres.append(genre) + } + + // Pre-compute genre IDs per unique name before entering the task group + let genreWithIds: [(MAGenre, [Int])] = uniqueGenres.map { genre in + let ids = genres + .filter { $0.name.caseInsensitiveCompare(genre.name) == .orderedSame } + .compactMap { Int($0.uri.components(separatedBy: "/").last ?? "") } + return (genre, ids) + } + + return await withTaskGroup(of: MAGenre?.self) { group in + for (genre, ids) in genreWithIds where !ids.isEmpty { + group.addTask { + do { + let artists = try await service.getArtistsByGenre(genreIds: ids, limit: 1) + if !artists.isEmpty { return genre } + let albums = try await service.getAlbumsByGenre(genreIds: ids, limit: 1) + return albums.isEmpty ? nil : genre + } catch { + return genre // Don't hide on network error + } + } + } + var result: [MAGenre] = [] + for await genre in group { + if let genre { result.append(genre) } + } + return result.sorted { $0.name < $1.name } + } + } + + /// Fetch all artists and albums that belong to the given genre name. + /// Extracts database IDs from matching genre URIs and uses the native + /// `genre` parameter of MA Server's library_items endpoints. + func browseGenresByName(_ name: String) async throws -> [MAMediaItem] { guard let service else { throw MAWebSocketClient.ClientError.notConnected } - return try await service.browseGenre(genreUri: genreUri) + + // Genre URIs look like "library://genre/26". Extract the integer IDs. + let genreIds = genres + .filter { $0.name.caseInsensitiveCompare(name) == .orderedSame } + .compactMap { Int($0.uri.components(separatedBy: "/").last ?? "") } + + guard !genreIds.isEmpty else { return [] } + + async let artists = service.getArtistsByGenre(genreIds: genreIds) + async let albums = service.getAlbumsByGenre(genreIds: genreIds) + let (artistResults, albumResults) = try await (artists, albums) + + var seen = Set() + let allItems = (artistResults + albumResults) + .filter { seen.insert($0.uri).inserted } + .sorted { $0.name < $1.name } + return allItems } func getPodcastEpisodes(podcastUri: String) async throws -> [MAMediaItem] { diff --git a/Mobile Music Assistant/ServicesMAService.swift b/Mobile Music Assistant/ServicesMAService.swift index b7a8663..c626ca5 100644 --- a/Mobile Music Assistant/ServicesMAService.swift +++ b/Mobile Music Assistant/ServicesMAService.swift @@ -360,39 +360,25 @@ final class MAService { ) } - /// Browse items under a genre URI. - /// MA returns provider sub-folders at the first level, so we auto-expand - /// them with a second browse pass to surface actual artists/albums. - func browseGenre(genreUri: String) async throws -> [MAMediaItem] { - logger.debug("Browsing genre \(genreUri)") - let firstLevel = try await webSocketClient.sendCommand( - "music/browse", - args: ["uri": genreUri], + /// Get artists belonging to one or more genre IDs. + /// Uses the native `genre` parameter of music/artists/library_items (int | list[int]). + func getArtistsByGenre(genreIds: [Int], limit: Int = 500) async throws -> [MAMediaItem] { + logger.debug("Fetching artists for genre ids \(genreIds)") + return try await webSocketClient.sendCommand( + "music/artists/library_items", + args: ["limit": limit, "genre": genreIds as [Any]], resultType: [MAMediaItem].self ) + } - // If first level already contains real media items, return them. - let realItems = firstLevel.filter { - guard let t = $0.mediaType else { return false } - return t != .unknown - } - if !realItems.isEmpty { return realItems } - - // Otherwise these are sub-folders (providers) — browse each one. - var allItems: [MAMediaItem] = [] - var seen = Set() - for folder in firstLevel { - let items = (try? await webSocketClient.sendCommand( - "music/browse", - args: ["uri": folder.uri], - resultType: [MAMediaItem].self - )) ?? [] - for item in items where seen.insert(item.uri).inserted { - allItems.append(item) - } - } - logger.debug("Genre browse returned \(allItems.count) items after expanding \(firstLevel.count) folders") - return allItems + /// Get albums belonging to one or more genre IDs. + func getAlbumsByGenre(genreIds: [Int], limit: Int = 500) async throws -> [MAMediaItem] { + logger.debug("Fetching albums for genre ids \(genreIds)") + return try await webSocketClient.sendCommand( + "music/albums/library_items", + args: ["limit": limit, "genre": genreIds as [Any]], + resultType: [MAMediaItem].self + ) } /// Get radio stations diff --git a/Mobile Music Assistant/ViewsLibraryGenresView.swift b/Mobile Music Assistant/ViewsLibraryGenresView.swift index 625b504..1c73151 100644 --- a/Mobile Music Assistant/ViewsLibraryGenresView.swift +++ b/Mobile Music Assistant/ViewsLibraryGenresView.swift @@ -14,7 +14,8 @@ struct GenresView: View { @State private var errorMessage: String? @State private var showError = false - private var genres: [MAGenre] { service.libraryManager.genres } + // Deduplicated, non-empty genres — populated by MALibraryManager after load + filter pass. + private var genres: [MAGenre] { service.libraryManager.displayGenres } private var isLoading: Bool { service.libraryManager.isLoadingGenres } var body: some View { @@ -48,7 +49,7 @@ struct GenresView: View { await loadGenres(refresh: true) } .task { - if genres.isEmpty { + if service.libraryManager.genres.isEmpty { await loadGenres(refresh: true) } } @@ -141,13 +142,13 @@ struct GenreDetailView: View { private func loadItems() async { isLoading = true + defer { isLoading = false } do { - items = try await service.libraryManager.browseGenre(genreUri: genre.uri) + items = try await service.libraryManager.browseGenresByName(genre.name) } catch { errorMessage = error.localizedDescription showError = true } - isLoading = false } } diff --git a/MobileMusicAssistantTests/MALibraryManagerTests.swift b/MobileMusicAssistantTests/MALibraryManagerTests.swift index 05b3968..dbbf306 100644 --- a/MobileMusicAssistantTests/MALibraryManagerTests.swift +++ b/MobileMusicAssistantTests/MALibraryManagerTests.swift @@ -114,6 +114,185 @@ struct FavoriteURICollectionTests { } } +// MARK: - Genre Deduplication + +@Suite("MALibraryManager – Genre Deduplication") +struct GenreDeduplicationTests { + + private func uniqueByName(_ genres: [MAGenre]) -> [MAGenre] { + var seen = Set() + return genres.filter { seen.insert($0.name.lowercased()).inserted } + } + + private func genre(_ name: String, provider: String) -> MAGenre { + MAGenre(uri: "genre://\(name.lowercased())@\(provider)", name: name, metadata: nil) + } + + @Test("Single genre per provider is unchanged") + func singleProviderUnchanged() { + let genres = [genre("Rock", provider: "spotify"), genre("Pop", provider: "spotify")] + #expect(uniqueByName(genres).count == 2) + } + + @Test("Duplicate names across providers collapse to one entry") + func duplicatesCollapse() { + let genres = [ + genre("Rock", provider: "spotify"), + genre("Rock", provider: "local"), + genre("Rock", provider: "tidal"), + ] + let unique = uniqueByName(genres) + #expect(unique.count == 1) + #expect(unique[0].name == "Rock") + } + + @Test("Mixed duplicates and unique names are handled correctly") + func mixedList() { + let genres = [ + genre("Jazz", provider: "spotify"), + genre("Rock", provider: "spotify"), + genre("Rock", provider: "local"), + genre("Blues", provider: "local"), + ] + let unique = uniqueByName(genres) + #expect(unique.count == 3) + #expect(unique.map(\.name).sorted() == ["Blues", "Jazz", "Rock"]) + } + + @Test("Case-insensitive deduplication treats rock and Rock as the same") + func caseInsensitive() { + let genres = [genre("rock", provider: "local"), genre("Rock", provider: "spotify")] + #expect(uniqueByName(genres).count == 1) + } + + @Test("browseGenresByName on manager with no genres returns empty without service crash") + func emptyGenresNoService() { + let manager = MALibraryManager(service: nil) + #expect(manager.genres.isEmpty) + // Without genres loaded, matching set is empty — no service call is attempted. + } +} + +// MARK: - Genre URI Parsing + +@Suite("MALibraryManager – Genre URI Parsing") +struct GenreURIParsingTests { + + /// Replicates the ID extraction used in browseGenresByName and filterNonEmptyGenres. + private func genreId(from uri: String) -> Int? { + Int(uri.components(separatedBy: "/").last ?? "") + } + + @Test("Standard library genre URI yields correct integer ID") + func standardURI() { + #expect(genreId(from: "library://genre/26") == 26) + } + + @Test("Small genre ID parses correctly") + func smallId() { + #expect(genreId(from: "library://genre/1") == 1) + } + + @Test("Large genre ID parses correctly") + func largeId() { + #expect(genreId(from: "library://genre/99999") == 99999) + } + + @Test("Non-integer last path component returns nil") + func nonIntegerComponent() { + #expect(genreId(from: "genre://Rock@local") == nil) + } + + @Test("URI with trailing slash returns nil for empty last component") + func trailingSlash() { + #expect(genreId(from: "library://genre/") == nil) + } + + @Test("Empty URI string returns nil") + func emptyURI() { + #expect(genreId(from: "") == nil) + } +} + +// MARK: - Genre ID Aggregation + +@Suite("MALibraryManager – Genre ID Aggregation") +struct GenreIDAggregationTests { + + /// Replicates the ID collection used in browseGenresByName and filterNonEmptyGenres. + private func idsForName(_ name: String, in genres: [MAGenre]) -> [Int] { + genres + .filter { $0.name.caseInsensitiveCompare(name) == .orderedSame } + .compactMap { Int($0.uri.components(separatedBy: "/").last ?? "") } + } + + private func genre(_ name: String, id: Int) -> MAGenre { + MAGenre(uri: "library://genre/\(id)", name: name, metadata: nil) + } + + @Test("Single genre yields its ID") + func singleGenreId() { + let genres = [genre("Rock", id: 26)] + #expect(idsForName("Rock", in: genres) == [26]) + } + + @Test("Multiple providers for the same genre name aggregate all IDs") + func duplicateNamesAggregateIds() { + let genres = [ + genre("Rock", id: 26), + genre("Rock", id: 7), + genre("Rock", id: 14), + ] + let ids = idsForName("Rock", in: genres) + #expect(Set(ids) == Set([26, 7, 14])) + #expect(ids.count == 3) + } + + @Test("Different genre names yield independent ID sets") + func separateNames() { + let genres = [genre("Rock", id: 26), genre("Jazz", id: 5)] + #expect(idsForName("Rock", in: genres) == [26]) + #expect(idsForName("Jazz", in: genres) == [5]) + } + + @Test("Name matching is case-insensitive") + func caseInsensitiveMatch() { + let genres = [genre("rock", id: 1), genre("Rock", id: 2), genre("ROCK", id: 3)] + #expect(Set(idsForName("Rock", in: genres)) == Set([1, 2, 3])) + } + + @Test("Genre with non-integer URI is skipped by compactMap") + func invalidURISkipped() { + let bad = MAGenre(uri: "genre://Rock@local", name: "Rock", metadata: nil) + let good = genre("Rock", id: 26) + #expect(idsForName("Rock", in: [bad, good]) == [26]) + } + + @Test("No matching genre name yields empty ID list") + func noMatchReturnsEmpty() { + let genres = [genre("Jazz", id: 5)] + #expect(idsForName("Rock", in: genres).isEmpty) + } +} + +// MARK: - Display Genres State + +@Suite("MALibraryManager – Display Genres") +struct DisplayGenresTests { + + @Test("displayGenres starts empty before any load") + func displayGenresStartEmpty() { + #expect(MALibraryManager(service: nil).displayGenres.isEmpty) + } + + @Test("displayGenres and genres are independent collections") + func displayGenresIsSeparate() { + let manager = MALibraryManager(service: nil) + #expect(manager.genres.isEmpty) + #expect(manager.displayGenres.isEmpty) + } +} + // MARK: - MALibraryManager Initial State @Suite("MALibraryManager – Initial State")