Fix crash when chapters have 0 length durations (#2060)

Co-authored-by: Tiago Marques <[email protected]>

app/src/androidTest/java/au/com/shiftyjelly/pocketcasts/models/db/ChapterDaoTest.kt

@@ -119,6 +119,78 @@ class ChapterDaoTest {
         assertEquals(chapters2, result)
     }
 
+    @Test
+    fun replaceAllChaptersIfExistingCountIsSmaller() = runBlocking {
+        val chapters1 = List(5) { index ->
+            Chapter(
+                title = "$index-0",
+                episodeUuid = "episode-id",
+                startTimeMs = 0L + index,
+            )
+        }
+        chapterDao.replaceAllChapters("episode-id", chapters1)
+
+        val chapters2 = List(6) { index ->
+            Chapter(
+                title = "$index-1",
+                episodeUuid = "episode-id",
+                startTimeMs = 0L + index,
+            )
+        }
+        chapterDao.replaceAllChaptersIfMoreIsPassed("episode-id", chapters2)
+
+        val result = chapterDao.findAll()
+        assertEquals(chapters2, result)
+    }
+
+    @Test
+    fun doNotReplaceAllChaptersIfExistingCountIsEqual() = runBlocking {
+        val chapters1 = List(5) { index ->
+            Chapter(
+                title = "$index-0",
+                episodeUuid = "episode-id",
+                startTimeMs = 0L + index,
+            )
+        }
+        chapterDao.replaceAllChapters("episode-id", chapters1)
+
+        val chapters2 = List(5) { index ->
+            Chapter(
+                title = "$index-1",
+                episodeUuid = "episode-id",
+                startTimeMs = 0L + index,
+            )
+        }
+        chapterDao.replaceAllChaptersIfMoreIsPassed("episode-id", chapters2)
+
+        val result = chapterDao.findAll()
+        assertEquals(chapters1, result)
+    }
+
+    @Test
+    fun doNotReplaceAllChaptersIfExistingCountIsLarger() = runBlocking {
+        val chapters1 = List(6) { index ->
+            Chapter(
+                title = "$index-0",
+                episodeUuid = "episode-id",
+                startTimeMs = 0L + index,
+            )
+        }
+        chapterDao.replaceAllChapters("episode-id", chapters1)
+
+        val chapters2 = List(5) { index ->
+            Chapter(
+                title = "$index-1",
+                episodeUuid = "episode-id",
+                startTimeMs = 0L + index,
+            )
+        }
+        chapterDao.replaceAllChaptersIfMoreIsPassed("episode-id", chapters2)
+
+        val result = chapterDao.findAll()
+        assertEquals(chapters1, result)
+    }
+
     @Test
     fun doNotDeleteChaptersForOtherEpisodes() = runBlocking {
         val id1 = "episode-id-1"

app/src/main/java/au/com/shiftyjelly/pocketcasts/ui/MainActivity.kt

@@ -760,7 +760,6 @@ class MainActivity :
             upNextQueue to useEpisodeArtwork
         }
             .onEach { (upNextQueue, useEpisodeArtwork) ->
-                updatePlaybackStateDeselectedChapterIndices(upNextQueue)
                 binding.playerBottomSheet.setUpNext(
                     upNext = upNextQueue,
                     theme = theme,
@@ -888,22 +887,6 @@ class MainActivity :
         })
     }
 
-    private fun updatePlaybackStateDeselectedChapterIndices(upNextQueue: UpNextQueue.State) {
-        (upNextQueue as? UpNextQueue.State.Loaded)?.let {
-            val lastPlaybackState = viewModel.lastPlaybackState
-            if (lastPlaybackState?.chapters?.getList()?.isEmpty() == true) return
-            val currentEpisodeDeselectedChapterIndicesChanged = playbackManager.getCurrentEpisode()?.let { currentEpisode ->
-                currentEpisode.uuid == it.episode.uuid &&
-                    lastPlaybackState != null &&
-                    it.episode.deselectedChapters.sorted() !=
-                    lastPlaybackState.chapters.getList().filterNot { it.selected }.map { it.index }.sorted()
-            } ?: false
-            if (currentEpisodeDeselectedChapterIndicesChanged) {
-                playbackManager.updatePlaybackStateDeselectedChapterIndices()
-            }
-        }
-    }
-
     override fun whatsNewDismissed(fromConfirmAction: Boolean) {
         if (fromConfirmAction) return
         if (settings.getEndOfYearShowModal()) showEndOfYearModal()

fastlane/Fastfile

@@ -414,8 +414,8 @@ platform :android do
     android_firebase_test(
       project_id: firebase_secret(name: 'project_id'),
       key_file: GOOGLE_FIREBASE_SECRETS_PATH,
-      model: 'Nexus5',
-      version: 23,
+      model: 'Pixel2.arm',
+      version: 30,
       test_apk_path: File.join(apk_dir, 'androidTest', 'debug', 'app-debug-androidTest.apk'),
       apk_path: File.join(apk_dir, 'debug', 'app-debug.apk'),
       results_output_dir: File.join(PROJECT_ROOT_FOLDER, 'build', 'instrumented-tests')

modules/services/model/src/main/java/au/com/shiftyjelly/pocketcasts/models/db/dao/ChapterDao.kt

@@ -26,6 +26,19 @@ abstract class ChapterDao {
         insertAll(chapters.filter { it.episodeUuid == episodeUuid })
     }
 
+    @Transaction
+    open suspend fun replaceAllChaptersIfMoreIsPassed(episodeUuid: String, chapters: List<Chapter>) {
+        val storedChapters = findEpisodeChapters(episodeUuid)
+        if (storedChapters.size >= chapters.size) {
+            return
+        }
+        deleteForEpisode(episodeUuid)
+        insertAll(chapters.filter { it.episodeUuid == episodeUuid })
+    }
+
+    @Query("SELECT * FROM episode_chapters WHERE episode_uuid IS :episodeUuid ORDER BY start_time ASC")
+    protected abstract suspend fun findEpisodeChapters(episodeUuid: String): List<Chapter>
+
     @Query("SELECT * FROM episode_chapters WHERE episode_uuid IS :episodeUuid ORDER BY start_time ASC")
     protected abstract fun _observerChaptersForEpisode(episodeUuid: String): Flow<List<Chapter>>
 

modules/services/model/src/main/java/au/com/shiftyjelly/pocketcasts/models/to/Chapter.kt

@@ -34,9 +34,9 @@ data class Chapter(
         }
     }
 
-    fun calculateProgress(playbackPosition: Duration): Float = if (playbackPosition == Duration.ZERO || playbackPosition !in this) {
-        0f
-    } else {
-        (playbackPosition - startTime).inWholeMilliseconds.toFloat() / duration.inWholeMilliseconds.toFloat()
+    fun calculateProgress(playbackPosition: Duration): Float = when {
+        playbackPosition == Duration.ZERO || playbackPosition !in this -> 0f
+        duration.inWholeMilliseconds == 0L -> 0f
+        else -> (playbackPosition - startTime).inWholeMilliseconds.toFloat() / duration.inWholeMilliseconds
     }
 }

modules/services/model/src/main/java/au/com/shiftyjelly/pocketcasts/models/to/Chapters.kt

@@ -1,6 +1,5 @@
 package au.com.shiftyjelly.pocketcasts.models.to
 
-import au.com.shiftyjelly.pocketcasts.models.entity.BaseEpisode
 import au.com.shiftyjelly.pocketcasts.utils.featureflag.Feature
 import au.com.shiftyjelly.pocketcasts.utils.featureflag.FeatureFlag
 import kotlin.time.Duration
@@ -82,19 +81,14 @@ data class Chapters(
         return getChapterIndex(time) == items.size - 1
     }
 
-    fun updateChaptersTimes(episodeDuration: Duration) = copy(
-        items = items.mapIndexed { index, chapter ->
-            when (index) {
-                0 -> chapter.copy(startTime = Duration.ZERO)
-                items.lastIndex -> chapter.copy(endTime = episodeDuration)
-                else -> chapter
-            }
-        },
-    )
-
-    fun updateDeselectedState(currentEpisode: BaseEpisode?) = copy(
-        items = items.map { chapter ->
-            chapter.copy(selected = currentEpisode?.deselectedChapters?.contains(chapter.index) == false)
-        },
-    )
+    fun toDbChapters(episodeId: String) = items.map { chapter ->
+        DbChapter(
+            episodeUuid = episodeId,
+            startTimeMs = chapter.startTime.inWholeMilliseconds,
+            endTimeMs = chapter.endTime.inWholeMilliseconds,
+            title = chapter.title,
+            imageUrl = chapter.imagePath,
+            url = chapter.url?.toString(),
+        )
+    }
 }

modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt

@@ -990,19 +990,6 @@ open class PlaybackManager @Inject constructor(
         }
     }
 
-    fun updatePlaybackStateDeselectedChapterIndices() {
-        launch {
-            playbackStateRelay.blockingFirst().let { playbackState ->
-                playbackStateRelay.accept(
-                    playbackState.copy(
-                        chapters = playbackState.chapters.updateDeselectedState(getCurrentEpisode()),
-                        lastChangeFrom = LastChangeFrom.OnChapterIndicesUpdated.value,
-                    ),
-                )
-            }
-        }
-    }
-
     fun clearUpNextAsync() {
         launch {
             upNextQueue.clearUpNext()
@@ -1531,17 +1518,13 @@ open class PlaybackManager @Inject constructor(
 
     private fun onMetadataAvailable(episodeMetadata: EpisodeFileMetadata) {
         playbackStateRelay.blockingFirst().let { playbackState ->
-            val newChapters = episodeMetadata.chapters
-                .updateChaptersTimes(playbackState.durationMs.milliseconds)
-                .updateDeselectedState(getCurrentEpisode())
-            val chapters = maxOf(playbackState.chapters, newChapters) { a, b -> a.size.compareTo(b.size) }
-
-            playbackStateRelay.accept(
-                playbackState.copy(
-                    chapters = chapters,
-                    lastChangeFrom = LastChangeFrom.OnMetadataAvailable.value,
-                ),
-            )
+            launch {
+                chapterManager.updateChapters(
+                    playbackState.episodeUuid,
+                    episodeMetadata.chapters.toDbChapters(playbackState.episodeUuid),
+                    forceUpdate = false,
+                )
+            }
         }
     }
 
@@ -1551,17 +1534,12 @@ open class PlaybackManager @Inject constructor(
     private fun onEpisodeChanged(episodeUuid: String) {
         observeChaptersJob?.cancel()
         observeChaptersJob = chapterManager.observerChaptersForEpisode(episodeUuid)
-            .onEach { onRemoteChaptersAvailable(it) }
+            .onEach { onChaptersAvailable(it) }
             .launchIn(this)
     }
 
-    private fun onRemoteChaptersAvailable(remoteChapters: Chapters) {
+    private fun onChaptersAvailable(chapters: Chapters) {
         playbackStateRelay.blockingFirst().let { playbackState ->
-            val newChapters = remoteChapters
-                .updateChaptersTimes(playbackState.durationMs.milliseconds)
-                .updateDeselectedState(getCurrentEpisode())
-            val chapters = maxOf(playbackState.chapters, newChapters) { a, b -> a.size.compareTo(b.size) }
-
             playbackStateRelay.accept(playbackState.copy(chapters = chapters))
         }
     }

modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/podcast/ChapterManager.kt

@@ -5,7 +5,11 @@ import kotlinx.coroutines.flow.Flow
 import au.com.shiftyjelly.pocketcasts.models.to.DbChapter as Chapter
 
 interface ChapterManager {
-    suspend fun updateChapters(episodeUuid: String, chapters: List<Chapter>)
+    suspend fun updateChapters(
+        episodeUuid: String,
+        chapters: List<Chapter>,
+        forceUpdate: Boolean = true,
+    )
 
     fun observerChaptersForEpisode(episodeUuid: String): Flow<Chapters>
 }

modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/podcast/ChapterManagerImpl.kt

@@ -1,48 +1,65 @@
 package au.com.shiftyjelly.pocketcasts.repositories.podcast
 
 import au.com.shiftyjelly.pocketcasts.models.db.dao.ChapterDao
-import au.com.shiftyjelly.pocketcasts.models.db.dao.EpisodeDao
+import au.com.shiftyjelly.pocketcasts.models.entity.BaseEpisode
 import au.com.shiftyjelly.pocketcasts.models.to.Chapter
 import au.com.shiftyjelly.pocketcasts.models.to.Chapters
 import au.com.shiftyjelly.pocketcasts.models.to.DbChapter
 import javax.inject.Inject
 import kotlin.time.Duration
 import kotlin.time.Duration.Companion.milliseconds
-import kotlinx.coroutines.flow.map
+import kotlinx.coroutines.flow.combine
+import kotlinx.coroutines.flow.distinctUntilChangedBy
 import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 
 class ChapterManagerImpl @Inject constructor(
     private val chapterDao: ChapterDao,
-    private val episodeDao: EpisodeDao,
+    private val episodeManager: EpisodeManager,
 ) : ChapterManager {
-    override suspend fun updateChapters(episodeUuid: String, chapters: List<DbChapter>) {
-        chapterDao.replaceAllChapters(episodeUuid, chapters)
+    override suspend fun updateChapters(
+        episodeUuid: String,
+        chapters: List<DbChapter>,
+        forceUpdate: Boolean,
+    ) {
+        if (forceUpdate) {
+            chapterDao.replaceAllChapters(episodeUuid, chapters)
+        } else {
+            chapterDao.replaceAllChaptersIfMoreIsPassed(episodeUuid, chapters)
+        }
     }
 
-    override fun observerChaptersForEpisode(episodeUuid: String) = chapterDao
-        .observerChaptersForEpisode(episodeUuid)
-        .map { it.toChapters(episodeUuid) }
+    override fun observerChaptersForEpisode(episodeUuid: String) = combine(
+        episodeManager.observeEpisodeByUuid(episodeUuid).distinctUntilChangedBy(BaseEpisode::deselectedChapters),
+        chapterDao.observerChaptersForEpisode(episodeUuid),
+    ) { episode, dbChapters -> dbChapters.toChapters(episode) }
+
+    private fun List<DbChapter>.toChapters(episode: BaseEpisode): Chapters {
+        val chaptersList = asSequence()
+            .fixChapterTimestamps(episode)
+            .filter { it.duration > Duration.ZERO }
+            .mapIndexed { index, chapter -> chapter.copy(index = index + 1) }
+            .toList()
+        return Chapters(chaptersList)
+    }
 
-    private suspend fun List<DbChapter>.toChapters(episodeUuid: String): Chapters {
-        val deselectedChapters = episodeDao.findByUuid(episodeUuid)?.deselectedChapters.orEmpty()
-        val chaptersList = withIndex().windowed(size = 2, partialWindows = true) { window ->
-            val index = window[0].index + 1
-            val firstChapter = window[0].value
-            val secondChapter = window.getOrNull(1)?.value
+    private fun Sequence<DbChapter>.fixChapterTimestamps(episode: BaseEpisode) = withIndex().windowed(size = 2, partialWindows = true) { window ->
+        val index = window[0].index
+        val chapterIndex = window[0].index + 1
+        val firstChapter = window[0].value
+        val secondChapter = window.getOrNull(1)?.value
 
-            val secondEndTime = secondChapter?.startTimeMs?.milliseconds ?: Duration.INFINITE
-            val fixedEndTime = firstChapter.endTimeMs?.milliseconds?.takeIf { it <= secondEndTime } ?: secondEndTime
+        val newStartTime = if (index == 0) Duration.ZERO else firstChapter.startTimeMs.milliseconds
+        val secondStartTime = secondChapter?.startTimeMs?.milliseconds ?: episode.durationMs.milliseconds
+        val newEndTime = firstChapter.endTimeMs?.milliseconds?.takeIf { it <= secondStartTime && it > newStartTime } ?: secondStartTime
 
-            Chapter(
-                title = firstChapter.title.orEmpty(),
-                startTime = firstChapter.startTimeMs.milliseconds,
-                endTime = fixedEndTime,
-                url = firstChapter.url?.toHttpUrlOrNull(),
-                imagePath = firstChapter.imageUrl,
-                index = index,
-                selected = index !in deselectedChapters,
-            )
-        }
-        return Chapters(chaptersList)
+        Chapter(
+            title = firstChapter.title.orEmpty(),
+            startTime = newStartTime,
+            endTime = newEndTime,
+            url = firstChapter.url?.toHttpUrlOrNull(),
+            imagePath = firstChapter.imageUrl,
+            index = chapterIndex,
+            selected = chapterIndex !in episode.deselectedChapters,
+        )
     }
 }