diff --git a/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewScreen.kt b/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewScreen.kt index 97d06c5631..5fd88b9df1 100644 --- a/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewScreen.kt +++ b/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewScreen.kt @@ -64,9 +64,7 @@ import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp -import androidx.core.net.toUri import com.nextcloud.talk.R -import com.nextcloud.talk.utils.FileUtils import kotlinx.coroutines.launch private const val MAX_ADD_MORE_FILES = 10 @@ -89,17 +87,22 @@ internal fun FileAttachmentPreviewContent( onDismiss: () -> Unit, onSend: (files: List, caption: String, compressImages: Boolean, allowUpdate: Boolean) -> Unit ) { - val context = LocalContext.current val currentFiles = viewModel.files - val hasCompressibleMedia = currentFiles.any { isCompressible(FileUtils.resolveMimeType(context, it.toUri())) } var caption by rememberSaveable { mutableStateOf("") } - var compressImages by rememberSaveable { mutableStateOf(hasCompressibleMedia && initialCompressImages) } + var compressImages by rememberSaveable { mutableStateOf(initialCompressImages) } var allowUpdate by rememberSaveable { mutableStateOf(false) } LaunchedEffect(currentFiles.toSet(), compressImages) { viewModel.describeFiles(compressImages) } + LaunchedEffect(currentFiles.toSet()) { + viewModel.resolveMimeTypes() + } val fileDescriptions = currentFiles.mapNotNull { viewModel.descriptionsByUri[it] } + // The type is resolved off the main thread, separately from the full descriptions: asking a content provider + // (e.g. Nextcloud Files) for it during composition would block the main thread on every recomposition, + // while waiting for the descriptions would hold Send back until large files are copied. + val hasCompressibleMedia = currentFiles.any { isCompressible(viewModel.mimeTypesByUri[it]) } val pagerState = rememberPagerState(pageCount = { fileDescriptions.size }) val coroutineScope = rememberCoroutineScope() @@ -180,7 +183,9 @@ internal fun FileAttachmentPreviewContent( caption = caption, onCaptionChange = { caption = it }, sendEnabled = currentFiles.isNotEmpty(), - onSend = { onSend(currentFiles.toList(), caption, compressImages, allowUpdate) } + onSend = { + onSend(currentFiles.toList(), caption, compressImages, allowUpdate) + } ) } } diff --git a/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt b/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt index 94790258ab..f365753a92 100644 --- a/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModel.kt @@ -11,8 +11,13 @@ import androidx.compose.runtime.mutableStateListOf import androidx.compose.runtime.mutableStateMapOf import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import androidx.core.net.toUri +import com.nextcloud.talk.utils.FileUtils +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import javax.inject.Inject /** @@ -21,13 +26,33 @@ import javax.inject.Inject * dialog's original arguments. Everything else on the screen — caption text, drag/scroll * position, HQ toggle animation state — is ephemeral UI state and stays in Compose's `remember`. */ -internal class FileAttachmentPreviewViewModel @Inject constructor(private val context: Context) : ViewModel() { +internal class FileAttachmentPreviewViewModel( + private val context: Context, + private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, + private val mainDispatcher: CoroutineDispatcher = Dispatchers.Main.immediate, + private val describer: (Context, String, Boolean) -> FileDescription = ::describeFile, + private val mimeTypeResolver: (Context, String) -> String? = { ctx, uri -> + FileUtils.resolveMimeType(ctx, uri.toUri()) + } +) : ViewModel() { + + // Dagger cannot use the default values above, so it gets a constructor with the context only + @Inject + constructor(context: Context) : this(context, ioDispatcher = Dispatchers.IO) val files = mutableStateListOf() private val _descriptionsByUri = mutableStateMapOf() val descriptionsByUri: Map get() = _descriptionsByUri + private val _mimeTypesByUri = mutableStateMapOf() + + /** Cheap, type-only counterpart of [descriptionsByUri]; filled before the (slow) full descriptions. */ + val mimeTypesByUri: Map get() = _mimeTypesByUri + + private var describeJob: Job? = null + private var mimeTypeJob: Job? = null + /** No-op after the first call, so re-entering (e.g. after rotation) doesn't wipe edits made since. */ fun setInitialFiles(initialFiles: List) { if (files.isEmpty()) { @@ -56,10 +81,32 @@ internal class FileAttachmentPreviewViewModel @Inject constructor(private val co */ fun describeFiles(compress: Boolean) { val snapshot = files.toList() - viewModelScope.launch(Dispatchers.IO) { - val described = snapshot.associateWith { describeFile(context, it, compress) } - _descriptionsByUri.clear() - _descriptionsByUri.putAll(described) + // a stale job finishing last would retainAll away the description of a file added since + describeJob?.cancel() + describeJob = viewModelScope.launch(ioDispatcher) { + val described = snapshot.associateWith { describer(context, it, compress) } + // the blocking copy above cannot be interrupted, so a cancelled job reaches this point too; + // withContext checks for cancellation and runs on main, where cancel() is also called + withContext(mainDispatcher) { + _descriptionsByUri.putAll(described) + _descriptionsByUri.keys.retainAll(described.keys) + } + } + } + + /** + * Resolves only the MIME type of every current file. Unlike [describeFiles] this does not copy + * content:// files into the cache, so it is quick enough not to hold anything back. + */ + fun resolveMimeTypes() { + val snapshot = files.toList() + mimeTypeJob?.cancel() + mimeTypeJob = viewModelScope.launch(ioDispatcher) { + val resolved = snapshot.associateWith { mimeTypeResolver(context, it) } + withContext(mainDispatcher) { + _mimeTypesByUri.putAll(resolved) + _mimeTypesByUri.keys.retainAll(resolved.keys) + } } } } diff --git a/app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt new file mode 100644 index 0000000000..7becb4d81d --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/attachmentpreview/FileAttachmentPreviewViewModelTest.kt @@ -0,0 +1,124 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.attachmentpreview + +import android.app.Application +import android.content.Context +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import org.mockito.Mockito.mock +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class FileAttachmentPreviewViewModelTest { + + private val testScope = TestScope() + private val described = mutableListOf() + private lateinit var viewModel: FileAttachmentPreviewViewModel + + @Before + fun setUp() { + viewModel = FileAttachmentPreviewViewModel( + context = mock(Context::class.java), + ioDispatcher = StandardTestDispatcher(testScope.testScheduler), + mainDispatcher = StandardTestDispatcher(testScope.testScheduler), + describer = { _, uri, _ -> + described.add(uri) + FileDescription(uri, uri, MediaKind.OTHER, "text/plain", null) + }, + mimeTypeResolver = { _, uri -> if (uri.endsWith(".mp4")) "video/mp4" else "text/plain" } + ) + } + + @Test + fun `mime types are resolved without describing the files`() { + viewModel.setInitialFiles(listOf("content://a/clip.mp4")) + + viewModel.resolveMimeTypes() + testScope.advanceUntilIdle() + + assertEquals("video/mp4", viewModel.mimeTypesByUri["content://a/clip.mp4"]) + assertTrue(viewModel.descriptionsByUri.isEmpty()) + assertTrue(described.isEmpty()) + } + + @Test + fun `mime types of removed files are dropped`() { + viewModel.setInitialFiles(listOf("content://a/1.txt", "content://a/2.txt")) + viewModel.resolveMimeTypes() + testScope.advanceUntilIdle() + + viewModel.removeFile("content://a/1.txt") + viewModel.resolveMimeTypes() + testScope.advanceUntilIdle() + + assertEquals(setOf("content://a/2.txt"), viewModel.mimeTypesByUri.keys) + } + + @Test + fun `a newer describeFiles call cancels the previous job`() { + viewModel.setInitialFiles(listOf("content://a/1.txt")) + viewModel.describeFiles(true) + viewModel.addFiles(listOf("content://a/2.txt")) + viewModel.describeFiles(true) + testScope.advanceUntilIdle() + + assertEquals(listOf("content://a/1.txt", "content://a/2.txt"), described) + assertEquals(setOf("content://a/1.txt", "content://a/2.txt"), viewModel.descriptionsByUri.keys) + } + + @Test + fun `a cancelled describeFiles job that is still copying does not drop a newer description`() { + val started = CountDownLatch(1) + val release = CountDownLatch(1) + viewModel = FileAttachmentPreviewViewModel( + context = mock(Context::class.java), + ioDispatcher = kotlinx.coroutines.Dispatchers.IO, + mainDispatcher = StandardTestDispatcher(testScope.testScheduler), + describer = { _, uri, _ -> + if (uri == "content://a/1.txt" && started.count == 1L) { + started.countDown() + release.await(5, TimeUnit.SECONDS) + } + FileDescription(uri, uri, MediaKind.OTHER, "text/plain", null) + } + ) + viewModel.setInitialFiles(listOf("content://a/1.txt")) + viewModel.describeFiles(true) + assertTrue(started.await(5, TimeUnit.SECONDS)) + + viewModel.addFiles(listOf("content://a/2.txt")) + viewModel.describeFiles(true) + Thread.sleep(300) + testScope.advanceUntilIdle() + assertEquals( + setOf("content://a/1.txt", "content://a/2.txt"), + viewModel.descriptionsByUri.keys + ) + + release.countDown() + Thread.sleep(300) + testScope.advanceUntilIdle() + + assertEquals( + setOf("content://a/1.txt", "content://a/2.txt"), + viewModel.descriptionsByUri.keys + ) + } +}