From d56c66866338869d91221aaa3d613326b189a1db Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 11 Jul 2023 17:48:31 +0200 Subject: [PATCH 01/16] 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 02/16] 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 03/16] 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 From a2b84ac6177b6769571214b932687b71419e5c53 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:40:02 +0200 Subject: [PATCH 04/16] Ensure CI run all the tests. There were some failing tests, but the CI does not see it. It seems that koverMergedReport does not run all the tests (?). --- .github/workflows/nightlyReports.yml | 2 +- .github/workflows/tests.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightlyReports.yml b/.github/workflows/nightlyReports.yml index 5cd92d6cb1..4805bf44d1 100644 --- a/.github/workflows/nightlyReports.yml +++ b/.github/workflows/nightlyReports.yml @@ -27,7 +27,7 @@ jobs: java-version: '17' - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: ✅ Upload kover report if: always() diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2926159a00..44fdf58323 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -38,7 +38,7 @@ jobs: cache-read-only: ${{ github.ref != 'refs/heads/develop' }} - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: 📈 Verify coverage run: ./gradlew koverMergedVerify $CI_GRADLE_ARG_PROPERTIES -Pci-build=true From af520ddc0071a927b2c37bbfae07410e028d1f2e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:43:18 +0200 Subject: [PATCH 05/16] Fix failing test. Code is now aligned with the comment. --- .../android/libraries/core/extensions/BasicExtensions.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt b/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt index 562f62de6d..db07432df0 100644 --- a/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt +++ b/libraries/core/src/main/kotlin/io/element/android/libraries/core/extensions/BasicExtensions.kt @@ -64,7 +64,7 @@ fun String?.insertBeforeLast(insert: String, delimiter: String = "."): String { * Throws if length is < 1. */ fun String.ellipsize(length: Int): String { - require(length > 1) + require(length >= 1) if (this.length <= length) { return this From e85de6b3007b9f5edfe0dfc5b5fe48aeed1d477e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 09:59:52 +0200 Subject: [PATCH 06/16] Rework DeeplinkParser to fix a test (and fix a bug in release mode). The test was failing in release mode because there is not check on `RoomId` format, so INVITE_LIST value ("invites") is seen as a valid RoomId. First check for known paths, then try to parse as RoomId. The tryOrNull will return null only in debug mode, so I think we can remove it. Error was: value of: getFromIntent(...) expected: InviteList(sessionId=@alice:server.org) but was : Room(sessionId=@alice:server.org, roomId=invites, threadId=null) at io.element.android.libraries.deeplink.DeeplinkParserTest.nominal cases(DeeplinkParserTest.kt:54) --- .../libraries/deeplink/DeeplinkParser.kt | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt b/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt index 7d2c8af135..7a5f9d5772 100644 --- a/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt +++ b/libraries/deeplink/src/main/kotlin/io/element/android/libraries/deeplink/DeeplinkParser.kt @@ -18,7 +18,6 @@ package io.element.android.libraries.deeplink import android.content.Intent import android.net.Uri -import io.element.android.libraries.core.data.tryOrNull import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.api.core.ThreadId @@ -37,21 +36,15 @@ class DeeplinkParser @Inject constructor() { if (host != HOST) return null val pathBits = path.orEmpty().split("/").drop(1) val sessionId = pathBits.elementAtOrNull(0)?.let(::SessionId) ?: return null - val screenPathComponent = pathBits.elementAtOrNull(1) - val roomId = tryOrNull { screenPathComponent?.let(::RoomId) } - return when { - roomId != null -> { + return when (val screenPathComponent = pathBits.elementAtOrNull(1)) { + null -> DeeplinkData.Root(sessionId) + DeepLinkPaths.INVITE_LIST -> DeeplinkData.InviteList(sessionId) + else -> { + val roomId = screenPathComponent.let(::RoomId) val threadId = pathBits.elementAtOrNull(2)?.let(::ThreadId) DeeplinkData.Room(sessionId, roomId, threadId) } - screenPathComponent == DeepLinkPaths.INVITE_LIST -> { - DeeplinkData.InviteList(sessionId) - } - screenPathComponent == null -> { - DeeplinkData.Root(sessionId) - } - else -> null } } } From bb1991fe4acbe41c2137c7aa5042b82300c3acfc Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 11:26:13 +0200 Subject: [PATCH 07/16] More log about Node lifecycle. Will help to track user navigation. --- .../android/appnav/NotLoggedInFlowNode.kt | 9 ------ .../libraries/architecture/BackstackNode.kt | 14 ++++++++- .../libraries/architecture/LifecycleExt.kt | 30 +++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index 4b89b442d7..f9a0c00ec5 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -20,7 +20,6 @@ import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import com.bumble.appyx.core.composable.Children -import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -35,7 +34,6 @@ import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler import io.element.android.libraries.di.AppScope import kotlinx.parcelize.Parcelize -import timber.log.Timber @ContributesNode(AppScope::class) class NotLoggedInFlowNode @AssistedInject constructor( @@ -51,13 +49,6 @@ class NotLoggedInFlowNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins, ) { - init { - lifecycle.subscribe( - onCreate = { Timber.v("OnCreate") }, - onDestroy = { Timber.v("OnDestroy") } - ) - } - sealed interface NavTarget : Parcelable { @Parcelize object OnBoarding : NavTarget diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt index ec607e9281..ec22c5e21f 100644 --- a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/BackstackNode.kt @@ -19,6 +19,7 @@ package io.element.android.libraries.architecture import androidx.compose.runtime.Stable import com.bumble.appyx.core.children.ChildEntry import com.bumble.appyx.core.modality.BuildContext +import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.ParentNode import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.navmodel.backstack.BackStack @@ -39,4 +40,15 @@ abstract class BackstackNode( buildContext = buildContext, plugins = plugins, childKeepMode = childKeepMode, -) +) { + override fun onBuilt() { + super.onBuilt() + lifecycle.logLifecycle(this::class.java.simpleName) + whenChildAttached { _, child -> + // BackstackNode will be logged by their parent. + if (child !is BackstackNode<*>) { + child.lifecycle.logLifecycle(child::class.java.simpleName) + } + } + } +} diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt new file mode 100644 index 0000000000..7c42c4cfe3 --- /dev/null +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/LifecycleExt.kt @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.architecture + +import androidx.lifecycle.Lifecycle +import com.bumble.appyx.core.lifecycle.subscribe +import timber.log.Timber + +fun Lifecycle.logLifecycle(name: String) { + subscribe( + onCreate = { Timber.tag("Lifecycle").d("onCreate $name") }, + onPause = { Timber.tag("Lifecycle").d("onPause $name") }, + onResume = { Timber.tag("Lifecycle").d("onResume $name") }, + onDestroy = { Timber.tag("Lifecycle").d("onDestroy $name") }, + ) +} From 5622517dffb6fcf7e97b4d2613c026dcd0f34958 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:05:31 +0200 Subject: [PATCH 08/16] Fix image not loading after a clear cache. --- .../io/element/android/appnav/LoggedInFlowNode.kt | 2 -- .../element/android/appnav/NotLoggedInFlowNode.kt | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt index bc73efde71..3380dd91db 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -154,8 +154,6 @@ class LoggedInFlowNode @AssistedInject constructor( syncService.stopSync() }, onDestroy = { - val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() - Coil.setImageLoader(imageLoaderFactory) plugins().forEach { it.onFlowReleased(id, inputs.matrixClient) } appNavigationStateService.onLeavingSpace(id) appNavigationStateService.onLeavingSession(id) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index f9a0c00ec5..ac17f6ad00 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -19,7 +19,9 @@ package io.element.android.appnav import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier +import coil.Coil import com.bumble.appyx.core.composable.Children +import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -32,7 +34,9 @@ import io.element.android.features.login.api.LoginEntryPoint import io.element.android.features.onboarding.api.OnBoardingEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler +import io.element.android.libraries.architecture.bindings import io.element.android.libraries.di.AppScope +import io.element.android.libraries.matrix.ui.di.MatrixUIBindings import kotlinx.parcelize.Parcelize @ContributesNode(AppScope::class) @@ -49,6 +53,16 @@ class NotLoggedInFlowNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins, ) { + override fun onBuilt() { + super.onBuilt() + lifecycle.subscribe( + onCreate = { + val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() + Coil.setImageLoader(imageLoaderFactory) + }, + ) + } + sealed interface NavTarget : Parcelable { @Parcelize object OnBoarding : NavTarget From 47b684f7247099e152cf9891e04d5550247085d8 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:23:46 +0200 Subject: [PATCH 09/16] Let RootFlowNode manage MatrixClientsHolder save and restoration. --- .../io/element/android/x/MainActivity.kt | 8 +---- .../io/element/android/x/di/AppBindings.kt | 2 -- .../io/element/android/appnav/RootFlowNode.kt | 7 +++++ .../android/appnav/di/MatrixClientsHolder.kt | 31 +++++++++---------- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/app/src/main/kotlin/io/element/android/x/MainActivity.kt b/app/src/main/kotlin/io/element/android/x/MainActivity.kt index be9f6134ba..cf37b22159 100644 --- a/app/src/main/kotlin/io/element/android/x/MainActivity.kt +++ b/app/src/main/kotlin/io/element/android/x/MainActivity.kt @@ -52,8 +52,7 @@ class MainActivity : NodeComponentActivity() { Timber.tag(loggerTag.value).w("onCreate, with savedInstanceState: ${savedInstanceState != null}") installSplashScreen() super.onCreate(savedInstanceState) - appBindings = bindings() - appBindings.matrixClientsHolder().restore(savedInstanceState) + appBindings = bindings() WindowCompat.setDecorFitsSystemWindows(window, false) setContent { MainContent(appBindings) @@ -125,9 +124,4 @@ class MainActivity : NodeComponentActivity() { super.onDestroy() Timber.tag(loggerTag.value).w("onDestroy") } - - override fun onSaveInstanceState(outState: Bundle) { - super.onSaveInstanceState(outState) - bindings().matrixClientsHolder().onSaveInstanceState(outState) - } } diff --git a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt index 59f7e98d20..4d75d8601e 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt @@ -17,13 +17,11 @@ package io.element.android.x.di import com.squareup.anvil.annotations.ContributesTo -import io.element.android.appnav.di.MatrixClientsHolder import io.element.android.libraries.designsystem.utils.SnackbarDispatcher import io.element.android.libraries.di.AppScope @ContributesTo(AppScope::class) interface AppBindings { - fun matrixClientsHolder(): MatrixClientsHolder fun mainDaggerComponentOwner(): MainDaggerComponentsOwner fun snackbarDispatcher(): SnackbarDispatcher } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt index 8803d574d9..4150bcaebc 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -30,6 +30,7 @@ import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.node.node import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.core.plugin.plugins +import com.bumble.appyx.core.state.MutableSavedStateMap import com.bumble.appyx.navmodel.backstack.BackStack import com.bumble.appyx.navmodel.backstack.operation.pop import com.bumble.appyx.navmodel.backstack.operation.push @@ -90,10 +91,16 @@ class RootFlowNode @AssistedInject constructor( ) { override fun onBuilt() { + matrixClientsHolder.restore(buildContext.savedStateMap) super.onBuilt() observeLoggedInState() } + override fun onSaveInstanceState(state: MutableSavedStateMap) { + super.onSaveInstanceState(state) + matrixClientsHolder.save(state) + } + private fun observeLoggedInState() { combine( cacheService.onClearedCacheEventFlow(), diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 092cffd630..24c082627f 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -16,13 +16,11 @@ package io.element.android.appnav.di -import android.os.Bundle -import io.element.android.libraries.di.AppScope -import io.element.android.libraries.di.SingleIn +import com.bumble.appyx.core.state.MutableSavedStateMap +import com.bumble.appyx.core.state.SavedStateMap import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId -import io.element.android.libraries.matrix.api.core.UserId import kotlinx.coroutines.runBlocking import timber.log.Timber import java.util.concurrent.ConcurrentHashMap @@ -30,7 +28,6 @@ import javax.inject.Inject private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" -@SingleIn(AppScope::class) class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { private val sessionIdsToMatrixClient = ConcurrentHashMap() @@ -55,16 +52,18 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: return sessionIdsToMatrixClient[sessionId] } - @Suppress("DEPRECATION") - fun restore(savedInstanceState: Bundle?) { - if (savedInstanceState == null || sessionIdsToMatrixClient.isNotEmpty()) return - val userIds = savedInstanceState.getSerializable(SAVE_INSTANCE_KEY) as? Array - if (userIds.isNullOrEmpty()) return + @Suppress("UNCHECKED_CAST") + fun restore(state: SavedStateMap?) { + if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { + Timber.w("Restore with non-empty map") + } + val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + if (sessionIds.isNullOrEmpty()) return // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { - userIds.forEach { userId -> - Timber.v("Restore matrix session: $userId") - authenticationService.restoreSession(userId) + sessionIds.forEach { sessionId -> + Timber.d("Restore matrix session: $sessionId") + authenticationService.restoreSession(sessionId) .onSuccess { matrixClient -> add(matrixClient) } @@ -75,9 +74,9 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: } } - fun onSaveInstanceState(outState: Bundle) { + fun save(state: MutableSavedStateMap) { val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.v("Save matrix session keys = $sessionKeys") - outState.putSerializable(SAVE_INSTANCE_KEY, sessionKeys) + Timber.d("Save matrix session keys = $sessionKeys") + state[SAVE_INSTANCE_KEY] = sessionKeys } } From 1627dbfd279988def218ef49fac59177fb9cca33 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 12:39:41 +0200 Subject: [PATCH 10/16] Improve logs. --- .../io/element/android/appnav/di/MatrixClientsHolder.kt | 4 +++- .../libraries/matrix/api/room/RoomSummaryDataSource.kt | 3 ++- .../android/libraries/matrix/impl/room/RoomListExtensions.kt | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 24c082627f..bbb14d4d29 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -54,10 +54,12 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: @Suppress("UNCHECKED_CAST") fun restore(state: SavedStateMap?) { + Timber.d("Restore state") if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { Timber.w("Restore with non-empty map") } val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") if (sessionIds.isNullOrEmpty()) return // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { @@ -76,7 +78,7 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: fun save(state: MutableSavedStateMap) { val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.d("Save matrix session keys = $sessionKeys") + Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") state[SAVE_INSTANCE_KEY] = sessionKeys } } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt index e6c7b34056..d677d56ed9 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/RoomSummaryDataSource.kt @@ -38,12 +38,13 @@ interface RoomSummaryDataSource { suspend fun RoomSummaryDataSource.awaitAllRoomsAreLoaded(timeout: Duration = Duration.INFINITE) { try { + Timber.d("awaitAllRoomsAreLoaded: wait") withTimeout(timeout) { allRoomsLoadingState().firstOrNull { it is RoomSummaryDataSource.LoadingState.Loaded } } } catch (timeoutException: TimeoutCancellationException) { - Timber.v("AwaitAllRooms: no response after $timeout") + Timber.d("awaitAllRoomsAreLoaded: no response after $timeout") } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt index 824d477fd3..84c4eeaefb 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomListExtensions.kt @@ -62,7 +62,7 @@ fun RoomListService.roomOrNull(roomId: String): RoomListItem? { return try { room(roomId) } catch (exception: RoomListException) { - Timber.e(exception, "Failed finding room with id=$roomId") + Timber.d(exception, "Failed finding room with id=$roomId.") return null } } From 67fd2ebba95ef6cb53502c14e3c4927293f24003 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:11:26 +0200 Subject: [PATCH 11/16] Fix warning (rename the base parameter name). --- .../io/element/android/libraries/matrix/api/room/MatrixRoom.kt | 2 +- .../android/libraries/matrix/test/room/FakeMatrixRoom.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt index 3ad391ead8..05dfda714c 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt @@ -86,7 +86,7 @@ interface MatrixRoom : Closeable { suspend fun toggleReaction(emoji: String, eventId: EventId): Result - suspend fun forwardEvent(eventId: EventId, rooms: List): Result + suspend fun forwardEvent(eventId: EventId, roomIds: List): Result suspend fun retrySendMessage(transactionId: String): Result diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt index 9392fb7985..3f265f392a 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt @@ -239,7 +239,7 @@ class FakeMatrixRoom( override suspend fun sendFile(file: File, fileInfo: FileInfo, progressCallback: ProgressCallback?): Result = fakeSendMedia(progressCallback) - override suspend fun forwardEvent(eventId: EventId, rooms: List): Result = simulateLongTask { + override suspend fun forwardEvent(eventId: EventId, roomIds: List): Result = simulateLongTask { forwardEventResult } From 92f5c96936be6a509c61c91fb47a553f42b1dbb2 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:14:44 +0200 Subject: [PATCH 12/16] Use the param (bad copy paste) --- .../libraries/push/impl/notifications/NotificationFactory.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt index 0addc1276a..9dd6c7d4cd 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotificationFactory.kt @@ -123,7 +123,7 @@ class NotificationFactory @Inject constructor( val roomMeta = roomNotifications.filterIsInstance().map { it.meta } val invitationMeta = invitationNotifications.filterIsInstance().map { it.meta } val simpleMeta = simpleNotifications.filterIsInstance().map { it.meta } - val fallbackMeta = simpleNotifications.filterIsInstance().map { it.meta } + val fallbackMeta = fallbackNotifications.filterIsInstance().map { it.meta } return when { roomMeta.isEmpty() && invitationMeta.isEmpty() && simpleMeta.isEmpty() -> SummaryNotification.Removed else -> SummaryNotification.Update( From 19fc90385c8abd8846274387639b51bcb0b62370 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:15:25 +0200 Subject: [PATCH 13/16] Fix another warning. --- .../features/messages/forward/ForwardMessagesPresenterTests.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt index 82526e100d..502305ab1d 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/forward/ForwardMessagesPresenterTests.kt @@ -65,7 +65,6 @@ class ForwardMessagesPresenterTests { }.test { val initialState = awaitItem() skipItems(1) - val summary = aRoomSummaryDetail() initialState.eventSink(ForwardMessagesEvents.ToggleSearchActive) assertThat(awaitItem().isSearchActive).isTrue() From c8912060fb349bd4e7087f7844997c26aa67e542 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 14:16:10 +0200 Subject: [PATCH 14/16] Fix another warning. --- .../eventformatter/impl/RoomMembershipContentFormatter.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt b/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt index f8ac3b491c..6a65a9bd1e 100644 --- a/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt +++ b/libraries/eventformatter/impl/src/main/kotlin/io/element/android/libraries/eventformatter/impl/RoomMembershipContentFormatter.kt @@ -34,7 +34,7 @@ class RoomMembershipContentFormatter @Inject constructor( ): CharSequence? { val userId = membershipContent.userId val memberIsYou = matrixClient.isMe(userId) - return when (val change = membershipContent.change) { + return when (membershipContent.change) { MembershipChange.JOINED -> if (memberIsYou) { sp.getString(R.string.state_event_room_join_by_you) } else { From 0d45096b5935690cb8a024c816de8b8b4eb3efb7 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 15:11:40 +0200 Subject: [PATCH 15/16] Split task in 2, due to the fact that when we run kover on the CI, run only debug test variants. Error was: Some problems were found with the configuration of task ':koverMergedHtmlReport' (type 'KoverHtmlTask'). - Gradle detected a problem with the following location: '/home/runner/work/element-x-android/element-x-android/features/analytics/api/build/tmp/kotlin-classes/release'. Reason: Task ':koverMergedHtmlReport' uses this output of task ':features:analytics:api:compileReleaseKotlin' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Possible solutions: 1. Declare task ':features:analytics:api:compileReleaseKotlin' as an input of ':koverMergedHtmlReport'. 2. Declare an explicit dependency on ':features:analytics:api:compileReleaseKotlin' from ':koverMergedHtmlReport' using Task#dependsOn. 3. Declare an explicit dependency on ':features:analytics:api:compileReleaseKotlin' from ':koverMergedHtmlReport' using Task#mustRunAfter. ... --- .github/workflows/nightlyReports.yml | 5 ++++- .github/workflows/tests.yml | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightlyReports.yml b/.github/workflows/nightlyReports.yml index 4805bf44d1..cbe9935fc7 100644 --- a/.github/workflows/nightlyReports.yml +++ b/.github/workflows/nightlyReports.yml @@ -26,8 +26,11 @@ jobs: distribution: 'temurin' # See 'Supported distributions' for available options java-version: '17' + - name: ⚙️ Run unit & screenshot tests, debug and release + run: ./gradlew test $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: ✅ Upload kover report if: always() diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 44fdf58323..f5df6b1362 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -37,8 +37,11 @@ jobs: with: cache-read-only: ${{ github.ref != 'refs/heads/develop' }} + - name: ⚙️ Run unit & screenshot tests, debug and release + run: ./gradlew test $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + - name: ⚙️ Run unit & screenshot tests, generate kover report - run: ./gradlew test koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true + run: ./gradlew koverMergedReport $CI_GRADLE_ARG_PROPERTIES -Pci-build=true - name: 📈 Verify coverage run: ./gradlew koverMergedVerify $CI_GRADLE_ARG_PROPERTIES -Pci-build=true From d3a95afe863945ee5640a87c1c4d93023b43d2c1 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 12 Jul 2023 15:32:00 +0200 Subject: [PATCH 16/16] Fix crash at first startup. Inject NotLoggedInImageLoaderFactory directly to NotLoggedInFlowNode --- .../io/element/android/appnav/NotLoggedInFlowNode.kt | 7 +++---- .../android/libraries/matrix/ui/di/MatrixUIBindings.kt | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt index ac17f6ad00..1ed1aec678 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/NotLoggedInFlowNode.kt @@ -34,9 +34,8 @@ import io.element.android.features.login.api.LoginEntryPoint import io.element.android.features.onboarding.api.OnBoardingEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler -import io.element.android.libraries.architecture.bindings import io.element.android.libraries.di.AppScope -import io.element.android.libraries.matrix.ui.di.MatrixUIBindings +import io.element.android.libraries.matrix.ui.media.NotLoggedInImageLoaderFactory import kotlinx.parcelize.Parcelize @ContributesNode(AppScope::class) @@ -45,6 +44,7 @@ class NotLoggedInFlowNode @AssistedInject constructor( @Assisted plugins: List, private val onBoardingEntryPoint: OnBoardingEntryPoint, private val loginEntryPoint: LoginEntryPoint, + private val notLoggedInImageLoaderFactory: NotLoggedInImageLoaderFactory, ) : BackstackNode( backstack = BackStack( initialElement = NavTarget.OnBoarding, @@ -57,8 +57,7 @@ class NotLoggedInFlowNode @AssistedInject constructor( super.onBuilt() lifecycle.subscribe( onCreate = { - val imageLoaderFactory = bindings().notLoggedInImageLoaderFactory() - Coil.setImageLoader(imageLoaderFactory) + Coil.setImageLoader(notLoggedInImageLoaderFactory) }, ) } diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt index a5734f5b9c..919971b307 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixUIBindings.kt @@ -19,10 +19,8 @@ package io.element.android.libraries.matrix.ui.di import com.squareup.anvil.annotations.ContributesTo import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.ui.media.LoggedInImageLoaderFactory -import io.element.android.libraries.matrix.ui.media.NotLoggedInImageLoaderFactory @ContributesTo(SessionScope::class) interface MatrixUIBindings { fun loggedInImageLoaderFactory(): LoggedInImageLoaderFactory - fun notLoggedInImageLoaderFactory(): NotLoggedInImageLoaderFactory }