From a9b17923bdfe7cf10d1145a844334cc0926a7499 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 May 2024 12:22:20 +0200 Subject: [PATCH 1/9] Simplify ForwardMessagesState by using AsyncAction. --- .../impl/forward/ForwardMessagesNode.kt | 3 +- .../impl/forward/ForwardMessagesPresenter.kt | 33 ++++++--------- .../impl/forward/ForwardMessagesState.kt | 7 +--- .../forward/ForwardMessagesStateProvider.kt | 20 ++++------ .../impl/forward/ForwardMessagesView.kt | 40 +++++-------------- .../forward/ForwardMessagesPresenterTest.kt | 22 +++++----- 6 files changed, 42 insertions(+), 83 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesNode.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesNode.kt index 04d562abde..37aabfd4b8 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesNode.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesNode.kt @@ -36,7 +36,6 @@ import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.roomselect.api.RoomSelectEntryPoint import io.element.android.libraries.roomselect.api.RoomSelectMode -import kotlinx.collections.immutable.ImmutableList import kotlinx.parcelize.Parcelize @ContributesNode(RoomScope::class) @@ -99,7 +98,7 @@ class ForwardMessagesNode @AssistedInject constructor( } } - private fun onForwardSuccess(roomIds: ImmutableList) { + private fun onForwardSuccess(roomIds: List) { navigateUp() if (roomIds.size == 1) { val targetRoomId = roomIds.first() diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenter.kt index 9cfdbcd14d..3b311c7ced 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenter.kt @@ -18,15 +18,13 @@ package io.element.android.features.messages.impl.forward import androidx.compose.runtime.Composable import androidx.compose.runtime.MutableState -import androidx.compose.runtime.derivedStateOf -import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember 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.AsyncAction import io.element.android.libraries.architecture.Presenter +import io.element.android.libraries.architecture.runCatchingUpdatingState import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.timeline.TimelineProvider @@ -38,7 +36,7 @@ import kotlinx.coroutines.launch class ForwardMessagesPresenter @AssistedInject constructor( @Assisted eventId: String, - private val matrixCoroutineScope: CoroutineScope, + private val appCoroutineScope: CoroutineScope, private val timelineProvider: TimelineProvider, ) : Presenter { private val eventId: EventId = EventId(eventId) @@ -48,28 +46,22 @@ class ForwardMessagesPresenter @AssistedInject constructor( fun create(eventId: String): ForwardMessagesPresenter } - private val forwardingActionState: MutableState>> = mutableStateOf(AsyncData.Uninitialized) + private val forwardingActionState: MutableState>> = mutableStateOf(AsyncAction.Uninitialized) fun onRoomSelected(roomIds: List) { - matrixCoroutineScope.forwardEvent(eventId, roomIds.toPersistentList(), forwardingActionState) + appCoroutineScope.forwardEvent(eventId, roomIds.toPersistentList(), forwardingActionState) } @Composable override fun present(): ForwardMessagesState { - val forwardingSucceeded by remember { - derivedStateOf { forwardingActionState.value.dataOrNull() } - } - fun handleEvents(event: ForwardMessagesEvents) { when (event) { - ForwardMessagesEvents.ClearError -> forwardingActionState.value = AsyncData.Uninitialized + ForwardMessagesEvents.ClearError -> forwardingActionState.value = AsyncAction.Uninitialized } } return ForwardMessagesState( - isForwarding = forwardingActionState.value.isLoading(), - error = (forwardingActionState.value as? AsyncData.Failure)?.error, - forwardingSucceeded = forwardingSucceeded, + forwardAction = forwardingActionState.value, eventSink = { handleEvents(it) } ) } @@ -77,12 +69,11 @@ class ForwardMessagesPresenter @AssistedInject constructor( private fun CoroutineScope.forwardEvent( eventId: EventId, roomIds: ImmutableList, - isForwardMessagesState: MutableState>>, + isForwardMessagesState: MutableState>>, ) = launch { - isForwardMessagesState.value = AsyncData.Loading() - timelineProvider.getActiveTimeline().forwardEvent(eventId, roomIds).fold( - { isForwardMessagesState.value = AsyncData.Success(roomIds) }, - { isForwardMessagesState.value = AsyncData.Failure(it) } - ) + suspend { + timelineProvider.getActiveTimeline().forwardEvent(eventId, roomIds).getOrThrow() + roomIds + }.runCatchingUpdatingState(isForwardMessagesState) } } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesState.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesState.kt index d64c4dd7e9..166ab7b65d 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesState.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesState.kt @@ -16,13 +16,10 @@ package io.element.android.features.messages.impl.forward +import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.matrix.api.core.RoomId -import kotlinx.collections.immutable.ImmutableList data class ForwardMessagesState( - // TODO Migrate to an Async - val isForwarding: Boolean, - val error: Throwable?, - val forwardingSucceeded: ImmutableList?, + val forwardAction: AsyncAction>, val eventSink: (ForwardMessagesEvents) -> Unit ) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt index aa33b1cbd9..e66741c3bf 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt @@ -17,34 +17,30 @@ package io.element.android.features.messages.impl.forward import androidx.compose.ui.tooling.preview.PreviewParameterProvider +import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.matrix.api.core.RoomId -import kotlinx.collections.immutable.ImmutableList -import kotlinx.collections.immutable.persistentListOf open class ForwardMessagesStateProvider : PreviewParameterProvider { override val values: Sequence get() = sequenceOf( aForwardMessagesState(), aForwardMessagesState( - isForwarding = true, + forwardAction = AsyncAction.Loading, ), aForwardMessagesState( - forwardingSucceeded = persistentListOf(RoomId("!room2:domain")), + forwardAction = AsyncAction.Success( + listOf(RoomId("!room2:domain")), + ) ), aForwardMessagesState( - error = Throwable("error"), + forwardAction = AsyncAction.Failure(Throwable("error")), ), - // Add other states here ) } fun aForwardMessagesState( - isForwarding: Boolean = false, - error: Throwable? = null, - forwardingSucceeded: ImmutableList? = null, + forwardAction: AsyncAction> = AsyncAction.Uninitialized, ) = ForwardMessagesState( - isForwarding = isForwarding, - error = error, - forwardingSucceeded = forwardingSucceeded, + forwardAction = forwardAction, eventSink = {} ) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesView.kt index 030b137c04..310e0c85fe 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesView.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesView.kt @@ -17,45 +17,25 @@ package io.element.android.features.messages.impl.forward import androidx.compose.runtime.Composable -import androidx.compose.ui.Modifier import androidx.compose.ui.tooling.preview.PreviewParameter -import io.element.android.libraries.designsystem.components.ProgressDialog -import io.element.android.libraries.designsystem.components.dialogs.ErrorDialog -import io.element.android.libraries.designsystem.components.dialogs.ErrorDialogDefaults +import io.element.android.libraries.designsystem.components.async.AsyncActionView import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.matrix.api.core.RoomId -import kotlinx.collections.immutable.ImmutableList @Composable fun ForwardMessagesView( state: ForwardMessagesState, - onForwardSuccess: (ImmutableList) -> Unit, - modifier: Modifier = Modifier, + onForwardSuccess: (List) -> Unit, ) { - if (state.forwardingSucceeded != null) { - onForwardSuccess(state.forwardingSucceeded) - return - } - - if (state.isForwarding) { - ProgressDialog(modifier) - } - - if (state.error != null) { - ForwardingErrorDialog( - modifier = modifier, - onDismiss = { state.eventSink(ForwardMessagesEvents.ClearError) }, - ) - } -} - -@Composable -private fun ForwardingErrorDialog(onDismiss: () -> Unit, modifier: Modifier = Modifier) { - ErrorDialog( - content = ErrorDialogDefaults.title, - onDismiss = onDismiss, - modifier = modifier, + AsyncActionView( + async = state.forwardAction, + onSuccess = { + onForwardSuccess(it) + }, + onErrorDismiss = { + state.eventSink(ForwardMessagesEvents.ClearError) + }, ) } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenterTest.kt index 99734de4ee..e30011999f 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesPresenterTest.kt @@ -20,6 +20,7 @@ import app.cash.molecule.RecompositionMode import app.cash.molecule.moleculeFlow import app.cash.turbine.test import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.test.AN_EVENT_ID @@ -28,13 +29,11 @@ import io.element.android.libraries.matrix.test.room.aRoomSummaryDetails import io.element.android.libraries.matrix.test.timeline.FakeTimeline import io.element.android.libraries.matrix.test.timeline.LiveTimelineProvider import io.element.android.tests.testutils.WarmUpRule -import io.element.android.tests.testutils.lambda.assert import io.element.android.tests.testutils.lambda.lambdaRecorder import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test -import java.lang.IllegalStateException class ForwardMessagesPresenterTest { @get:Rule @@ -47,9 +46,7 @@ class ForwardMessagesPresenterTest { presenter.present() }.test { val initialState = awaitItem() - assertThat(initialState.isForwarding).isFalse() - assertThat(initialState.error).isNull() - assertThat(initialState.forwardingSucceeded).isNull() + assertThat(initialState.forwardAction.isUninitialized()).isTrue() } } @@ -70,11 +67,10 @@ class ForwardMessagesPresenterTest { val summary = aRoomSummaryDetails() presenter.onRoomSelected(listOf(summary.roomId)) val forwardingState = awaitItem() - assertThat(forwardingState.isForwarding).isTrue() + assertThat(forwardingState.forwardAction.isLoading()).isTrue() val successfulForwardState = awaitItem() - assertThat(successfulForwardState.isForwarding).isFalse() - assertThat(successfulForwardState.forwardingSucceeded).isNotNull() - assert(forwardEventLambda).isCalledOnce() + assertThat(successfulForwardState.forwardAction).isEqualTo(AsyncAction.Success(listOf(summary.roomId))) + forwardEventLambda.assertions().isCalledOnce() } } @@ -96,11 +92,11 @@ class ForwardMessagesPresenterTest { presenter.onRoomSelected(listOf(summary.roomId)) skipItems(1) val failedForwardState = awaitItem() - assertThat(failedForwardState.error).isNotNull() + assertThat(failedForwardState.forwardAction.isFailure()).isTrue() // Then clear error failedForwardState.eventSink(ForwardMessagesEvents.ClearError) - assertThat(awaitItem().error).isNull() - assert(forwardEventLambda).isCalledOnce() + assertThat(awaitItem().forwardAction.isUninitialized()).isTrue() + forwardEventLambda.assertions().isCalledOnce() } } @@ -111,6 +107,6 @@ class ForwardMessagesPresenterTest { ) = ForwardMessagesPresenter( eventId = eventId.value, timelineProvider = LiveTimelineProvider(fakeMatrixRoom), - matrixCoroutineScope = coroutineScope, + appCoroutineScope = coroutineScope, ) } From 3cf27ccf54491ea250766baba303787f9e1cf5ae Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 May 2024 12:29:09 +0200 Subject: [PATCH 2/9] Add test on ForwardMessagesView --- .../forward/ForwardMessagesStateProvider.kt | 3 +- .../impl/forward/ForwardMessagesViewTest.kt | 80 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesViewTest.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt index e66741c3bf..11d0f7ae8c 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesStateProvider.kt @@ -40,7 +40,8 @@ open class ForwardMessagesStateProvider : PreviewParameterProvider> = AsyncAction.Uninitialized, + eventSink: (ForwardMessagesEvents) -> Unit = {} ) = ForwardMessagesState( forwardAction = forwardAction, - eventSink = {} + eventSink = eventSink ) diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesViewTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesViewTest.kt new file mode 100644 index 0000000000..a510cb0a99 --- /dev/null +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/forward/ForwardMessagesViewTest.kt @@ -0,0 +1,80 @@ +/* + * Copyright (c) 2024 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.features.messages.impl.forward + +import androidx.activity.ComponentActivity +import androidx.compose.ui.test.junit4.AndroidComposeTestRule +import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.test.ext.junit.runners.AndroidJUnit4 +import io.element.android.libraries.architecture.AsyncAction +import io.element.android.libraries.matrix.api.core.RoomId +import io.element.android.libraries.matrix.test.AN_EXCEPTION +import io.element.android.libraries.matrix.test.A_ROOM_ID +import io.element.android.libraries.testtags.TestTags +import io.element.android.tests.testutils.EnsureNeverCalledWithParam +import io.element.android.tests.testutils.EventsRecorder +import io.element.android.tests.testutils.ensureCalledOnceWithParam +import io.element.android.tests.testutils.pressTag +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TestRule +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class ForwardMessagesViewTest { + @get:Rule val rule = createAndroidComposeRule() + + @Test + fun `cancel error emits the expected event`() { + val eventsRecorder = EventsRecorder() + rule.setForwardMessagesView( + aForwardMessagesState( + forwardAction = AsyncAction.Failure(AN_EXCEPTION), + eventSink = eventsRecorder + ), + ) + rule.pressTag(TestTags.dialogPositive.value) + eventsRecorder.assertSingle(ForwardMessagesEvents.ClearError) + } + + @Test + fun `success invokes onForwardSuccess`() { + val data = listOf(A_ROOM_ID) + val eventsRecorder = EventsRecorder(expectEvents = false) + ensureCalledOnceWithParam?>(data) { callback -> + rule.setForwardMessagesView( + aForwardMessagesState( + forwardAction = AsyncAction.Success(data), + eventSink = eventsRecorder + ), + onForwardSuccess = callback, + ) + } + } +} + +private fun AndroidComposeTestRule.setForwardMessagesView( + state: ForwardMessagesState, + onForwardSuccess: (List) -> Unit = EnsureNeverCalledWithParam(), +) { + setContent { + ForwardMessagesView( + state = state, + onForwardSuccess = onForwardSuccess, + ) + } +} From 1444436d3eefce3dcdd2dc58e7a8d2d6e338e23c Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 May 2024 14:28:12 +0200 Subject: [PATCH 3/9] Update wrong comment --- .../android/libraries/matrix/api/roomlist/RoomListService.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListService.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListService.kt index dc8e0988e4..ca2d21a706 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListService.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListService.kt @@ -54,8 +54,8 @@ interface RoomListService { ): DynamicRoomList /** - * returns a [DynamicRoomList] object of all rooms we want to display. - * This will exclude some rooms like the invites, or spaces. + * Returns a [DynamicRoomList] object of all rooms we want to display. + * If you want to get a filtered room list, consider using [createRoomList]. */ val allRooms: DynamicRoomList From 8005fa6f9100709ca41aa5c20bd2ecaf7800dfdd Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 May 2024 15:12:27 +0200 Subject: [PATCH 4/9] Ensure that we can "not filter" the rooms. --- .../android/libraries/matrix/api/roomlist/RoomListFilter.kt | 2 ++ .../libraries/matrix/impl/roomlist/RoomListFilterTest.kt | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt index 8f5526a6e0..4af0e463fd 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt @@ -20,6 +20,7 @@ sealed interface RoomListFilter { companion object { /** * Create a filter that matches all the given filters. + * If not filters are provided, all the room will match. */ fun all(vararg filters: RoomListFilter): RoomListFilter { return All(filters.toList()) @@ -35,6 +36,7 @@ sealed interface RoomListFilter { /** * A filter that matches all the given filters. + * If [filters] is empty, all the room will match. */ data class All( val filters: List diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListFilterTest.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListFilterTest.kt index 0916480c9e..02fb9cd24b 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListFilterTest.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListFilterTest.kt @@ -136,4 +136,10 @@ class RoomListFilterTest { ) assertThat(roomSummaries.filter(filter)).isEmpty() } + + @Test + fun `Room list filter all with empty list`() = runTest { + val filter = RoomListFilter.all() + assertThat(roomSummaries.filter(filter)).isEqualTo(roomSummaries) + } } From 73b2e925b55cf487461f230687900704a97f2435 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 May 2024 17:31:15 +0200 Subject: [PATCH 5/9] Ensure that room where is user is invited are not listed when forwarding a message. --- .../roomselect/impl/RoomSelectPresenter.kt | 44 ++++++------ .../impl/RoomSelectSearchDataSource.kt | 70 +++++++++++++++++++ .../impl/RoomSelectPresenterTest.kt | 51 +++++++++----- 3 files changed, 125 insertions(+), 40 deletions(-) create mode 100644 libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt diff --git a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt index 77eb7b9845..e56d4dce09 100644 --- a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt +++ b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenter.kt @@ -19,6 +19,7 @@ package io.element.android.libraries.roomselect.impl import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -28,17 +29,14 @@ import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.designsystem.theme.components.SearchBarResultState -import io.element.android.libraries.matrix.api.MatrixClient -import io.element.android.libraries.matrix.api.roomlist.RoomSummary import io.element.android.libraries.matrix.api.roomlist.RoomSummaryDetails import io.element.android.libraries.roomselect.api.RoomSelectMode -import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.persistentListOf -import kotlinx.collections.immutable.toPersistentList +import kotlinx.collections.immutable.toImmutableList class RoomSelectPresenter @AssistedInject constructor( @Assisted private val mode: RoomSelectMode, - private val client: MatrixClient, + private val dataSource: RoomSelectSearchDataSource, ) : Presenter { @AssistedFactory interface Factory { @@ -48,22 +46,26 @@ class RoomSelectPresenter @AssistedInject constructor( @Composable override fun present(): RoomSelectState { var selectedRooms by remember { mutableStateOf(persistentListOf()) } - var query by remember { mutableStateOf("") } + var searchQuery by remember { mutableStateOf("") } var isSearchActive by remember { mutableStateOf(false) } - var results: SearchBarResultState> by remember { mutableStateOf(SearchBarResultState.Initial()) } - val summaries by client.roomListService.allRooms.summaries.collectAsState(initial = emptyList()) + LaunchedEffect(Unit) { + dataSource.load() + } + + LaunchedEffect(searchQuery) { + dataSource.setSearchQuery(searchQuery) + } - LaunchedEffect(query, summaries) { - val filteredSummaries = summaries.filterIsInstance() - .map { it.details } - .filter { it.name.orEmpty().contains(query, ignoreCase = true) } - .distinctBy { it.roomId } // This should be removed once we're sure no duplicate Rooms can be received - .toPersistentList() - results = if (filteredSummaries.isNotEmpty()) { - SearchBarResultState.Results(filteredSummaries) - } else { - SearchBarResultState.NoResultsFound() + val roomSummaryDetailsList by dataSource.roomSummaries.collectAsState(initial = persistentListOf()) + + val searchResults by remember { + derivedStateOf { + when { + roomSummaryDetailsList.isNotEmpty() -> SearchBarResultState.Results(roomSummaryDetailsList.toImmutableList()) + isSearchActive -> SearchBarResultState.NoResultsFound() + else -> SearchBarResultState.Initial() + } } } @@ -80,15 +82,15 @@ class RoomSelectPresenter @AssistedInject constructor( // } } RoomSelectEvents.RemoveSelectedRoom -> selectedRooms = persistentListOf() - is RoomSelectEvents.UpdateQuery -> query = event.query + is RoomSelectEvents.UpdateQuery -> searchQuery = event.query RoomSelectEvents.ToggleSearchActive -> isSearchActive = !isSearchActive } } return RoomSelectState( mode = mode, - resultState = results, - query = query, + resultState = searchResults, + query = searchQuery, isSearchActive = isSearchActive, selectedRooms = selectedRooms, eventSink = { handleEvents(it) } diff --git a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt new file mode 100644 index 0000000000..a6705e65ba --- /dev/null +++ b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt @@ -0,0 +1,70 @@ +/* + * Copyright (c) 2024 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.roomselect.impl + +import io.element.android.libraries.core.coroutine.CoroutineDispatchers +import io.element.android.libraries.matrix.api.room.CurrentUserMembership +import io.element.android.libraries.matrix.api.roomlist.RoomList +import io.element.android.libraries.matrix.api.roomlist.RoomListFilter +import io.element.android.libraries.matrix.api.roomlist.RoomListService +import io.element.android.libraries.matrix.api.roomlist.RoomSummary +import io.element.android.libraries.matrix.api.roomlist.RoomSummaryDetails +import io.element.android.libraries.matrix.api.roomlist.loadAllIncrementally +import kotlinx.collections.immutable.PersistentList +import kotlinx.collections.immutable.toPersistentList +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flowOn +import kotlinx.coroutines.flow.map +import javax.inject.Inject + +private const val PAGE_SIZE = 30 + +class RoomSelectSearchDataSource @Inject constructor( + roomListService: RoomListService, + coroutineDispatchers: CoroutineDispatchers, +) { + private val roomList = roomListService.createRoomList( + pageSize = PAGE_SIZE, + initialFilter = RoomListFilter.all(), + source = RoomList.Source.All, + ) + + val roomSummaries: Flow> = roomList.filteredSummaries + .map { roomSummaries -> + roomSummaries + .filterIsInstance() + .map { it.details } + .filter { it.currentUserMembership == CurrentUserMembership.JOINED } + .distinctBy { it.roomId } // This should be removed once we're sure no duplicate Rooms can be received + .toPersistentList() + } + .flowOn(coroutineDispatchers.computation) + + suspend fun load() = coroutineScope { + roomList.loadAllIncrementally(this) + } + + suspend fun setSearchQuery(searchQuery: String) = coroutineScope { + val filter = if (searchQuery.isBlank()) { + RoomListFilter.all() + } else { + RoomListFilter.NormalizedMatchRoomName(searchQuery) + } + roomList.updateFilter(filter) + } +} diff --git a/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt b/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt index 3bc08b483f..b4117d3d40 100644 --- a/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt +++ b/libraries/roomselect/impl/src/test/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectPresenterTest.kt @@ -21,13 +21,16 @@ import app.cash.molecule.moleculeFlow import app.cash.turbine.test import com.google.common.truth.Truth.assertThat import io.element.android.libraries.designsystem.theme.components.SearchBarResultState +import io.element.android.libraries.matrix.api.roomlist.RoomListFilter +import io.element.android.libraries.matrix.api.roomlist.RoomListService import io.element.android.libraries.matrix.api.roomlist.RoomSummary -import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.room.aRoomSummaryDetails import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService import io.element.android.libraries.roomselect.api.RoomSelectMode import io.element.android.tests.testutils.WarmUpRule +import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.collections.immutable.persistentListOf +import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test @@ -38,7 +41,7 @@ class RoomSelectPresenterTest { @Test fun `present - initial state`() = runTest { - val presenter = aPresenter() + val presenter = createRoomSelectPresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -46,24 +49,18 @@ class RoomSelectPresenterTest { assertThat(initialState.selectedRooms).isEmpty() assertThat(initialState.resultState).isInstanceOf(SearchBarResultState.Initial::class.java) assertThat(initialState.isSearchActive).isFalse() - // Search is run automatically - val searchState = awaitItem() - assertThat(searchState.resultState).isInstanceOf(SearchBarResultState.NoResultsFound::class.java) } } @Test fun `present - toggle search active`() = runTest { - val presenter = aPresenter() + val presenter = createRoomSelectPresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { val initialState = awaitItem() - skipItems(1) - initialState.eventSink(RoomSelectEvents.ToggleSearchActive) assertThat(awaitItem().isSearchActive).isTrue() - initialState.eventSink(RoomSelectEvents.ToggleSearchActive) assertThat(awaitItem().isSearchActive).isFalse() } @@ -74,43 +71,59 @@ class RoomSelectPresenterTest { val roomListService = FakeRoomListService().apply { postAllRooms(listOf(RoomSummary.Filled(aRoomSummaryDetails()))) } - val client = FakeMatrixClient(roomListService = roomListService) - val presenter = aPresenter(client = client) + val presenter = createRoomSelectPresenter( + roomListService = roomListService + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { val initialState = awaitItem() assertThat(awaitItem().resultState as? SearchBarResultState.Results).isEqualTo(SearchBarResultState.Results(listOf(aRoomSummaryDetails()))) - + initialState.eventSink(RoomSelectEvents.ToggleSearchActive) + skipItems(1) initialState.eventSink(RoomSelectEvents.UpdateQuery("string not contained")) + assertThat( + roomListService.allRooms.currentFilter.value + ).isEqualTo( + RoomListFilter.NormalizedMatchRoomName("string not contained") + ) assertThat(awaitItem().query).isEqualTo("string not contained") + roomListService.postAllRooms( + emptyList() + ) assertThat(awaitItem().resultState).isInstanceOf(SearchBarResultState.NoResultsFound::class.java) } } @Test fun `present - select and remove a room`() = runTest { - val presenter = aPresenter() + val roomListService = FakeRoomListService().apply { + postAllRooms(listOf(RoomSummary.Filled(aRoomSummaryDetails()))) + } + val presenter = createRoomSelectPresenter( + roomListService = roomListService, + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { val initialState = awaitItem() - skipItems(1) val summary = aRoomSummaryDetails() - initialState.eventSink(RoomSelectEvents.SetSelectedRoom(summary)) assertThat(awaitItem().selectedRooms).isEqualTo(persistentListOf(summary)) - initialState.eventSink(RoomSelectEvents.RemoveSelectedRoom) assertThat(awaitItem().selectedRooms).isEmpty() + cancel() } } - private fun aPresenter( + private fun TestScope.createRoomSelectPresenter( mode: RoomSelectMode = RoomSelectMode.Forward, - client: FakeMatrixClient = FakeMatrixClient(), + roomListService: RoomListService = FakeRoomListService(), ) = RoomSelectPresenter( mode = mode, - client = client, + dataSource = RoomSelectSearchDataSource( + roomListService = roomListService, + coroutineDispatchers = testCoroutineDispatchers(), + ), ) } From 063fd484228dd12d2c5fe0b3f99e3ca5eafcde60 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 30 May 2024 17:40:14 +0200 Subject: [PATCH 6/9] Rename function. --- .../impl/report/ReportMessagePresenterTest.kt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/report/ReportMessagePresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/report/ReportMessagePresenterTest.kt index 405070fab6..b5408434de 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/report/ReportMessagePresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/report/ReportMessagePresenterTest.kt @@ -37,7 +37,7 @@ class ReportMessagePresenterTest { @Test fun `presenter - initial state`() = runTest { - val presenter = aPresenter() + val presenter = createReportMessagePresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -50,7 +50,7 @@ class ReportMessagePresenterTest { @Test fun `presenter - update reason`() = runTest { - val presenter = aPresenter() + val presenter = createReportMessagePresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -64,7 +64,7 @@ class ReportMessagePresenterTest { @Test fun `presenter - toggle block user`() = runTest { - val presenter = aPresenter() + val presenter = createReportMessagePresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -82,7 +82,7 @@ class ReportMessagePresenterTest { @Test fun `presenter - handle successful report and block user`() = runTest { val room = FakeMatrixRoom() - val presenter = aPresenter(matrixRoom = room) + val presenter = createReportMessagePresenter(matrixRoom = room) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -99,7 +99,7 @@ class ReportMessagePresenterTest { @Test fun `presenter - handle successful report`() = runTest { val room = FakeMatrixRoom() - val presenter = aPresenter(matrixRoom = room) + val presenter = createReportMessagePresenter(matrixRoom = room) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -116,7 +116,7 @@ class ReportMessagePresenterTest { val room = FakeMatrixRoom().apply { givenReportContentResult(Result.failure(Exception("Failed to report content"))) } - val presenter = aPresenter(matrixRoom = room) + val presenter = createReportMessagePresenter(matrixRoom = room) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -132,7 +132,7 @@ class ReportMessagePresenterTest { } } - private fun aPresenter( + private fun createReportMessagePresenter( inputs: ReportMessagePresenter.Inputs = ReportMessagePresenter.Inputs(AN_EVENT_ID, A_USER_ID), matrixRoom: MatrixRoom = FakeMatrixRoom(), snackbarDispatcher: SnackbarDispatcher = SnackbarDispatcher(), From ba7481ed6ab8c412d7fd35224e8879845d82e1e6 Mon Sep 17 00:00:00 2001 From: ElementBot Date: Fri, 31 May 2024 07:11:08 +0000 Subject: [PATCH 7/9] Update screenshots --- ...ull_ForwardMessagesView-Day-2_2_null_3,NEXUS_5,1.0,en].png | 4 ++-- ...l_ForwardMessagesView-Night-2_3_null_3,NEXUS_5,1.0,en].png | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Day-2_2_null_3,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Day-2_2_null_3,NEXUS_5,1.0,en].png index 850b46049a..033d95a4a7 100644 --- a/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Day-2_2_null_3,NEXUS_5,1.0,en].png +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Day-2_2_null_3,NEXUS_5,1.0,en].png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:a8e4490d507c06fae92e6f05aed7c31ae5a20e2a52bc23efc7db7d6e1290dc36 -size 10807 +oid sha256:fa56a2ab854d0d54d78ab3f25f0347b83d4a0df9f9fef64ad455b2192ee6b206 +size 10934 diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Night-2_3_null_3,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Night-2_3_null_3,NEXUS_5,1.0,en].png index a4bbfff73f..3aa24e0dba 100644 --- a/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Night-2_3_null_3,NEXUS_5,1.0,en].png +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[f.messages.impl.forward_ForwardMessagesView_null_ForwardMessagesView-Night-2_3_null_3,NEXUS_5,1.0,en].png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:a90e9ecf5cc6c042d9a72401b378ae0de46244803a9729fe339df7b7b676e603 -size 8225 +oid sha256:31ec9cd927ccec096cddc0a76cb44260b1777081e59e466c28384607e5ed2e2d +size 8376 From a997289e7ee555cd9d7891b953b288d6a930cd19 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 31 May 2024 10:55:13 +0200 Subject: [PATCH 8/9] Improve comment --- .../android/libraries/matrix/api/roomlist/RoomListFilter.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt index 4af0e463fd..271b1d9b24 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/roomlist/RoomListFilter.kt @@ -20,7 +20,7 @@ sealed interface RoomListFilter { companion object { /** * Create a filter that matches all the given filters. - * If not filters are provided, all the room will match. + * If no filters are provided, all the rooms will match. */ fun all(vararg filters: RoomListFilter): RoomListFilter { return All(filters.toList()) From c37d9ca6a46d6361582fb55b49a6b54e6f4d9afb Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 31 May 2024 10:56:28 +0200 Subject: [PATCH 9/9] Add doc on RoomSelectSearchDataSource --- .../libraries/roomselect/impl/RoomSelectSearchDataSource.kt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt index a6705e65ba..021c597e1e 100644 --- a/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt +++ b/libraries/roomselect/impl/src/main/kotlin/io/element/android/libraries/roomselect/impl/RoomSelectSearchDataSource.kt @@ -34,6 +34,10 @@ import javax.inject.Inject private const val PAGE_SIZE = 30 +/** + * DataSource for RoomSummaryDetails that can be filtered by a search query, + * and which only includes rooms the user has joined. + */ class RoomSelectSearchDataSource @Inject constructor( roomListService: RoomListService, coroutineDispatchers: CoroutineDispatchers,