Media: address PR review

2023-06-07 15:41:07 +02:00
parent b72291a91d
commit 865d521545
7 changed files with 120 additions and 102 deletions
--- a/build.gradle.kts
+++ b/build.gradle.kts
@@ -224,6 +224,7 @@ koverMerged {
                excludes += "io.element.android.libraries.matrix.api.room.MatrixRoomMembersState*"
                excludes += "io.element.android.libraries.push.impl.notifications.NotificationState*"
                excludes += "io.element.android.features.messages.impl.media.local.pdf.PdfViewerState"
+                excludes += "io.element.android.features.messages.impl.media.local.LocalMediaViewState"
            }
            bound {
                minValue = 90
--- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/helper/fileExtensionAndSize.kt
+++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/helper/fileExtensionAndSize.kt
@@ -16,10 +16,18 @@

 package io.element.android.features.messages.impl.media.helper

+import android.webkit.MimeTypeMap
+
 fun formatFileExtensionAndSize(name: String, size: String?): String {
-    val fileExtension = name.substringAfterLast('.', "").uppercase()
+    val fileExtension = name.substringAfterLast('.', "")
+    // Makes sure the extension is known by the system, otherwise default to binary extension.
+    val safeExtension = if (MimeTypeMap.getSingleton().hasExtension(fileExtension)) {
+        fileExtension.uppercase()
+    } else {
+        "BIN"
+    }
    return buildString {
-        append(fileExtension)
+        append(safeExtension)
        if (size != null) {
            append(' ')
            append("($size)")
--- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/local/LocalMediaView.kt
+++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/local/LocalMediaView.kt
@@ -34,7 +34,11 @@ import androidx.compose.material.icons.outlined.Attachment
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
+import androidx.compose.runtime.Stable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.draw.clip
@@ -70,40 +74,52 @@ import me.saket.telephoto.zoomable.coil.ZoomableAsyncImage
 import me.saket.telephoto.zoomable.rememberZoomableImageState
 import me.saket.telephoto.zoomable.rememberZoomableState

+@Stable
+class LocalMediaViewState {
+    var isReady: Boolean by mutableStateOf(false)
+}
+
+@Composable
+fun rememberLocalMediaViewState(): LocalMediaViewState {
+    return remember {
+        LocalMediaViewState()
+    }
+}
+
@SuppressLint("UnsafeOptInUsageError")
@Composable
 fun LocalMediaView(
    localMedia: LocalMedia?,
    modifier: Modifier = Modifier,
-    info: MediaInfo? = localMedia?.info,
-    onReady: () -> Unit = {},
+    localMediaViewState: LocalMediaViewState = rememberLocalMediaViewState(),
+    mediaInfo: MediaInfo? = localMedia?.info,
 ) {
    val zoomableState = rememberZoomableState(
        zoomSpec = ZoomSpec(maxZoomFactor = 5f)
    )
-    val mimeType = info?.mimeType
+    val mimeType = mediaInfo?.mimeType
    when {
        mimeType.isMimeTypeImage() -> MediaImageView(
+            localMediaViewState = localMediaViewState,
            localMedia = localMedia,
            zoomableState = zoomableState,
-            onReady = onReady,
            modifier = modifier
        )
        mimeType.isMimeTypeVideo() -> MediaVideoView(
+            localMediaViewState = localMediaViewState,
            localMedia = localMedia,
-            onReady = onReady,
            modifier = modifier
        )
        mimeType == MimeTypes.Pdf -> MediaPDFView(
+            localMediaViewState = localMediaViewState,
            localMedia = localMedia,
            zoomableState = zoomableState,
-            onReady = onReady,
            modifier = modifier
        )
        else -> MediaFileView(
+            localMediaViewState = localMediaViewState,
            uri = localMedia?.uri,
-            info = info,
-            onReady = onReady,
+            info = mediaInfo,
            modifier = modifier
        )
    }
@@ -111,9 +127,9 @@ fun LocalMediaView(

@Composable
 private fun MediaImageView(
+    localMediaViewState: LocalMediaViewState,
    localMedia: LocalMedia?,
    zoomableState: ZoomableState,
-    onReady: () -> Unit,
    modifier: Modifier = Modifier,
 ) {
    if (LocalInspectionMode.current) {
@@ -124,11 +140,7 @@ private fun MediaImageView(
        )
    } else {
        val zoomableImageState = rememberZoomableImageState(zoomableState)
-        LaunchedEffect(zoomableImageState.isImageDisplayed) {
-            if (zoomableImageState.isImageDisplayed) {
-                onReady()
-            }
-        }
+        localMediaViewState.isReady = zoomableImageState.isImageDisplayed
        ZoomableAsyncImage(
            modifier = modifier.fillMaxSize(),
            state = zoomableImageState,
@@ -142,14 +154,14 @@ private fun MediaImageView(
@UnstableApi
@Composable
 fun MediaVideoView(
+    localMediaViewState: LocalMediaViewState,
    localMedia: LocalMedia?,
-    onReady: () -> Unit,
    modifier: Modifier = Modifier,
 ) {
    val context = LocalContext.current
    val playerListener = object : Player.Listener {
        override fun onRenderedFirstFrame() {
-            onReady()
+            localMediaViewState.isReady = true
        }
    }
    val exoPlayer = remember {
@@ -196,35 +208,27 @@ fun MediaVideoView(

@Composable
 fun MediaPDFView(
+    localMediaViewState: LocalMediaViewState,
    localMedia: LocalMedia?,
    zoomableState: ZoomableState,
-    onReady: () -> Unit,
    modifier: Modifier = Modifier,
 ) {
    val pdfViewerState = rememberPdfViewerState(
        model = localMedia?.uri,
        zoomableState = zoomableState
    )
-    LaunchedEffect(pdfViewerState.isLoaded) {
-        if (pdfViewerState.isLoaded) {
-            onReady()
-        }
-    }
+    localMediaViewState.isReady = pdfViewerState.isLoaded
    PdfViewer(pdfViewerState = pdfViewerState, modifier = modifier)
 }

@Composable
 fun MediaFileView(
+    localMediaViewState: LocalMediaViewState,
    uri: Uri?,
    info: MediaInfo?,
-    onReady: () -> Unit,
    modifier: Modifier = Modifier,
 ) {
-    LaunchedEffect(Unit) {
-        if(uri != null) {
-            onReady()
-        }
-    }
+    localMediaViewState.isReady = uri != null
    Box(modifier = modifier, contentAlignment = Alignment.Center) {
        Column(horizontalAlignment = Alignment.CenterHorizontally) {
            Box(
@@ -236,26 +240,29 @@ fun MediaFileView(
            ) {
                Icon(
                    imageVector = Icons.Outlined.Attachment,
-                    contentDescription = "OpenFile",
+                    contentDescription = null,
                    tint = MaterialTheme.colorScheme.background,
                    modifier = Modifier
                        .size(32.dp)
                        .rotate(-45f),
                )
            }
-            if(info == null) return
-            Spacer(modifier = Modifier.height(16.dp))
-            Text(
-                text = info.name,
-                maxLines = 2,
-                fontSize = 16.sp,
-                overflow = TextOverflow.Ellipsis
-            )
-            Spacer(modifier = Modifier.height(4.dp))
-            Text(
-                text = formatFileExtensionAndSize(info.name, info.formattedFileSize),
-                fontSize = 14.sp,
-            )
+            if (info != null) {
+                Spacer(modifier = Modifier.height(16.dp))
+                Text(
+                    text = info.name,
+                    maxLines = 2,
+                    fontSize = 16.sp,
+                    overflow = TextOverflow.Ellipsis
+                )
+                Spacer(modifier = Modifier.height(4.dp))
+                Text(
+                    text = formatFileExtensionAndSize(info.name, info.formattedFileSize),
+                    fontSize = 14.sp,
+                    maxLines = 1,
+                    overflow = TextOverflow.Ellipsis,
+                )
+            }
        }
    }
 }
--- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/viewer/MediaViewerPresenter.kt
+++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/viewer/MediaViewerPresenter.kt
@@ -116,46 +116,37 @@ class MediaViewerPresenter @AssistedInject constructor(
    }

    private fun CoroutineScope.saveOnDisk(localMedia: Async<LocalMedia>) = launch {
-        when (localMedia) {
-            is Async.Success -> {
-                localMediaActions.saveOnDisk(localMedia.state)
-                    .onSuccess {
-                        val snackbarMessage = SnackbarMessage(StringR.string.common_file_saved_on_disk_android)
-                        snackbarDispatcher.post(snackbarMessage)
-                    }
-                    .onFailure {
-                        val snackbarMessage = SnackbarMessage(mediaActionsError(it))
-                        snackbarDispatcher.post(snackbarMessage)
-                    }
-            }
-            else -> Unit
-        }
+        if (localMedia is Async.Success) {
+            localMediaActions.saveOnDisk(localMedia.state)
+                .onSuccess {
+                    val snackbarMessage = SnackbarMessage(StringR.string.common_file_saved_on_disk_android)
+                    snackbarDispatcher.post(snackbarMessage)
+                }
+                .onFailure {
+                    val snackbarMessage = SnackbarMessage(mediaActionsError(it))
+                    snackbarDispatcher.post(snackbarMessage)
+                }
+        } else Unit
    }

    private fun CoroutineScope.share(localMedia: Async<LocalMedia>) = launch {
-        when (localMedia) {
-            is Async.Success -> {
-                localMediaActions.share(localMedia.state)
-                    .onFailure {
-                        val snackbarMessage = SnackbarMessage(mediaActionsError(it))
-                        snackbarDispatcher.post(snackbarMessage)
-                    }
-            }
-            else -> Unit
-        }
+        if (localMedia is Async.Success) {
+            localMediaActions.share(localMedia.state)
+                .onFailure {
+                    val snackbarMessage = SnackbarMessage(mediaActionsError(it))
+                    snackbarDispatcher.post(snackbarMessage)
+                }
+        } else Unit
    }

    private fun CoroutineScope.open(localMedia: Async<LocalMedia>) = launch {
-        when (localMedia) {
-            is Async.Success -> {
-                localMediaActions.open(localMedia.state)
-                    .onFailure {
-                        val snackbarMessage = SnackbarMessage(mediaActionsError(it))
-                        snackbarDispatcher.post(snackbarMessage)
-                    }
-            }
-            else -> Unit
-        }
+        if (localMedia is Async.Success) {
+            localMediaActions.open(localMedia.state)
+                .onFailure {
+                    val snackbarMessage = SnackbarMessage(mediaActionsError(it))
+                    snackbarDispatcher.post(snackbarMessage)
+                }
+        } else Unit
    }

    private fun mediaActionsError(throwable: Throwable): Int {
--- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/viewer/MediaViewerStateProvider.kt
+++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/viewer/MediaViewerStateProvider.kt
@@ -50,6 +50,10 @@ open class MediaViewerStateProvider : PreviewParameterProvider<MediaViewerState>
                ),
                aPdfInfo(),
            ),
+            aMediaViewerState(
+                Async.Loading(),
+                aFileInfo(),
+            ),
            aMediaViewerState(
                Async.Success(
                    LocalMedia(Uri.EMPTY, aFileInfo())
--- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/viewer/MediaViewerView.kt
+++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/media/viewer/MediaViewerView.kt
@@ -46,12 +46,15 @@ import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.layout.ContentScale
+import androidx.compose.ui.platform.LocalInspectionMode
 import androidx.compose.ui.res.stringResource
 import androidx.compose.ui.tooling.preview.Preview
 import androidx.compose.ui.tooling.preview.PreviewParameter
 import androidx.compose.ui.unit.dp
 import coil.compose.AsyncImage
+import io.element.android.features.messages.impl.media.local.LocalMedia
 import io.element.android.features.messages.impl.media.local.LocalMediaView
+import io.element.android.features.messages.impl.media.local.rememberLocalMediaViewState
 import io.element.android.libraries.architecture.Async
 import io.element.android.libraries.architecture.isLoading
 import io.element.android.libraries.designsystem.components.button.BackButton
@@ -82,28 +85,9 @@ fun MediaViewerView(
        state.eventSink(MediaViewerEvents.ClearLoadingError)
    }

-    var showProgress by remember {
-        mutableStateOf(false)
-    }
-
-    // Trick to avoid showing progress indicator if the media is already on disk.
-    // When sdk will expose download progress we'll be able to remove this.
-    LaunchedEffect(state.downloadedMedia) {
-        showProgress = false
-        delay(100)
-        if (state.downloadedMedia.isLoading()) {
-            showProgress = true
-        }
-    }
-
-    var showThumbnail by remember {
-        mutableStateOf(true)
-    }
-
-    fun onMediaReady() {
-        showThumbnail = false
-    }
-
+    val localMediaViewState = rememberLocalMediaViewState()
+    val showThumbnail = !localMediaViewState.isReady
+    val showProgress = rememberShowProgress(state.downloadedMedia)
    val snackbarHostState = rememberSnackbarHostState(snackbarMessage = state.snackbarMessage)

    Scaffold(
@@ -151,9 +135,9 @@ fun MediaViewerView(
                    )
                }
                LocalMediaView(
+                    localMediaViewState = localMediaViewState,
                    localMedia = state.downloadedMedia.dataOrNull(),
-                    info = state.mediaInfo,
-                    onReady = ::onMediaReady
+                    mediaInfo = state.mediaInfo,
                )
                ThumbnailView(
                    thumbnailSource = state.thumbnailSource,
@@ -164,6 +148,27 @@ fun MediaViewerView(
    }
 }

+@Composable
+private fun rememberShowProgress(downloadedMedia: Async<LocalMedia>): Boolean {
+    var showProgress by remember {
+        mutableStateOf(false)
+    }
+    if (LocalInspectionMode.current) {
+        showProgress = downloadedMedia.isLoading()
+    } else {
+        // Trick to avoid showing progress indicator if the media is already on disk.
+        // When sdk will expose download progress we'll be able to remove this.
+        LaunchedEffect(downloadedMedia) {
+            showProgress = false
+            delay(100)
+            if (downloadedMedia.isLoading()) {
+                showProgress = true
+            }
+        }
+    }
+    return showProgress
+}
+
@Composable
 private fun MediaViewerTopBar(
    actionsEnabled: Boolean,
--- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemFileView.kt
+++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/components/event/TimelineItemFileView.kt
@@ -79,6 +79,8 @@ fun TimelineItemFileView(
                text = content.fileExtensionAndSize,
                color = MaterialTheme.colorScheme.secondary,
                fontSize = 12.sp,
+                maxLines = 1,
+                overflow = TextOverflow.Ellipsis,
            )
        }
    }