diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverTestSupport.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverTestSupport.kt new file mode 100644 index 0000000000..621c1a865c --- /dev/null +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverTestSupport.kt @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.actions + +import android.content.Context +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import java.io.File +import java.util.UUID + +/** + * Shared harness for the MediaSaverToDisk instrumented tests: writes a small payload + * file, drives [MediaSaverToDisk.save] with the given MIME type, and asserts the save + * reported success. Package-level support object per the AvifInstrumentedTestSupport + * precedent. + */ +object MediaSaverTestSupport { + /** Drives one save and fails the test if it reported an error or never succeeded. */ + fun saveAndAssertSuccess( + context: Context, + mimeType: String, + ) { + val localFile = File(context.cacheDir, "media-saver-${UUID.randomUUID()}.bin") + localFile.writeBytes(ByteArray(2048) { it.toByte() }) + + var failure: Throwable? = null + var succeeded = false + + try { + runBlocking { + MediaSaverToDisk.save( + localFile = localFile, + mimeType = mimeType, + context = context, + onSuccess = { succeeded = true }, + onError = { failure = it }, + ) + } + } finally { + localFile.delete() + } + + // Surfaces e.g. the #4009 IllegalArgumentException as the test failure message. + assertNull("save() reported an error: ${failure?.message}", failure) + assertTrue("save() never reported success", succeeded) + } +} diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt index 18b13e561b..d7c62872a9 100644 --- a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt @@ -27,11 +27,8 @@ import android.os.Environment import android.os.ParcelFileDescriptor import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry -import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull -import org.junit.Assert.assertTrue import org.junit.Assume.assumeTrue import org.junit.Before import org.junit.Test @@ -49,12 +46,25 @@ import java.io.IOException * * There is no JVM coverage of any of this: Build.VERSION.SDK_INT is 0 under * returnDefaultValues, so unit tests can only reach the routing function, never the writer. + * + * **Running this suite:** below Q the storage grant must exist before the app process + * forks (external storage is mounted at fork time), and Gradle's connectedAndroidTest + * installs and instruments with no window to grant in between - so these tests skip + * under it. Drive them manually on an API 26-28 device: + * ``` + * ./gradlew :amethyst:assemblePlayDebug :amethyst:assemblePlayDebugAndroidTest + * adb install -r -g amethyst/build/outputs/apk/play/debug/amethyst-play-arm64-v8a-debug.apk + * adb install -r -g amethyst/build/outputs/apk/androidTest/play/debug/amethyst-play-debug-androidTest.apk + * adb shell am instrument -w -e class com.vitorpamplona.amethyst.ui.actions.MediaSaverToDiskLegacyStorageTest \ + * com.vitorpamplona.amethyst.debug.test/androidx.test.runner.AndroidJUnitRunner + * ``` */ @RunWith(AndroidJUnit4::class) class MediaSaverToDiskLegacyStorageTest { private val context get() = InstrumentationRegistry.getInstrumentation().targetContext - private val watchedDirs = listOf("Movies", "Pictures", "Download", "Music") + /** Every directory production can write to, straight from the routing table. */ + private val watchedDirs = MediaSaverToDisk.MediaStoreTarget.entries.map { it.relativeDirectory } private val createdFiles = mutableListOf() @Before @@ -85,9 +95,8 @@ class MediaSaverToDiskLegacyStorageTest { // reaches it and every write fails with EACCES. Probe for real writability and // skip rather than report a routing failure that is really a harness problem. assumeTrue( - "External storage is not writable by this process. Below API 29 the grant must " + - "exist before the process starts - install with `adb install -r -g` and drive " + - "the run with `am instrument`; Gradle's connectedAndroidTest cannot grant in time.", + "External storage is not writable by this process; below API 29 the grant must " + + "exist at install time. See this class's KDoc for the exact run recipe.", canWriteToPublicStorage(), ) } @@ -96,7 +105,9 @@ class MediaSaverToDiskLegacyStorageTest { try { val dir = amethystDir("Movies").apply { if (!exists()) mkdirs() } val probe = File(dir, ".write-probe-${System.nanoTime()}") - probe.createNewFile().also { probe.delete() } + val writable = probe.createNewFile() + probe.delete() + writable } catch (e: IOException) { false } @@ -128,26 +139,7 @@ class MediaSaverToDiskLegacyStorageTest { ) { val before = snapshot() - val localFile = File(context.cacheDir, "legacy-save-${System.nanoTime()}.bin") - localFile.writeBytes(ByteArray(2048) { it.toByte() }) - - var failure: Throwable? = null - var succeeded = false - - runBlocking { - MediaSaverToDisk.save( - localFile = localFile, - mimeType = mimeType, - context = context, - onSuccess = { succeeded = true }, - onError = { failure = it }, - ) - } - - localFile.delete() - - assertNull("save() reported an error: ${failure?.message}", failure) - assertTrue("save() never reported success", succeeded) + MediaSaverTestSupport.saveAndAssertSuccess(context, mimeType) val added = snapshot().mapValues { (dir, names) -> names - before.getValue(dir) } added.forEach { (dir, names) -> names.forEach { createdFiles.add(File(amethystDir(dir), it)) } } diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt index 58323edd74..615a086546 100644 --- a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt @@ -21,41 +21,33 @@ package com.vitorpamplona.amethyst.ui.actions import android.content.ContentResolver +import android.content.ContentUris import android.net.Uri import android.os.Build import android.provider.MediaStore import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry -import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Assume.assumeTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import java.io.File /** - * End-to-end regression test for issue #4009. - * - * MediaProvider validates RELATIVE_PATH's primary directory against the collection - * being written to. Filing a video under "Pictures" threw - * `IllegalArgumentException: Primary directory Pictures not allowed for - * content://media/external/video/media; allowed directories are [DCIM, Movies]` - * on Android 10; later releases accept the mismatch and silently misfile the video. - * - * This drives the real ContentResolver, so it catches both symptoms: the insert has - * to succeed AND the row has to land in the directory the collection accepts. + * End-to-end regression test for issue #4009: drives the real ContentResolver, so it + * catches both symptoms of a collection/directory mismatch - Android 10 rejects the + * insert outright (the quoted rejection lives in [MediaSaverToDisk.MediaStoreTarget]'s + * KDoc), and later releases accept it and silently misfile the video. */ @RunWith(AndroidJUnit4::class) class MediaSaverToDiskMediaStoreTest { private val context get() = InstrumentationRegistry.getInstrumentation().targetContext private val resolver: ContentResolver get() = context.contentResolver - /** Only rows this test inserted, identified by id in the collection they went into. */ - private val created = mutableListOf>() + /** Only rows this test inserted, as item Uris in the collection they went into. */ + private val created = mutableListOf() @Before fun requiresScopedStorage() { @@ -64,9 +56,7 @@ class MediaSaverToDiskMediaStoreTest { @After fun cleanUp() { - created.forEach { (collection, id) -> - resolver.delete(collection, "${MediaStore.MediaColumns._ID} = ?", arrayOf(id.toString())) - } + created.forEach { resolver.delete(it, null, null) } } @Test @@ -91,27 +81,7 @@ class MediaSaverToDiskMediaStoreTest { // this suite is meant to be runnable on a real device holding real media. val highWaterMark = maxIdIn(collection) - val localFile = File(context.cacheDir, "media-saver-${System.nanoTime()}.bin") - localFile.writeBytes(ByteArray(2048) { it.toByte() }) - - var failure: Throwable? = null - var succeeded = false - - runBlocking { - MediaSaverToDisk.save( - localFile = localFile, - mimeType = mimeType, - context = context, - onSuccess = { succeeded = true }, - onError = { failure = it }, - ) - } - - localFile.delete() - - // Surfaces the #4009 IllegalArgumentException as the test failure message. - assertNull("save() reported an error: ${failure?.message}", failure) - assertTrue("save() never reported success", succeeded) + MediaSaverTestSupport.saveAndAssertSuccess(context, mimeType) return rowInsertedAfter(collection, highWaterMark) } @@ -139,7 +109,7 @@ class MediaSaverToDiskMediaStoreTest { "${MediaStore.MediaColumns._ID} ASC", )?.use { cursor -> assertTrue("save() reported success but inserted no row into $collection", cursor.moveToFirst()) - created.add(collection to cursor.getLong(0)) + created.add(ContentUris.withAppendedId(collection, cursor.getLong(0))) return cursor.getString(1) } return null diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDisk.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDisk.kt index 5d3ae348ab..fb5e37cffd 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDisk.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDisk.kt @@ -58,10 +58,11 @@ object MediaSaverToDisk { resolveBlossom: suspend (String) -> String? = { null }, onSuccess: () -> Any?, onError: (Throwable) -> Any?, - ) = withContext(Dispatchers.IO) { + ) { + // No dispatch here: save() and downloadAndSave() both move themselves to IO. when { videoUri.isNullOrBlank() -> { - return@withContext + return } videoUri.startsWith("file") -> { @@ -191,9 +192,9 @@ object MediaSaverToDisk { /** * Copies a local file into the gallery. Suspending and dispatched to IO like - * [downloadAndSave]: callers reach this from a click handler, and one of them - * (the storage-permission callback in FullScreenViewerChrome) launches on the - * main dispatcher, where copying a whole video would block the UI thread. + * [downloadAndSave]: callers reach this from click handlers, and a + * storage-permission callback among them launches on the main dispatcher, + * where copying a whole video would block the UI thread. */ @OptIn(ExperimentalUuidApi::class) suspend fun save( @@ -205,9 +206,6 @@ object MediaSaverToDisk { ) { withContext(Dispatchers.IO) { try { - val extension = - mimeType?.let { MimeTypeMap.getSingleton().getExtensionFromMimeType(it) } ?: "" - // use{}: readAll leaves its source open, so without this the file // descriptor stays open until the finalizer runs. localFile.inputStream().source().buffer().use { buffer -> @@ -219,6 +217,8 @@ object MediaSaverToDisk { contentResolver = context.contentResolver, ) } else { + val extension = + mimeType?.let { MimeTypeMap.getSingleton().getExtensionFromMimeType(it) } ?: "" saveContentDefault( fileName = "${Uuid.random()}.$extension", contentType = mimeType ?: "",