Merge pull request #1619 from theovilardo/fix/prefetch-artist-and-album-tap

Fix/prefetch artist and album tap

Author: theovilardo

app/src/main/java/com/theveloper/pixelplay/data/repository/ArtistImageRepository.kt

@@ -11,6 +11,7 @@ import com.theveloper.pixelplay.data.network.deezer.DeezerApiService
 import com.theveloper.pixelplay.utils.NetworkRetryUtils
 import com.theveloper.pixelplay.utils.isRetryableNetworkError
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.async
 import kotlinx.coroutines.awaitAll
 import kotlinx.coroutines.sync.Mutex
@@ -138,6 +139,8 @@ class ArtistImageRepository @Inject constructor(
                         } else {
                             Timber.tag(TAG).d("Skipping prefetch for $artistName") //check
                         }
+                    } catch (e: CancellationException) {
+                        throw e
                     } catch (e: Exception) {
                         Timber.tag(TAG).w("Failed to prefetch image for $artistName: ${e.message}")
                     }
@@ -194,6 +197,8 @@ class ArtistImageRepository @Inject constructor(
                     null
                 }
             }
+        } catch (e: CancellationException) {
+            throw e
         } catch (e: Exception) {
             Timber.tag(TAG).e("Error fetching artist image for $artistName: ${e.message}")
             // Consider transient errors? For now treating as failed to avoid spam.

app/src/main/java/com/theveloper/pixelplay/data/repository/MusicRepositoryImpl.kt

@@ -109,6 +109,7 @@ class MusicRepositoryImpl @Inject constructor(
     // Tracks the active prefetch job so a new flow emission cancels the previous one.
     @Volatile private var prefetchJob: Job? = null
     @Volatile private var currentSongArtistPrefetchJob: Job? = null
+    @Volatile private var currentSongArtistPrefetchSongId: Long? = null
     @Volatile private var telegramDownloadSyncObserverStarted = false
     private val telegramCacheManager: com.theveloper.pixelplay.data.telegram.TelegramCacheManager
         get() = telegramCacheManagerProvider.get()
@@ -310,7 +311,16 @@ class MusicRepositoryImpl @Inject constructor(
             .onEach { artists ->
                 val missingImages = artists.missingImageCandidates()
                 if (missingImages.isNotEmpty()) {
-                    currentSongArtistPrefetchJob?.cancel()
+                    val isNewSong = currentSongArtistPrefetchSongId != songId
+                    if (isNewSong) {
+                        currentSongArtistPrefetchJob?.cancel()
+                        currentSongArtistPrefetchSongId = songId
+                    } else if (currentSongArtistPrefetchJob?.isActive == true) {
+                        // Room re-emits as artist rows are updated; keep the current song batch
+                        // alive so one successful image write does not cancel the remaining fetches.
+                        return@onEach
+                    }
+
                     currentSongArtistPrefetchJob = repositoryScope.launch {
                         artistImageRepository.prefetchArtistImages(missingImages)
                     }

app/src/main/java/com/theveloper/pixelplay/presentation/components/AlbumCarouselSelection.kt

@@ -151,7 +151,7 @@ fun AlbumCarouselSection(
                         .fillMaxSize()
                         .aspectRatio(1f)
                         .clickable(
-                            enabled = isFocusedItem && song.albumId > 0L,
+                            enabled = isFocusedItem && song.albumId != -1L,
                             interactionSource = remember { MutableInteractionSource() },
                             indication = null,
                             onClick = { onAlbumClick(song) }

app/src/main/java/com/theveloper/pixelplay/presentation/viewmodel/PlayerViewModel.kt

@@ -2044,7 +2044,7 @@ class PlayerViewModel @Inject constructor(
     }
 
     fun triggerAlbumNavigationFromPlayer(albumId: Long) {
-        if (albumId <= 0) {
+        if (albumId == -1L) {
             Log.d("AlbumDebug", "triggerAlbumNavigationFromPlayer ignored invalid albumId=$albumId")
             return
         }

app/src/test/java/com/theveloper/pixelplay/data/repository/ArtistImageRepositoryTest.kt

@@ -1,5 +1,18 @@
 package com.theveloper.pixelplay.data.repository
 
+import com.theveloper.pixelplay.data.database.MusicDao
+import com.theveloper.pixelplay.data.network.deezer.DeezerApiService
+import com.theveloper.pixelplay.data.network.deezer.DeezerArtist
+import com.theveloper.pixelplay.data.network.deezer.DeezerSearchResponse
+import io.mockk.coEvery
+import io.mockk.coJustRun
+import io.mockk.coVerify
+import io.mockk.mockk
+import java.util.concurrent.atomic.AtomicInteger
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.awaitCancellation
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.test.runTest
 import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.Test
@@ -18,4 +31,51 @@ class ArtistImageRepositoryTest {
         assertTrue(sampleSize >= 4)
         assertEquals(8, sampleSize)
     }
+
+    @Test
+    fun `cancelled prefetch does not mark artist as failed for the session`() = runTest {
+        val deezerApiService = mockk<DeezerApiService>()
+        val musicDao = mockk<MusicDao>()
+        val repository = ArtistImageRepository(deezerApiService, musicDao)
+        val firstAttemptStarted = CompletableDeferred<Unit>()
+        val searchAttempts = AtomicInteger(0)
+        val rawUrl = "https://cdn-images.dzcdn.net/images/artist/250x250-000000-80-0-0.jpg"
+        val upgradedUrl = "https://cdn-images.dzcdn.net/images/artist/1000x1000-000000-80-0-0.jpg"
+
+        coEvery { musicDao.getArtistIdByNormalizedName("Artist Name") } returns 42L
+        coEvery { musicDao.getArtistImageUrl(42L) } returns null
+        coEvery { musicDao.getArtistImageUrlByNormalizedName("Artist Name") } returns null
+        coJustRun { musicDao.updateArtistImageUrl(42L, any()) }
+        coEvery { deezerApiService.searchArtist("Artist Name", 1) } coAnswers {
+            when (searchAttempts.incrementAndGet()) {
+                1 -> {
+                    firstAttemptStarted.complete(Unit)
+                    awaitCancellation()
+                }
+
+                else -> DeezerSearchResponse(
+                    data = listOf(
+                        DeezerArtist(
+                            id = 7L,
+                            name = "Artist Name",
+                            pictureBig = rawUrl
+                        )
+                    )
+                )
+            }
+        }
+
+        val prefetchJob = launch {
+            repository.prefetchArtistImages(listOf(42L to "Artist Name"))
+        }
+        firstAttemptStarted.await()
+        prefetchJob.cancel()
+        prefetchJob.join()
+
+        val imageUrl = repository.getArtistImageUrl("Artist Name", 42L)
+
+        assertEquals(upgradedUrl, imageUrl)
+        assertEquals(2, searchAttempts.get())
+        coVerify(exactly = 1) { musicDao.updateArtistImageUrl(42L, upgradedUrl) }
+    }
 }

app/src/test/java/com/theveloper/pixelplay/presentation/viewmodel/PlayerViewModelTest.kt

@@ -7,6 +7,8 @@ import com.google.common.util.concurrent.ListenableFuture
 import com.theveloper.pixelplay.data.model.SearchFilterType
 import com.theveloper.pixelplay.data.model.SearchHistoryItem
 import com.theveloper.pixelplay.data.model.SearchResultItem
+import com.theveloper.pixelplay.data.model.Album
+import com.theveloper.pixelplay.data.model.Artist
 import com.theveloper.pixelplay.data.model.Song
 import com.theveloper.pixelplay.data.model.SortOption
 import com.theveloper.pixelplay.data.model.StorageFilter
@@ -389,22 +391,43 @@ class PlayerViewModelTest {
         assertEquals(Player.REPEAT_MODE_OFF, controllerRepeatMode)
     }
 
+    @Test
+    fun `album navigation from player accepts synthetic negative album ids`() = runTest {
+        playerViewModel.albumNavigationRequests.test {
+            playerViewModel.triggerAlbumNavigationFromPlayer(-42L)
+            advanceUntilIdle()
+
+            assertEquals(-42L, awaitItem())
+            cancelAndConsumeRemainingEvents()
+        }
+    }
+
+    @Test
+    fun `album navigation from player still ignores sentinel album id`() = runTest {
+        playerViewModel.albumNavigationRequests.test {
+            playerViewModel.triggerAlbumNavigationFromPlayer(-1L)
+            advanceUntilIdle()
+
+            expectNoEvents()
+        }
+    }
+
     @Nested
     @DisplayName("Shuffle Functionality")
     inner class ShuffleFunctionalityTests {
 
         private val song1 = Song(id = "1", title = "Song 1", artist = "Artist A", genre = "Rock", albumArtUriString = "cover1.png", artistId = 1L, albumId = 1L, contentUriString = "content://dummy/1", duration = 180000L, bitrate = null, sampleRate = null, album = "Album", path = "path", mimeType = "audio/mpeg")
         private val song2 = Song(id = "2", title = "Song 2", artist = "Artist B", genre = "Pop", albumArtUriString = "cover2.png", artistId = 2L, albumId = 2L, contentUriString = "content://dummy/2", duration = 200000L, bitrate = null, sampleRate = null, album = "Album", path = "path", mimeType = "audio/mpeg")
         private val song3 = Song(id = "3", title = "Song 3", artist = "Artist C", genre = "Jazz", albumArtUriString = "cover3.png", artistId = 3L, albumId = 3L, contentUriString = "content://dummy/3", duration = 210000L, bitrate = null, sampleRate = null, album = "Album", path = "path", mimeType = "audio/mpeg")
-        
-        @Test
-        fun `shuffleAllSongs calls prepareShuffledQueue with random songs`() = runTest {
-            // Arrange
-            val randomSongs = listOf(song2, song3, song1)
-            coEvery { mockMusicRepository.getRandomSongs(500) } returns randomSongs
-            
-            // Mock queue preparation to return a valid shuffled queue and start song
-            coEvery { mockQueueStateHolder.prepareShuffledQueueSuspending(randomSongs, any()) } returns Pair(randomSongs, song2)
+
+        private fun stubShuffledPlayback(
+            songs: List<Song>,
+            queueName: String,
+            startSong: Song = songs.first()
+        ) {
+            coEvery {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(songs, queueName, true)
+            } returns Pair(songs, startSong)
 
             mockkObject(MediaItemBuilder)
             every { MediaItemBuilder.build(any()) } returns MediaItem.Builder()
@@ -414,14 +437,126 @@ class PlayerViewModelTest {
             val mockedPlaybackUri = mockk<android.net.Uri>(relaxed = true)
             every { mockedPlaybackUri.scheme } returns "file"
             every { MediaItemBuilder.playbackUri(any()) } returns mockedPlaybackUri
+        }
+
+        @Test
+        fun `shuffleAllSongs calls prepareShuffledQueue with random songs at index zero`() = runTest {
+            val randomSongs = listOf(song2, song3, song1)
+            coEvery { mockMusicRepository.getRandomSongs(500) } returns randomSongs
+            stubShuffledPlayback(randomSongs, "All Songs (Shuffled)", startSong = song2)
 
-            // Act
             playerViewModel.shuffleAllSongs()
             advanceUntilIdle()
 
-            // Assert
             coVerify { mockMusicRepository.getRandomSongs(500) }
-            coVerify { mockQueueStateHolder.prepareShuffledQueueSuspending(randomSongs, "All Songs (Shuffled)") }
+            coVerify {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(
+                    randomSongs,
+                    "All Songs (Shuffled)",
+                    true
+                )
+            }
+        }
+
+        @Test
+        fun `playRandomSong calls prepareShuffledQueue with startAtZero`() = runTest {
+            val randomSongs = listOf(song3, song1, song2)
+            coEvery { mockMusicRepository.getRandomSongs(500) } returns randomSongs
+            stubShuffledPlayback(randomSongs, "All Songs (Shuffled)", startSong = song3)
+
+            playerViewModel.playRandomSong()
+            advanceUntilIdle()
+
+            coVerify {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(
+                    randomSongs,
+                    "All Songs (Shuffled)",
+                    true
+                )
+            }
+        }
+
+        @Test
+        fun `shuffleFavoriteSongs calls prepareShuffledQueue with startAtZero`() = runTest {
+            val favoriteSongs = listOf(song1, song3)
+            coEvery { mockMusicRepository.getFavoriteSongsOnce(StorageFilter.ALL) } returns favoriteSongs
+            stubShuffledPlayback(favoriteSongs, "Liked Songs (Shuffled)", startSong = song1)
+
+            playerViewModel.shuffleFavoriteSongs()
+            advanceUntilIdle()
+
+            coVerify { mockMusicRepository.getFavoriteSongsOnce(StorageFilter.ALL) }
+            coVerify {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(
+                    favoriteSongs,
+                    "Liked Songs (Shuffled)",
+                    true
+                )
+            }
+        }
+
+        @Test
+        fun `shuffleRandomAlbum calls prepareShuffledQueue with startAtZero`() = runTest {
+            val album = Album(
+                id = 77L,
+                title = "Album Roulette",
+                artist = "Artist A",
+                year = 2024,
+                dateAdded = 0L,
+                albumArtUriString = null,
+                songCount = 3
+            )
+            every { mockLibraryStateHolder.albums } returns MutableStateFlow(persistentListOf(album))
+            every { mockMusicRepository.getSongsForAlbum(album.id) } returns flowOf(listOf(song1, song2, song3))
+            stubShuffledPlayback(listOf(song1, song2, song3), album.title, startSong = song2)
+
+            playerViewModel.shuffleRandomAlbum()
+            advanceUntilIdle()
+
+            coVerify {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(
+                    listOf(song1, song2, song3),
+                    "Album Roulette",
+                    true
+                )
+            }
+        }
+
+        @Test
+        fun `shuffleRandomArtist calls prepareShuffledQueue with startAtZero`() = runTest {
+            val artist = Artist(id = 88L, name = "Artist Roulette", songCount = 3)
+            every { mockLibraryStateHolder.artists } returns MutableStateFlow(persistentListOf(artist))
+            every { mockMusicRepository.getSongsForArtist(artist.id) } returns flowOf(listOf(song3, song2, song1))
+            stubShuffledPlayback(listOf(song3, song2, song1), artist.name, startSong = song3)
+
+            playerViewModel.shuffleRandomArtist()
+            advanceUntilIdle()
+
+            coVerify {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(
+                    listOf(song3, song2, song1),
+                    "Artist Roulette",
+                    true
+                )
+            }
+        }
+
+        @Test
+        fun `triggerShuffleAllFromTile uses startAtZero when library is already loaded`() = runTest {
+            val songs = listOf(song1, song2, song3)
+            _allSongsFlow.value = songs.toImmutableList()
+            stubShuffledPlayback(songs, "All Songs (Shuffled)", startSong = song1)
+
+            playerViewModel.triggerShuffleAllFromTile()
+            advanceUntilIdle()
+
+            coVerify {
+                mockQueueStateHolder.prepareShuffledQueueSuspending(
+                    songs,
+                    "All Songs (Shuffled)",
+                    true
+                )
+            }
         }
     }
 

app/src/test/java/com/theveloper/pixelplay/utils/QueueUtilsTest.kt

@@ -74,6 +74,27 @@ class QueueUtilsTest {
         )
     }
 
+    @Test
+    fun buildAnchoredShuffleQueueSuspending_startAtZero_placesAnchorFirst() = runBlocking {
+        val songs = buildSongs(32)
+        val anchorIndex = 11
+
+        val shuffled = QueueUtils.buildAnchoredShuffleQueueSuspending(
+            currentQueue = songs,
+            anchorIndex = anchorIndex,
+            startAtZero = true,
+            random = Random(99)
+        )
+
+        assertEquals("Anchor song must become the first item", songs[anchorIndex].id, shuffled.first().id)
+        assertEquals("Queue size must stay the same", songs.size, shuffled.size)
+        assertEquals(
+            "Shuffled queue must contain the same songs",
+            songs.map { it.id }.toSet(),
+            shuffled.map { it.id }.toSet()
+        )
+    }
+
     private fun buildSongs(count: Int): List<Song> = List(count) { index ->
         Song(
             id = "song-$index",