From 4541445e392e097c4d811c035e6eeee06dbcbdd8 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Thu, 20 Aug 2026 19:33:33 -0700 Subject: [PATCH] fix: prevent downloads sheet crash during active downloads Fixes #802 --- .../settings/fragment/DownloadData.kt | 65 ++++++ .../settings/fragment/DownloadsFragment.kt | 123 ++++------- .../settings/fragment/DownloadDataTest.kt | 209 ++++++++++++++++++ 3 files changed, 315 insertions(+), 82 deletions(-) create mode 100644 app/src/main/java/fulguris/settings/fragment/DownloadData.kt create mode 100644 app/src/test/java/fulguris/settings/fragment/DownloadDataTest.kt diff --git a/app/src/main/java/fulguris/settings/fragment/DownloadData.kt b/app/src/main/java/fulguris/settings/fragment/DownloadData.kt new file mode 100644 index 000000000..9766f1cb8 --- /dev/null +++ b/app/src/main/java/fulguris/settings/fragment/DownloadData.kt @@ -0,0 +1,65 @@ +package fulguris.settings.fragment + +import android.app.DownloadManager +import android.database.Cursor +import androidx.core.database.getIntOrNull +import androidx.core.database.getLongOrNull +import androidx.core.database.getStringOrNull + +internal data class DownloadData( + val id: Long, + val title: String?, + val status: Int, + val localUri: String?, + val uri: String?, + val bytesDownloaded: Long, + val totalSize: Long, + val lastModified: Long, + val mimeType: String? +) { + companion object { + fun fromCursor(cursor: Cursor): DownloadData? { + val id = cursor.requiredLong(DownloadManager.COLUMN_ID)?.takeIf { it >= 0L } + ?: return null + val status = cursor.requiredInt(DownloadManager.COLUMN_STATUS) ?: return null + + return DownloadData( + id = id, + title = cursor.optionalString(DownloadManager.COLUMN_TITLE), + status = status, + localUri = cursor.optionalString(DownloadManager.COLUMN_LOCAL_URI), + uri = cursor.optionalString(DownloadManager.COLUMN_URI), + bytesDownloaded = cursor.optionalLong( + DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR, + 0L + ), + totalSize = cursor.optionalLong(DownloadManager.COLUMN_TOTAL_SIZE_BYTES, -1L), + lastModified = cursor.optionalLong( + DownloadManager.COLUMN_LAST_MODIFIED_TIMESTAMP, + 0L + ), + mimeType = cursor.optionalString(DownloadManager.COLUMN_MEDIA_TYPE) + ) + } + + private fun Cursor.requiredLong(columnName: String): Long? { + val index = getColumnIndex(columnName) + return if (index >= 0) getLongOrNull(index) else null + } + + private fun Cursor.requiredInt(columnName: String): Int? { + val index = getColumnIndex(columnName) + return if (index >= 0) getIntOrNull(index) else null + } + + private fun Cursor.optionalLong(columnName: String, defaultValue: Long): Long { + val index = getColumnIndex(columnName) + return if (index >= 0) getLongOrNull(index) ?: defaultValue else defaultValue + } + + private fun Cursor.optionalString(columnName: String): String? { + val index = getColumnIndex(columnName) + return if (index >= 0) getStringOrNull(index) else null + } + } +} diff --git a/app/src/main/java/fulguris/settings/fragment/DownloadsFragment.kt b/app/src/main/java/fulguris/settings/fragment/DownloadsFragment.kt index 3a139475a..cbbb2a6a5 100644 --- a/app/src/main/java/fulguris/settings/fragment/DownloadsFragment.kt +++ b/app/src/main/java/fulguris/settings/fragment/DownloadsFragment.kt @@ -22,6 +22,7 @@ import com.google.android.material.dialog.MaterialAlertDialogBuilder import dagger.hilt.android.AndroidEntryPoint import fulguris.R import fulguris.extensions.copyToClipboard +import fulguris.extensions.firstOrNullMap import fulguris.extensions.toast import fulguris.utils.Utils import timber.log.Timber @@ -340,12 +341,11 @@ class DownloadsFragment : PreferenceFragmentCompat() { try { processedCount++ - val id = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_ID)) - seenDownloadIds.add(id) - - // Use the shared updateDownload logic (doesn't update actions/count) - updateDownloadFromCursor(cursor) - // + val downloadData = decodeDownloadData(cursor) + if (downloadData != null) { + seenDownloadIds.add(downloadData.id) + updateDownload(downloadData) + } updateDownloadCount() // Schedule next download @@ -403,36 +403,12 @@ class DownloadsFragment : PreferenceFragmentCompat() { } } - /** - * Data class to hold download information extracted from cursor - */ - private data class DownloadData( - val id: Long, - val title: String?, - val status: Int, - val localUri: String?, - val uri: String?, - val bytesDownloaded: Long, - val totalSize: Long, - val lastModified: Long, - val mimeType: String? - ) - - /** - * Create DownloadData from cursor position - */ - private fun createDownloadData(cursor: Cursor): DownloadData { - return DownloadData( - id = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_ID)), - title = cursor.getString(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_TITLE)), - status = cursor.getInt(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_STATUS)), - localUri = cursor.getString(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_LOCAL_URI)), - uri = cursor.getString(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_URI)), - bytesDownloaded = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR)), - totalSize = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_TOTAL_SIZE_BYTES)), - lastModified = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_LAST_MODIFIED_TIMESTAMP)), - mimeType = cursor.getString(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_MEDIA_TYPE)) - ) + private fun decodeDownloadData(cursor: Cursor): DownloadData? { + return DownloadData.fromCursor(cursor).also { data -> + if (data == null) { + Timber.w("Skipping download row without a valid ID or status") + } + } } /** @@ -444,11 +420,7 @@ class DownloadsFragment : PreferenceFragmentCompat() { val cursor = downloadManager.query(query) return cursor?.use { - if (it.moveToFirst()) { - createDownloadData(it) - } else { - null - } + it.firstOrNullMap(::decodeDownloadData) } } @@ -467,7 +439,7 @@ class DownloadsFragment : PreferenceFragmentCompat() { if (cursor != null && cursor.moveToFirst()) { try { - updateDownloadFromCursor(cursor) + decodeDownloadData(cursor)?.let(::updateDownload) // Update action states and download count after updating the preference updateDownloadCount() } finally { @@ -479,15 +451,10 @@ class DownloadsFragment : PreferenceFragmentCompat() { } } - /** - * Update a download from cursor data. This is the core implementation used by both - * list population and individual updates. Does not call updateActionStates/updateDownloadCount - * so caller can batch those calls. - */ - private fun updateDownloadFromCursor(cursor: Cursor) { - val id = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_ID)) - val status = cursor.getInt(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_STATUS)) - val localUri = cursor.getString(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_LOCAL_URI)) + private fun updateDownload(downloadData: DownloadData) { + val id = downloadData.id + val status = downloadData.status + val localUri = downloadData.localUri // Update flags based on this download's status hasAnyDownloads = true @@ -516,14 +483,12 @@ class DownloadsFragment : PreferenceFragmentCompat() { if (downloadPref != null) { // Update existing preference - val downloadData = createDownloadData(cursor) downloadPref.updateFromDownloadData(downloadData) // Update icon in case status or file type changed setDownloadIcon(downloadPref, downloadData.status, downloadData.mimeType, downloadData.localUri) Timber.d("updateDownloadFromCursor: Updated existing preference for download $id") } else { // Create new preference - val downloadData = createDownloadData(cursor) downloadPref = createDownloadPreference(downloadData) downloadsListCategory.addPreference(downloadPref) Timber.d("updateDownloadFromCursor: Created new preference for download $id") @@ -1747,16 +1712,18 @@ class DownloadsFragment : PreferenceFragmentCompat() { var isActive = false if (cursor.moveToFirst()) { - val status = cursor.getInt(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_STATUS)) - - isActive = status == DownloadManager.STATUS_RUNNING || - status == DownloadManager.STATUS_PENDING || - status == DownloadManager.STATUS_PAUSED - + val downloadData = DownloadData.fromCursor(cursor) + if (downloadData == null) { + Timber.w("updateProgress($downloadId): Invalid cursor data") + } else { + isActive = downloadData.status == DownloadManager.STATUS_RUNNING || + downloadData.status == DownloadManager.STATUS_PENDING || + downloadData.status == DownloadManager.STATUS_PAUSED - // Only update if this download is active or needs a summary refresh - if (isActive || summary == null) { - updateFromCursor(cursor) + // Only update if this download is active or needs a summary refresh + if (isActive || summary == null) { + updateSummary(downloadData) + } } } else { Timber.w("updateProgress($downloadId): No cursor data found") @@ -1766,26 +1733,12 @@ class DownloadsFragment : PreferenceFragmentCompat() { return isActive } - /** - * Update the preference summary from cursor data. - * This only updates the text, not the entire preference UI. - */ - fun updateFromCursor(cursor: Cursor) { - val status = cursor.getInt(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_STATUS)) - val bytesDownloaded = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR)) - val bytesTotal = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_TOTAL_SIZE_BYTES)) - val lastModified = cursor.getLong(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_LAST_MODIFIED_TIMESTAMP)) - val localUri = cursor.getString(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_LOCAL_URI)) - - summary = formatSummary(status, bytesDownloaded, bytesTotal, lastModified, localUri) - } - /** * Initialize the preference summary from DownloadData. * Called once during preference creation. */ fun initializeFromDownloadData(data: DownloadData) { - summary = formatSummary(data.status, data.bytesDownloaded, data.totalSize, data.lastModified, data.localUri) + updateSummary(data) } /** @@ -1799,7 +1752,17 @@ class DownloadsFragment : PreferenceFragmentCompat() { } // Update summary - summary = formatSummary(data.status, data.bytesDownloaded, data.totalSize, data.lastModified, data.localUri) + updateSummary(data) + } + + private fun updateSummary(data: DownloadData) { + summary = formatSummary( + data.status, + data.bytesDownloaded, + data.totalSize, + data.lastModified, + data.localUri + ) } /** @@ -1867,7 +1830,3 @@ class DownloadsFragment : PreferenceFragmentCompat() { - - - - diff --git a/app/src/test/java/fulguris/settings/fragment/DownloadDataTest.kt b/app/src/test/java/fulguris/settings/fragment/DownloadDataTest.kt new file mode 100644 index 000000000..e66be72ed --- /dev/null +++ b/app/src/test/java/fulguris/settings/fragment/DownloadDataTest.kt @@ -0,0 +1,209 @@ +package fulguris.settings.fragment + +import android.app.DownloadManager +import android.database.MatrixCursor +import fulguris.TestApplication +import fulguris.extensions.useMap +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +@RunWith(RobolectricTestRunner::class) +@Config(application = TestApplication::class, sdk = [29]) +class DownloadDataTest { + + @Test + fun `running download decodes null transient metadata and unknown size`() { + val cursor = downloadCursor( + id = 42L, + status = DownloadManager.STATUS_RUNNING, + localUri = null, + mimeType = null, + bytesDownloaded = 0L, + totalSize = -1L + ) + + val data = cursor.use { + assertThat(it.moveToFirst()).isTrue() + DownloadData.fromCursor(it) + } + + assertThat(data).isEqualTo( + DownloadData( + id = 42L, + title = "active.bin", + status = DownloadManager.STATUS_RUNNING, + localUri = null, + uri = "https://example.com/active.bin", + bytesDownloaded = 0L, + totalSize = -1L, + lastModified = 1234L, + mimeType = null + ) + ) + } + + @Test + fun `active download states decode zero and partial byte counts`() { + val rows = listOf( + Triple(DownloadManager.STATUS_RUNNING, 0L, -1L), + Triple(DownloadManager.STATUS_PENDING, 0L, 0L), + Triple(DownloadManager.STATUS_PAUSED, 25L, 100L) + ) + + rows.forEachIndexed { index, (status, downloaded, total) -> + val cursor = downloadCursor( + id = index + 1L, + status = status, + bytesDownloaded = downloaded, + totalSize = total + ) + + val data = cursor.use { + assertThat(it.moveToFirst()).isTrue() + DownloadData.fromCursor(it) + } + + assertThat(data).isNotNull + assertThat(data!!.status).isEqualTo(status) + assertThat(data.bytesDownloaded).isEqualTo(downloaded) + assertThat(data.totalSize).isEqualTo(total) + } + } + + @Test + fun `completed download preserves full metadata`() { + val cursor = downloadCursor( + id = 7L, + title = "finished.pdf", + status = DownloadManager.STATUS_SUCCESSFUL, + localUri = "file:///storage/emulated/0/Download/finished.pdf", + uri = "https://example.com/finished.pdf", + bytesDownloaded = 4096L, + totalSize = 4096L, + lastModified = 987654321L, + mimeType = "application/pdf" + ) + + val data = cursor.use { + assertThat(it.moveToFirst()).isTrue() + DownloadData.fromCursor(it) + } + + assertThat(data).isEqualTo( + DownloadData( + id = 7L, + title = "finished.pdf", + status = DownloadManager.STATUS_SUCCESSFUL, + localUri = "file:///storage/emulated/0/Download/finished.pdf", + uri = "https://example.com/finished.pdf", + bytesDownloaded = 4096L, + totalSize = 4096L, + lastModified = 987654321L, + mimeType = "application/pdf" + ) + ) + } + + @Test + fun `missing optional columns use safe defaults`() { + val cursor = MatrixCursor( + arrayOf( + DownloadManager.COLUMN_ID, + DownloadManager.COLUMN_STATUS + ) + ).apply { + addRow(arrayOf(11L, DownloadManager.STATUS_PENDING)) + } + + val data = cursor.use { + assertThat(it.moveToFirst()).isTrue() + DownloadData.fromCursor(it) + } + + assertThat(data).isEqualTo( + DownloadData( + id = 11L, + title = null, + status = DownloadManager.STATUS_PENDING, + localUri = null, + uri = null, + bytesDownloaded = 0L, + totalSize = -1L, + lastModified = 0L, + mimeType = null + ) + ) + } + + @Test + fun `row missing download id is skipped without aborting subsequent rows`() { + val cursor = MatrixCursor( + arrayOf( + DownloadManager.COLUMN_ID, + DownloadManager.COLUMN_STATUS + ) + ).apply { + addRow(arrayOf(null, DownloadManager.STATUS_RUNNING)) + addRow(arrayOf(99L, DownloadManager.STATUS_SUCCESSFUL)) + } + + val decodedRows = cursor.useMap(DownloadData::fromCursor).filterNotNull() + + assertThat(decodedRows.map(DownloadData::id)).containsExactly(99L) + } + + @Test + fun `row missing status is rejected`() { + val cursor = MatrixCursor(arrayOf(DownloadManager.COLUMN_ID)).apply { + addRow(arrayOf(5L)) + } + + val data = cursor.use { + assertThat(it.moveToFirst()).isTrue() + DownloadData.fromCursor(it) + } + + assertThat(data).isNull() + } + + private fun downloadCursor( + id: Long, + title: String? = "active.bin", + status: Int, + localUri: String? = null, + uri: String? = "https://example.com/active.bin", + bytesDownloaded: Long, + totalSize: Long, + lastModified: Long = 1234L, + mimeType: String? = null + ) = MatrixCursor( + arrayOf( + DownloadManager.COLUMN_ID, + DownloadManager.COLUMN_TITLE, + DownloadManager.COLUMN_STATUS, + DownloadManager.COLUMN_LOCAL_URI, + DownloadManager.COLUMN_URI, + DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR, + DownloadManager.COLUMN_TOTAL_SIZE_BYTES, + DownloadManager.COLUMN_LAST_MODIFIED_TIMESTAMP, + DownloadManager.COLUMN_MEDIA_TYPE + ) + ).apply { + addRow( + arrayOf( + id, + title, + status, + localUri, + uri, + bytesDownloaded, + totalSize, + lastModified, + mimeType + ) + ) + } +}