laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-06 11:48:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: save videos to Movies/ instead of Pictures/ (#4009)

Browse Source 
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
This commit is contained in:
davotoula
2026-08-31 12:38:49 +02:00
parent 5ea4d6770e
commit cbd0a4a174
3 changed files with 193 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/androidTest/java/com/vitorpamplona/amethyst/ui/actions/MediaStoreTargetInstrumentedTest.kt
+45
View File
@@ -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
+117 -63
View File
@@ -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:"
amethyst/src/test/java/com/vitorpamplona/amethyst/ui/actions/MediaSaverToDiskTest.kt
+31
View File
@@ -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)
}
}