From 865d52154539c5d89c78ea468023a20eaf6fce4d Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 7 Jun 2023 15:41:07 +0200 Subject: [PATCH] Media: address PR review --- build.gradle.kts | 1 + .../impl/media/helper/fileExtensionAndSize.kt | 12 ++- .../impl/media/local/LocalMediaView.kt | 91 ++++++++++--------- .../impl/media/viewer/MediaViewerPresenter.kt | 59 +++++------- .../media/viewer/MediaViewerStateProvider.kt | 4 + .../impl/media/viewer/MediaViewerView.kt | 53 ++++++----- .../components/event/TimelineItemFileView.kt | 2 + 7 files changed, 120 insertions(+), 102 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 7ddcc3dc86..6e34d336bc 100644 --- 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 diff --git 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 index 43d6ba2a9b..fcf64eb24f 100644 --- 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)") diff --git 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 index 502e10e0a4..9683be9aeb 100644 --- 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, + ) + } } } } diff --git 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 index 816e3c5acb..88bf7c91c2 100644 --- 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) = 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) = 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) = 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 { diff --git 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 index 2705072f0d..786ec984b7 100644 --- 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 ), aPdfInfo(), ), + aMediaViewerState( + Async.Loading(), + aFileInfo(), + ), aMediaViewerState( Async.Success( LocalMedia(Uri.EMPTY, aFileInfo()) diff --git 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 index 64beccfa42..afbb9bb331 100644 --- 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): 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, diff --git 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 index 5bc6787e4a..b785734296 100644 --- 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, ) } }