From a537f51fa29a7b933f3b8f57812e8d3f422b379b Mon Sep 17 00:00:00 2001 From: Mark Pitman Date: Wed, 19 Aug 2026 23:35:59 -0700 Subject: [PATCH] Include the currently-playing episode in the Next Up queue (issue #196) The currently-playing episode was fully removed from the queue table on play (issue #171) and only ever shown via the mini/expanded player -- Next Up itself had no idea it existed. It's now a real queue entry like any other, always kept at the front, and shown in the list clearly marked "Now Playing" (a small play icon + label swapped in for the date) rather than hidden. - QueueRepository: moveToFront() replaces the old remove-on-play, peekFront() replaces the old pop-and-remove popNext() (advancing to an episode shouldn't also dequeue it now that it's meant to stay queued while it plays), and moveToEnd() gets a failed episode out of the front slot without discarding it outright. - PlaybackController.play()/restoreLastPlayingItem() move the episode to the queue's front instead of removing it. The old requeuePreviousEpisode() (re-add if unfinished) is replaced by dropFromQueueIfFinished() (remove if it turns out to have finished) -- the previous episode never actually left the queue this time, so there's nothing to add back, only a stale finished row to potentially clean up. - PlaybackService: onPlaybackStateChanged(STATE_ENDED) drops the finished episode's row itself now (previously unnecessary, since play() used to do that dequeue upfront) -- deferred until after playNextQueued() so the issue #82 silence-gap fix isn't undone. onPlayerError moves the failed episode to the back instead, since it never actually finished. Both pass playNextQueued() an itemId to treat as "not really next" so it doesn't just find the very episode it's meant to be skipping past -- needed because a lone queued episode moved to the back is still the only (and therefore front) entry. startPreloadingQueueHead() now skips the currently-playing entry when picking what to preload next, for the same reason. No UI filtering was needed for the queue list itself -- ReorderableQueueList already fully supported an isCurrentlyPlaying row (highlight, disabled long-press-to-play, etc.) from when that state was only ever transient; it just needed the explicit "Now Playing" label added. --- .../data/repository/QueueRepository.kt | 38 ++++++++++++--- .../mycasts/playback/PlaybackController.kt | 44 ++++++++++------- .../mycasts/playback/PlaybackService.kt | 48 +++++++++++++++---- .../mycasts/queue/ReorderableQueueList.kt | 33 ++++++++++--- .../data/repository/QueueRepositoryTest.kt | 48 +++++++++++++++++-- .../playback/PlaybackControllerTest.kt | 46 +++++++++++++++--- .../mycasts/queue/QueueViewModelTest.kt | 11 +++-- 7 files changed, 215 insertions(+), 53 deletions(-) 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