From d56c66866338869d91221aaa3d613326b189a1db Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 11 Jul 2023 17:48:31 +0200 Subject: [PATCH 1/3] Improve UX on Block/Unblock user action. Add loading and error case. And make the value (a bit more) live. --- .../impl/blockuser/BlockUserSection.kt | 63 +++++++++++++++---- .../details/RoomMemberDetailsEvents.kt | 1 + .../details/RoomMemberDetailsPresenter.kt | 46 ++++++++++---- .../members/details/RoomMemberDetailsState.kt | 4 +- .../details/RoomMemberDetailsStateProvider.kt | 6 +- .../RoomMemberDetailsPresenterTests.kt | 31 ++++++++- .../libraries/matrix/test/FakeMatrixClient.kt | 5 +- 7 files changed, 122 insertions(+), 34 deletions(-) diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt index e70a7d0533..de171b06fa 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt @@ -25,30 +25,67 @@ import androidx.compose.ui.res.stringResource import io.element.android.features.roomdetails.impl.R import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsEvents import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsState +import io.element.android.libraries.architecture.Async +import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.designsystem.components.dialogs.ConfirmationDialog +import io.element.android.libraries.designsystem.components.dialogs.RetryDialog import io.element.android.libraries.designsystem.components.preferences.PreferenceCategory import io.element.android.libraries.designsystem.components.preferences.PreferenceText +import io.element.android.libraries.ui.strings.CommonStrings @Composable internal fun BlockUserSection(state: RoomMemberDetailsState, modifier: Modifier = Modifier) { PreferenceCategory(showDivider = false, modifier = modifier) { - if (state.isBlocked) { - PreferenceText( - title = stringResource(R.string.screen_dm_details_unblock_user), - icon = Icons.Outlined.Block, - onClick = { state.eventSink(RoomMemberDetailsEvents.UnblockUser(needsConfirmation = true)) }, - ) - } else { - PreferenceText( - title = stringResource(R.string.screen_dm_details_block_user), - icon = Icons.Outlined.Block, - tintColor = MaterialTheme.colorScheme.error, - onClick = { state.eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = true)) }, - ) + when (state.isBlocked) { + is Async.Failure -> { + PreferenceBlockUser(state.isBlocked.prevData, false, state.eventSink, modifier) + RetryDialog( + content = stringResource(CommonStrings.error_unknown), + onDismiss = { state.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) }, + onRetry = { + val event = when (state.isBlocked.prevData) { + true -> RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false) + false -> RoomMemberDetailsEvents.BlockUser(needsConfirmation = false) + null -> /*Should not happen */ RoomMemberDetailsEvents.ClearBlockUserError + } + state.eventSink(event) + }, + ) + } + is Async.Loading -> PreferenceBlockUser(state.isBlocked.prevData, true, state.eventSink, modifier) + is Async.Success -> PreferenceBlockUser(state.isBlocked.data, false, state.eventSink, modifier) + Async.Uninitialized -> PreferenceBlockUser(null, true, state.eventSink, modifier) } } } +@Composable +private fun PreferenceBlockUser( + isBlocked: Boolean?, + isLoading: Boolean, + eventSink: (RoomMemberDetailsEvents) -> Unit, + modifier: Modifier = Modifier, +) { + if (isBlocked.orFalse()) { + PreferenceText( + title = stringResource(R.string.screen_dm_details_unblock_user), + icon = Icons.Outlined.Block, + onClick = { if (!isLoading) eventSink(RoomMemberDetailsEvents.UnblockUser(needsConfirmation = true)) }, + loadingCurrentValue = isLoading, + modifier = modifier, + ) + } else { + PreferenceText( + title = stringResource(R.string.screen_dm_details_block_user), + icon = Icons.Outlined.Block, + tintColor = MaterialTheme.colorScheme.error, + onClick = { if (!isLoading) eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = true)) }, + loadingCurrentValue = isLoading, + modifier = modifier, + ) + } +} + @Composable internal fun BlockUserDialogs(state: RoomMemberDetailsState) { when (state.displayConfirmationDialog) { diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt index 5848561f3e..c09d9a1f70 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsEvents.kt @@ -19,5 +19,6 @@ package io.element.android.features.roomdetails.impl.members.details sealed interface RoomMemberDetailsEvents { data class BlockUser(val needsConfirmation: Boolean = false) : RoomMemberDetailsEvents data class UnblockUser(val needsConfirmation: Boolean = false) : RoomMemberDetailsEvents + object ClearBlockUserError : RoomMemberDetailsEvents object ClearConfirmationDialog : RoomMemberDetailsEvents } diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt index 9d3c391ace..3be83a2fef 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsPresenter.kt @@ -28,6 +28,7 @@ import androidx.compose.runtime.setValue import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsState.ConfirmationDialog +import io.element.android.libraries.architecture.Async import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.matrix.api.MatrixClient @@ -53,8 +54,13 @@ class RoomMemberDetailsPresenter @AssistedInject constructor( var confirmationDialog by remember { mutableStateOf(null) } val roomMember by room.getRoomMemberAsState(roomMemberId) // the room member is not really live... - val isBlocked = remember { - mutableStateOf(roomMember?.isIgnored.orFalse()) + val isBlocked: MutableState> = remember(roomMember) { + val isIgnored = roomMember?.isIgnored + if (isIgnored == null) { + mutableStateOf(Async.Uninitialized) + } else { + mutableStateOf(Async.Success(isIgnored)) + } } LaunchedEffect(Unit) { room.updateMembers() @@ -79,6 +85,9 @@ class RoomMemberDetailsPresenter @AssistedInject constructor( } } RoomMemberDetailsEvents.ClearConfirmationDialog -> confirmationDialog = null + RoomMemberDetailsEvents.ClearBlockUserError -> { + isBlocked.value = Async.Success(isBlocked.value.dataOrNull().orFalse()) + } } } @@ -105,20 +114,31 @@ class RoomMemberDetailsPresenter @AssistedInject constructor( ) } - private fun CoroutineScope.blockUser(userId: UserId, isBlockedState: MutableState) = launch { + private fun CoroutineScope.blockUser(userId: UserId, isBlockedState: MutableState>) = launch { + isBlockedState.value = Async.Loading(false) client.ignoreUser(userId) - .map { - isBlockedState.value = true - room.updateMembers() - } - + .fold( + onSuccess = { + isBlockedState.value = Async.Success(true) + room.updateMembers() + }, + onFailure = { + isBlockedState.value = Async.Failure(it, false) + } + ) } - private fun CoroutineScope.unblockUser(userId: UserId, isBlockedState: MutableState) = launch { + private fun CoroutineScope.unblockUser(userId: UserId, isBlockedState: MutableState>) = launch { + isBlockedState.value = Async.Loading(true) client.unignoreUser(userId) - .map { - isBlockedState.value = false - room.updateMembers() - } + .fold( + onSuccess = { + isBlockedState.value = Async.Success(false) + room.updateMembers() + }, + onFailure = { + isBlockedState.value = Async.Failure(it, true) + } + ) } } diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt index 0a2895db09..0d3423e179 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsState.kt @@ -16,11 +16,13 @@ package io.element.android.features.roomdetails.impl.members.details +import io.element.android.libraries.architecture.Async + data class RoomMemberDetailsState( val userId: String, val userName: String?, val avatarUrl: String?, - val isBlocked: Boolean, + val isBlocked: Async, val displayConfirmationDialog: ConfirmationDialog? = null, val isCurrentUser: Boolean, val eventSink: (RoomMemberDetailsEvents) -> Unit diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt index d8e7ce5ad3..6883b20898 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/members/details/RoomMemberDetailsStateProvider.kt @@ -17,15 +17,17 @@ package io.element.android.features.roomdetails.impl.members.details import androidx.compose.ui.tooling.preview.PreviewParameterProvider +import io.element.android.libraries.architecture.Async open class RoomMemberDetailsStateProvider : PreviewParameterProvider { override val values: Sequence get() = sequenceOf( aRoomMemberDetailsState(), aRoomMemberDetailsState().copy(userName = null), - aRoomMemberDetailsState().copy(isBlocked = true), + aRoomMemberDetailsState().copy(isBlocked = Async.Success(true)), aRoomMemberDetailsState().copy(displayConfirmationDialog = RoomMemberDetailsState.ConfirmationDialog.Block), aRoomMemberDetailsState().copy(displayConfirmationDialog = RoomMemberDetailsState.ConfirmationDialog.Unblock), + aRoomMemberDetailsState().copy(isBlocked = Async.Loading(true)), // Add other states here ) } @@ -34,7 +36,7 @@ fun aRoomMemberDetailsState() = RoomMemberDetailsState( userId = "@daniel:domain.com", userName = "Daniel", avatarUrl = null, - isBlocked = false, + isBlocked = Async.Success(false), isCurrentUser = false, eventSink = {}, ) diff --git a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt index 912f354c89..94b940bb17 100644 --- a/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt +++ b/features/roomdetails/impl/src/test/kotlin/io/element/android/features/roomdetails/members/details/RoomMemberDetailsPresenterTests.kt @@ -25,7 +25,9 @@ import io.element.android.features.roomdetails.impl.members.aRoomMember import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsEvents import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsPresenter import io.element.android.features.roomdetails.impl.members.details.RoomMemberDetailsState +import io.element.android.libraries.architecture.Async import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState +import io.element.android.libraries.matrix.test.A_THROWABLE import io.element.android.libraries.matrix.test.FakeMatrixClient import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runTest @@ -50,7 +52,7 @@ class RoomMemberDetailsPresenterTests { Truth.assertThat(initialState.userId).isEqualTo(roomMember.userId.value) Truth.assertThat(initialState.userName).isEqualTo(roomMember.displayName) Truth.assertThat(initialState.avatarUrl).isEqualTo(roomMember.avatarUrl) - Truth.assertThat(initialState.isBlocked).isEqualTo(roomMember.isIgnored) + Truth.assertThat(initialState.isBlocked).isEqualTo(Async.Success(roomMember.isIgnored)) skipItems(1) val loadedState = awaitItem() Truth.assertThat(loadedState.userName).isEqualTo("A custom name") @@ -129,10 +131,33 @@ class RoomMemberDetailsPresenterTests { }.test { val initialState = awaitItem() initialState.eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = false)) - Truth.assertThat(awaitItem().isBlocked).isTrue() + Truth.assertThat(awaitItem().isBlocked.isLoading()).isTrue() + Truth.assertThat(awaitItem().isBlocked.dataOrNull()).isTrue() initialState.eventSink(RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false)) - Truth.assertThat(awaitItem().isBlocked).isFalse() + Truth.assertThat(awaitItem().isBlocked.isLoading()).isTrue() + Truth.assertThat(awaitItem().isBlocked.dataOrNull()).isFalse() + } + } + + @Test + fun `present - BlockUser with error`() = runTest { + val room = aMatrixRoom() + val roomMember = aRoomMember() + val matrixClient = FakeMatrixClient() + matrixClient.givenIgnoreUserResult(Result.failure(A_THROWABLE)) + val presenter = RoomMemberDetailsPresenter(matrixClient, room, roomMember.userId) + moleculeFlow(RecompositionClock.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + initialState.eventSink(RoomMemberDetailsEvents.BlockUser(needsConfirmation = false)) + Truth.assertThat(awaitItem().isBlocked.isLoading()).isTrue() + val errorState = awaitItem() + Truth.assertThat(errorState.isBlocked.errorOrNull()).isEqualTo(A_THROWABLE) + // Clear error + initialState.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) + Truth.assertThat(awaitItem().isBlocked).isEqualTo(Async.Success(false)) } } diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt index fcd2f161e9..1a654ac8d4 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/FakeMatrixClient.kt @@ -38,6 +38,7 @@ import io.element.android.libraries.matrix.test.room.FakeMatrixRoom import io.element.android.libraries.matrix.test.room.FakeRoomSummaryDataSource import io.element.android.libraries.matrix.test.sync.FakeSyncService import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService +import io.element.android.tests.testutils.simulateLongTask import kotlinx.coroutines.delay class FakeMatrixClient( @@ -72,11 +73,11 @@ class FakeMatrixClient( return findDmResult } - override suspend fun ignoreUser(userId: UserId): Result { + override suspend fun ignoreUser(userId: UserId): Result = simulateLongTask { return ignoreUserResult } - override suspend fun unignoreUser(userId: UserId): Result { + override suspend fun unignoreUser(userId: UserId): Result = simulateLongTask { return unignoreUserResult } From 2cc548f14511a73dcbdffc926bcdda81578425ed Mon Sep 17 00:00:00 2001 From: ElementBot Date: Tue, 11 Jul 2023 16:19:22 +0000 Subject: [PATCH 2/3] Update screenshots --- ...emberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png | 3 +++ ...mberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png | 3 +++ 2 files changed, 6 insertions(+) create mode 100644 tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png create mode 100644 tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..50217fd9f2 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewDarkPreview--3_3_null_5,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:c7ca068387cff8faf728a989488e7c4b5b07983c4b24162ff82a14fd90b82d05 +size 20609 diff --git a/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..cab17c4d00 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/io.element.android.tests.uitests_ScreenshotTest_preview_tests[io.element.android.features.roomdetails.impl.members.details_null_DefaultGroup_RoomMemberDetailsViewLightPreview--2_2_null_5,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:e1599be0c5a37083c015378ee47a78a90dcf670f4df5d85dd25d39f4b2edbdf6 +size 21147 From 38b91a75929ecf7d6c3f7da30c3a30bcbe8ea77e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:37:13 +0200 Subject: [PATCH 3/3] Fix issue about modifier. --- .../impl/blockuser/BlockUserSection.kt | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt index de171b06fa..cccf682c9c 100644 --- a/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt +++ b/features/roomdetails/impl/src/main/kotlin/io/element/android/features/roomdetails/impl/blockuser/BlockUserSection.kt @@ -37,26 +37,26 @@ import io.element.android.libraries.ui.strings.CommonStrings internal fun BlockUserSection(state: RoomMemberDetailsState, modifier: Modifier = Modifier) { PreferenceCategory(showDivider = false, modifier = modifier) { when (state.isBlocked) { - is Async.Failure -> { - PreferenceBlockUser(state.isBlocked.prevData, false, state.eventSink, modifier) - RetryDialog( - content = stringResource(CommonStrings.error_unknown), - onDismiss = { state.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) }, - onRetry = { - val event = when (state.isBlocked.prevData) { - true -> RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false) - false -> RoomMemberDetailsEvents.BlockUser(needsConfirmation = false) - null -> /*Should not happen */ RoomMemberDetailsEvents.ClearBlockUserError - } - state.eventSink(event) - }, - ) - } - is Async.Loading -> PreferenceBlockUser(state.isBlocked.prevData, true, state.eventSink, modifier) - is Async.Success -> PreferenceBlockUser(state.isBlocked.data, false, state.eventSink, modifier) - Async.Uninitialized -> PreferenceBlockUser(null, true, state.eventSink, modifier) + is Async.Failure -> PreferenceBlockUser(isBlocked = state.isBlocked.prevData, isLoading = false, eventSink = state.eventSink) + is Async.Loading -> PreferenceBlockUser(isBlocked = state.isBlocked.prevData, isLoading = true, eventSink = state.eventSink) + is Async.Success -> PreferenceBlockUser(isBlocked = state.isBlocked.data, isLoading = false, eventSink = state.eventSink) + Async.Uninitialized -> PreferenceBlockUser(isBlocked = null, isLoading = true, eventSink = state.eventSink) } } + if (state.isBlocked is Async.Failure) { + RetryDialog( + content = stringResource(CommonStrings.error_unknown), + onDismiss = { state.eventSink(RoomMemberDetailsEvents.ClearBlockUserError) }, + onRetry = { + val event = when (state.isBlocked.prevData) { + true -> RoomMemberDetailsEvents.UnblockUser(needsConfirmation = false) + false -> RoomMemberDetailsEvents.BlockUser(needsConfirmation = false) + null -> /*Should not happen */ RoomMemberDetailsEvents.ClearBlockUserError + } + state.eventSink(event) + }, + ) + } } @Composable