Update PixelPlayer features and fixes#599
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates PixelPlayer to make multi-artist parsing a default feature rather than an optional toggle. The main changes replace all references to song.artist with song.displayArtist throughout the UI and business logic, ensuring consistent display of artist information that respects multi-artist tags. The PR also includes debugging enhancements for artist navigation, performance optimizations in sync progress reporting, and removes the now-obsolete artist separation toggle from settings.
Key Changes:
- Replaced
song.artistwithsong.displayArtistacross all UI components and data layers for consistent multi-artist display - Removed artist separation enabled/disabled toggle and made multi-artist parsing always active
- Added debug logging for artist navigation and loading operations
- Optimized SyncWorker progress reporting to batch updates every 50 songs instead of per-song
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| PlayerViewModel.kt | Updated media metadata and artist display to use displayArtist property; added debug logging for artist navigation |
| ArtistSettingsViewModel.kt | Removed artist separation enabled toggle logic and related state management |
| ArtistDetailViewModel.kt | Added debug logging for artist data loading |
| PlaylistDetailScreen.kt | Updated song picker to display artists using displayArtist property |
| MashupScreen.kt | Updated song display to use displayArtist property; removed trailing whitespace |
| LibraryScreen.kt | Updated song list items to use displayArtist for artist display |
| HomeScreen.kt | Updated favorite songs wrapper to use displayArtist property |
| ArtistSettingsScreen.kt | Removed artist separation toggle UI, switched to shared SwitchSettingItem component, updated documentation text, removed unused imports |
| FullPlayerContent.kt | Updated player metadata display to use displayArtist property |
| ExternalPlayerOverlay.kt | Updated external player artist display to use displayArtist property |
| UnifiedPlayerSheet.kt | Updated mini player to use displayArtist for artist display |
| SongInfoBottomSheet.kt | Updated song info sheet to display artist using displayArtist property |
| QueueBottomSheet.kt | Updated queue and playlist song items to use displayArtist property |
| EditSongSheet.kt | Updated song editor to initialize and display artist using displayArtist |
| AiMetadataDialog.kt | Updated AI metadata field checking to use displayArtist property |
| SyncWorker.kt | Contains unresolved merge conflicts; removed artist separation parameter; added rescan detection; optimized progress reporting with batching; updated artist preservation logic |
| PlaybackStatsRepository.kt | Updated playback statistics to use displayArtist for artist display |
| MusicRepositoryImpl.kt | Added comprehensive artist cross-reference mapping; introduced shared flows for artists and cross-refs; updated all query methods to use multi-artist data |
| LyricsRepositoryImpl.kt | Updated lyrics fetching to use displayArtist for API requests |
| UserPreferencesRepository.kt | Removed artist separation enabled preference and related flow/setter; removed unused import |
| Song.kt | Enhanced displayArtist property with fallback artist splitting logic using default delimiters |
| SongEntity.kt | Optimized toSongWithArtistRefs by using map lookup instead of linear search for cross-references |
| MusicDao.kt | Added getAllArtistsRaw() and getAllSongArtistCrossRefs() queries for comprehensive artist data access |
| AiPlaylistGenerator.kt | Updated AI playlist generation to use displayArtist in prompts |
| AiMetadataGenerator.kt | Updated AI metadata generation to use displayArtist in prompts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim().takeIf { it.isNotEmpty() } | ||
| >>>>>>> theirs | ||
| ?: songArtistNameTrimmed | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs | ||
| ======= | ||
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | ||
| >>>>>>> theirs |
There was a problem hiding this comment.
This code contains unresolved merge conflict markers that must be resolved before merging. The conflict shows multiple competing versions of how to determine the primaryArtistName. All merge conflict markers (<<<<<<< ours, =======, >>>>>>> theirs) must be removed and only one correct implementation should remain.
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim().takeIf { it.isNotEmpty() } | |
| >>>>>>> theirs | |
| ?: songArtistNameTrimmed | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| ======= | |
| val primaryArtistName = artistsForSong.firstOrNull()?.trim() ?: song.artistName | |
| >>>>>>> theirs | |
| val primaryArtistName = artistsForSong.firstOrNull() | |
| ?.trim() | |
| ?.takeIf { it.isNotEmpty() } | |
| ?: songArtistNameTrimmed |
| val displayArtist: String | ||
| get() = if (artists.isNotEmpty()) { | ||
| artists.sortedByDescending { it.isPrimary }.joinToString(", ") { it.name } | ||
| } else { | ||
| artist | ||
| get() { | ||
| if (artists.isNotEmpty()) { | ||
| return artists.sortedByDescending { it.isPrimary }.joinToString(", ") { it.name } | ||
| } | ||
| val split = artist.splitArtistsByDelimiters(defaultArtistDelimiters) | ||
| return if (split.isNotEmpty()) split.joinToString(", ") else artist | ||
| } |
There was a problem hiding this comment.
The displayArtist property computes string splitting every time it's accessed. When the artists list is empty, it calls splitArtistsByDelimiters on each access. Since this is used in UI components that may render frequently, consider caching this computed value or making displayArtist a regular property initialized during object creation to avoid repeated computation.
| * Process songs with multi-artist support. | ||
| * When artist separation is enabled, splits artist names by delimiters | ||
| * and creates proper cross-references. | ||
| * Splits artist names by delimiters and creates proper cross-references. |
There was a problem hiding this comment.
The comment says "Splits artist names by delimiters" but doesn't mention that this always happens now (artist separation is no longer optional). Consider updating the comment to clarify that artist parsing is always enabled and that the artistDelimiters parameter controls which delimiters are used for splitting.
| * Splits artist names by delimiters and creates proper cross-references. | |
| * | |
| * This method always parses and splits artist names into multiple artists, | |
| * using the provided [artistDelimiters] as the set of delimiters for splitting. | |
| * It then creates proper song–artist cross-references based on the parsed artists. | |
| * | |
| * @param songs The list of songs to preprocess and deduplicate. | |
| * @param artistDelimiters Delimiters used to split a single artist string into multiple artists. | |
| * @param groupByAlbumArtist Whether albums should be grouped by album artist instead of track artist. |
| private val allArtistsFlow = musicDao.getAllArtistsRaw() | ||
|
|
||
| private val allCrossRefsFlow = musicDao.getAllSongArtistCrossRefs() |
There was a problem hiding this comment.
The allArtistsFlow and allCrossRefsFlow are being combined with multiple other flows throughout this repository. Each time any of the source flows emits, all flows will re-emit and the mapping logic will re-execute. This could lead to performance issues if the database is large or updates are frequent. Consider using more targeted queries that only fetch the necessary data for each specific operation, or use caching strategies to avoid reprocessing unchanged data.
| private val allArtistsFlow = musicDao.getAllArtistsRaw() | |
| private val allCrossRefsFlow = musicDao.getAllSongArtistCrossRefs() | |
| private val allArtistsFlow = musicDao.getAllArtistsRaw().conflate() | |
| private val allCrossRefsFlow = musicDao.getAllSongArtistCrossRefs().conflate() |
This pull request introduces comprehensive improvements to multi-artist support throughout the codebase. The main focus is on correctly handling songs with multiple artists using a junction table, improving how artist information is displayed and used, and updating repository and DAO methods to work with the new data model. Additionally, some legacy settings related to artist separation are removed, and data access is optimized.
Key changes include:
Multi-Artist Data Handling and Display:
Songdata class now uses a newdisplayArtistproperty that intelligently combines artist names from the artists list or splits the legacy artist string using common delimiters, ensuring accurate display for multi-artist tracks. (app/src/main/java/com/theveloper/pixelplay/data/model/Song.kt)song.artistfor display or AI/lyrics queries are replaced withsong.displayArtistto ensure consistency and correctness. (app/src/main/java/com/theveloper/pixelplay/data/ai/AiMetadataGenerator.kt,app/src/main/java/com/theveloper/pixelplay/data/ai/AiPlaylistGenerator.kt,app/src/main/java/com/theveloper/pixelplay/data/repository/LyricsRepositoryImpl.kt) [1] [2] [3]Repository and DAO Enhancements:
MusicRepositoryImplandMusicDaoare updated to fully support multi-artist relationships using theSongArtistCrossRefjunction table, including new queries for all artists and all cross-references, and new mapping logic to constructSongobjects with correct artist lists. (app/src/main/java/com/theveloper/pixelplay/data/database/MusicDao.kt,app/src/main/java/com/theveloper/pixelplay/data/repository/MusicRepositoryImpl.kt,app/src/main/java/com/theveloper/pixelplay/data/database/SongEntity.kt) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]Settings and Preferences Cleanup:
artistSeparationEnabledpreference and related logic are removed, as multi-artist parsing is now always enabled by default. (app/src/main/java/com/theveloper/pixelplay/data/preferences/UserPreferencesRepository.kt) [1] [2]Code Quality and Performance:
app/src/main/java/com/theveloper/pixelplay/data/repository/MusicRepositoryImpl.kt,app/src/main/java/com/theveloper/pixelplay/data/database/SongEntity.kt) [1] [2]These changes collectively ensure that multi-artist tracks are handled accurately throughout the app, both in data storage and user-facing features.
Multi-Artist Data Model and Display:
Songdata class with a robustdisplayArtistproperty that combines artists from the list or splits the legacy artist string as needed.song.displayArtistinstead of the legacyartistfield. [1] [2] [3]Repository and DAO Updates:
Songobjects with correct artist lists using cross-reference tables. [1] [2] [3] [4] [5] [6] [7] [8] [9]Settings Cleanup:
artistSeparationEnabledsetting and related code, as multi-artist support is now always enabled. [1] [2]Performance and Code Quality: