From cbd0a4a1746472394bdef6656fe564e5e86ac111 Mon Sep 17 00:00:00 2001 From: davotoula Date: Sat, 29 Aug 2026 21:57:19 +0200 Subject: [PATCH 1/3] fix: save videos to Movies/ instead of Pictures/ (#4009) MediaProvider validates the primary directory of RELATIVE_PATH against the collection being inserted into. saveContentQ paired MediaStore.Video.Media.EXTERNAL_CONTENT_URI with Environment.DIRECTORY_PICTURES, so every video save built content://media/external/video/media + "Pictures/Amethyst" and Android 10 rejected it with IllegalArgumentException: Primary directory Pictures not allowed for content://media/external/video/media; allowed directories are [DCIM, Movies] Newer Android releases don't reject the mismatch, which is why the crash only reproduces on older devices - but the file still landed under Pictures/ rather than Movies/ everywhere, confirmed on a current device. Collection and directory now travel together in a MediaStoreTarget enum, so the two cannot drift apart again, and the catch-all falls through to Downloads (which accepts any file) instead of the Video collection. The MIME type is resolved above the SDK_INT fork and both writers route through the enum: the API level now decides how a file is written, never which directory it belongs in, so the pre-Q path stops filing videos and PDFs under Pictures/ too. The directory names are spelled out as literals because Environment's DIRECTORY_* are plain static fields that the unit-test android.jar nulls out. The JVM test covers the routing; MediaStoreTargetInstrumentedTest pins the literals back to the platform constants on-device. Stop leaking a file descriptor and blocking the UI on local saves --- .../MediaStoreTargetInstrumentedTest.kt | 45 +++++ .../amethyst/ui/actions/MediaSaverToDisk.kt | 180 ++++++++++++------ .../ui/actions/MediaSaverToDiskTest.kt | 31 +++ 3 files changed, 193 insertions(+), 63 deletions(-) create mode 100644 amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaStoreTargetInstrumentedTest.kt diff --git a/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaStoreTargetInstrumentedTest.kt b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaStoreTargetInstrumentedTest.kt new file mode 100644 index 0000000000..c95e7797ef --- /dev/null +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaStoreTargetInstrumentedTest.kt @@ -0,0 +1,45 @@ +/* + * 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.os.Environment +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.vitorpamplona.amethyst.ui.actions.MediaSaverToDisk.MediaStoreTarget +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith + +/** + * [MediaStoreTarget] spells its directories out as literals because Environment's + * DIRECTORY_* fields are plain statics that the unit-test android.jar leaves null. + * This is the other half of that trade: on a real device the literals are checked + * against the platform constants they stand in for. + */ +@RunWith(AndroidJUnit4::class) +class MediaStoreTargetInstrumentedTest { + @Test + fun directoriesMatchThePlatformConstants() { + assertEquals(Environment.DIRECTORY_PICTURES, MediaStoreTarget.IMAGES.relativeDirectory) + assertEquals(Environment.DIRECTORY_MUSIC, MediaStoreTarget.AUDIO.relativeDirectory) + assertEquals(Environment.DIRECTORY_MOVIES, MediaStoreTarget.VIDEO.relativeDirectory) + assertEquals(Environment.DIRECTORY_DOWNLOADS, MediaStoreTarget.DOWNLOADS.relativeDirectory) + } +} 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 34ebb9abe4..5d3ae348ab 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 @@ -24,6 +24,7 @@ import android.content.ContentResolver import android.content.ContentValues import android.content.Context import android.media.MediaScannerConnection +import android.net.Uri import android.os.Build import android.os.Environment import android.provider.MediaStore @@ -131,18 +132,25 @@ object MediaSaverToDisk { } val trimmedUrl = trimInlineMetaData(downloadUrl) - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - val headerType = - response - .header("Content-Type") - ?.substringBefore(";") - ?.trim() + val headerType = + response + .header("Content-Type") + ?.substringBefore(";") + ?.trim() - val realType = - headerType?.takeIf(::isSaveableMimeType) - ?: mimeType?.takeIf(::isSaveableMimeType) - ?: getMimeTypeFromExtension(trimmedUrl).takeIf(::isSaveableMimeType) - ?: "" + // Resolved for both paths: the API level decides how the file is + // written, never which directory it belongs in. + val realType = + headerType?.takeIf(::isSaveableMimeType) + ?: mimeType?.takeIf(::isSaveableMimeType) + ?: getMimeTypeFromExtension(trimmedUrl).takeIf(::isSaveableMimeType) + ?: "" + + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + // Deliberately Q-only: MediaStore refuses an insert without a usable + // type, so there is nothing to do but report it. The legacy path has + // always written whatever it downloaded and still does - an unresolved + // type lands in Downloads, which accepts any file. check(realType.isNotBlank()) { "Can't find out the content type" } saveContentQ( @@ -154,6 +162,7 @@ object MediaSaverToDisk { } else { saveContentDefault( fileName = File(trimmedUrl).name, + contentType = realType, contentSource = response.body.source(), context = context, ) @@ -176,44 +185,54 @@ object MediaSaverToDisk { private fun isSaveableMimeType(type: String): Boolean = type.isNotBlank() && ( - type.startsWith("image/", ignoreCase = true) || - type.startsWith("video/", ignoreCase = true) || - type.startsWith("audio/", ignoreCase = true) || + MediaStoreTarget.of(type) != MediaStoreTarget.DOWNLOADS || type.equals(PDF_MIME_TYPE, ignoreCase = true) ) + /** + * 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. + */ @OptIn(ExperimentalUuidApi::class) - fun save( + suspend fun save( localFile: File, mimeType: String?, context: Context, onSuccess: () -> Any?, onError: (Throwable) -> Any?, ) { - try { - val extension = - mimeType?.let { MimeTypeMap.getSingleton().getExtensionFromMimeType(it) } ?: "" - val buffer = localFile.inputStream().source().buffer() + withContext(Dispatchers.IO) { + try { + val extension = + mimeType?.let { MimeTypeMap.getSingleton().getExtensionFromMimeType(it) } ?: "" - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - saveContentQ( - displayName = Uuid.random().toString(), - contentType = mimeType ?: "", - contentSource = buffer, - contentResolver = context.contentResolver, - ) - } else { - saveContentDefault( - fileName = "${Uuid.random()}.$extension", - contentSource = buffer, - context = context, - ) + // use{}: readAll leaves its source open, so without this the file + // descriptor stays open until the finalizer runs. + localFile.inputStream().source().buffer().use { buffer -> + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { + saveContentQ( + displayName = Uuid.random().toString(), + contentType = mimeType ?: "", + contentSource = buffer, + contentResolver = context.contentResolver, + ) + } else { + saveContentDefault( + fileName = "${Uuid.random()}.$extension", + contentType = mimeType ?: "", + contentSource = buffer, + context = context, + ) + } + } + onSuccess() + } catch (e: Exception) { + if (e is CancellationException) throw e + Log.w("MediaSaverToDisk", "Unable to save", e) + onError(e) } - onSuccess() - } catch (e: Exception) { - if (e is CancellationException) throw e - Log.w("MediaSaverToDisk", "Unable to save", e) - onError(e) } } @@ -225,29 +244,7 @@ object MediaSaverToDisk { contentResolver: ContentResolver, ) { val cleanMimeType = normalizeMimeTypeForMediaStore(contentType.substringBefore(";").trim()) - - val (masterUri, baseDir) = - when { - cleanMimeType.startsWith("image/", ignoreCase = true) -> { - MediaStore.Images.Media.EXTERNAL_CONTENT_URI to Environment.DIRECTORY_PICTURES - } - - cleanMimeType.startsWith("audio/", ignoreCase = true) -> { - // Audio content goes into the Music MediaStore + folder. Routing it through - // Video.EXTERNAL_CONTENT_URI (the previous fall-through behavior) crashes - // with IllegalArgumentException because MediaProvider rejects audio/* into - // the Video collection. - MediaStore.Audio.Media.EXTERNAL_CONTENT_URI to Environment.DIRECTORY_MUSIC - } - - cleanMimeType.equals(PDF_MIME_TYPE, ignoreCase = true) -> { - MediaStore.Downloads.EXTERNAL_CONTENT_URI to Environment.DIRECTORY_DOWNLOADS - } - - else -> { - MediaStore.Video.Media.EXTERNAL_CONTENT_URI to Environment.DIRECTORY_PICTURES - } - } + val target = MediaStoreTarget.of(cleanMimeType) val contentValues = ContentValues().apply { @@ -255,11 +252,11 @@ object MediaSaverToDisk { put(MediaStore.MediaColumns.MIME_TYPE, cleanMimeType) put( MediaStore.MediaColumns.RELATIVE_PATH, - baseDir + File.separatorChar + AMETHYST_SUBDIRECTORY, + target.relativeDirectory + File.separatorChar + AMETHYST_SUBDIRECTORY, ) } - val uri = contentResolver.insert(masterUri, contentValues) + val uri = contentResolver.insert(target.collectionUri(), contentValues) checkNotNull(uri) { "Can't insert the new content" } try { @@ -276,12 +273,15 @@ object MediaSaverToDisk { private fun saveContentDefault( fileName: String, + contentType: String, contentSource: BufferedSource, context: Context, ) { + val baseDir = MediaStoreTarget.of(contentType).relativeDirectory + val subdirectory = File( - Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_PICTURES), + Environment.getExternalStoragePublicDirectory(baseDir), AMETHYST_SUBDIRECTORY, ).apply { if (!exists()) mkdirs() @@ -307,6 +307,60 @@ object MediaSaverToDisk { else -> mimeType } + /** + * The MediaStore collection a download is filed under, together with the public + * directory it is written to. + * + * MediaProvider validates the primary directory of [MediaStore.MediaColumns.RELATIVE_PATH] + * against the collection being inserted into and rejects a mismatch with + * `IllegalArgumentException: Primary directory Pictures not allowed for + * content://media/external/video/media; allowed directories are [DCIM, Movies]`. + * A collection usually accepts more than one directory; these are the ones Amethyst + * files under. + */ + internal enum class MediaStoreTarget( + val relativeDirectory: String, + ) { + // The directory names are the values of Environment.DIRECTORY_PICTURES, _MUSIC, + // _MOVIES and _DOWNLOADS. They are spelled out because those are plain static + // fields that the unit-test android.jar leaves null, which would make this + // mapping impossible to cover off-device. MediaStoreTargetInstrumentedTest pins + // them back to the platform constants on-device. + IMAGES("Pictures"), + AUDIO("Music"), + VIDEO("Movies"), + DOWNLOADS("Download"), + ; + + /** + * Has to stay a method. The EXTERNAL_CONTENT_URI fields are null under the same + * unit-test android.jar, and MediaStore.Downloads only exists from API 29, so + * reading them from the constructor would break class init off-device and below Q. + */ + @RequiresApi(Build.VERSION_CODES.Q) + fun collectionUri(): Uri = + when (this) { + IMAGES -> MediaStore.Images.Media.EXTERNAL_CONTENT_URI + AUDIO -> MediaStore.Audio.Media.EXTERNAL_CONTENT_URI + VIDEO -> MediaStore.Video.Media.EXTERNAL_CONTENT_URI + DOWNLOADS -> MediaStore.Downloads.EXTERNAL_CONTENT_URI + } + + companion object { + /** + * PDFs, and anything that isn't image, audio or video content, go to Downloads + * — the one collection that accepts every kind of file. + */ + fun of(mimeType: String): MediaStoreTarget = + when { + mimeType.startsWith("image/", ignoreCase = true) -> IMAGES + mimeType.startsWith("audio/", ignoreCase = true) -> AUDIO + mimeType.startsWith("video/", ignoreCase = true) -> VIDEO + else -> DOWNLOADS + } + } + } + private const val AMETHYST_SUBDIRECTORY = "Amethyst" private const val PDF_MIME_TYPE = "application/pdf" private const val BLOSSOM_SCHEME = "blossom:" diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskTest.kt index f89b326f56..f5c8c65674 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskTest.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.amethyst.ui.actions +import com.vitorpamplona.amethyst.ui.actions.MediaSaverToDisk.MediaStoreTarget import org.junit.Assert.assertEquals import org.junit.Test @@ -47,4 +48,34 @@ class MediaSaverToDiskTest { assertEquals("image/png", MediaSaverToDisk.normalizeMimeTypeForMediaStore("image/png")) assertEquals("audio/mpeg", MediaSaverToDisk.normalizeMimeTypeForMediaStore("audio/mpeg")) } + + @Test + fun routesEachMediaKindToItsOwnCollection() { + assertEquals(MediaStoreTarget.IMAGES, MediaStoreTarget.of("image/jpeg")) + assertEquals(MediaStoreTarget.AUDIO, MediaStoreTarget.of("audio/mpeg")) + assertEquals(MediaStoreTarget.VIDEO, MediaStoreTarget.of("video/mp4")) + } + + @Test + fun routesPdfsAndUnknownTypesToDownloads() { + assertEquals(MediaStoreTarget.DOWNLOADS, MediaStoreTarget.of("application/pdf")) + assertEquals(MediaStoreTarget.DOWNLOADS, MediaStoreTarget.of("application/zip")) + assertEquals(MediaStoreTarget.DOWNLOADS, MediaStoreTarget.of("")) + } + + @Test + fun routingIsCaseInsensitive() { + assertEquals(MediaStoreTarget.IMAGES, MediaStoreTarget.of("Image/PNG")) + assertEquals(MediaStoreTarget.AUDIO, MediaStoreTarget.of("Audio/MPEG")) + assertEquals(MediaStoreTarget.VIDEO, MediaStoreTarget.of("Video/MP4")) + } + + @Test + fun eachCollectionIsPairedWithADirectoryMediaProviderAcceptsForIt() { + // Video content filed under "Pictures" is the rejection reported in #4009. + assertEquals("Pictures", MediaStoreTarget.IMAGES.relativeDirectory) + assertEquals("Music", MediaStoreTarget.AUDIO.relativeDirectory) + assertEquals("Movies", MediaStoreTarget.VIDEO.relativeDirectory) + assertEquals("Download", MediaStoreTarget.DOWNLOADS.relativeDirectory) + } } From af49bcc396abc77371dd22c3a8ed5b25b29e2a8f Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 31 Aug 2026 09:21:07 +0200 Subject: [PATCH 2/3] On device testing test: only ever delete MediaStore rows the test itself inserted test: cover the pre-Q save path on API 26 test: cover #4009 end-to-end against a real MediaStore on API 29 --- .../MediaSaverToDiskLegacyStorageTest.kt | 163 ++++++++++++++++++ .../actions/MediaSaverToDiskMediaStoreTest.kt | 147 ++++++++++++++++ 2 files changed, 310 insertions(+) create mode 100644 amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt create mode 100644 amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt 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 new file mode 100644 index 0000000000..18b13e561b --- /dev/null +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt @@ -0,0 +1,163 @@ +/* + * 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.Manifest +import android.content.pm.PackageManager +import android.os.Build +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 +import org.junit.runner.RunWith +import java.io.File +import java.io.IOException + +/** + * Covers the pre-Q writer, which MediaStore never sees: below API 29 saveContentDefault + * writes straight to a public directory and lets the media scanner index it. + * + * That path used to hardcode Pictures for every content type, so videos, audio and PDFs + * were all filed under Pictures/Amethyst. It now routes through the same MediaStoreTarget + * as the MediaStore path. minSdk is 26, so this range ships. + * + * 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. + */ +@RunWith(AndroidJUnit4::class) +class MediaSaverToDiskLegacyStorageTest { + private val context get() = InstrumentationRegistry.getInstrumentation().targetContext + + private val watchedDirs = listOf("Movies", "Pictures", "Download", "Music") + private val createdFiles = mutableListOf() + + @Before + fun onlyBelowScopedStorage() { + assumeTrue("saveContentDefault only runs below API 29", Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) + + // The legacy writer needs the runtime permission; no androidx.test:rules on the + // classpath, so grant it through the instrumentation shell instead. The output has + // to be drained: executeShellCommand runs asynchronously and closing the descriptor + // early kills the command before it applies. + val fd = + InstrumentationRegistry + .getInstrumentation() + .uiAutomation + .executeShellCommand( + "pm grant ${context.packageName} android.permission.WRITE_EXTERNAL_STORAGE", + ) + ParcelFileDescriptor.AutoCloseInputStream(fd).use { it.readBytes() } + + assertEquals( + "WRITE_EXTERNAL_STORAGE was not granted; the legacy writer cannot be exercised", + PackageManager.PERMISSION_GRANTED, + context.checkSelfPermission(Manifest.permission.WRITE_EXTERNAL_STORAGE), + ) + + // Holding the permission is not enough below Q: external storage is mounted into + // the process when it forks, so a grant to an already-running process never + // 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.", + canWriteToPublicStorage(), + ) + } + + private fun canWriteToPublicStorage(): Boolean = + try { + val dir = amethystDir("Movies").apply { if (!exists()) mkdirs() } + val probe = File(dir, ".write-probe-${System.nanoTime()}") + probe.createNewFile().also { probe.delete() } + } catch (e: IOException) { + false + } + + @After + fun cleanUp() { + createdFiles.forEach { it.delete() } + } + + @Test + fun videoGoesToMovies() = assertRoutes("video/mp4", "Movies") + + @Test + fun imageGoesToPictures() = assertRoutes("image/jpeg", "Pictures") + + @Test + fun audioGoesToMusic() = assertRoutes("audio/mpeg", "Music") + + @Test + fun pdfGoesToDownloads() = assertRoutes("application/pdf", "Download") + + /** + * Saves one file and asserts it appeared under [expectedDir]/Amethyst and nowhere else. + * Checking the other directories is the point: the bug was everything landing in Pictures. + */ + private fun assertRoutes( + mimeType: String, + expectedDir: String, + ) { + 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) + + val added = snapshot().mapValues { (dir, names) -> names - before.getValue(dir) } + added.forEach { (dir, names) -> names.forEach { createdFiles.add(File(amethystDir(dir), it)) } } + + val dirsThatGrew = added.filterValues { it.isNotEmpty() }.keys + assertEquals("$mimeType should land only in $expectedDir/Amethyst", setOf(expectedDir), dirsThatGrew) + assertEquals("expected exactly one new file", 1, added.getValue(expectedDir).size) + } + + private fun amethystDir(publicDir: String) = File(Environment.getExternalStoragePublicDirectory(publicDir), "Amethyst") + + private fun snapshot(): Map> = watchedDirs.associateWith { amethystDir(it).list()?.toSet() ?: emptySet() } +} 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 new file mode 100644 index 0000000000..58323edd74 --- /dev/null +++ b/amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt @@ -0,0 +1,147 @@ +/* + * 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.ContentResolver +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. + */ +@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>() + + @Before + fun requiresScopedStorage() { + assumeTrue("saveContentQ only runs on API 29+", Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) + } + + @After + fun cleanUp() { + created.forEach { (collection, id) -> + resolver.delete(collection, "${MediaStore.MediaColumns._ID} = ?", arrayOf(id.toString())) + } + } + + @Test + fun savingAVideoLandsInMoviesAndNotPictures() { + val relativePath = saveAndReadBackRelativePath("video/mp4", MediaStore.Video.Media.EXTERNAL_CONTENT_URI) + + assertEquals("Movies/Amethyst/", relativePath) + } + + @Test + fun savingAnImageStillLandsInPictures() { + val relativePath = saveAndReadBackRelativePath("image/jpeg", MediaStore.Images.Media.EXTERNAL_CONTENT_URI) + + assertEquals("Pictures/Amethyst/", relativePath) + } + + private fun saveAndReadBackRelativePath( + mimeType: String, + collection: Uri, + ): String? { + // Anything at or below this id predates the test and must never be read or deleted: + // 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) + + return rowInsertedAfter(collection, highWaterMark) + } + + private fun maxIdIn(collection: Uri): Long { + resolver + .query(collection, arrayOf(MediaStore.MediaColumns._ID), null, null, "${MediaStore.MediaColumns._ID} DESC") + ?.use { cursor -> + if (cursor.moveToFirst()) return cursor.getLong(0) + } + return -1L + } + + /** Reads back the row the save just inserted and records it for cleanup. */ + private fun rowInsertedAfter( + collection: Uri, + highWaterMark: Long, + ): String? { + resolver + .query( + collection, + arrayOf(MediaStore.MediaColumns._ID, MediaStore.MediaColumns.RELATIVE_PATH), + "${MediaStore.MediaColumns._ID} > ?", + arrayOf(highWaterMark.toString()), + "${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)) + return cursor.getString(1) + } + return null + } +} From defbdfbb280a5bba53edd04b521f2fea87cc0e08 Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 31 Aug 2026 12:48:36 +0200 Subject: [PATCH 3/3] Code reviews: apply review cleanups to the media-save fix and its tests - Extract the duplicated drive-the-save harness (temp file, runBlocking save, success/error assertions) from both instrumented e2e tests into MediaSaverTestSupport, following the AvifInstrumentedTestSupport precedent, with UUID-based filenames per the existing convention. - MediaSaverToDiskMediaStoreTest: record inserted rows as item Uris (ContentUris.withAppendedId) instead of Pair + hand-built _ID selection; trim the KDoc paragraph that re-quoted the MediaProvider rejection verbatim - the canonical copy lives on MediaStoreTarget. - MediaSaverToDiskLegacyStorageTest: derive watchedDirs from MediaStoreTarget.entries instead of a third hand-maintained directory list; move the exact run recipe (assemble, install -g, am instrument) into the class KDoc and point the skip message at it; unfold the write-probe .also puzzle. - MediaSaverToDisk: drop the outer withContext in saveDownloadingIfNeeded (both delegates now dispatch themselves, leaving the decision in the leaf writers); scope `val extension` to the pre-Q branch that uses it; drop the rot-prone composable file name from save()'s KDoc. Considered and left alone: isSaveableMimeType deriving from MediaStoreTarget.of (kept - one definition of the accepted set beats re-spelling the prefix triple); the nested Dispatchers.IO in save()/downloadAndSave (load-bearing for direct call sites, fast-path no-op when nested); the redundant launch(Dispatchers.IO) at two call sites outside this branch. --- .../ui/actions/MediaSaverTestSupport.kt | 66 +++++++++++++++++++ .../MediaSaverToDiskLegacyStorageTest.kt | 48 ++++++-------- .../actions/MediaSaverToDiskMediaStoreTest.kt | 50 +++----------- .../amethyst/ui/actions/MediaSaverToDisk.kt | 16 ++--- 4 files changed, 104 insertions(+), 76 deletions(-) create mode 100644 amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverTestSupport.kt 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 ?: "",