Apply suggestion:

- Added `MediaSource.safeUrl` property replacing `withCleanUrl` method.
- Made `url` private so it can't be used externally.
- Reverted code in `CoilMediaFetcher`
- Also add tests
This commit is contained in:
Jorge Martín
2026-02-27 09:33:03 +01:00
parent 7fe0cc4d45
commit cdd850d4dd
9 changed files with 50 additions and 41 deletions

View File

@@ -30,5 +30,5 @@ data class TimelineItemStickerContent(
/* Stickers are supposed to be small images so /* Stickers are supposed to be small images so
we allow using the mediaSource (unless the url is empty) */ we allow using the mediaSource (unless the url is empty) */
val preferredMediaSource = if (mediaSource.url.isEmpty()) thumbnailSource else mediaSource val preferredMediaSource = if (mediaSource.safeUrl.isEmpty()) thumbnailSource else mediaSource
} }

View File

@@ -9,7 +9,7 @@
package io.element.android.libraries.matrix.api.media package io.element.android.libraries.matrix.api.media
import android.os.Parcelable import android.os.Parcelable
import androidx.core.net.toUri import kotlinx.parcelize.IgnoredOnParcel
import kotlinx.parcelize.Parcelize import kotlinx.parcelize.Parcelize
@Parcelize @Parcelize
@@ -17,31 +17,20 @@ data class MediaSource(
/** /**
* Url of the media. * Url of the media.
*/ */
val url: String, private val url: String,
/** /**
* This is used to hold data for encrypted media. * This is used to hold data for encrypted media.
*/ */
val json: String? = null, val json: String? = null,
) : Parcelable ) : Parcelable {
/** /**
* Returns a new [MediaSource] with a valid URL. * A URL with invalid parts (like `#fragment`, if it's an MXC url) removed.
*/ */
fun MediaSource.withCleanUrl(): MediaSource { @IgnoredOnParcel
val uri = this.url.toUri() val safeUrl = if (url.startsWith("mxc")) {
if (uri.scheme != "mxc") return this
// We've seen some MXC urls in the wild having some `mxc://foo/bar#auto` fragment suffix, which is invalid // We've seen some MXC urls in the wild having some `mxc://foo/bar#auto` fragment suffix, which is invalid
val cleanedUrl = buildString { url.substringBefore("#")
append(uri.scheme) } else {
if (!this.endsWith("://")) { url
append("://")
}
append(uri.host)
if (uri.path != null) {
append(uri.path)
} }
} }
return this.copy(url = cleanedUrl)
}

View File

@@ -0,0 +1,26 @@
/*
* Copyright (c) 2026 Element Creations Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial.
* Please see LICENSE files in the repository root for full details.
*/
package io.element.android.libraries.matrix.api.media
import com.google.common.truth.Truth.assertThat
import io.element.android.libraries.matrix.test.media.aMediaSource
import org.junit.Test
class MediaSourceTest {
@Test
fun `safeUrl removes the fragment part in MXC urls`() {
val mediaSource = aMediaSource(url = "mxc://matrix.org/url#fragment")
assertThat(mediaSource.safeUrl).isEqualTo("mxc://matrix.org/url")
}
@Test
fun `safeUrl keeps the fragment part in a non-MXC url`() {
val mediaSource = aMediaSource(url = "https://matrix.org/url#fragment")
assertThat(mediaSource.safeUrl).isEqualTo("https://matrix.org/url#fragment")
}
}

View File

@@ -14,7 +14,6 @@ import io.element.android.libraries.core.mimetype.MimeTypes
import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MatrixMediaLoader
import io.element.android.libraries.matrix.api.media.MediaFile import io.element.android.libraries.matrix.api.media.MediaFile
import io.element.android.libraries.matrix.api.media.MediaSource import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.api.media.withCleanUrl
import kotlinx.coroutines.withContext import kotlinx.coroutines.withContext
import org.matrix.rustcomponents.sdk.Client import org.matrix.rustcomponents.sdk.Client
import org.matrix.rustcomponents.sdk.use import org.matrix.rustcomponents.sdk.use
@@ -92,7 +91,7 @@ class RustMediaLoader(
return if (json != null) { return if (json != null) {
RustMediaSource.fromJson(json) RustMediaSource.fromJson(json)
} else { } else {
RustMediaSource.fromUrl(withCleanUrl().url) RustMediaSource.fromUrl(safeUrl)
} }
} }
} }

View File

@@ -16,7 +16,6 @@ import coil3.fetch.SourceFetchResult
import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MatrixMediaLoader
import io.element.android.libraries.matrix.api.media.MediaSource import io.element.android.libraries.matrix.api.media.MediaSource
import io.element.android.libraries.matrix.api.media.toFile import io.element.android.libraries.matrix.api.media.toFile
import io.element.android.libraries.matrix.api.media.withCleanUrl
import okio.Buffer import okio.Buffer
import okio.FileSystem import okio.FileSystem
import okio.Path.Companion.toOkioPath import okio.Path.Companion.toOkioPath
@@ -28,15 +27,11 @@ internal class CoilMediaFetcher(
private val mediaData: MediaRequestData, private val mediaData: MediaRequestData,
) : Fetcher { ) : Fetcher {
override suspend fun fetch(): FetchResult? { override suspend fun fetch(): FetchResult? {
val source = mediaData.source val mediaSource = mediaData.source
val mediaSource = when { if (mediaSource == null) {
source == null -> {
Timber.e("MediaData source is null") Timber.e("MediaData source is null")
return null return null
} }
source.url.startsWith("mxc:") -> source.withCleanUrl()
else -> source
}
return when (val kind = mediaData.kind) { return when (val kind = mediaData.kind) {
is MediaRequestData.Kind.Content -> fetchContent(mediaSource) is MediaRequestData.Kind.Content -> fetchContent(mediaSource)
is MediaRequestData.Kind.Thumbnail -> fetchThumbnail(mediaSource, kind) is MediaRequestData.Kind.Thumbnail -> fetchThumbnail(mediaSource, kind)

View File

@@ -25,5 +25,5 @@ internal class MediaRequestDataKeyer : Keyer<MediaRequestData> {
} }
private fun MediaRequestData.toKey(): String? { private fun MediaRequestData.toKey(): String? {
return source?.let { "${it.url}_$kind" } return source?.let { "${it.safeUrl}_$kind" }
} }

View File

@@ -126,7 +126,7 @@ class MediaViewerDataSource(
when (mediaItem) { when (mediaItem) {
is MediaItem.DateSeparator -> Unit is MediaItem.DateSeparator -> Unit
is MediaItem.Event -> { is MediaItem.Event -> {
val sourceUrl = mediaItem.mediaSource().url val sourceUrl = mediaItem.mediaSource().safeUrl
val localMedia = localMediaStates.getOrPut(sourceUrl) { val localMedia = localMediaStates.getOrPut(sourceUrl) {
mutableStateOf(AsyncData.Uninitialized) mutableStateOf(AsyncData.Uninitialized)
} }
@@ -153,7 +153,7 @@ class MediaViewerDataSource(
}.toImmutableList() }.toImmutableList()
fun clearLoadingError(data: MediaViewerPageData.MediaViewerData) { fun clearLoadingError(data: MediaViewerPageData.MediaViewerData) {
localMediaStates[data.mediaSource.url]?.value = AsyncData.Uninitialized localMediaStates[data.mediaSource.safeUrl]?.value = AsyncData.Uninitialized
} }
suspend fun loadMore(direction: Timeline.PaginationDirection) { suspend fun loadMore(direction: Timeline.PaginationDirection) {
@@ -162,7 +162,7 @@ class MediaViewerDataSource(
suspend fun loadMedia(data: MediaViewerPageData.MediaViewerData) { suspend fun loadMedia(data: MediaViewerPageData.MediaViewerData) {
Timber.d("loadMedia for ${data.eventId}") Timber.d("loadMedia for ${data.eventId}")
val localMediaState = localMediaStates.getOrPut(data.mediaSource.url) { val localMediaState = localMediaStates.getOrPut(data.mediaSource.safeUrl) {
mutableStateOf(AsyncData.Uninitialized) mutableStateOf(AsyncData.Uninitialized)
} }
localMediaState.value = AsyncData.Loading() localMediaState.value = AsyncData.Loading()

View File

@@ -101,7 +101,7 @@ class DefaultNotificationMediaRepo(
} }
} }
private fun MediaSource.cachedFile(): File? = mxcTools.mxcUri2FilePath(url)?.let { private fun MediaSource.cachedFile(): File? = mxcTools.mxcUri2FilePath(safeUrl)?.let {
File("${cacheDir.path}/$CACHE_NOTIFICATION_SUBDIR/$it") File("${cacheDir.path}/$CACHE_NOTIFICATION_SUBDIR/$it")
} }
} }

View File

@@ -95,7 +95,7 @@ class DefaultVoiceMessageMediaRepo(
} }
} }
private val cachedFile: File? = mxcTools.mxcUri2FilePath(mediaSource.url)?.let { private val cachedFile: File? = mxcTools.mxcUri2FilePath(mediaSource.safeUrl)?.let {
File("${cacheDir.path}/$CACHE_VOICE_SUBDIR/$it") File("${cacheDir.path}/$CACHE_VOICE_SUBDIR/$it")
} }
} }