Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix slow paged room list regression #6233

Closed
wants to merge 4 commits into from
Closed

Fix slow paged room list regression #6233

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b8eef9b
fix slow paged room list
BillCarsonFr Jun 3, 2022
43b76f7
Fix param name
BillCarsonFr Jun 3, 2022
adf8c5f
code review
BillCarsonFr Jun 7, 2022
89e528d
Changes space subtitle in explore rooms from joined string to direct …
ericdecanini Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6233.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix | performance regression on roomlist + proper display of space parents in explore rooms.
11 changes: 9 additions & 2 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/RoomService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,12 +226,19 @@ interface RoomService {
): LiveData<PagedList<RoomSummary>>

/**
* TODO Doc.
* Get's a live paged list from a filter that can be dynamically updated.
*
* @param queryParams The filter to use
* @param pagedListConfig The paged list configuration (page size, initial load, prefetch distance...)
* @param sortOrder defines how to sort the results
* @param getFlattenParents When true, the list of known parents and grand parents summaries will be resolved.
* This can have significant impact on performance, better be used only on manageable list (filtered by displayName, ..).
*/
fun getFilteredPagedRoomSummariesLive(
queryParams: RoomSummaryQueryParams,
pagedListConfig: PagedList.Config = defaultPagedListConfig,
sortOrder: RoomSortOrder = RoomSortOrder.ACTIVITY
sortOrder: RoomSortOrder = RoomSortOrder.ACTIVITY,
getFlattenParents: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getFlattenParents: Boolean = false,
getFlattenParents: Boolean = false, // Heavily impacts performance when true. Only use when space parent is absolutely required

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but please add this mention in the Kdoc and not here, or it will not be exported.

): UpdatableLivePageResult

/**
Expand Down
5 changes: 3 additions & 2 deletions ...-android/src/main/java/org/matrix/android/sdk/internal/session/room/DefaultRoomService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,10 @@ internal class DefaultRoomService @Inject constructor(
override fun getFilteredPagedRoomSummariesLive(
queryParams: RoomSummaryQueryParams,
pagedListConfig: PagedList.Config,
sortOrder: RoomSortOrder
sortOrder: RoomSortOrder,
getFlattenParents: Boolean
): UpdatableLivePageResult {
return roomSummaryDataSource.getUpdatablePagedRoomSummariesLive(queryParams, pagedListConfig, sortOrder, getFlattenedParents = true)
return roomSummaryDataSource.getUpdatablePagedRoomSummariesLive(queryParams, pagedListConfig, sortOrder, getFlattenParents)
}

override fun getRoomCountLive(queryParams: RoomSummaryQueryParams): LiveData<Int> {
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilderGroup.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class RoomListSectionBuilderGroup(
},
{ qpm ->
val name = stringProvider.getString(R.string.bottom_action_rooms)
val updatableFilterLivePageResult = session.roomService().getFilteredPagedRoomSummariesLive(qpm)
val updatableFilterLivePageResult = session.roomService().getFilteredPagedRoomSummariesLive(qpm, getFlattenParents = true)
onUpdatable(updatableFilterLivePageResult)

val itemCountFlow = updatableFilterLivePageResult.livePagedList.asFlow()
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/im/vector/app/features/home/room/list/RoomListSectionBuilderSpace.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ class RoomListSectionBuilderSpace(
},
{ queryParams ->
val name = stringProvider.getString(R.string.bottom_action_rooms)
val updatableFilterLivePageResult = session.roomService().getFilteredPagedRoomSummariesLive(queryParams)
val updatableFilterLivePageResult = session.roomService().getFilteredPagedRoomSummariesLive(queryParams, getFlattenParents = true)
onUpdatable(updatableFilterLivePageResult)

val itemCountFlow = updatableFilterLivePageResult.livePagedList.asFlow()
Expand Down
4 changes: 3 additions & 1 deletion vector/src/main/java/im/vector/app/features/home/room/list/RoomSummaryItemFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,9 @@ class RoomSummaryItemFactory @Inject constructor(

private fun getSearchResultSubtitle(roomSummary: RoomSummary): String {
val userId = roomSummary.directUserId
val spaceName = roomSummary.spaceParents?.firstOrNull()?.roomSummary?.name
val spaceName = roomSummary.flattenParents
.takeIf { it.isNotEmpty() }
?.joinToString(", ") { it.name }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is nice, I'm thinking now to maintain parity with web, we only take the last element in flattenParents for the subtitle. If design would deem it's better to have all parents separated by comma, we can have a meta ticket to change that across all platforms

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look to me what web is taking the last one only. What are the requirement exactly?
Can you take over it as I believe you are the one that initialy developed it?

val canonicalAlias = roomSummary.canonicalAlias

return (userId ?: spaceName ?: canonicalAlias).orEmpty()
Expand Down