Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Android media sharing/saving filenames so they retain a valid image extension by stripping URL query/fragment parts and supporting Bluesky’s @jpeg-style suffixes (issue #1924).
Changes:
- Update
UiMedia.getFileName()to ignore query/fragment and derive extensions from.or@separators with sensible fallbacks. - Add common tests covering query params, missing extensions, and Bluesky
@jpegURLs. - Update Android sharing codepaths to use safer filenames (including
getFileName()for status media sharing) and add fallback extension inference inMediaScreen.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/model/UiMedia.kt | Improves extension extraction (strip ?/#, support @jpeg, fallback by media type). |
| shared/src/commonTest/kotlin/dev/dimension/flare/ui/model/UiMediaFileNameTest.kt | Adds tests for query params, no-extension URLs, and Bluesky @jpeg format. |
| app/src/main/java/dev/dimension/flare/ui/screen/media/StatusMediaScreen.kt | Uses UiMedia.getFileName() for shared image cache file naming. |
| app/src/main/java/dev/dimension/flare/ui/screen/media/MediaScreen.kt | Normalizes filename for save/share and adds extension fallback via MIME type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fileName = "$fileName.$extension" | ||
| } | ||
| saveByteArrayToDownloads(context, byteArray, fileName) | ||
| } | ||
| withContext(Dispatchers.Main) { |
There was a problem hiding this comment.
In save(), the success toast is shown unconditionally after the openSnapshot(uri) call. If diskCache is null or openSnapshot(uri) returns null (e.g., media not yet cached), this will still display “save success” even though nothing was written. Consider moving the success toast inside the .use { ... } block and adding an ?: run { ... } branch (similar to StatusMediaScreen.shareMedia) to show a “downloading/failed” message instead.
| var fileName = uri.substringBefore("?").substringBefore("#").substringAfterLast("/") | ||
| val lastAt = fileName.lastIndexOf('@') | ||
| val lastDot = fileName.lastIndexOf('.') | ||
| if (lastAt > lastDot && lastAt < fileName.length - 1) { | ||
| fileName = fileName.substring(0, lastAt) + "." + fileName.substring(lastAt + 1) |
There was a problem hiding this comment.
The filename normalization logic (strip query/fragment, convert Bluesky @jpeg suffix, add fallback extension) is duplicated in both save() and share(). Please extract this into a small helper (e.g., fun normalizeFileNameFromUri(uri, fallbackBaseName, mimeType?)) so the two paths can’t drift and future fixes only need to be made once.
| @@ -325,10 +340,28 @@ private fun mediaPresenter( | |||
| scope.launch { | |||
| context.imageLoader.diskCache?.openSnapshot(uri)?.use { | |||
There was a problem hiding this comment.
In share(), openSnapshot(uri) is nullable but there’s no else branch for the null case. Right now sharing silently does nothing if the media isn’t in the disk cache yet (or disk cache is disabled). Add an ?: run { ... } fallback to surface a user-visible message (and avoid proceeding) when the snapshot is unavailable.
| context.imageLoader.diskCache?.openSnapshot(uri)?.use { | |
| val snapshot = | |
| context.imageLoader.diskCache?.openSnapshot(uri) | |
| ?: run { | |
| Toast.makeText( | |
| context, | |
| "Unable to share image. Please try again after it has finished loading.", | |
| Toast.LENGTH_SHORT, | |
| ).show() | |
| return@launch | |
| } | |
| snapshot.use { |
fix #1924