diff options
| -rw-r--r-- | .coverage-baseline | 2 | ||||
| -rw-r--r-- | CLAUDE.md | 24 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/Daos.kt | 10 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/Repositories.kt | 3 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/queue/QueueOrdering.kt | 9 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt | 5 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/ReachabilityTest.kt | 29 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/data/FakeDaos.kt | 4 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt | 23 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt | 25 |
10 files changed, 118 insertions, 16 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index 09c9fdf..e9816e1 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -71.31 +71.37 @@ -54,11 +54,23 @@ with `adb shell monkey`. Use the shared wrappers, which report from exit codes, and a logcat capture started before launch: ``` -/workspace/devtools/bin/gradle-test /workspace/listen -- --rerun-tasks +/workspace/devtools/bin/gradle-test /workspace/listen -- --rerun-tasks +/workspace/devtools/bin/gradle-report /workspace/listen detekt /workspace/devtools/bin/android-launch-check org.terst.listen \ --apk app/build/outputs/apk/debug/app-debug.apk --settle 15 +/workspace/devtools/bin/android-ui org.terst.listen dump +/workspace/devtools/bin/publish-apk \ + app/build/outputs/apk/debug/app-debug.apk listen-debug.apk ``` +**Never read `app/build/reports/detekt/detekt.xml` directly.** An invalid config aborts detekt +before it analyses anything and leaves the previous run's report in place; it was read as +current findings three times. `gradle-report` refuses a report older than the run. + +**Never `adb shell input tap X Y`.** A blind tap landed in FeeDreader's settings and the +screenshot after it was reported as Listen. `android-ui` checks the foreground package first +and taps by label. + See `/workspace/devtools/README.md`. These exist because grepped console text has produced wrong claims three separate times across these repos. @@ -130,3 +142,13 @@ intro/outro skipping. Schema fields for manual skip offsets exist but nothing re - **Feeds are big.** A real subscription in testing produced 2753 episodes in one refresh. Any per-episode work — a query in a loop, an O(n²) match — needs to be considered at that scale. + +- **The playing episode is always the head of the queue.** `advance()` drops the head, so if + something played from a show list isn't there, finishing it deletes a queued episode unplayed. + `play()` calls `playNow()` first to hold that invariant. Don't add a playback entry point that + skips it. + +- **Two reachability tests guard dead code**, one for helper objects and one for DAO methods. + Both exist because code was written, tested, documented and never called: `findFeedLink` (the + share-sheet bug) and `QueueRepository.advance` (the queue silently stopped). Adding to their + allowlists needs a reason, not just a name. diff --git a/app/src/main/java/org/terst/listen/data/Daos.kt b/app/src/main/java/org/terst/listen/data/Daos.kt index 2376644..deb350a 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -84,9 +84,6 @@ interface EpisodeDao { before: Long, downloaded: DownloadState = DownloadState.DOWNLOADED, ): List<EpisodeEntity> - - @Query("SELECT * FROM episodes WHERE playedAtMs IS NULL ORDER BY publishedAtMs DESC LIMIT :limit") - fun observeUnplayed(limit: Int): Flow<List<EpisodeEntity>> } @Dao @@ -98,13 +95,6 @@ interface QueueDao { ) fun observeQueue(): Flow<List<EpisodeEntity>> - @Query( - """SELECT e.* FROM episodes e - INNER JOIN queue q ON q.episodeId = e.id - ORDER BY q.position""" - ) - suspend fun getQueue(): List<EpisodeEntity> - @Query("SELECT * FROM queue ORDER BY position") suspend fun getEntries(): List<QueueEntity> diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index fb869d8..60d1590 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -76,6 +76,9 @@ class QueueRepository(private val dao: QueueDao) { suspend fun playNext(episodeId: String) = rewrite { QueueOrdering.playNext(it, episodeId) } + /** Promotes an episode to the head, where "what is playing" lives. */ + suspend fun playNow(episodeId: String) = rewrite { QueueOrdering.playNow(it, episodeId) } + suspend fun remove(episodeId: String) = rewrite { QueueOrdering.remove(it, episodeId) } suspend fun move(from: Int, to: Int) = rewrite { QueueOrdering.move(it, from, to) } diff --git a/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt b/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt index 4cc6543..d657477 100644 --- a/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt +++ b/app/src/main/java/org/terst/listen/queue/QueueOrdering.kt @@ -26,6 +26,15 @@ object QueueOrdering { return listOf(rest.first(), episodeId) + rest.drop(1) } + /** + * Makes an episode the head, which is what is playing. + * + * The invariant this exists to hold: [advance] drops the head, so if the playing episode + * isn't there, finishing it deletes a different episode from the queue unplayed. + */ + fun playNow(queue: List<String>, episodeId: String): List<String> = + listOf(episodeId) + queue.filterNot { it == episodeId } + fun remove(queue: List<String>, episodeId: String): List<String> = queue.filterNot { it == episodeId } diff --git a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt index fa501a4..aef40ac 100644 --- a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt @@ -58,6 +58,11 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) { /** Starts an episode, resuming where the listener left off and at the show's speed. */ fun play(episode: EpisodeEntity) { viewModelScope.launch { + // Anything played becomes the head of the queue. Without that, finishing an + // episode started from a show list would advance past -- and silently discard -- + // whatever was actually queued. + graph.queue.playNow(episode.id) + val show = graph.subscriptions.getById(episode.subscriptionId) val speed = show?.playbackSpeed ?: DEFAULT_SPEED connection.play( diff --git a/app/src/test/java/org/terst/listen/ReachabilityTest.kt b/app/src/test/java/org/terst/listen/ReachabilityTest.kt index 4eaed9e..d129756 100644 --- a/app/src/test/java/org/terst/listen/ReachabilityTest.kt +++ b/app/src/test/java/org/terst/listen/ReachabilityTest.kt @@ -70,6 +70,35 @@ class ReachabilityTest { .toList() } + /** + * Room DAOs are the other place dead code hides -- FeeDreader accumulated three superseded + * methods that an audit, not a test, eventually found. Matching is on `.name(` across all + * of main with declarations stripped, because a DAO call's receiver varies by site and a + * @Transaction default method legitimately calls its siblings in the same file. + */ + @Test + fun `every dao method has a caller`() { + val sources = mainDir.walkTopDown().filter { it.extension == "kt" }.toList() + val declaration = Regex("""(?:suspend )?fun (\w+)\(""") + val corpus = sources.joinToString("\n") { declaration.replace(it.readText(), "DECL(") } + + val daoFile = File(mainDir, "data/Daos.kt") + assertTrue("expected ${daoFile.path}", daoFile.isFile) + + val dead = Regex("""^ (?:suspend )?fun (\w+)""", RegexOption.MULTILINE) + .findAll(daoFile.readText()) + .map { it.groupValues[1] } + .distinct() + // Both a qualified call and a bare one: a @Transaction default method calls its + // siblings unqualified. Heuristic on purpose -- it errs toward saying "called", + // since a false alarm here costs more attention than a missed one. + .filterNot { corpus.contains(".$it(") || Regex("""\b$it\(""").containsMatchIn(corpus) } + .toList() + + assertTrue("No caller for DAO method(s): ${dead.sorted()}. Wire them up or delete them.", + dead.isEmpty()) + } + @Test fun `every helper object function has a caller`() { val sources = mainDir.walkTopDown().filter { it.extension == "kt" }.toList() diff --git a/app/src/test/java/org/terst/listen/data/FakeDaos.kt b/app/src/test/java/org/terst/listen/data/FakeDaos.kt index 821e1b2..f313687 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -103,9 +103,6 @@ class FakeEpisodeDao : EpisodeDao { it.downloadState == downloaded && it.playedAtMs != null && it.playedAtMs!! < before } - override fun observeUnplayed(limit: Int): Flow<List<EpisodeEntity>> = - flowOf(rows.filter { it.playedAtMs == null }.take(limit)) - private fun replace(id: String, transform: (EpisodeEntity) -> EpisodeEntity) { val index = rows.indexOfFirst { it.id == id } if (index >= 0) { @@ -125,7 +122,6 @@ class FakeQueueDao : QueueDao { } override fun observeQueue(): Flow<List<EpisodeEntity>> = flowOf(ordered()) - override suspend fun getQueue() = ordered() override suspend fun getEntries() = entries.sortedBy { it.position } override suspend fun upsertAll(entries: List<QueueEntity>) { diff --git a/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt b/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt index a3ed294..dde5bdb 100644 --- a/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt +++ b/app/src/test/java/org/terst/listen/data/QueueRepositoryTest.kt @@ -81,6 +81,29 @@ class QueueRepositoryTest { assertContiguous() } + /** + * The bug this prevents: play an episode that isn't in the queue, let it finish, and the + * old advance dropped whatever was at the head -- a different episode, removed unplayed. + */ + @Test + fun `playing an unqueued episode does not consume a queued one`() = runTest { + listOf("a", "b").forEach { repo.append(it) } + repo.playNow("d") + assertEquals(listOf("d", "a", "b"), ids()) + + assertEquals("a", repo.advance()) + assertEquals("nothing may be dropped except the episode that finished", listOf("a", "b"), ids()) + assertContiguous() + } + + @Test + fun `play now promotes to the head`() = runTest { + listOf("a", "b", "c").forEach { repo.append(it) } + repo.playNow("c") + assertEquals(listOf("c", "a", "b"), ids()) + assertContiguous() + } + @Test fun `advance drops the finished episode and reports the next`() = runTest { listOf("a", "b", "c").forEach { repo.append(it) } diff --git a/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt b/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt index 556351f..93ee0a0 100644 --- a/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt +++ b/app/src/test/java/org/terst/listen/queue/QueueOrderingTest.kt @@ -69,6 +69,31 @@ class QueueOrderingTest { assertEquals(queue, QueueOrdering.move(queue, from = 99, to = 0)) } + /** + * Playing an episode makes it the head, which is what keeps advance honest: advance drops + * the head, so unless the playing episode is there, finishing it silently deletes somebody + * else's place in the queue. + */ + @Test + fun `play now puts the episode at the head`() { + assertEquals(listOf("d", "a", "b", "c"), QueueOrdering.playNow(queue, "d")) + } + + @Test + fun `play now on a queued episode moves it to the head without duplicating`() { + assertEquals(listOf("c", "a", "b"), QueueOrdering.playNow(queue, "c")) + } + + @Test + fun `play now on the current head changes nothing`() { + assertEquals(listOf("a", "b", "c"), QueueOrdering.playNow(queue, "a")) + } + + @Test + fun `play now on an empty queue starts it`() { + assertEquals(listOf("d"), QueueOrdering.playNow(emptyList(), "d")) + } + @Test fun `advancing drops the finished episode and returns the next`() { val (next, remaining) = QueueOrdering.advance(queue) |