Conversation
…rics changes reverted
… display with full history, and refine shuffling smoothness
- Optimize initial library scan by using bulk insert for empty databases - Remove "Quick Sync" option from settings (redundant with auto-sync) - Add granular 1-by-1 progress updates for Full Rescan and Rebuild Database - Enable readable crash logs in release builds: - Preserve line numbers in stack traces
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive refactoring of the app's sync system and removes the lyrics refresh feature to prevent API abuse. The changes focus on optimizing library scanning performance, improving queue management, and enhancing developer experience.
Key Changes:
- Introduced three sync modes (INCREMENTAL, FULL, REBUILD) with smart detection of fresh installs for optimized bulk inserts
- Removed bulk lyrics refresh functionality to prevent lrclib API abuse, including UI components and ViewModel logic
- Refactored player queue rebuilding to use direct DualPlayerEngine access, avoiding Binder transaction limits and preserving the currently playing MediaItem to prevent audio glitches
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| SettingsViewModel.kt | Removed lyrics refresh state flows and methods; added fullSyncLibrary() and rebuildDatabase() functions |
| PlayerViewModel.kt | Added DualPlayerEngine injection; refactored rebuildPlayerQueue() to preserve current song and use direct engine access |
| SettingsComponents.kt | Replaced single sync button with separate "Full Rescan" and "Rebuild Database" buttons with appropriate styling |
| SettingsCategoryScreen.kt | Removed lyrics refresh UI and warning dialog; added rebuild database confirmation dialog |
| LibraryScreen.kt | Changed pull-to-refresh to use incrementalSync() instead of generic sync() |
| UnifiedPlayerSheet.kt | Added song info bottom sheet integration for queue items with navigation and editing capabilities |
| QueueBottomSheet.kt | Modified to display full queue including history; enabled song info access via three-dot menu |
| AppModule.kt | Configured Coil ImageLoader with memory cache (20% of app memory) and 100MB disk cache |
| SyncWorker.kt | Implemented three sync modes with incremental updates, fresh install detection, and configurable progress granularity |
| SyncManager.kt | Added incrementalSync(), fullSync(), and rebuildDatabase() methods with documentation |
| UserPreferencesRepository.kt | Added LAST_SYNC_TIMESTAMP preference with getter/setter methods |
| MusicDao.kt | Added incrementalSyncMusicData() method for efficient upsert/delete operations |
| proguard-rules.pro | Enabled line number preservation and disabled obfuscation for better crash reporting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only clear if explicitly requested (FULL/REBUILD) and not a fresh start. | ||
| // If it's a fresh start (INCREMENTAL turned FULL), no need to clear. | ||
| // Actually, if it's FULL or REBUILD, we SHOULD clear to be safe, unless it IS fresh. | ||
| // But if it was INCREMENTAL and we promoted it because it's fresh, we definitely don't need to clear. |
There was a problem hiding this comment.
The logic for clearing the database has confusing and contradictory comments. Lines 172-175 contain nested comments like "Actually, if it's FULL or REBUILD, we SHOULD clear to be safe, unless it IS fresh. But if it was INCREMENTAL and we promoted it because it's fresh, we definitely don't need to clear." This suggests unclear logic. The condition !isFreshInstall && syncMode != SyncMode.INCREMENTAL means it will clear for FULL and REBUILD modes when not a fresh install, but the comment suggests confusion about the intended behavior.
| // Only clear if explicitly requested (FULL/REBUILD) and not a fresh start. | |
| // If it's a fresh start (INCREMENTAL turned FULL), no need to clear. | |
| // Actually, if it's FULL or REBUILD, we SHOULD clear to be safe, unless it IS fresh. | |
| // But if it was INCREMENTAL and we promoted it because it's fresh, we definitely don't need to clear. | |
| // For non-incremental syncs (FULL/REBUILD) on an existing library, clear all music data first. | |
| // On a fresh install we skip clearing because there is no existing library to remove. |
| // Replace items BEFORE current index (indices < currentIndex) | ||
| if (currentIndex > 0) { | ||
| enginePlayer.removeMediaItems(0, currentIndex) | ||
| } | ||
| if (itemsBefore.isNotEmpty()) { | ||
| enginePlayer.addMediaItems(0, itemsBefore) | ||
| } | ||
|
|
||
| // Note: We intentionally DO NOT touch the item at 'currentIndex'. | ||
| // This ensures playback continues seamlessly. | ||
| } |
There was a problem hiding this comment.
There's a potential logic error in the queue rebuilding algorithm. The code removes and adds items relative to currentIndex, but this index refers to the OLD queue's current position. After removing items BEFORE currentIndex (line 3117), the currentIndex becomes invalid because the list size has changed. When you then try to add items at position 0 (line 3120), the item that was at currentIndex is no longer at that position - it's now at position itemsBefore.size. This could cause the wrong song to play or other queue position issues.
| player.setMediaItems(reorderedItems, safeIndex, positionMs) | ||
| // Replace items BEFORE current index (indices < currentIndex) | ||
| if (currentIndex > 0) { | ||
| enginePlayer.removeMediaItems(0, currentIndex) |
There was a problem hiding this comment.
Inconsistent indentation: This line uses a leading space before enginePlayer.removeMediaItems while other similar lines in the function don't. This should use consistent indentation without the extra leading space for better code readability.
| enginePlayer.removeMediaItems(0, currentIndex) | |
| enginePlayer.removeMediaItems(0, currentIndex) |
| onDismiss = { onRemoveSong(song.id) }, | ||
| isFromPlaylist = false, | ||
| onMoreOptionsClick = {}, | ||
| isFromPlaylist = true, |
There was a problem hiding this comment.
Setting isFromPlaylist = true for queue items is misleading. The queue is not a playlist, and this flag may cause incorrect behavior or UI presentation. This should be false to accurately represent that these items are from the playback queue, not a user-created playlist.
| isFromPlaylist = true, | |
| isFromPlaylist = false, |
| // Use granular progress updates (1 by 1) for Full and Rebuild modes as requested | ||
| val progressBatchSize = if (syncMode == SyncMode.FULL || syncMode == SyncMode.REBUILD) 1 else 50 |
There was a problem hiding this comment.
Setting progressBatchSize = 1 for Full and Rebuild modes will cause extremely frequent progress updates (one per song). For large libraries with thousands of songs, this could cause performance issues due to excessive WorkManager progress updates and potential UI recomposition overhead. Consider using a more reasonable batch size like 10-25 to balance granular feedback with performance.
| // Use granular progress updates (1 by 1) for Full and Rebuild modes as requested | |
| val progressBatchSize = if (syncMode == SyncMode.FULL || syncMode == SyncMode.REBUILD) 1 else 50 | |
| // Use more granular progress updates for Full and Rebuild modes while avoiding per-song updates | |
| val progressBatchSize = if (syncMode == SyncMode.FULL || syncMode == SyncMode.REBUILD) 20 else 50 |
|
|
||
| // Library Sync | ||
| val LAST_SYNC_TIMESTAMP = longPreferencesKey("last_sync_timestamp") |
There was a problem hiding this comment.
The LAST_SYNC_TIMESTAMP preference is defined but never actually used in the sync logic. The incremental sync implementation in SyncWorker relies on comparing mediaStoreSong.dateAdded > localSong.dateAdded (line 103 of SyncWorker.kt), which doesn't use this timestamp. Either the timestamp should be utilized in the sync logic to track when the last sync occurred, or this unused preference should be removed to avoid confusion.
| import androidx.compose.ui.graphics.Color | ||
| import androidx.compose.ui.res.painterResource | ||
| import androidx.compose.ui.text.font.FontWeight | ||
| import androidx.compose.ui.text.style.TextOverflow |
There was a problem hiding this comment.
The RefreshLyricsItem composable function and its import of LyricsRefreshProgress are no longer needed after removing the lyrics refresh functionality. The data classes FailedSongInfo and LyricsRefreshProgress defined in SettingsViewModel.kt are unused but still imported here (line 69). This composable should be removed along with the unused import to complete the cleanup of the removed feature.
| // 2. Items BEFORE the current song | ||
| val itemsBefore = withContext(Dispatchers.Default) { | ||
| if (targetIndex > 0) { | ||
| targetQueue.subList(0, targetIndex).map { buildMediaItemFromSong(it) } |
There was a problem hiding this comment.
Inconsistent indentation: This line uses a leading space before targetQueue.subList while other similar lines in the function don't. This should use consistent indentation without the extra leading space for better code readability.
| targetQueue.subList(0, targetIndex).map { buildMediaItemFromSong(it) } | |
| targetQueue.subList(0, targetIndex).map { buildMediaItemFromSong(it) } |
| Toast.makeText(context, "Playing next", Toast.LENGTH_SHORT).show() | ||
| }, | ||
| onAddToPlayList = { | ||
| // Trigger playlist selection dialog (if implemented in ViewModel or UI) | ||
| // For now we might need a placeholder or check how it is implemented elsewhere. | ||
| // playerViewModel doesn't seem to have 'openAddToPlaylistDialog'. | ||
| // Maybe we can skip this or implement if simple. | ||
| // SongInfoBottomSheet usually handles the UI for it? No, it has onAddToPlayList callback. | ||
| // Let's leave it empty or log for now if we don't have a ready handler | ||
| Log.d("UnifiedPlayerSheet", "Add to playlist clicked for ${song.title}") | ||
| selectedSongForInfo = null | ||
| }, | ||
| onDeleteFromDevice = { activity, songToDelete, onResult -> | ||
| playerViewModel.deleteFromDevice(activity, songToDelete, onResult) | ||
| selectedSongForInfo = null | ||
| }, | ||
| onNavigateToAlbum = { | ||
| playerViewModel.collapsePlayerSheet() | ||
| animateQueueSheet(false) | ||
| selectedSongForInfo = null | ||
| // We need navigation logic here. | ||
| // navController.navigate(Screen.AlbumDetail.createRoute(song.albumId)) | ||
| // song.albumId is a Long. | ||
| // Check Screen.AlbumDetail route format. | ||
| // Assume standard createRoute(id). | ||
| if (song.albumId != -1L) { | ||
| navController.navigate(Screen.AlbumDetail.createRoute(song.albumId)) | ||
| } | ||
| }, | ||
| onNavigateToArtist = { | ||
| playerViewModel.collapsePlayerSheet() | ||
| animateQueueSheet(false) | ||
| selectedSongForInfo = null | ||
| if (song.artistId != -1L) { | ||
| navController.navigate(Screen.ArtistDetail.createRoute(song.artistId)) | ||
| } | ||
| }, | ||
| onEditSong = { title, artist, album, genre, lyrics, trackNumber, coverArtUpdate -> | ||
| playerViewModel.editSongMetadata(song, title, artist, album, genre, lyrics, trackNumber, coverArtUpdate) | ||
| selectedSongForInfo = null | ||
| }, | ||
| generateAiMetadata = { fields -> playerViewModel.generateAiMetadata(song, fields) }, | ||
| removeFromListTrigger = { | ||
| // This is usually used to remove from a specific list (like 'Favorites'). | ||
| // In Queue, we have specific 'Remove' button. | ||
| // But maybe the user wants to remove from queue via this menu? | ||
| playerViewModel.removeSongFromQueue(song.id) | ||
| selectedSongForInfo = null |
There was a problem hiding this comment.
Inconsistent indentation: Lines 1417, 1427, 1456, and 1464 use leading spaces before statements while other similar lines don't. This should use consistent indentation without extra leading spaces for better code readability.
| if (mediaStoreSongs.isNotEmpty()) { | ||
| // Fetch existing local songs to preserve their editable metadata | ||
| val localSongsMap = musicDao.getAllSongsList().associateBy { it.id } | ||
| val isFreshInstall = localSongsMap.isEmpty() |
There was a problem hiding this comment.
In REBUILD mode, the database is cleared before fetching songs (line 79), but then isFreshInstall is determined by checking if localSongsMap.isEmpty() (line 96). Since REBUILD clears the database, localSongsMap will always be empty, making isFreshInstall always true for REBUILD mode. This could lead to incorrect logic flow in the sync branching at lines 156 and 171, potentially skipping necessary operations or choosing the wrong sync path.
| val isFreshInstall = localSongsMap.isEmpty() | |
| // A fresh install means no existing songs before this sync, not a cleared DB in REBUILD mode | |
| val isFreshInstall = localSongsMap.isEmpty() && syncMode != SyncMode.REBUILD |
lostf1sh
changed the title
🚀 Sync Logic Optimization
Faster initial scan: Detects empty database (fresh install) and uses optimized bulk insert instead of incremental sync overhead
Granular progress updates: Full Rescan and Rebuild Database now show 1-by-1 progress for better UX
Removed Quick Sync: Simplified settings by removing redundant option (auto-sync handles incremental updates)
🎵 Queue Enhancements
Added song info bottom sheet accessible directly from queue
Enhanced queue interaction and usability
🧹 Settings Cleanup
Removed "Refresh Lyrics" bulk feature to prevent API abuse on lrclib
Simplified library refresh options to "Full Rescan" and "Rebuild Database"
🐛 Developer Experience
Readable crash logs in release builds: Enabled line number preservation in ProGuard
Disabled obfuscation: App is open source, no need to hide code - crash reports now show full class/method names