Add data transfer tools, lyrics improvements, and navigation UX updates#992
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive data backup/restore system with selectable sections, implements global haptics control, improves lyrics handling with embedded lyrics prioritization and parallel API search, restores AI playlist functionality, and enhances navigation UX with infinite tab sliding and animated bottom nav icons.
Changes:
- Implemented export/import backup flow supporting preferences, favorites, lyrics, search history, and transition rules with selectable sections
- Added global haptics toggle with app-wide application through view hierarchy
- Improved lyrics system by prioritizing embedded lyrics, persisting fetched lyrics to file metadata, and parallelizing API search strategies for faster results
- Restored AI playlist generation action in library playlist controls
- Added compact library infinite tab sliding behavior with proper modulo arithmetic for seamless cycling
- Implemented animated bottom nav icons with scale effects and search double-tap handling to activate search bar
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| strings.xml | Added message string for embedded lyrics availability notification |
| SettingsViewModel.kt | Added haptics preference, backup manager integration, and export/import methods with progress tracking |
| PlayerViewModel.kt | Exposed haptics enabled state flow and search nav double-tap event emission |
| LyricsStateHolder.kt | Prioritized embedded lyrics in manual fetch, added metadata persistence, and song update event flows |
| SettingsCategoryScreen.kt | Added backup/restore UI with section selection dialogs and activity result launchers |
| SearchScreen.kt | Integrated double-tap event handling to activate search bar |
| LibraryScreen.kt | Implemented infinite pager logic for compact navigation mode with helper functions |
| LibraryActionRow.kt | Restored AI playlist button alongside import M3U button |
| CustomNavigationBarItem.kt | Changed onClick to always trigger, added icon scale animation for selection |
| PlayerInternalNavigationBar.kt | Added double-tap detection logic for search icon with timestamp tracking |
| LyricsRepositoryImpl.kt | Parallelized search strategies using channels for first non-empty result, added UNSYNCEDLYRICS tag support |
| UserPreferencesRepository.kt | Added haptics preference and backup export/import methods for all preference types |
| PreferenceBackupEntry.kt | New data class for preference backup representation |
| AudioMetadataReader.kt | Added lyrics field extraction from LYRICS and UNSYNCEDLYRICS tags |
| TransitionDao.kt | Added batch insert, get all, and clear all methods for backup support |
| SearchHistoryDao.kt | Added batch insert and get all methods for backup support |
| LyricsDao.kt | Added batch insert and get all methods for backup support |
| FavoritesDao.kt | Added batch insert, get all, and clear all methods for backup support |
| AppDataBackupManager.kt | New manager class implementing JSON-based export/import for selected data sections |
| MainActivity.kt | Applied haptics setting to root view hierarchy in LaunchedEffect |
Comments suppressed due to low confidence (1)
app/src/main/java/com/theveloper/pixelplay/presentation/components/PlayerInternalNavigationBar.kt:117
- The
onClicklambda is no longer wrapped in aremembercall, which means it will be recreated on every recomposition. This could cause unnecessary recompositions of child components that depend on this lambda. The previous implementation usedremember(navController, item.screen.route)to memoize the callback. Consider wrapping this lambda in arememberblock with appropriate keys to avoid unnecessary recompositions.
val onClickLambda: () -> Unit = {
val isSearchTab = item.screen.route == Screen.Search.route
val isAlreadySelected = currentRoute == item.screen.route
if (isSearchTab && isAlreadySelected) {
val now = SystemClock.elapsedRealtime()
if (now - lastSearchTapTimestamp <= 350L) {
onSearchIconDoubleTap()
lastSearchTapTimestamp = 0L
} else {
lastSearchTapTimestamp = now
}
} else if (!isAlreadySelected) {
navController.navigate(item.screen.route) {
popUpTo(navController.graph.id) { inclusive = true; saveState = false }
launchSingleTop = true
restoreState = false
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val sectionLabels = remember { | ||
| mapOf( | ||
| BackupSection.PREFERENCES to "Preferences and playlists", | ||
| BackupSection.FAVORITES to "Favorites", | ||
| BackupSection.LYRICS to "Lyrics", | ||
| BackupSection.SEARCH_HISTORY to "Search history", | ||
| BackupSection.TRANSITIONS to "Transition rules" | ||
| ) | ||
| } |
There was a problem hiding this comment.
The backup section labels map is recreated on every recomposition of the AlertDialog. Consider moving this map outside the composable function as a constant or using remember without keys to ensure it's only created once.
| runCatching { | ||
| songMetadataEditor.editSongMetadata( | ||
| songId = songId, | ||
| newTitle = song.title, | ||
| newArtist = song.artist, | ||
| newAlbum = song.album, | ||
| newGenre = song.genre ?: "", | ||
| newLyrics = normalizedLyrics, | ||
| newTrackNumber = song.trackNumber, | ||
| coverArtUpdate = coverArtUpdate | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The persistLyricsToFileMetadataIfPossible method silently ignores all errors when writing lyrics to file metadata (using runCatching without error handling). While this might be intentional to prevent blocking the lyrics flow, it makes debugging difficult when metadata writes fail. Consider at least logging failures so users and developers can diagnose issues with file permissions or unsupported file formats.
| .fillMaxHeight() | ||
| .clickable( | ||
| onClick = { if (!selected) onClick() else null }, | ||
| onClick = onClick, |
There was a problem hiding this comment.
The navigation bar's onClick handler was changed from preventing clicks when already selected to always calling onClick. This changes the behavior for all tabs, not just the search tab. For tabs other than search, this means clicking an already-selected tab will now trigger navigation logic unnecessarily. The previous behavior of only navigating when not selected was more efficient. Consider preserving the old behavior for non-search tabs by checking isAlreadySelected for all tabs and only allowing re-clicks for search with double-tap logic.
| onClick = onClick, | |
| onClick = { | |
| if (!selected) { | |
| onClick() | |
| } | |
| }, |
| val exportLauncher = rememberLauncherForActivityResult( | ||
| contract = ActivityResultContracts.CreateDocument("application/json") | ||
| ) { uri -> | ||
| if (uri != null) { | ||
| settingsViewModel.exportAppData(uri, exportSections) | ||
| } | ||
| } | ||
|
|
||
| val importLauncher = rememberLauncherForActivityResult( | ||
| contract = ActivityResultContracts.GetContent() | ||
| ) { uri -> | ||
| if (uri != null) { | ||
| settingsViewModel.importAppData(uri, importSections) | ||
| } | ||
| } |
There was a problem hiding this comment.
The export data UI disables the button when a transfer is in progress, but the import launcher could still be invoked before the export completes if the user rapidly switches between dialogs. Consider also checking isDataTransferInProgress before opening the file picker dialogs, or add mutual exclusion between export and import operations to prevent race conditions.
| userPreferencesRepository.importPreferencesFromBackup( | ||
| entries = it, | ||
| clearExisting = true | ||
| ) |
There was a problem hiding this comment.
The importPreferencesFromBackup method uses clearExisting = true by default, which means importing any preferences will wipe all existing preferences, not just the ones being imported. This could be unexpected behavior if a user imports a partial backup. Consider either warning users about this in the UI, or implementing selective import that only overwrites preferences that exist in the backup file.
| horizontalArrangement = Arrangement.SpaceAround, | ||
| verticalAlignment = Alignment.CenterVertically | ||
| ) { | ||
| var lastSearchTapTimestamp by remember { mutableStateOf(0L) } |
There was a problem hiding this comment.
The double-tap state variable lastSearchTapTimestamp is declared inside the forEach loop, which means each navigation item will have its own independent timestamp state. This will cause the double-tap detection to not work correctly when there are multiple navigation items, as the state will be recreated for each item in the loop. The variable should be declared outside the forEach loop to maintain a single shared state for double-tap detection across all navigation items.
| val channel = Channel<RemoteSearchBatch>(capacity = strategies.size) | ||
| val jobs = strategies.map { strategy -> | ||
| launch { | ||
| val responses = runCatching { strategy.request() } | ||
| .getOrNull() | ||
| ?.toList() | ||
| .orEmpty() | ||
| channel.trySend( | ||
| RemoteSearchBatch( | ||
| strategyName = strategy.name, | ||
| responses = responses | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The parallel lyrics search implementation has a potential race condition. When multiple strategies return results simultaneously, the channel.trySend() calls may fail silently (since trySend returns a result that's not being checked), and the repeat loop will block indefinitely on channel.receive() waiting for results that were never successfully sent. This can cause the search to hang. Consider using channel.send() instead to suspend and ensure all results are delivered, or check the result of trySend() and handle failures appropriately.
| val sectionLabels = remember { | ||
| mapOf( | ||
| BackupSection.PREFERENCES to "Preferences and playlists", | ||
| BackupSection.FAVORITES to "Favorites", | ||
| BackupSection.LYRICS to "Lyrics", | ||
| BackupSection.SEARCH_HISTORY to "Search history", | ||
| BackupSection.TRANSITIONS to "Transition rules" | ||
| ) | ||
| } |
There was a problem hiding this comment.
The same section labels map is duplicated in both the export and import dialogs (lines 899-907 and 962-970). This violates the DRY principle and makes maintenance harder. Consider extracting this map to a constant at the file or class level to avoid duplication.
| if (showExportDataDialog) { | ||
| val sectionLabels = remember { | ||
| mapOf( | ||
| BackupSection.PREFERENCES to "Preferences and playlists", |
There was a problem hiding this comment.
The backup section descriptions mention "Preferences and playlists" but the backup manager does not appear to export/import playlist data. The PREFERENCES section only handles user preferences from DataStore. If playlists are stored separately (possibly in a different DAO or repository), they should either be included in the backup or the label should be updated to remove the mention of playlists to avoid misleading users.
|
|
||
| LaunchedEffect(hapticsEnabled, rootView) { | ||
| rootView.isHapticFeedbackEnabled = hapticsEnabled | ||
| rootView.rootView?.isHapticFeedbackEnabled = hapticsEnabled |
There was a problem hiding this comment.
Setting rootView.rootView?.isHapticFeedbackEnabled appears to be redundant since rootView should already be the root view in most cases, and accessing .rootView on it would return the same view. This could also potentially cause confusion or unexpected behavior if the view hierarchy is complex. Consider either removing this line or adding a comment explaining why both are needed.
| rootView.rootView?.isHapticFeedbackEnabled = hapticsEnabled |
Context
This PR bundles a set of quality-of-life and reliability improvements across data transfer, lyrics, navigation, and library UX.
The main goals were:
What Changed
1) Data Transfer (Export/Import)
PREFERENCESFAVORITESLYRICSSEARCH_HISTORYTRANSITIONSKey files:
app/src/main/java/com/theveloper/pixelplay/data/backup/AppDataBackupManager.ktapp/src/main/java/com/theveloper/pixelplay/data/preferences/PreferenceBackupEntry.ktapp/src/main/java/com/theveloper/pixelplay/presentation/viewmodel/SettingsViewModel.ktapp/src/main/java/com/theveloper/pixelplay/presentation/screens/SettingsCategoryScreen.ktFavoritesDao.kt,LyricsDao.kt,SearchHistoryDao.kt,TransitionDao.kt2) Lyrics Reliability + Metadata Behavior
LYRICSUNSYNCEDLYRICSKey files:
app/src/main/java/com/theveloper/pixelplay/presentation/viewmodel/LyricsStateHolder.ktapp/src/main/java/com/theveloper/pixelplay/data/media/AudioMetadataReader.ktapp/src/main/java/com/theveloper/pixelplay/data/repository/LyricsRepositoryImpl.ktapp/src/main/res/values/strings.xml3) Lyrics Search Performance
runSearchStrategiesFast(...)helper to run multiple query forms concurrently and return on first non-empty response batch.Expected impact:
Key file:
app/src/main/java/com/theveloper/pixelplay/data/repository/LyricsRepositoryImpl.kt4) Navigation and Interaction UX
Key files:
app/src/main/java/com/theveloper/pixelplay/data/preferences/UserPreferencesRepository.ktapp/src/main/java/com/theveloper/pixelplay/presentation/viewmodel/PlayerViewModel.ktapp/src/main/java/com/theveloper/pixelplay/MainActivity.ktapp/src/main/java/com/theveloper/pixelplay/presentation/components/scoped/CustomNavigationBarItem.ktapp/src/main/java/com/theveloper/pixelplay/presentation/components/PlayerInternalNavigationBar.ktapp/src/main/java/com/theveloper/pixelplay/presentation/screens/SearchScreen.kt5) Library UX Improvements
Key files:
app/src/main/java/com/theveloper/pixelplay/presentation/screens/LibraryScreen.ktapp/src/main/java/com/theveloper/pixelplay/presentation/components/subcomps/LibraryActionRow.ktValidation
Executed:
./gradlew :app:compileDebugKotlin -x lint./gradlew :app:installDebug -x lintNotes:
Risk / Tradeoffs
Scope