Merge pull request #4023 from davotoula/fix/save-video-to-movies-dir

Save videos to Movies/ instead of Pictures/ (fixes #4009)

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)
+    }
+}

amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskLegacyStorageTest.kt

@@ -0,0 +1,155 @@
+/*
+ * 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 org.junit.After
+import org.junit.Assert.assertEquals
+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.
+ *
+ * **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
+
+    /** Every directory production can write to, straight from the routing table. */
+    private val watchedDirs = MediaSaverToDisk.MediaStoreTarget.entries.map { it.relativeDirectory }
+    private val createdFiles = mutableListOf<File>()
+
+    @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 at install time. See this class's KDoc for the exact run recipe.",
+            canWriteToPublicStorage(),
+        )
+    }
+
+    private fun canWriteToPublicStorage(): Boolean =
+        try {
+            val dir = amethystDir("Movies").apply { if (!exists()) mkdirs() }
+            val probe = File(dir, ".write-probe-${System.nanoTime()}")
+            val writable = probe.createNewFile()
+            probe.delete()
+            writable
+        } 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()
+
+        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)) } }
+
+        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<String, Set<String>> = watchedDirs.associateWith { amethystDir(it).list()?.toSet() ?: emptySet() }
+}

amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskMediaStoreTest.kt

@@ -0,0 +1,117 @@
+/*
+ * 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.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 org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Assume.assumeTrue
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+/**
+ * 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, as item Uris in the collection they went into. */
+    private val created = mutableListOf<Uri>()
+
+    @Before
+    fun requiresScopedStorage() {
+        assumeTrue("saveContentQ only runs on API 29+", Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q)
+    }
+
+    @After
+    fun cleanUp() {
+        created.forEach { resolver.delete(it, null, null) }
+    }
+
+    @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)
+
+        MediaSaverTestSupport.saveAndAssertSuccess(context, mimeType)
+
+        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(ContentUris.withAppendedId(collection, cursor.getLong(0)))
+                return cursor.getString(1)
+            }
+        return null
+    }
+}

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)
+    }
+}

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
@@ -57,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") -> {
@@ -131,18 +133,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 +163,7 @@ object MediaSaverToDisk {
                     } else {
                         saveContentDefault(
                             fileName = File(trimmedUrl).name,
+                            contentType = realType,
                             contentSource = response.body.source(),
                             context = context,
                         )
@@ -176,44 +186,53 @@ 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 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)
-    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()
-
-            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,
-                )
+        withContext(Dispatchers.IO) {
+            try {
+                // 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 {
+                        val extension =
+                            mimeType?.let { MimeTypeMap.getSingleton().getExtensionFromMimeType(it) } ?: ""
+                        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:"

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)
+    }
 }