From 4601e2acd33c67bd60920cb4b9351946941dba8d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 25 Jan 2024 10:19:56 +0100 Subject: [PATCH] Change type of `ViewFileState.lines` from `ImmutableList` to `AsyncData>` to properly handle loading and error states. --- features/viewfolder/impl/build.gradle.kts | 1 + .../viewfolder/impl/file/FileContentReader.kt | 11 +--- .../viewfolder/impl/file/ViewFilePresenter.kt | 11 ++-- .../viewfolder/impl/file/ViewFileState.kt | 4 +- .../impl/file/ViewFileStateProvider.kt | 33 ++++++---- .../viewfolder/impl/file/ViewFileView.kt | 65 ++++++++++++------- .../test/file/FakeFileContentReader.kt | 6 +- .../test/file/ViewFilePresenterTest.kt | 31 +++++++-- 8 files changed, 102 insertions(+), 60 deletions(-) diff --git a/features/viewfolder/impl/build.gradle.kts b/features/viewfolder/impl/build.gradle.kts index d3e838e1f5..67c818f185 100644 --- a/features/viewfolder/impl/build.gradle.kts +++ b/features/viewfolder/impl/build.gradle.kts @@ -47,4 +47,5 @@ dependencies { testImplementation(libs.test.truth) testImplementation(libs.test.turbine) testImplementation(projects.tests.testutils) + testImplementation(projects.libraries.matrix.test) } diff --git a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/FileContentReader.kt b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/FileContentReader.kt index 877f841fcb..bc723f70cb 100644 --- a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/FileContentReader.kt +++ b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/FileContentReader.kt @@ -24,21 +24,16 @@ import java.io.File import javax.inject.Inject interface FileContentReader { - suspend fun getLines(path: String): List + suspend fun getLines(path: String): Result> } @ContributesBinding(AppScope::class) class DefaultFileContentReader @Inject constructor( private val dispatchers: CoroutineDispatchers, ) : FileContentReader { - override suspend fun getLines(path: String): List = withContext(dispatchers.io) { - try { + override suspend fun getLines(path: String): Result> = withContext(dispatchers.io) { + runCatching { File(path).readLines() - } catch (exception: Exception) { - buildList { - add("Error reading file $path") - add(exception.toString()) - } } } } diff --git a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFilePresenter.kt b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFilePresenter.kt index 1a1e5ba3dd..91cc59ec05 100644 --- a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFilePresenter.kt +++ b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFilePresenter.kt @@ -26,8 +26,8 @@ import androidx.compose.runtime.setValue import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject +import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.architecture.Presenter -import kotlinx.collections.immutable.toImmutableList import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch @@ -57,13 +57,16 @@ class ViewFilePresenter @AssistedInject constructor( } } - var lines by remember { mutableStateOf(emptyList()) } + var lines: AsyncData> by remember { mutableStateOf(AsyncData.Loading()) } LaunchedEffect(Unit) { - lines = fileContentReader.getLines(path) + lines = fileContentReader.getLines(path).fold( + onSuccess = { AsyncData.Success(it) }, + onFailure = { AsyncData.Failure(it) } + ) } return ViewFileState( name = name, - lines = lines.toImmutableList(), + lines = lines, eventSink = ::handleEvent, ) } diff --git a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileState.kt b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileState.kt index 9971c4b7d0..9fb531c478 100644 --- a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileState.kt +++ b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileState.kt @@ -16,10 +16,10 @@ package io.element.android.features.viewfolder.impl.file -import kotlinx.collections.immutable.ImmutableList +import io.element.android.libraries.architecture.AsyncData data class ViewFileState( val name: String, - val lines: ImmutableList, + val lines: AsyncData>, val eventSink: (ViewFileEvents) -> Unit, ) diff --git a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileStateProvider.kt b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileStateProvider.kt index 9687e82405..f0612c0714 100644 --- a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileStateProvider.kt +++ b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileStateProvider.kt @@ -17,24 +17,29 @@ package io.element.android.features.viewfolder.impl.file import androidx.compose.ui.tooling.preview.PreviewParameterProvider -import kotlinx.collections.immutable.toImmutableList +import io.element.android.libraries.architecture.AsyncData open class ViewFileStateProvider : PreviewParameterProvider { override val values: Sequence get() = sequenceOf( aViewFileState(), + aViewFileState(lines = AsyncData.Loading()), + aViewFileState(lines = AsyncData.Failure(Exception("A failure"))), + aViewFileState(lines = AsyncData.Success(emptyList())), aViewFileState( - lines = listOf( - "Line 1", - "Line 2", - "Line 3 lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor" + - " incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam,", - "01-23 13:14:50.740 25818 25818 V verbose", - "01-23 13:14:50.740 25818 25818 D debug", - "01-23 13:14:50.740 25818 25818 I info", - "01-23 13:14:50.740 25818 25818 W warning", - "01-23 13:14:50.740 25818 25818 E error", - "01-23 13:14:50.740 25818 25818 A assertion", + lines = AsyncData.Success( + listOf( + "Line 1", + "Line 2", + "Line 3 lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor" + + " incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam,", + "01-23 13:14:50.740 25818 25818 V verbose", + "01-23 13:14:50.740 25818 25818 D debug", + "01-23 13:14:50.740 25818 25818 I info", + "01-23 13:14:50.740 25818 25818 W warning", + "01-23 13:14:50.740 25818 25818 E error", + "01-23 13:14:50.740 25818 25818 A assertion", + ) ) ) ) @@ -42,9 +47,9 @@ open class ViewFileStateProvider : PreviewParameterProvider { fun aViewFileState( name: String = "aName", - lines: List = emptyList(), + lines: AsyncData> = AsyncData.Uninitialized, ) = ViewFileState( name = name, - lines = lines.toImmutableList(), + lines = lines, eventSink = {}, ) diff --git a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileView.kt b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileView.kt index 265f6892ad..f98c13200c 100644 --- a/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileView.kt +++ b/features/viewfolder/impl/src/main/kotlin/io/element/android/features/viewfolder/impl/file/ViewFileView.kt @@ -41,6 +41,9 @@ import androidx.compose.ui.tooling.preview.PreviewParameter import androidx.compose.ui.unit.dp import io.element.android.compound.theme.ElementTheme import io.element.android.libraries.androidutils.system.copyToClipboard +import io.element.android.libraries.architecture.AsyncData +import io.element.android.libraries.designsystem.components.async.AsyncFailure +import io.element.android.libraries.designsystem.components.async.AsyncLoading import io.element.android.libraries.designsystem.components.button.BackButton import io.element.android.libraries.designsystem.icons.CompoundDrawables import io.element.android.libraries.designsystem.preview.ElementPreview @@ -103,35 +106,51 @@ fun ViewFileView( .padding(padding) .consumeWindowInsets(padding) ) { - LazyColumn( - modifier = Modifier.weight(1f) - ) { - if (state.lines.isEmpty()) { - item { - Spacer(Modifier.size(80.dp)) - Text( - text = "Empty file", - textAlign = TextAlign.Center, - color = MaterialTheme.colorScheme.tertiary, - modifier = Modifier.fillMaxWidth() - ) - } - } else { - itemsIndexed( - items = state.lines, - ) { index, line -> - LineRow( - lineNumber = index + 1, - line = line, - ) - } - } + when (state.lines) { + AsyncData.Uninitialized, + is AsyncData.Loading -> AsyncLoading() + is AsyncData.Success -> FileContent( + modifier = modifier.weight(1f), + lines = state.lines.data, + ) + is AsyncData.Failure -> AsyncFailure(throwable = state.lines.error, onRetry = null) } } } ) } +@Composable +private fun FileContent( + lines: List, + modifier: Modifier = Modifier, +) { + LazyColumn( + modifier = modifier + ) { + if (lines.isEmpty()) { + item { + Spacer(Modifier.size(80.dp)) + Text( + text = "Empty file", + textAlign = TextAlign.Center, + color = MaterialTheme.colorScheme.tertiary, + modifier = Modifier.fillMaxWidth() + ) + } + } else { + itemsIndexed( + items = lines, + ) { index, line -> + LineRow( + lineNumber = index + 1, + line = line, + ) + } + } + } +} + @Composable private fun LineRow( lineNumber: Int, diff --git a/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/FakeFileContentReader.kt b/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/FakeFileContentReader.kt index 9a8d51e329..5d8b658587 100644 --- a/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/FakeFileContentReader.kt +++ b/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/FakeFileContentReader.kt @@ -19,11 +19,11 @@ package io.element.android.features.viewfolder.test.file import io.element.android.features.viewfolder.impl.file.FileContentReader class FakeFileContentReader : FileContentReader { - private var result: List = emptyList() + private var result: Result> = Result.success(emptyList()) - fun givenResult(result: List) { + fun givenResult(result: Result>) { this.result = result } - override suspend fun getLines(path: String): List = result + override suspend fun getLines(path: String): Result> = result } diff --git a/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/ViewFilePresenterTest.kt b/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/ViewFilePresenterTest.kt index 868a812eb2..3b593410b2 100644 --- a/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/ViewFilePresenterTest.kt +++ b/features/viewfolder/impl/src/test/kotlin/io/element/android/features/viewfolder/test/file/ViewFilePresenterTest.kt @@ -25,6 +25,8 @@ import io.element.android.features.viewfolder.impl.file.FileSave import io.element.android.features.viewfolder.impl.file.FileShare import io.element.android.features.viewfolder.impl.file.ViewFileEvents import io.element.android.features.viewfolder.impl.file.ViewFilePresenter +import io.element.android.libraries.architecture.AsyncData +import io.element.android.libraries.matrix.test.AN_EXCEPTION import io.element.android.tests.testutils.WarmUpRule import kotlinx.coroutines.test.runTest import org.junit.Rule @@ -37,24 +39,26 @@ class ViewFilePresenterTest { @Test fun `present - initial state`() = runTest { val fileContentReader = FakeFileContentReader().apply { - givenResult(listOf("aLine")) + givenResult(Result.success(listOf("aLine"))) } val presenter = createPresenter(fileContentReader = fileContentReader) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(1) val initialState = awaitItem() assertThat(initialState.name).isEqualTo("aName") - assertThat(initialState.lines.size).isEqualTo(1) - assertThat(initialState.lines.first()).isEqualTo("aLine") + assertThat(initialState.lines).isInstanceOf(AsyncData.Loading::class.java) + val loadedState = awaitItem() + val lines = (loadedState.lines as AsyncData.Success).data + assertThat(lines.size).isEqualTo(1) + assertThat(lines.first()).isEqualTo("aLine") } } @Test fun `present - share should not have any side effect`() = runTest { val fileContentReader = FakeFileContentReader().apply { - givenResult(listOf("aLine")) + givenResult(Result.success(listOf("aLine"))) } val fileShare = FakeFileShare() val fileSave = FakeFileSave() @@ -70,10 +74,25 @@ class ViewFilePresenterTest { } } + @Test + fun `present - with error loading file`() = runTest { + val fileContentReader = FakeFileContentReader().apply { + givenResult(Result.failure(AN_EXCEPTION)) + } + val presenter = createPresenter(fileContentReader = fileContentReader) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(1) + val errorState = awaitItem() + assertThat(errorState.lines).isInstanceOf(AsyncData.Failure::class.java) + } + } + @Test fun `present - save should not have any side effect`() = runTest { val fileContentReader = FakeFileContentReader().apply { - givenResult(listOf("aLine")) + givenResult(Result.success(listOf("aLine"))) } val fileShare = FakeFileShare() val fileSave = FakeFileSave()