From cbd0a4a1746472394bdef6656fe564e5e86ac111 Mon Sep 17 00:00:00 2001 From: davotoula Date: Sat, 29 Aug 2026 21:57:19 +0200 Subject: [PATCH] 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) + } }