diff --git a/app/src/main/java/com/bugzapperlabs/mycasts/data/repository/QueueRepository.kt b/app/src/main/java/com/bugzapperlabs/mycasts/data/repository/QueueRepository.kt index 377d186e..b65f7261 100644 --- a/app/src/main/java/com/bugzapperlabs/mycasts/data/repository/QueueRepository.kt +++ b/app/src/main/java/com/bugzapperlabs/mycasts/data/repository/QueueRepository.kt @@ -45,6 +45,34 @@ class QueueRepository @Inject constructor( ) } + /** + * Ensures [itemId] is the queue's front entry (issue #196): moves it there if already queued, + * inserts it there (never as auto-queued) otherwise. This is how [com.bugzapperlabs.mycasts.playback.PlaybackController] + * marks an episode "now playing" -- the currently-playing episode is a real queue entry like + * any other, always at the front, rather than a special case hidden from the queue entirely, + * so it shows up in Next Up itself (clearly marked) instead of only via the mini/expanded player. + */ + suspend fun moveToFront(itemId: String) { + val position = queueDao.minPosition() - 1 + if (queueDao.findItemId(itemId) != null) { + queueDao.setPosition(itemId, position) + } else { + queueDao.insert(QueueEntry(itemId, position = position, addedAt = System.currentTimeMillis())) + } + } + + /** + * Moves [itemId] to the back of the queue if it's currently queued; a no-op otherwise (issue + * #196). Used to get a just-failed episode out of the front slot so [PlaybackService] doesn't + * immediately trip over it again advancing to whatever's next, without discarding it outright + * the way a finished episode is -- it never actually played, so it's still worth surfacing in + * Next Up for the user to retry. + */ + suspend fun moveToEnd(itemId: String) { + if (queueDao.findItemId(itemId) == null) return + queueDao.setPosition(itemId, queueDao.maxPosition() + 1) + } + /** Returns the entry that was removed (for issue #284's undo), or null if it wasn't queued. */ suspend fun remove(itemId: String): QueueEntry? { val entry = queueDao.getEntry(itemId) ?: return null @@ -61,12 +89,10 @@ class QueueRepository @Inject constructor( orderedItemIds.forEachIndexed { index, itemId -> queueDao.setPosition(itemId, index) } } - /** Removes and returns the item at the front of the queue, or null if the queue is empty. */ - suspend fun popNext(): String? { - val next = queueDao.firstItemId() ?: return null - queueDao.remove(next) - return next - } + /** The item at the front of the queue, without removing it (issue #196) -- the currently-playing + * episode is meant to stay queued (as the front entry) for as long as it's playing, so + * advancing to it shouldn't also dequeue it the way the old pop-and-remove semantics did. */ + suspend fun peekFront(): String? = queueDao.firstItemId() /** * Evicts this feed's oldest *auto-queued* episodes (earliest added, not earliest published) diff --git a/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackController.kt b/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackController.kt index c77c5af2..e65bcf02 100644 --- a/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackController.kt +++ b/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackController.kt @@ -222,15 +222,16 @@ class PlaybackController @Inject constructor( /** * Returns false without starting playback if streaming is disallowed and nothing is - * downloaded. Removes [item] from the Next Up queue unconditionally, even when playback then - * fails to start -- issue #171: it's already shown pinned to the top of the Next Up screen via - * the current-playback player bar, so a leftover queue entry for it would just be a duplicate - * further down the list. Mirrors the dequeue-then-play behavior [QueueRepository] callers used - * to have to do themselves (e.g. the old `QueueViewModel.playNow`), but now for every path that - * starts playback. + * downloaded. Moves [item] to the front of the Next Up queue unconditionally, even when + * playback then fails to start (issue #196): the currently-playing episode is meant to be a + * real, visible queue entry -- always the front one -- rather than hidden from Next Up + * entirely, so it shows up there clearly marked as playing instead of only via the mini/expanded + * player. Mirrors the move-to-front-then-play behavior [QueueRepository] callers used to have + * to do themselves (e.g. the old `QueueViewModel.playNow`), but now for every path that starts + * playback. */ suspend fun play(item: FeedItem, feedTitle: String?): Boolean { - queueRepository.remove(item.id) + queueRepository.moveToFront(item.id) return loadMedia(item, feedTitle, autoPlay = true) } @@ -247,6 +248,11 @@ class PlaybackController @Inject constructor( val itemId = settings.lastPlayingItemId ?: return val item = feedRepository.getItem(itemId)?.takeIf { it.feedId == feedId && !it.isRead } ?: return val feed = feedRepository.getFeed(feedId) + // issue #196: keeps the restored episode as the queue's front entry too, the same as any + // other path that makes an episode "current" -- it may already be there (a normal resume), + // but a cold start after the app's own process died mid-playback is also how a + // still-current episode could end up missing from the queue table altogether. + queueRepository.moveToFront(itemId) loadMedia(item, feed?.userTitle ?: feed?.title, autoPlay = false) } @@ -256,7 +262,7 @@ class PlaybackController @Inject constructor( val previousItemId = currentItemId if (previousItemId != null && previousItemId != item.id) { - requeuePreviousEpisode(previousItemId) + dropFromQueueIfFinished(previousItemId) } currentFeedId = item.feedId @@ -304,12 +310,17 @@ class PlaybackController @Inject constructor( /** * Whatever was playing before a switch stays in "Next Up" as long as it wasn't finished - * (issue #106) -- mirrors [restoreLastPlayingItem]'s `!isRead` check for "not completed". - * A no-op if it's already queued (e.g. the user played straight from the queue). + * (issue #106) -- since issue #196, it's already a real queue entry the whole time it's + * playing (see [play]/[QueueRepository.moveToFront]), so there's nothing to *add* here; this + * only needs to drop it if it turns out to have actually finished (mirrors + * [restoreLastPlayingItem]'s `!isRead` check for "not completed") -- a finished episode + * shouldn't linger in Next Up just because it still happened to be the queue's front entry + * when playback moved on. A no-op if it was never queued in the first place, or the user + * already removed it from Next Up themselves while it was still playing. */ - private suspend fun requeuePreviousEpisode(itemId: String) { - val previous = feedRepository.getItem(itemId)?.takeIf { !it.isRead } ?: return - queueRepository.addToFront(previous.id) + private suspend fun dropFromQueueIfFinished(itemId: String) { + val previous = feedRepository.getItem(itemId)?.takeIf { it.isRead } ?: return + queueRepository.remove(previous.id) } fun pause() { @@ -408,9 +419,10 @@ class PlaybackController @Inject constructor( positionTickerScope.launch(Dispatchers.IO) { settingsDataStore.setLastPlayingItem(null, null) // Explicit stop just ends "now playing" -- unlike natural completion, the episode - // isn't finished, so it goes back into Next Up the same way switching to a different - // episode already does (issue #191), instead of disappearing from the app entirely. - itemId?.let { requeuePreviousEpisode(it) } + // isn't finished, so it stays in Next Up the same way switching to a different episode + // already does (issue #191); it's already there as the front entry (issue #196), so + // this is only a safety net for the edge case where it's since been marked read. + itemId?.let { dropFromQueueIfFinished(it) } } } diff --git a/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackService.kt b/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackService.kt index c756500d..f248df62 100644 --- a/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackService.kt +++ b/app/src/main/java/com/bugzapperlabs/mycasts/playback/PlaybackService.kt @@ -196,16 +196,22 @@ class PlaybackService : MediaSessionService() { } /** - * Keeps [preloadManager] warming up whatever's currently at the head of Next Up (issue #87), - * reacting to every mutation that can change it -- add/remove/reorder/auto-queue eviction all - * flow through [QueueRepository.observeQueue]. Runs for the service's whole lifetime, not - * gated on anything currently playing, so the head is already warm by the time an episode - * actually finishes and [playNextQueued] needs it. + * Keeps [preloadManager] warming up whatever's next in line after the episode currently + * playing (issue #87), reacting to every mutation that can change it -- add/remove/reorder/ + * auto-queue eviction all flow through [QueueRepository.observeQueue]. Runs for the service's + * whole lifetime, not gated on anything currently playing, so the head is already warm by the + * time an episode actually finishes and [playNextQueued] needs it. + * + * Skips the queue's own front entry when it matches [Player.getCurrentMediaItem] (issue #196): + * the currently-playing episode is itself a real queue entry now, always at the front, so the + * "head" this preloads has to be the entry *after* it -- preloading the front entry itself + * would just be re-preloading whatever's already playing. */ @OptIn(markerClass = [UnstableApi::class]) private fun startPreloadingQueueHead() { serviceScope.launch { - queueRepository.observeQueue().map { it.firstOrNull() }.distinctUntilChanged { old, new -> old?.item?.id == new?.item?.id } + queueRepository.observeQueue().map { queue -> queue.firstOrNull { it.item.id != player.currentMediaItem?.mediaId } } + .distinctUntilChanged { old, new -> old?.item?.id == new?.item?.id } .collect { head -> preloadManager.reset() preloadedNext = null @@ -388,11 +394,19 @@ class PlaybackService : MediaSessionService() { // (setEnclosurePosition/markRead/auto-delete) below -- this episode never actually // completed, so none of that applies. if (advancingFromError) return + val itemId = player.currentMediaItem?.mediaId advancingFromError = true advanceWakeLock.acquire(ADVANCE_WAKE_LOCK_TIMEOUT_MS) serviceScope.launch { try { - playNextQueued() + // issue #196: the failed episode is still the queue's own front entry (it + // never stopped being "current" until now) -- move it out of the front before + // peeking the next one, or playNextQueued would just find this same broken + // episode again and loop on it forever. Moved to the back rather than removed + // outright: unlike a finished episode, this one never actually played, so it's + // still worth surfacing in Next Up for the user to retry later. + itemId?.let { queueRepository.moveToEnd(it) } + playNextQueued(excludeItemId = itemId) } finally { advancingFromError = false } @@ -411,7 +425,14 @@ class PlaybackService : MediaSessionService() { // done beforehand was pure added silence between episodes, on top of whatever // buffering the next episode's own prepare() needs (worse for a streamed // episode than a downloaded one, since that also has to open a connection). - playNextQueued() + // issue #196: the finished episode is still the queue's own front entry (it + // never stopped being "current" until now) -- excluded here rather than + // removed upfront (which would reintroduce the silence gap issue #82 just + // eliminated), since playNextQueued would otherwise just find this same + // already-finished episode again. The row itself is dropped for real just + // below, alongside the finished-episode bookkeeping it belongs next to. + playNextQueued(excludeItemId = itemId) + queueRepository.remove(itemId) feedRepository.setEnclosurePosition(itemId, null) feedRepository.markRead(itemId, true) // Storage cap / auto-cleanup (issue #71): only ever deletes an episode that @@ -437,9 +458,16 @@ class PlaybackService : MediaSessionService() { * issue #179: this has to keep working even with no UI/MediaController attached (backgrounded * or screen off), and this service is the one guaranteed to still be running when that happens. */ + // issue #196: the currently-playing episode is now a real queue entry itself (the front one), + // kept there for as long as it's playing rather than popped off on advance the way the old + // pop-and-remove semantics did -- see peekFront()'s doc. [excludeItemId] covers the one case + // that still needs to look past the front entry regardless: a just-failed episode moved to the + // back of the queue (PlaybackService.onPlayerError's moveToEnd) has nowhere to actually move to + // if it was the queue's only entry, so it would otherwise still be sitting at the front here, + // and get "advanced" into again -- the exact same broken episode, looping forever. @OptIn(markerClass = [UnstableApi::class]) - private suspend fun playNextQueued() { - val itemId = queueRepository.popNext() + private suspend fun playNextQueued(excludeItemId: String? = null) { + val itemId = queueRepository.peekFront()?.takeIf { it != excludeItemId } if (itemId == null) { player.clearMediaItems() settingsDataStore.setLastPlayingItem(null, null) diff --git a/app/src/main/java/com/bugzapperlabs/mycasts/queue/ReorderableQueueList.kt b/app/src/main/java/com/bugzapperlabs/mycasts/queue/ReorderableQueueList.kt index 2bd3f3af..2c27cfa7 100644 --- a/app/src/main/java/com/bugzapperlabs/mycasts/queue/ReorderableQueueList.kt +++ b/app/src/main/java/com/bugzapperlabs/mycasts/queue/ReorderableQueueList.kt @@ -26,6 +26,7 @@ import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.DragHandle import androidx.compose.material.icons.filled.Downloading import androidx.compose.material.icons.filled.OfflinePin +import androidx.compose.material.icons.filled.PlayArrow import androidx.compose.material.icons.filled.PlaylistRemove import androidx.compose.material.icons.filled.VerticalAlignBottom import androidx.compose.material.icons.filled.VerticalAlignTop @@ -611,12 +612,32 @@ private fun RowScope.QueueRowContent( ) } androidx.compose.foundation.layout.Row(verticalAlignment = Alignment.CenterVertically) { - Text( - text = EpisodeDateFormatter.format(episode.item.publishDate), - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurfaceVariant, - maxLines = 1, - ) + // issue #196: the currently-playing episode is a real (always-front) queue entry now, + // shown in this same list rather than hidden from it -- swaps the date for an explicit + // "Now Playing" label so that's unmistakable at a glance, not just inferable from the + // row's highlight color (issue #96) alone. + if (isCurrentlyPlaying) { + Icon( + Icons.Filled.PlayArrow, + contentDescription = null, + tint = MaterialTheme.colorScheme.primary, + modifier = Modifier.size(16.dp), + ) + Text( + text = stringResource(R.string.now_playing_title), + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.primary, + maxLines = 1, + modifier = Modifier.padding(start = 2.dp), + ) + } else { + Text( + text = EpisodeDateFormatter.format(episode.item.publishDate), + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 1, + ) + } // issue #192: EpisodeListScreen already shows this per-row (see its own doc for why // OfflinePin over DownloadDone), but the queue had no equivalent -- useful now that // issue #188 added a bulk "download all" action here, with no other way to see which diff --git a/app/src/test/java/com/bugzapperlabs/mycasts/data/repository/QueueRepositoryTest.kt b/app/src/test/java/com/bugzapperlabs/mycasts/data/repository/QueueRepositoryTest.kt index 0fdce77e..bb0f58c3 100644 --- a/app/src/test/java/com/bugzapperlabs/mycasts/data/repository/QueueRepositoryTest.kt +++ b/app/src/test/java/com/bugzapperlabs/mycasts/data/repository/QueueRepositoryTest.kt @@ -128,19 +128,57 @@ class QueueRepositoryTest { } @Test - fun popNext_removesAndReturnsFrontOfQueue() = runTest { + fun peekFront_returnsFrontOfQueueWithoutRemovingIt() = runTest { queueRepository.addToEnd("ep-1") queueRepository.addToEnd("ep-2") - val next = queueRepository.popNext() + val next = queueRepository.peekFront() assertEquals("ep-1", next) - assertEquals(listOf("ep-2"), queueRepository.observeQueue().first().map { it.item.id }) + assertEquals(listOf("ep-1", "ep-2"), queueRepository.observeQueue().first().map { it.item.id }) } @Test - fun popNext_emptyQueue_returnsNull() = runTest { - assertNull(queueRepository.popNext()) + fun peekFront_emptyQueue_returnsNull() = runTest { + assertNull(queueRepository.peekFront()) + } + + @Test + fun moveToFront_alreadyQueued_movesExistingEntryToFront() = runTest { + queueRepository.addToEnd("ep-1") + queueRepository.addToEnd("ep-2") + + queueRepository.moveToFront("ep-2") + + assertEquals(listOf("ep-2", "ep-1"), queueRepository.observeQueue().first().map { it.item.id }) + } + + @Test + fun moveToFront_notQueued_insertsAtFront() = runTest { + queueRepository.addToEnd("ep-1") + + queueRepository.moveToFront("ep-2") + + assertEquals(listOf("ep-2", "ep-1"), queueRepository.observeQueue().first().map { it.item.id }) + } + + @Test + fun moveToEnd_queuedEntry_movesToBackOfQueue() = runTest { + queueRepository.addToEnd("ep-1") + queueRepository.addToEnd("ep-2") + + queueRepository.moveToEnd("ep-1") + + assertEquals(listOf("ep-2", "ep-1"), queueRepository.observeQueue().first().map { it.item.id }) + } + + @Test + fun moveToEnd_notQueued_doesNothing() = runTest { + queueRepository.addToEnd("ep-1") + + queueRepository.moveToEnd("ep-2") + + assertEquals(listOf("ep-1"), queueRepository.observeQueue().first().map { it.item.id }) } @Test diff --git a/app/src/test/java/com/bugzapperlabs/mycasts/playback/PlaybackControllerTest.kt b/app/src/test/java/com/bugzapperlabs/mycasts/playback/PlaybackControllerTest.kt index 5bb00824..82c91d9c 100644 --- a/app/src/test/java/com/bugzapperlabs/mycasts/playback/PlaybackControllerTest.kt +++ b/app/src/test/java/com/bugzapperlabs/mycasts/playback/PlaybackControllerTest.kt @@ -11,11 +11,13 @@ import com.bugzapperlabs.mycasts.data.local.FeedItem import com.bugzapperlabs.mycasts.data.repository.FeedRepository import com.bugzapperlabs.mycasts.data.repository.QueueRepository import com.bugzapperlabs.mycasts.data.settings.SettingsDataStore +import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.runTest import okhttp3.OkHttpClient import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test @@ -100,12 +102,12 @@ class PlaybackControllerTest { } /** - * issue #171: the currently playing episode is already shown pinned to the top of the Next Up - * screen via the current-playback player bar, so a leftover Next Up queue entry for it would - * just be a duplicate -- playing an episode that's queued should dequeue it. + * issue #196: the currently-playing episode is a real Next Up queue entry itself -- always + * the front one, clearly marked as playing -- rather than hidden from the queue entirely, so + * playing an already-queued episode should move it to the front, not dequeue it. */ @Test - fun play_episodeAlreadyQueued_removesItFromQueue() = runTest { + fun play_episodeAlreadyQueued_movesItToFrontOfQueue() = runTest { val feedId = feedRepository.subscribe(Feed(title = "Feed")) val item = FeedItem( id = "episode-1", @@ -115,11 +117,43 @@ class PlaybackControllerTest { enclosureUrl = "https://example.com/ep1.mp3", enclosureType = "audio/mpeg", ) - feedRepository.insertItems(listOf(item)) + val otherItem = FeedItem( + id = "episode-2", + feedId = feedId, + title = "Episode Two", + itemGuid = "g-episode-2", + enclosureUrl = "https://example.com/ep2.mp3", + enclosureType = "audio/mpeg", + ) + feedRepository.insertItems(listOf(item, otherItem)) + queueRepository.addToEnd(otherItem.id) queueRepository.addToEnd(item.id) playbackController.play(item, "Feed") - assertFalse(queueRepository.isQueued(item.id)) + assertTrue(queueRepository.isQueued(item.id)) + assertEquals(listOf(item.id, otherItem.id), queueRepository.observeQueue().first().map { it.item.id }) + } + + /** + * issue #196: playing an episode that wasn't queued at all should insert it at the front, the + * same as moving an already-queued one there. + */ + @Test + fun play_episodeNotQueued_insertsItAtFrontOfQueue() = runTest { + val feedId = feedRepository.subscribe(Feed(title = "Feed")) + val item = FeedItem( + id = "episode-1", + feedId = feedId, + title = "Episode One", + itemGuid = "g-episode-1", + enclosureUrl = "https://example.com/ep1.mp3", + enclosureType = "audio/mpeg", + ) + feedRepository.insertItems(listOf(item)) + + playbackController.play(item, "Feed") + + assertTrue(queueRepository.isQueued(item.id)) } } diff --git a/app/src/test/java/com/bugzapperlabs/mycasts/queue/QueueViewModelTest.kt b/app/src/test/java/com/bugzapperlabs/mycasts/queue/QueueViewModelTest.kt index adf4ee91..e138f5f4 100644 --- a/app/src/test/java/com/bugzapperlabs/mycasts/queue/QueueViewModelTest.kt +++ b/app/src/test/java/com/bugzapperlabs/mycasts/queue/QueueViewModelTest.kt @@ -203,13 +203,16 @@ class QueueViewModelTest { } @Test - fun playNow_removesEpisodeFromQueue() = runTest(testDispatcher) { - val episode = viewModel.queue.first { it.size == 2 }.first() + fun playNow_movesEpisodeToFrontOfQueue() = runTest(testDispatcher) { + // issue #196: the currently-playing episode stays queued (as the front entry, clearly + // marked as playing) rather than being dequeued -- playing the queue's second episode + // should swap the two, not shrink the queue. + val episode = viewModel.queue.first { it.size == 2 }.last() viewModel.playNow(episode) - val state = viewModel.queue.first { it.size == 1 } - assertEquals(listOf("ep-2"), state.map { it.item.id }) + val state = viewModel.queue.first { it.first().item.id == "ep-2" } + assertEquals(listOf("ep-2", "ep-1"), state.map { it.item.id }) } @Test