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.

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

Room details: use member counts provided by rust instead of counting members ourselves #505

Closed
Tracked by #988
csmith opened this issue Jun 1, 2023 · 5 comments · Fixed by #530
Closed
Tracked by #988
Assignees

Comments

@csmith
Copy link
Contributor

csmith commented Jun 1, 2023

Currently to show the number of users in a room we wait for all the users to be synced, and then count them.

However, Matrix provides the number of users joined and invited to a room as part of the room metadata. The rust SDK bindings expose this as joinedMembersCount() and invitedMembersCount() on Room (but we don't currently use them on Android).

We should switch to using those, which will increase the performance of at least part of the room details screen.

@Johennes
Copy link
Contributor

Johennes commented Jun 1, 2023

@alfogrillo does this also need changing on iOS?

@alfogrillo
Copy link

@alfogrillo does this also need changing on iOS?

It depends on the UX we want.

Currently: iOS is loading the members on the room's details as Android is doing.
On the plus side the tap/navigation on the members list is instant. I'm not sure this has been done on purpose or not.

If we change it as the proposal above, a loader will spin before (or after) the navigation to the members list until the members are ready.
Currently we also need the owner RoomMember object to check power levels in the room's details.
I don't know if we have a way to fetch just it.

Wdyt @csmith?

@csmith
Copy link
Contributor Author

csmith commented Jun 5, 2023

If we change it as the proposal above, a loader will spin before (or after) the navigation to the members list until the members are ready.

On Android it turns out we already allow the user to click on "Members" while it's still loading (and showing the spinner), and they navigate to an empty members list with a spinner overlayed. So with this change we'd show a number instead of a spinner on the details page, but the behaviour wouldn't actually be any different.

Currently we also need the owner RoomMember object to check power levels in the room's details.
I don't know if we have a way to fetch just it.

There's Room.member(userId) which returns a single user (which is how Android is currently using for checking power levels). On the Rust side it still syncs the full member list (if it wasn't already synced), but side-steps the big delay when Rust returns the full list to us.

@alfogrillo
Copy link

On Android it turns out we already allow the user to click on "Members" while it's still loading (and showing the spinner), and they navigate to an empty members list with a spinner overlayed. So with this change we'd show a number instead of a spinner on the details page, but the behaviour wouldn't actually be any different.

Ok, this is different on iOS, we probably should align to Android somehow.

There's Room.member(userId) which returns a single user (which is how Android is currently using for checking power levels). On the Rust side it still syncs the full member list (if it wasn't already synced), but side-steps the big delay when Rust returns the full list to us.

Also this is different on iOS, we are just waiting all the members to check the power levels.
But given the current Rust implementation performances should be similar. 😐

@csmith csmith self-assigned this Jun 5, 2023
csmith added a commit that referenced this issue Jun 5, 2023
For the room details screen, use the member count as supplied by
matrix instead of waiting for the entire member list to be
retrieved and then manually adding up all the relevant users.

This removes the loading state of the member count, relying on
a spinner on the member list itself if the user actually wants
to see the members. (The performance of that will be improved
separately on the rust side in the future)

Closes #505
@alfogrillo
Copy link

alfogrillo commented Jun 5, 2023

Created a similar issue for iOS here

csmith added a commit that referenced this issue Jun 6, 2023
Use member count instead of counting members

For the room details screen, use the member count as supplied by
matrix instead of waiting for the entire member list to be
retrieved and then manually adding up all the relevant users.

This removes the loading state of the member count, relying on
a spinner on the member list itself if the user actually wants
to see the members. (The performance of that will be improved
separately on the rust side in the future)

Closes #505
bmarty pushed a commit that referenced this issue Jul 19, 2023
Use member count instead of counting members

For the room details screen, use the member count as supplied by
matrix instead of waiting for the entire member list to be
retrieved and then manually adding up all the relevant users.

This removes the loading state of the member count, relying on
a spinner on the member list itself if the user actually wants
to see the members. (The performance of that will be improved
separately on the rust side in the future)

Closes #505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants