Improve room member list loading UX (#2543)
Improve room member list UX: - Don't display the list in chunks anymore. - Use an indeterminate linear progress indicator to display some loading is being done (either loading the cached list or the updated one). - Try to make sure we don't display the members loaded from timeline items as the cached room list by mistake. * Update screenshots * Simplify member loading logic. --------- Co-authored-by: ElementBot <benoitm+elementbot@element.io>
This commit is contained in:
Jorge Martin Espinosa
GitHub
parent
7050550511
commit
e3a395f15a
19 changed files with 268 additions and 196 deletions
|
|
@ -162,7 +162,8 @@ class RustMatrixRoom(
|
|||
|
||||
init {
|
||||
timeline.membershipChangeEventReceived
|
||||
.onEach { roomMemberListFetcher.fetchRoomMembers() }
|
||||
// The new events should already be in the SDK cache, no need to fetch them from the server
|
||||
.onEach { roomMemberListFetcher.fetchRoomMembers(source = RoomMemberListFetcher.Source.CACHE) }
|
||||
.launchIn(roomCoroutineScope)
|
||||
}
|
||||
|
||||
|
|
@ -219,7 +220,15 @@ class RustMatrixRoom(
|
|||
override val activeMemberCount: Long
|
||||
get() = innerRoom.activeMembersCount().toLong()
|
||||
|
||||
override suspend fun updateMembers() = roomMemberListFetcher.fetchRoomMembers()
|
||||
override suspend fun updateMembers() {
|
||||
val useCache = membersStateFlow.value is MatrixRoomMembersState.Unknown
|
||||
val source = if (useCache) {
|
||||
RoomMemberListFetcher.Source.CACHE_AND_SERVER
|
||||
} else {
|
||||
RoomMemberListFetcher.Source.SERVER
|
||||
}
|
||||
roomMemberListFetcher.fetchRoomMembers(source = source)
|
||||
}
|
||||
|
||||
override suspend fun userDisplayName(userId: UserId): Result<String?> = withContext(roomDispatcher) {
|
||||
runCatching {
|
||||
|
|
|
|||
|
|
@ -16,9 +16,10 @@
|
|||
|
||||
package io.element.android.libraries.matrix.impl.room.member
|
||||
|
||||
import io.element.android.libraries.core.coroutine.parallelMap
|
||||
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
|
||||
import io.element.android.libraries.matrix.api.room.RoomMember
|
||||
import io.element.android.libraries.matrix.api.room.roomMembers
|
||||
import kotlinx.collections.immutable.ImmutableList
|
||||
import kotlinx.collections.immutable.toImmutableList
|
||||
import kotlinx.coroutines.CoroutineDispatcher
|
||||
import kotlinx.coroutines.ensureActive
|
||||
|
|
@ -42,6 +43,11 @@ internal class RoomMemberListFetcher(
|
|||
private val dispatcher: CoroutineDispatcher,
|
||||
private val pageSize: Int = 10_000,
|
||||
) {
|
||||
enum class Source {
|
||||
CACHE,
|
||||
CACHE_AND_SERVER,
|
||||
SERVER,
|
||||
}
|
||||
private val updatedRoomMemberMutex = Mutex()
|
||||
private val roomId = room.id()
|
||||
|
||||
|
|
@ -51,73 +57,86 @@ internal class RoomMemberListFetcher(
|
|||
/**
|
||||
* Fetches the room members for the given room.
|
||||
* It will emit the cached members first, and then the updated members in batches of [pageSize] items, through [membersFlow].
|
||||
* @param withCache Whether to load the cached members first. Defaults to true.
|
||||
* @param source Where we should load the members from. Defaults to [Source.CACHE_AND_SERVER].
|
||||
*/
|
||||
suspend fun fetchRoomMembers(withCache: Boolean = true) {
|
||||
suspend fun fetchRoomMembers(source: Source = Source.CACHE_AND_SERVER) {
|
||||
if (updatedRoomMemberMutex.isLocked) {
|
||||
Timber.i("Room members are already being updated for room $roomId")
|
||||
return
|
||||
}
|
||||
updatedRoomMemberMutex.withLock {
|
||||
withContext(dispatcher) {
|
||||
// Load cached members as fallback and to get faster results
|
||||
if (withCache) {
|
||||
if (_membersFlow.value !is MatrixRoomMembersState.Ready) {
|
||||
fetchCachedRoomMembers()
|
||||
} else {
|
||||
Timber.i("Cached members not found for $roomId")
|
||||
_membersFlow.run {
|
||||
when (source) {
|
||||
Source.CACHE -> {
|
||||
fetchCachedRoomMembers(asPendingState = false)
|
||||
}
|
||||
Source.CACHE_AND_SERVER -> {
|
||||
fetchCachedRoomMembers(asPendingState = true)
|
||||
fetchRemoteRoomMembers()
|
||||
}
|
||||
Source.SERVER -> {
|
||||
fetchRemoteRoomMembers()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
val prevRoomMembers = (_membersFlow.value as? MatrixRoomMembersState.Ready)?.roomMembers?.toImmutableList()
|
||||
_membersFlow.value = MatrixRoomMembersState.Pending(prevRoomMembers = prevRoomMembers)
|
||||
|
||||
try {
|
||||
// Start loading new members
|
||||
parseAndEmitMembers(room.members())
|
||||
} catch (exception: CancellationException) {
|
||||
Timber.d("Cancelled loading updated members for room $roomId")
|
||||
throw exception
|
||||
} catch (exception: Exception) {
|
||||
Timber.e(exception, "Failed to load updated members for room $roomId")
|
||||
_membersFlow.value = MatrixRoomMembersState.Error(exception, prevRoomMembers)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
internal suspend fun fetchCachedRoomMembers() = withContext(dispatcher) {
|
||||
private suspend fun MutableStateFlow<MatrixRoomMembersState>.fetchCachedRoomMembers(asPendingState: Boolean = true) {
|
||||
Timber.i("Loading cached members for room $roomId")
|
||||
try {
|
||||
val iterator = room.membersNoSync()
|
||||
parseAndEmitMembers(iterator)
|
||||
// Send current member list with pending state to notify the UI that we are loading new members
|
||||
emit(pendingWithCurrentMembers())
|
||||
val members = parseAndEmitMembers(room.membersNoSync())
|
||||
val newState = if (asPendingState) {
|
||||
MatrixRoomMembersState.Pending(prevRoomMembers = members)
|
||||
} else {
|
||||
MatrixRoomMembersState.Ready(members)
|
||||
}
|
||||
emit(newState)
|
||||
} catch (exception: CancellationException) {
|
||||
Timber.d("Cancelled loading cached members for room $roomId")
|
||||
throw exception
|
||||
} catch (exception: Exception) {
|
||||
Timber.e(exception, "Failed to load cached members for room $roomId")
|
||||
_membersFlow.value = MatrixRoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList())
|
||||
emit(MatrixRoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList()))
|
||||
}
|
||||
}
|
||||
|
||||
private suspend fun parseAndEmitMembers(roomMembersIterator: RoomMembersIterator) {
|
||||
roomMembersIterator.use { iterator ->
|
||||
val results = buildList {
|
||||
private suspend fun MutableStateFlow<MatrixRoomMembersState>.fetchRemoteRoomMembers() {
|
||||
try {
|
||||
// Send current member list with pending state to notify the UI that we are loading new members
|
||||
emit(pendingWithCurrentMembers())
|
||||
// Start loading new members
|
||||
emit(MatrixRoomMembersState.Ready(parseAndEmitMembers(room.members())))
|
||||
} catch (exception: CancellationException) {
|
||||
Timber.d("Cancelled loading updated members for room $roomId")
|
||||
throw exception
|
||||
} catch (exception: Exception) {
|
||||
Timber.e(exception, "Failed to load updated members for room $roomId")
|
||||
emit(MatrixRoomMembersState.Error(exception, _membersFlow.value.roomMembers()?.toImmutableList()))
|
||||
}
|
||||
}
|
||||
|
||||
private suspend fun parseAndEmitMembers(roomMembersIterator: RoomMembersIterator): ImmutableList<RoomMember> {
|
||||
return roomMembersIterator.use { iterator ->
|
||||
val results = buildList(capacity = roomMembersIterator.len().toInt()) {
|
||||
while (true) {
|
||||
// Loading the whole membersIterator as a stop-gap measure.
|
||||
// We should probably implement some sort of paging in the future.
|
||||
coroutineContext.ensureActive()
|
||||
val chunk = iterator.nextChunk(pageSize.toUInt())
|
||||
// Load next chunk. If null (no more items), exit the loop
|
||||
val members = chunk?.parallelMap(RoomMemberMapper::map) ?: break
|
||||
val members = chunk?.map(RoomMemberMapper::map) ?: break
|
||||
addAll(members)
|
||||
Timber.i("Emitting first $size members for room $roomId")
|
||||
_membersFlow.value = MatrixRoomMembersState.Ready(toImmutableList())
|
||||
Timber.i("Loaded first $size members for room $roomId")
|
||||
}
|
||||
}
|
||||
if (results.isEmpty()) {
|
||||
_membersFlow.value = MatrixRoomMembersState.Ready(results.toImmutableList())
|
||||
}
|
||||
results.toImmutableList()
|
||||
}
|
||||
}
|
||||
|
||||
private fun pendingWithCurrentMembers() = MatrixRoomMembersState.Pending(_membersFlow.value.roomMembers().orEmpty().toImmutableList())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -21,6 +21,9 @@ import com.google.common.truth.Truth.assertThat
|
|||
import io.element.android.libraries.matrix.api.core.UserId
|
||||
import io.element.android.libraries.matrix.api.room.MatrixRoomMembersState
|
||||
import io.element.android.libraries.matrix.api.room.roomMembers
|
||||
import io.element.android.libraries.matrix.impl.room.member.RoomMemberListFetcher.Source.CACHE
|
||||
import io.element.android.libraries.matrix.impl.room.member.RoomMemberListFetcher.Source.CACHE_AND_SERVER
|
||||
import io.element.android.libraries.matrix.impl.room.member.RoomMemberListFetcher.Source.SERVER
|
||||
import io.element.android.libraries.matrix.test.A_ROOM_ID
|
||||
import io.element.android.libraries.matrix.test.A_USER_ID
|
||||
import io.element.android.libraries.matrix.test.A_USER_ID_2
|
||||
|
|
@ -38,7 +41,7 @@ import uniffi.matrix_sdk.RoomMemberRole
|
|||
|
||||
class RoomMemberListFetcherTest {
|
||||
@Test
|
||||
fun `fetchCachedRoomMembers - emits cached members, if any`() = runTest {
|
||||
fun `fetchRoomMembers with CACHE source - emits cached members, if any`() = runTest {
|
||||
val room = FakeRustRoom(getMembersNoSync = {
|
||||
FakeRoomMembersIterator(
|
||||
listOf(
|
||||
|
|
@ -53,44 +56,53 @@ class RoomMemberListFetcherTest {
|
|||
fetcher.membersFlow.test {
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
|
||||
fetcher.fetchCachedRoomMembers()
|
||||
fetcher.fetchRoomMembers(source = CACHE)
|
||||
|
||||
val readyItem = awaitItem()
|
||||
assertThat(readyItem).isInstanceOf(MatrixRoomMembersState.Ready::class.java)
|
||||
assertThat((readyItem as? MatrixRoomMembersState.Ready)?.roomMembers?.size).isEqualTo(3)
|
||||
// Loading state
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
|
||||
val cachedItemsState = awaitItem()
|
||||
assertThat(cachedItemsState).isInstanceOf(MatrixRoomMembersState.Ready::class.java)
|
||||
assertThat((cachedItemsState as? MatrixRoomMembersState.Ready)?.roomMembers).hasSize(3)
|
||||
|
||||
// Assert only the 'no sync' method was called, so no new member sync happened
|
||||
assertThat(room.membersNoSyncCallCount).isEqualTo(1)
|
||||
assertThat(room.membersCallCount).isEqualTo(0)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchCachedRoomMembers - emits empty list, if no members exist`() = runTest {
|
||||
fun `fetchRoomMembers with CACHE source - emits empty list, if no members exist`() = runTest {
|
||||
val room = FakeRustRoom(getMembersNoSync = {
|
||||
FakeRoomMembersIterator(emptyList())
|
||||
})
|
||||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default)
|
||||
fetcher.membersFlow.test {
|
||||
fetcher.fetchCachedRoomMembers()
|
||||
fetcher.fetchRoomMembers(source = CACHE)
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
assertThat(awaitItem().roomMembers()).isEmpty()
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
assertThat((awaitItem() as? MatrixRoomMembersState.Ready)?.roomMembers).isEmpty()
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchCachedRoomMembers - emits Error on error found`() = runTest {
|
||||
fun `fetchRoomMembers with CACHE source - emits Error on error found`() = runTest {
|
||||
val room = FakeRustRoom(getMembersNoSync = {
|
||||
error("Some unexpected issue")
|
||||
})
|
||||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default)
|
||||
fetcher.membersFlow.test {
|
||||
fetcher.fetchCachedRoomMembers()
|
||||
fetcher.fetchRoomMembers(source = CACHE)
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Error::class.java)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchCachedRoomMembers - emits items using page size`() = runTest {
|
||||
fun `fetchRoomMembers with CACHE source - emits all items at once`() = runTest {
|
||||
val room = FakeRustRoom(getMembersNoSync = {
|
||||
FakeRoomMembersIterator(
|
||||
listOf(
|
||||
|
|
@ -103,18 +115,21 @@ class RoomMemberListFetcherTest {
|
|||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default, pageSize = 2)
|
||||
fetcher.membersFlow.test {
|
||||
fetcher.fetchCachedRoomMembers()
|
||||
fetcher.fetchRoomMembers(source = CACHE)
|
||||
|
||||
// Initial state
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
assertThat((awaitItem() as? MatrixRoomMembersState.Ready)?.roomMembers?.size).isEqualTo(2)
|
||||
assertThat((awaitItem() as? MatrixRoomMembersState.Ready)?.roomMembers?.size).isEqualTo(3)
|
||||
// Started loading cached members
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
// Finished loading cached members
|
||||
assertThat((awaitItem() as? MatrixRoomMembersState.Ready)?.roomMembers).hasSize(3)
|
||||
|
||||
ensureAllEventsConsumed()
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchRoomMembers - with 'withCache' set to false emits only new members, if any`() = runTest {
|
||||
fun `fetchRoomMembers with SERVER source - emits only new members, if any`() = runTest {
|
||||
val room = FakeRustRoom(getMembers = {
|
||||
FakeRoomMembersIterator(
|
||||
listOf(
|
||||
|
|
@ -127,21 +142,25 @@ class RoomMemberListFetcherTest {
|
|||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default)
|
||||
fetcher.membersFlow.test {
|
||||
fetcher.fetchRoomMembers(withCache = false)
|
||||
fetcher.fetchRoomMembers(source = SERVER)
|
||||
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
assertThat((awaitItem() as? MatrixRoomMembersState.Ready)?.roomMembers?.size).isEqualTo(3)
|
||||
|
||||
// Assert only the 'sync' method was called, so a new member sync happened
|
||||
assertThat(room.membersNoSyncCallCount).isEqualTo(0)
|
||||
assertThat(room.membersCallCount).isEqualTo(1)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchRoomMembers - on error it emits an Error item`() = runTest {
|
||||
fun `fetchRoomMembers with SERVER source - on error it emits an Error item`() = runTest {
|
||||
val room = FakeRustRoom(getMembers = { error("An unexpected error") })
|
||||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default)
|
||||
fetcher.membersFlow.test {
|
||||
fetcher.fetchRoomMembers(withCache = false)
|
||||
fetcher.fetchRoomMembers(source = SERVER)
|
||||
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
|
|
@ -150,7 +169,7 @@ class RoomMemberListFetcherTest {
|
|||
}
|
||||
|
||||
@Test
|
||||
fun `fetchRoomMembers - with 'withCache' returns cached items first, then new ones`() = runTest {
|
||||
fun `fetchRoomMembers with CACHE_AND_SERVER source - returns cached items first, then new ones`() = runTest {
|
||||
val room = FakeRustRoom(
|
||||
getMembersNoSync = {
|
||||
FakeRoomMembersIterator(listOf(fakeRustRoomMember(A_USER_ID_4)))
|
||||
|
|
@ -168,56 +187,28 @@ class RoomMemberListFetcherTest {
|
|||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default)
|
||||
fetcher.membersFlow.test {
|
||||
fetcher.fetchRoomMembers(withCache = true)
|
||||
fetcher.fetchRoomMembers(source = CACHE_AND_SERVER)
|
||||
// Initial
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Unknown::class.java)
|
||||
// Loading cached
|
||||
awaitItem().let { pending ->
|
||||
assertThat(pending).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
assertThat(pending.roomMembers()).isEmpty()
|
||||
}
|
||||
// Loaded cached
|
||||
awaitItem().let { cached ->
|
||||
assertThat(cached).isInstanceOf(MatrixRoomMembersState.Ready::class.java)
|
||||
assertThat(cached).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
assertThat(cached.roomMembers()).hasSize(1)
|
||||
}
|
||||
// Start loading new
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
awaitItem().let { ready ->
|
||||
assertThat(ready).isInstanceOf(MatrixRoomMembersState.Ready::class.java)
|
||||
assertThat(ready.roomMembers()).hasSize(3)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `fetchRoomMembers - with 'withCache' skips cache if there is already a ready state`() = runTest {
|
||||
val room = FakeRustRoom(
|
||||
getMembersNoSync = {
|
||||
FakeRoomMembersIterator(listOf(fakeRustRoomMember(A_USER_ID_4)))
|
||||
},
|
||||
getMembers = {
|
||||
FakeRoomMembersIterator(
|
||||
listOf(
|
||||
fakeRustRoomMember(A_USER_ID),
|
||||
fakeRustRoomMember(A_USER_ID_2),
|
||||
fakeRustRoomMember(A_USER_ID_3),
|
||||
)
|
||||
)
|
||||
}
|
||||
)
|
||||
|
||||
val fetcher = RoomMemberListFetcher(room, Dispatchers.Default)
|
||||
// Set a ready state
|
||||
fetcher.fetchRoomMembers(withCache = false)
|
||||
|
||||
fetcher.membersFlow.test {
|
||||
// Start loading new members
|
||||
fetcher.fetchRoomMembers(withCache = true)
|
||||
// Previous ready state
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Ready::class.java)
|
||||
// New pending state
|
||||
assertThat(awaitItem()).isInstanceOf(MatrixRoomMembersState.Pending::class.java)
|
||||
// New ready state
|
||||
awaitItem().let { ready ->
|
||||
assertThat(ready).isInstanceOf(MatrixRoomMembersState.Ready::class.java)
|
||||
assertThat(ready.roomMembers()).hasSize(3)
|
||||
}
|
||||
// Assert both member methods were called, so both the cache was hit and a new member sync happened
|
||||
assertThat(room.membersNoSyncCallCount).isEqualTo(1)
|
||||
assertThat(room.membersCallCount).isEqualTo(1)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -226,15 +217,20 @@ class FakeRustRoom(
|
|||
private val getMembers: () -> RoomMembersIterator = { FakeRoomMembersIterator() },
|
||||
private val getMembersNoSync: () -> RoomMembersIterator = { FakeRoomMembersIterator() },
|
||||
) : Room(NoPointer) {
|
||||
var membersCallCount = 0
|
||||
var membersNoSyncCallCount = 0
|
||||
|
||||
override fun id(): String {
|
||||
return A_ROOM_ID.value
|
||||
}
|
||||
|
||||
override suspend fun members(): RoomMembersIterator {
|
||||
membersCallCount++
|
||||
return getMembers()
|
||||
}
|
||||
|
||||
override suspend fun membersNoSync(): RoomMembersIterator {
|
||||
membersNoSyncCallCount++
|
||||
return getMembersNoSync()
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue