From 1dbf2319ff677360f67e05b6a8ee1750810c9008 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 18 Nov 2025 17:37:15 +0100 Subject: [PATCH] Avoid creating many PowerLevelRoomMemberComparator instance and also avoid doing the sorting twice. --- .../impl/roles/ChangeRolesPresenter.kt | 14 +++-------- .../impl/roles/ChangeRolesState.kt | 23 ++++++++++++------- .../impl/roles/ChangeRolesStateProvider.kt | 14 ++++++++--- .../impl/roles/ChangeRolesView.kt | 2 +- .../impl/roles/MembersByRoleTest.kt | 5 ++-- 5 files changed, 33 insertions(+), 25 deletions(-) diff --git a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesPresenter.kt b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesPresenter.kt index 2b02cdfdb2..86012462ce 100644 --- a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesPresenter.kt +++ b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesPresenter.kt @@ -61,6 +61,8 @@ class ChangeRolesPresenter( fun create(role: RoomMember.Role): ChangeRolesPresenter } + private val powerLevelRoomMemberComparator = PowerLevelRoomMemberComparator() + @Composable override fun present(): ChangeRolesState { val dataSource = remember { RoomMemberListDataSource(room, dispatchers) } @@ -176,17 +178,7 @@ class ChangeRolesPresenter( } private fun List.groupedByRole(): MembersByRole { - val groupedMembers = MembersByRole(this) - return MembersByRole( - owners = groupedMembers.owners.sorted(), - admins = groupedMembers.admins.sorted(), - moderators = groupedMembers.moderators.sorted(), - members = groupedMembers.members.sorted(), - ) - } - - private fun Iterable.sorted(): ImmutableList { - return sortedWith(PowerLevelRoomMemberComparator()).toImmutableList() + return MembersByRole(this, powerLevelRoomMemberComparator) } private fun CoroutineScope.save( diff --git a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesState.kt b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesState.kt index 45ab913a31..6bcc13fb9a 100644 --- a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesState.kt +++ b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesState.kt @@ -13,8 +13,8 @@ import io.element.android.libraries.designsystem.theme.components.SearchBarResul import io.element.android.libraries.matrix.api.core.UserId import io.element.android.libraries.matrix.api.room.RoomMember import io.element.android.libraries.matrix.api.user.MatrixUser -import io.element.android.libraries.matrix.ui.room.PowerLevelRoomMemberComparator import kotlinx.collections.immutable.ImmutableList +import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList data class ChangeRolesState( @@ -35,16 +35,23 @@ data class MembersByRole( val moderators: ImmutableList, val members: ImmutableList, ) { - constructor(members: List) : this( - owners = members.filter { it.role is RoomMember.Role.Owner }.sorted(), - admins = members.filter { it.role == RoomMember.Role.Admin }.sorted(), - moderators = members.filter { it.role == RoomMember.Role.Moderator }.sorted(), - members = members.filter { it.role == RoomMember.Role.User }.sorted(), + constructor() : this( + owners = persistentListOf(), + admins = persistentListOf(), + moderators = persistentListOf(), + members = persistentListOf(), + ) + + constructor(members: List, comparator: Comparator) : this( + owners = members.filter { it.role is RoomMember.Role.Owner }.sorted(comparator), + admins = members.filter { it.role == RoomMember.Role.Admin }.sorted(comparator), + moderators = members.filter { it.role == RoomMember.Role.Moderator }.sorted(comparator), + members = members.filter { it.role == RoomMember.Role.User }.sorted(comparator), ) fun isEmpty() = owners.isEmpty() && admins.isEmpty() && moderators.isEmpty() && members.isEmpty() } -private fun Iterable.sorted(): ImmutableList { - return sortedWith(PowerLevelRoomMemberComparator()).toImmutableList() +private fun Iterable.sorted(comparator: Comparator): ImmutableList { + return sortedWith(comparator).toImmutableList() } diff --git a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesStateProvider.kt b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesStateProvider.kt index 14662bacdd..df74b71ae1 100644 --- a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesStateProvider.kt +++ b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesStateProvider.kt @@ -17,6 +17,7 @@ import io.element.android.libraries.matrix.api.room.RoomMembershipState import io.element.android.libraries.matrix.api.user.MatrixUser import io.element.android.libraries.matrix.ui.components.aMatrixUser import io.element.android.libraries.matrix.ui.components.aMatrixUserList +import io.element.android.libraries.matrix.ui.room.PowerLevelRoomMemberComparator import io.element.android.libraries.previewutils.room.aRoomMember import io.element.android.libraries.previewutils.room.aRoomMemberList import kotlinx.collections.immutable.ImmutableList @@ -36,7 +37,12 @@ class ChangeRolesStateProvider : PreviewParameterProvider { aChangeRolesStateWithSelectedUsers().copy( query = "Alice", isSearchActive = true, - searchResults = SearchBarResultState.Results(MembersByRole(aRoomMemberList().take(1).toImmutableList())), + searchResults = SearchBarResultState.Results( + MembersByRole( + members = aRoomMemberList().take(1), + comparator = PowerLevelRoomMemberComparator(), + ) + ), selectedUsers = aMatrixUserList().take(1).toImmutableList(), ), aChangeRolesStateWithSelectedUsers().copy(savingState = AsyncAction.ConfirmingCancellation), @@ -87,7 +93,8 @@ internal fun aChangeRolesStateWithSelectedUsers() = aChangeRolesState( } else { roomMember } - } + }, + comparator = PowerLevelRoomMemberComparator(), ) ), hasPendingChanges = true, @@ -126,7 +133,8 @@ internal fun aChangeRolesStateWithOwners( displayName = "David", role = RoomMember.Role.User, ), - ) + ), + comparator = PowerLevelRoomMemberComparator(), ), ), canRemoveMember = { userId -> diff --git a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesView.kt b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesView.kt index 81b11aa837..3a981b66b6 100644 --- a/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesView.kt +++ b/features/rolesandpermissions/impl/src/main/kotlin/io/element/android/features/rolesandpermissions/impl/roles/ChangeRolesView.kt @@ -145,7 +145,7 @@ fun ChangeRolesView( SearchResultsList( currentRole = state.role, lazyListState = lazyListState, - searchResults = (state.searchResults as? SearchBarResultState.Results)?.results ?: MembersByRole(emptyList()), + searchResults = (state.searchResults as? SearchBarResultState.Results)?.results ?: MembersByRole(), selectedUsers = state.selectedUsers, canRemoveMember = state.canChangeMemberRole, onToggleSelection = { state.eventSink(ChangeRolesEvent.UserSelectionToggled(it.toMatrixUser())) }, diff --git a/features/rolesandpermissions/impl/src/test/kotlin/io/element/android/features/rolesandpermissions/impl/roles/MembersByRoleTest.kt b/features/rolesandpermissions/impl/src/test/kotlin/io/element/android/features/rolesandpermissions/impl/roles/MembersByRoleTest.kt index 2e1d18a6ca..1de879abdc 100644 --- a/features/rolesandpermissions/impl/src/test/kotlin/io/element/android/features/rolesandpermissions/impl/roles/MembersByRoleTest.kt +++ b/features/rolesandpermissions/impl/src/test/kotlin/io/element/android/features/rolesandpermissions/impl/roles/MembersByRoleTest.kt @@ -18,6 +18,7 @@ import io.element.android.libraries.matrix.test.A_USER_ID_5 import io.element.android.libraries.matrix.test.A_USER_ID_6 import io.element.android.libraries.matrix.test.A_USER_ID_7 import io.element.android.libraries.matrix.test.room.aRoomMember +import io.element.android.libraries.matrix.ui.room.PowerLevelRoomMemberComparator import kotlinx.collections.immutable.persistentListOf import org.junit.Test @@ -33,7 +34,7 @@ class MembersByRoleTest { aRoomMember(A_USER_ID_6, displayName = "Justin", role = RoomMember.Role.Owner(isCreator = true)), aRoomMember(A_USER_ID_7, displayName = "Mallory", role = RoomMember.Role.Owner(isCreator = false)), ) - val membersByRole = MembersByRole(members = members) + val membersByRole = MembersByRole(members = members, comparator = PowerLevelRoomMemberComparator()) assertThat(membersByRole.owners).containsExactly( aRoomMember(A_USER_ID_6, displayName = "Justin", role = RoomMember.Role.Owner(isCreator = true)), aRoomMember(A_USER_ID_7, displayName = "Mallory", role = RoomMember.Role.Owner(isCreator = false)), @@ -52,7 +53,7 @@ class MembersByRoleTest { @Test fun `isEmpty - only returns true with no members of any role`() { - val emptyMembersByRole = MembersByRole(emptyList()) + val emptyMembersByRole = MembersByRole() assertThat(emptyMembersByRole.isEmpty()).isTrue() val membersByRoleWithOwners = MembersByRole(