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 new file mode 100644 index 0000000000..d7c62872a9 --- /dev/null +++ b/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() + + @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> = 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..615a086546 --- /dev/null +++ b/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() + + @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 + } +} 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..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 @@ -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:" 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) + } }