From b364cee500de37a416d4027e61d5d0102dbd9326 Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 18 Sep 2024 21:07:39 +0200 Subject: [PATCH 1/2] Room list : debounce subscribe to visible rooms. --- .../android/features/roomlist/impl/RoomListPresenter.kt | 8 ++++++-- .../libraries/matrix/impl/roomlist/RustRoomListService.kt | 6 +----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt b/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt index 2983b6b695..bd65b2634d 100644 --- a/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt +++ b/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt @@ -61,7 +61,7 @@ import kotlinx.collections.immutable.toPersistentList import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job -import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.first @@ -73,6 +73,7 @@ import kotlinx.coroutines.launch import javax.inject.Inject private const val EXTENDED_RANGE_SIZE = 40 +private const val SUBSCRIBE_TO_VISIBLE_ROOMS_DEBOUNCE_IN_MILLIS = 300L class RoomListPresenter @Inject constructor( private val client: MatrixClient, @@ -301,7 +302,10 @@ class RoomListPresenter @Inject constructor( private var currentUpdateVisibleRangeJob: Job? = null private fun CoroutineScope.updateVisibleRange(range: IntRange) { currentUpdateVisibleRangeJob?.cancel() - currentUpdateVisibleRangeJob = launch(SupervisorJob()) { + currentUpdateVisibleRangeJob = launch { + // Debounce the subscription to avoid subscribing to too many rooms + delay(SUBSCRIBE_TO_VISIBLE_ROOMS_DEBOUNCE_IN_MILLIS) + if (range.isEmpty()) return@launch val currentRoomList = roomListDataSource.allRooms.first() // Use extended range to 'prefetch' the next rooms info diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RustRoomListService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RustRoomListService.kt index b203ec958f..ee0be05e4b 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RustRoomListService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RustRoomListService.kt @@ -53,11 +53,7 @@ internal class RustRoomListService( } override suspend fun subscribeToVisibleRooms(roomIds: List) { - val toSubscribe = roomIds.filterNot { roomSyncSubscriber.isSubscribedTo(it) } - if (toSubscribe.isNotEmpty()) { - Timber.d("Subscribe to ${toSubscribe.size} rooms: $toSubscribe") - roomSyncSubscriber.batchSubscribe(toSubscribe) - } + roomSyncSubscriber.batchSubscribe(roomIds) } override val allRooms: DynamicRoomList = roomListFactory.createRoomList( From 4b2ea110371974fcd62f0768ac6502370bd8137b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Thu, 19 Sep 2024 08:15:12 +0200 Subject: [PATCH 2/2] Fix and add test --- .../roomlist/impl/RoomListPresenterTest.kt | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTest.kt b/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTest.kt index 8b7294b165..c77f59a0a8 100644 --- a/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTest.kt +++ b/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTest.kt @@ -85,13 +85,16 @@ import io.element.android.tests.testutils.lambda.value import io.element.android.tests.testutils.test import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test +import kotlin.time.Duration.Companion.seconds class RoomListPresenterTest { @get:Rule @@ -599,6 +602,38 @@ class RoomListPresenterTest { } } + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `present - UpdateVisibleRange will cancel the previous subscription if called too soon`() = runTest { + val subscribeToVisibleRoomsLambda = lambdaRecorder { _: List -> } + val roomListService = FakeRoomListService(subscribeToVisibleRoomsLambda = subscribeToVisibleRoomsLambda) + val scope = CoroutineScope(coroutineContext + SupervisorJob()) + val matrixClient = FakeMatrixClient( + roomListService = roomListService, + ) + val roomSummary = aRoomSummary( + currentUserMembership = CurrentUserMembership.INVITED + ) + roomListService.postAllRoomsLoadingState(RoomList.LoadingState.Loaded(1)) + roomListService.postAllRooms(listOf(roomSummary)) + val presenter = createRoomListPresenter( + coroutineScope = scope, + client = matrixClient, + ) + presenter.test { + val state = consumeItemsUntilPredicate { + it.contentState is RoomListContentState.Rooms + }.last() + + state.eventSink(RoomListEvents.UpdateVisibleRange(IntRange(0, 10))) + // If called again, it will cancel the current one, which should not result in a test failure + state.eventSink(RoomListEvents.UpdateVisibleRange(IntRange(0, 11))) + advanceTimeBy(1.seconds) + subscribeToVisibleRoomsLambda.assertions().isCalledOnce() + } + } + + @OptIn(ExperimentalCoroutinesApi::class) @Test fun `present - UpdateVisibleRange subscribes to rooms in visible range`() = runTest { val subscribeToVisibleRoomsLambda = lambdaRecorder { _: List -> } @@ -622,10 +657,12 @@ class RoomListPresenterTest { }.last() state.eventSink(RoomListEvents.UpdateVisibleRange(IntRange(0, 10))) + advanceTimeBy(1.seconds) subscribeToVisibleRoomsLambda.assertions().isCalledOnce() - // If called again, it will cancel the current one, which should not result in a test failure + // If called again, it will subscribe to the next items state.eventSink(RoomListEvents.UpdateVisibleRange(IntRange(0, 11))) + advanceTimeBy(1.seconds) subscribeToVisibleRoomsLambda.assertions().isCalledExactly(2) } }