From aa9c0a386f19bb58c50c81fd79d9509d2883cffe Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sat, 26 Sep 2026 17:37:33 +0000 Subject: Skip marked breaks by default, and drop the instrumented-coverage half Two things. Ad-chapter skipping is on by default, at Peter's request after living with it. It shipped off out of caution, being the one part of the skip feature that removes audio nobody has heard -- but it only ever acts on sections a publisher explicitly marked as not content, never on a guess from a title, and doing that without being asked each time is the point. Migration 5 -> 6 moves existing shows rather than leaving the new default to apply only to new ones. The column's SQL default stays 0 while the Kotlin default becomes true. Room writes every column on insert, so the SQL default is only ever consulted by an ALTER TABLE that has already happened; changing it would mean recreating the table for a value nothing reads. Said so at the field. Two unit tests were leaning on the old default to mean "not opted in". They now say `skipAdChapters = false` explicitly, which is what they meant. Second: enableAndroidTestCoverage is gone again, a day after adding it. It was the "both" half of the coverage change, and measuring it made the trade obvious -- it instruments every class in the debug build, which cost 25MB on the APK (118MB -> 143MB) plus runtime probes, on every sideload, for a report that would be run rarely. I had already published one of those builds. The framework-adapter exclusions carry this on their own; they are what moved the number from 65% to 90%, and what makes a future drop mean something. No permission changes. 428 unit tests green, 6 migration tests on a device, detekt and lint clean, coverage 89.97 -> 90.12. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- .coverage-baseline | 2 +- CLAUDE.md | 14 +- app/build.gradle.kts | 36 +- .../org.terst.listen.data.AppDatabase/6.json | 499 +++++++++++++++++++++ .../java/org/terst/listen/data/MigrationTest.kt | 25 ++ .../main/java/org/terst/listen/data/AppDatabase.kt | 24 +- .../main/java/org/terst/listen/data/Entities.kt | 12 +- .../main/java/org/terst/listen/ui/SkipSettings.kt | 6 +- .../terst/listen/playback/PlaybackAdvancerTest.kt | 5 +- .../terst/listen/playback/PlayheadPolicyTest.kt | 17 +- 10 files changed, 584 insertions(+), 56 deletions(-) create mode 100644 app/schemas/org.terst.listen.data.AppDatabase/6.json diff --git a/.coverage-baseline b/.coverage-baseline index c289320..0670581 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -89.97 +90.12 diff --git a/CLAUDE.md b/CLAUDE.md index 84a7fd5..67b228f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -136,8 +136,9 @@ out of a possibly-partial MP3, and the JSON path covers the shows that mark chap - **Ad-chapter skipping trusts `toc: false` and nothing else.** No title matching: a show discussing its own sponsorship deal has a chapter called "Sponsors" that is the actual - programme. It is also off by default per show, being the one part of the feature that can - remove audio nobody has heard. + programme. It shipped off by default out of caution and Peter asked for it on after living + with it (migration 5 → 6 moves existing shows); it is still per-show and still only ever acts + on sections a publisher explicitly marked as not content. - **Auto-downloads are not retried.** A rule's download that fails is left FAILED rather than requeued: retrying forever against a 404 would burn the battery, and the failed row is visible @@ -204,10 +205,11 @@ out of a possibly-partial MP3, and the JSON path covers the shows that mark chap pulled RetentionSweeper out of its Worker, both of which found real bugs. That value only survives if the list stays honest. - `jacocoFullReport` adds instrumented coverage on top, for when you want the fuller picture - with a device attached. The commit gate deliberately does not read it: device data only exists - after a device run, so folding it in would make the gate report different numbers for - identical code. + Feeding instrumented coverage into the report as well was tried and abandoned: + `enableAndroidTestCoverage` instruments every class in the debug build, which measured at + +25MB on the APK (118MB → 143MB) plus runtime probes, on every sideload, for a report that + would be run rarely. It would also have made the gate report different numbers for identical + code depending on whether a device run happened recently. - **Detekt config has been relaxed five times**, all documented inline in `config/detekt/detekt.yml`. Twice the better answer was restructuring instead: `Screens.kt` was split per screen and diff --git a/app/build.gradle.kts b/app/build.gradle.kts index eba61d1..bcbe4f9 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -99,9 +99,11 @@ android { buildTypes { debug { enableUnitTestCoverage = true - // Produces device coverage data for jacocoFullReport. The commit gate deliberately - // does not read it -- see that task. - enableAndroidTestCoverage = true + // No enableAndroidTestCoverage. Turning it on to feed instrumented data into a + // coverage report instruments every class in the debug build: measured, it added + // 25MB to the APK (118MB -> 143MB) and runtime probes to code Peter actually uses. + // That is a real cost on every sideload for a report that would be run rarely, so + // the framework-adapter exclusions carry this on their own. } release { isMinifyEnabled = false @@ -297,31 +299,3 @@ tasks.register("jacocoTestReport") { sourceDirectories.setFrom(files("src/main/java")) executionData.setFrom(fileTree(layout.buildDirectory) { include("**/*.exec") }) } - -/** - * Unit *and* instrumented coverage, for when you want the fuller picture. - * - * Deliberately not what the commit gate reads. Instrumented data only exists after a device run, - * so folding it into the gate would make the number depend on whether one happened recently and - * on stale .ec files lying around -- a gate that reports different figures for identical code is - * worse than a narrow one. Run this with a device attached: - * - * ./gradlew connectedDebugAndroidTest jacocoFullReport - */ -tasks.register("jacocoFullReport") { - dependsOn("testDebugUnitTest") - reports { - xml.required.set(true) - html.required.set(true) - } - val kotlinDebugTree = fileTree("${layout.buildDirectory.get()}/tmp/kotlin-classes/debug") { - exclude(coverageExclusions) - } - val javaDebugTree = fileTree("${layout.buildDirectory.get()}/intermediates/javac/debug") { - exclude(coverageExclusions) - } - classDirectories.setFrom(files(kotlinDebugTree, javaDebugTree)) - sourceDirectories.setFrom(files("src/main/java")) - // .ec is what a device run writes, alongside the JVM run's .exec. - executionData.setFrom(fileTree(layout.buildDirectory) { include("**/*.exec", "**/*.ec") }) -} diff --git a/app/schemas/org.terst.listen.data.AppDatabase/6.json b/app/schemas/org.terst.listen.data.AppDatabase/6.json new file mode 100644 index 0000000..1820193 --- /dev/null +++ b/app/schemas/org.terst.listen.data.AppDatabase/6.json @@ -0,0 +1,499 @@ +{ + "formatVersion": 1, + "database": { + "version": 6, + "identityHash": "4a70b3102d3822eabaf209d5a446fc40", + "entities": [ + { + "tableName": "subscriptions", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `feedUrl` TEXT NOT NULL, `kind` TEXT NOT NULL, `title` TEXT NOT NULL, `description` TEXT, `author` TEXT, `imageUrl` TEXT, `link` TEXT, `addedAtMs` INTEGER NOT NULL, `etag` TEXT, `lastModified` TEXT, `lastFetchedAtMs` INTEGER, `lastError` TEXT, `playbackSpeed` REAL, `skipIntroMs` INTEGER NOT NULL, `skipOutroMs` INTEGER NOT NULL, `dripEnabled` INTEGER NOT NULL DEFAULT 0, `dripIntervalMs` INTEGER NOT NULL DEFAULT 86400000, `dripCount` INTEGER NOT NULL DEFAULT 1, `dripOldestFirst` INTEGER NOT NULL DEFAULT 1, `dripLastReleaseAtMs` INTEGER, `skipAdChapters` INTEGER NOT NULL DEFAULT 0, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "feedUrl", + "columnName": "feedUrl", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "kind", + "columnName": "kind", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "title", + "columnName": "title", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "description", + "columnName": "description", + "affinity": "TEXT" + }, + { + "fieldPath": "author", + "columnName": "author", + "affinity": "TEXT" + }, + { + "fieldPath": "imageUrl", + "columnName": "imageUrl", + "affinity": "TEXT" + }, + { + "fieldPath": "link", + "columnName": "link", + "affinity": "TEXT" + }, + { + "fieldPath": "addedAtMs", + "columnName": "addedAtMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "etag", + "columnName": "etag", + "affinity": "TEXT" + }, + { + "fieldPath": "lastModified", + "columnName": "lastModified", + "affinity": "TEXT" + }, + { + "fieldPath": "lastFetchedAtMs", + "columnName": "lastFetchedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "lastError", + "columnName": "lastError", + "affinity": "TEXT" + }, + { + "fieldPath": "playbackSpeed", + "columnName": "playbackSpeed", + "affinity": "REAL" + }, + { + "fieldPath": "skipIntroMs", + "columnName": "skipIntroMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "skipOutroMs", + "columnName": "skipOutroMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "dripEnabled", + "columnName": "dripEnabled", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "dripIntervalMs", + "columnName": "dripIntervalMs", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "86400000" + }, + { + "fieldPath": "dripCount", + "columnName": "dripCount", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + }, + { + "fieldPath": "dripOldestFirst", + "columnName": "dripOldestFirst", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + }, + { + "fieldPath": "dripLastReleaseAtMs", + "columnName": "dripLastReleaseAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "skipAdChapters", + "columnName": "skipAdChapters", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_subscriptions_feedUrl", + "unique": true, + "columnNames": [ + "feedUrl" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_subscriptions_feedUrl` ON `${TABLE_NAME}` (`feedUrl`)" + } + ] + }, + { + "tableName": "episodes", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `subscriptionId` TEXT NOT NULL, `guid` TEXT NOT NULL, `title` TEXT NOT NULL, `description` TEXT, `mediaUrl` TEXT NOT NULL, `mediaType` TEXT NOT NULL, `mediaLengthBytes` INTEGER, `durationMs` INTEGER, `publishedAtMs` INTEGER, `seasonNumber` INTEGER, `episodeNumber` INTEGER, `imageUrl` TEXT, `chaptersUrl` TEXT, `discoveredAtMs` INTEGER NOT NULL, `positionMs` INTEGER NOT NULL, `playedAtMs` INTEGER, `downloadState` TEXT NOT NULL, `localPath` TEXT, `downloadedAtMs` INTEGER, `dripReleasedAtMs` INTEGER, `appliedRuleId` TEXT, `appliedRuleReason` TEXT, `speedOverride` REAL, `chaptersJson` TEXT, PRIMARY KEY(`id`), FOREIGN KEY(`subscriptionId`) REFERENCES `subscriptions`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "subscriptionId", + "columnName": "subscriptionId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "guid", + "columnName": "guid", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "title", + "columnName": "title", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "description", + "columnName": "description", + "affinity": "TEXT" + }, + { + "fieldPath": "mediaUrl", + "columnName": "mediaUrl", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mediaType", + "columnName": "mediaType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mediaLengthBytes", + "columnName": "mediaLengthBytes", + "affinity": "INTEGER" + }, + { + "fieldPath": "durationMs", + "columnName": "durationMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "publishedAtMs", + "columnName": "publishedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "seasonNumber", + "columnName": "seasonNumber", + "affinity": "INTEGER" + }, + { + "fieldPath": "episodeNumber", + "columnName": "episodeNumber", + "affinity": "INTEGER" + }, + { + "fieldPath": "imageUrl", + "columnName": "imageUrl", + "affinity": "TEXT" + }, + { + "fieldPath": "chaptersUrl", + "columnName": "chaptersUrl", + "affinity": "TEXT" + }, + { + "fieldPath": "discoveredAtMs", + "columnName": "discoveredAtMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "positionMs", + "columnName": "positionMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "playedAtMs", + "columnName": "playedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "downloadState", + "columnName": "downloadState", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "localPath", + "columnName": "localPath", + "affinity": "TEXT" + }, + { + "fieldPath": "downloadedAtMs", + "columnName": "downloadedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "dripReleasedAtMs", + "columnName": "dripReleasedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "appliedRuleId", + "columnName": "appliedRuleId", + "affinity": "TEXT" + }, + { + "fieldPath": "appliedRuleReason", + "columnName": "appliedRuleReason", + "affinity": "TEXT" + }, + { + "fieldPath": "speedOverride", + "columnName": "speedOverride", + "affinity": "REAL" + }, + { + "fieldPath": "chaptersJson", + "columnName": "chaptersJson", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_episodes_subscriptionId", + "unique": false, + "columnNames": [ + "subscriptionId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_episodes_subscriptionId` ON `${TABLE_NAME}` (`subscriptionId`)" + }, + { + "name": "index_episodes_subscriptionId_guid", + "unique": true, + "columnNames": [ + "subscriptionId", + "guid" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_episodes_subscriptionId_guid` ON `${TABLE_NAME}` (`subscriptionId`, `guid`)" + }, + { + "name": "index_episodes_publishedAtMs", + "unique": false, + "columnNames": [ + "publishedAtMs" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_episodes_publishedAtMs` ON `${TABLE_NAME}` (`publishedAtMs`)" + } + ], + "foreignKeys": [ + { + "table": "subscriptions", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "subscriptionId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "queue", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`episodeId` TEXT NOT NULL, `position` INTEGER NOT NULL, PRIMARY KEY(`episodeId`), FOREIGN KEY(`episodeId`) REFERENCES `episodes`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "episodeId", + "columnName": "episodeId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "position", + "columnName": "position", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "episodeId" + ] + }, + "indices": [ + { + "name": "index_queue_position", + "unique": false, + "columnNames": [ + "position" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_queue_position` ON `${TABLE_NAME}` (`position`)" + } + ], + "foreignKeys": [ + { + "table": "episodes", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "episodeId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "rules", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `name` TEXT NOT NULL, `position` INTEGER NOT NULL, `enabled` INTEGER NOT NULL, `subscriptionId` TEXT, `titleContains` TEXT, `titleExcludes` TEXT, `minDurationMs` INTEGER, `maxDurationMs` INTEGER, `maxAgeMs` INTEGER, `seasonNumber` INTEGER, `action` TEXT NOT NULL, `autoDownload` INTEGER NOT NULL, `speed` REAL, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "name", + "columnName": "name", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "position", + "columnName": "position", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "enabled", + "columnName": "enabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "subscriptionId", + "columnName": "subscriptionId", + "affinity": "TEXT" + }, + { + "fieldPath": "titleContains", + "columnName": "titleContains", + "affinity": "TEXT" + }, + { + "fieldPath": "titleExcludes", + "columnName": "titleExcludes", + "affinity": "TEXT" + }, + { + "fieldPath": "minDurationMs", + "columnName": "minDurationMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "maxDurationMs", + "columnName": "maxDurationMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "maxAgeMs", + "columnName": "maxAgeMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "seasonNumber", + "columnName": "seasonNumber", + "affinity": "INTEGER" + }, + { + "fieldPath": "action", + "columnName": "action", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "autoDownload", + "columnName": "autoDownload", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "speed", + "columnName": "speed", + "affinity": "REAL" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_rules_position", + "unique": false, + "columnNames": [ + "position" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_rules_position` ON `${TABLE_NAME}` (`position`)" + } + ] + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '4a70b3102d3822eabaf209d5a446fc40')" + ] + } +} \ No newline at end of file diff --git a/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt b/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt index 181e853..cf691e0 100644 --- a/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt +++ b/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt @@ -197,4 +197,29 @@ class MigrationTest { assertEquals(2, cursor.getInt(0)) } } + + /** + * Ad-chapter skipping was shipped off and Peter asked for it on after living with it, so + * existing shows have to move rather than only new ones picking up the new default. + */ + @Test + fun migrates5To6TurningAdChapterSkippingOn() { + helper.createDatabase(name, 5).use { db -> + db.execSQL( + """INSERT INTO subscriptions + (id, feedUrl, kind, title, addedAtMs, skipIntroMs, skipOutroMs, dripEnabled, + dripIntervalMs, dripCount, dripOldestFirst, skipAdChapters) + VALUES ('s1', 'https://example.com/feed.xml', 'PODCAST', 'A Show', 1000, 0, 0, + 0, 86400000, 1, 1, 0)""" + ) + } + + val db = helper.runMigrationsAndValidate(name, 6, true, AppDatabase.MIGRATION_5_6) + + db.query("SELECT skipAdChapters, title FROM subscriptions WHERE id = 's1'").use { cursor -> + cursor.moveToFirst() + assertEquals(1, cursor.getInt(0)) + assertEquals("the show itself must survive the flip", "A Show", cursor.getString(1)) + } + } } diff --git a/app/src/main/java/org/terst/listen/data/AppDatabase.kt b/app/src/main/java/org/terst/listen/data/AppDatabase.kt index fe73ce0..4ff9f50 100644 --- a/app/src/main/java/org/terst/listen/data/AppDatabase.kt +++ b/app/src/main/java/org/terst/listen/data/AppDatabase.kt @@ -25,7 +25,7 @@ class Converters { QueueEntity::class, RuleEntity::class, ], - version = 5, + version = 6, exportSchema = true, ) @TypeConverters(Converters::class) @@ -141,6 +141,24 @@ abstract class AppDatabase : RoomDatabase() { override fun onCreate(db: SupportSQLiteDatabase) = seedDefaultFilters(db) } + /** + * Turns ad-chapter skipping on, which Peter asked for after living with it. + * + * It shipped off-by-default out of caution, since it is the one part of the skip + * feature that removes audio nobody has heard. In practice it only ever acts on + * chapters a publisher explicitly marked as not-content, and doing that automatically + * is the point. + * + * Only the stored rows move. The column's SQL default stays 0 because nothing inserts + * a subscription without it -- Room always writes every column -- so changing it would + * mean recreating the table for a value that is never read. + */ + val MIGRATION_5_6 = object : Migration(5, 6) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("UPDATE subscriptions SET skipAdChapters = 1") + } + } + fun get(context: Context): AppDatabase = instance ?: synchronized(this) { instance ?: build(context.applicationContext).also { instance = it } } @@ -149,7 +167,9 @@ abstract class AppDatabase : RoomDatabase() { Room.databaseBuilder(context, AppDatabase::class.java, "listen.db") // Listed one by one rather than spread from an array: every migration here is a // deliberate addition, and the list is the record of them. - .addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5) + .addMigrations( + MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5, MIGRATION_5_6, + ) .addCallback(SEED_DEFAULT_FILTERS) // No fallbackToDestructiveMigration. A listener's playback positions and play // history are not reproducible from anywhere, so a missing migration must fail diff --git a/app/src/main/java/org/terst/listen/data/Entities.kt b/app/src/main/java/org/terst/listen/data/Entities.kt index ed80c4b..547a4ad 100644 --- a/app/src/main/java/org/terst/listen/data/Entities.kt +++ b/app/src/main/java/org/terst/listen/data/Entities.kt @@ -88,11 +88,15 @@ data class SubscriptionEntity( /** * Whether to pass over chapters the publisher marked `toc: false`. * - * Off by default and opt-in per show. Acting on someone else's metadata to remove audio a - * listener has not heard is the one part of this feature that can eat an episode, so it is - * never on unless asked for. + * On by default, at Peter's request after living with it off. Only ever acts on sections a + * publisher explicitly marked as not part of the episode -- never guessed from a title -- + * and doing that without being asked each time is the point of the feature. + * + * The SQL default stays 0 while the Kotlin default is true: Room writes every column on + * insert, so the SQL one is only consulted by an ALTER TABLE that has already happened. + * Changing it would mean recreating the table for a value nothing reads. */ - @ColumnInfo(defaultValue = "0") val skipAdChapters: Boolean = false, + @ColumnInfo(defaultValue = "0") val skipAdChapters: Boolean = true, ) /** One a day, the cadence a drip subscription is for. */ diff --git a/app/src/main/java/org/terst/listen/ui/SkipSettings.kt b/app/src/main/java/org/terst/listen/ui/SkipSettings.kt index 7c6146c..106e3fd 100644 --- a/app/src/main/java/org/terst/listen/ui/SkipSettings.kt +++ b/app/src/main/java/org/terst/listen/ui/SkipSettings.kt @@ -66,9 +66,9 @@ fun SkipSettings( Text("Skip marked breaks", Modifier.padding(start = 12.dp)) } Text( - "Only where the show publishes chapters and marks a section as not part of the " + - "episode. Nothing is guessed from chapter titles, so a show discussing its own " + - "sponsors keeps playing.", + "On by default. Only acts where the show publishes chapters and marks a section " + + "as not part of the episode -- nothing is guessed from chapter titles, so a " + + "show discussing its own sponsors keeps playing.", style = MaterialTheme.typography.bodySmall, ) } diff --git a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt index e22be1e..171114d 100644 --- a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt @@ -49,6 +49,7 @@ class PlaybackAdvancerTest { skipIntroMs: Long = 0, playbackSpeed: Float? = null, kind: SubscriptionKind = SubscriptionKind.PODCAST, + skipAdChapters: Boolean = true, ): SubscriptionEntity { val entity = SubscriptionEntity( id = id, @@ -58,6 +59,7 @@ class PlaybackAdvancerTest { addedAtMs = 0, skipIntroMs = skipIntroMs, playbackSpeed = playbackSpeed, + skipAdChapters = skipAdChapters, ) subscriptions.insert(entity) return entity @@ -356,7 +358,8 @@ class PlaybackAdvancerTest { ), ) { now } - show(id = "s1") + // Explicitly off, rather than leaning on a default that has since flipped to on. + show(id = "s1", skipAdChapters = false) episode("a") assertEquals(emptyList(), fetching.contextFor("a").chapters) assertEquals(0, asked) diff --git a/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt b/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt index 1488559..d0a49f7 100644 --- a/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt @@ -69,22 +69,23 @@ class PlayheadPolicyTest { } @Test - fun `inside a marked break, on a show that opted in, it seeks past it`() { + fun `inside a marked break it seeks past it`() { assertEquals( TickAction.SeekTo(3 * minute), - decide( - show = show.copy(skipAdChapters = true), - chapters = chapters, - positionMs = 2 * minute + 5_000, - ), + decide(show = show, chapters = chapters, positionMs = 2 * minute + 5_000), ) } + /** Opted out explicitly: skipping is on by default, so this is the deliberate case. */ @Test - fun `a show that did not opt in keeps playing the break`() { + fun `a show that opted out keeps playing the break`() { assertEquals( TickAction.Nothing, - decide(show = show, chapters = chapters, positionMs = 2 * minute + 5_000), + decide( + show = show.copy(skipAdChapters = false), + chapters = chapters, + positionMs = 2 * minute + 5_000, + ), ) } -- cgit v1.2.3