Skip to content

Conversation

@AndyBodnar
Copy link

This addresses most of the improvements from #663's follow-up issue #664.

What changed:

Room/space avatars now load from the avatar cache instead of always showing initials. When you scroll through the space lobby, avatars will progressively load as they're fetched.

Each entry now shows more info in the subtitle: join status for rooms you haven't joined yet (Invited, Left, Knocked, Banned), visibility (Public, Private, or Knock to join), and a truncated topic if one exists.

Expanded/collapsed state is now saved when you navigate away from a space and restored when you come back. So if you had a subspace expanded, it stays expanded.

What I skipped:

  • The "Suggested" badge isn't possible yet because SpaceRoom in matrix-sdk-ui doesn't expose that field
  • Parent space header info (showing topic/visibility in the header) would need more work to fetch the parent space details

Testing:

  • cargo check, cargo clippy, cargo build --release all pass
  • Couldn't do runtime testing since I don't have a Matrix account set up, but the code follows the same patterns used elsewhere in the codebase for avatar caching and state persistence

Closes #664

…UI state persistence

Changes:
- Use avatar_cache to display room/space avatars instead of always showing initials
- Show join status (Invited, Left, Knocked, Banned) for rooms user hasn't joined
- Show Public/Private/Knock-to-join based on join_rule
- Display truncated topic in the info label
- Save/restore expanded_spaces state when switching between spaces
- Remove #[allow(unused)] from fields that are now used

This addresses most items from issue project-robius#664, except:
- "Suggested" badge: SpaceRoom struct in matrix-sdk-ui doesn't have this field yet
- Header space info: would need architectural changes to fetch parent space details
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Thanks @AndyBodnar, and welcome to the project! Wow, you're quick! I think you submitted this PR only a couple of hours after I created the issue, haha.

In general, things look pretty good, I appreciate that you followed the existing code usage/style patterns. Typically I wouldn't review PRs that haven't been tested at runtime, but I'll make an exception for your first submission.

I'm actually happy to accept this PR as-is since I'm about to completely restructure the SpaceLobbyScreen anyway. There are a few issues but nothing that is a serious blocker, just some things that will need to be fixed in the future:

  1. Pulling the avatar from the cache does work, but there are two things that can be improved there.
    1. In the SpaceLobbyScreen's event handler, we need to explicitly handle Event::Signal by handling any pending avatar_cache updates (since we now use that cache) and then redraw if there were any. Otherwise, the avatars don't actually get shown when they're fetched, they don't show up until you close/re-open the SpaceLobbyScreen.
    2. Just an optimization: once we fetch the avatar from the cache, we should store it locally in the SpaceRoomInfo' room_avatar field.
  2. As with the RoomScreen, we actually need to save the UI state when the SpaceLobbyScreen is destroyed, not just when showing a new one. Currently the state never actually gets saved, so there's nothing to restore.
  3. Truncating a string (e.g., the topic) is an okay approach if you know that you always want it to be limited to, say, 50 characters, but in general it's best to allow the Label to truncate it via flow: Right and wrap: Ellipsis (which is currently broken in makepad but will be fixed shortly).
  4. Displaying Public/Private for each subspace is actually a bit crowded and unnecessary. On second thought, I think we should probably just display that for the top-level main space, similar to what Element does.

Let me know if you want to fix these things in this PR, otherwise I can just merge it as-is and make the improvements soon after.

@kevinaboos kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Jan 18, 2026
- Add Event::Signal handling for avatar cache updates
- Store avatar data locally in SpaceRoomInfo (Arc<[u8]>)
- Make save_current_state() public for external callers
- Remove manual topic truncation (let Label handle it)
- Remove Public/Private display for subspaces
@AndyBodnar
Copy link
Author

Thanks @AndyBodnar, and welcome to the project! Wow, you're quick! I think you submitted this PR only a couple of hours after I created the issue, haha.

In general, things look pretty good, I appreciate that you followed the existing code usage/style patterns. Typically I wouldn't review PRs that haven't been tested at runtime, but I'll make an exception for your first submission.

I'm actually happy to accept this PR as-is since I'm about to completely restructure the SpaceLobbyScreen anyway. There are a few issues but nothing that is a serious blocker, just some things that will need to be fixed in the future:

  1. Pulling the avatar from the cache does work, but there are two things that can be improved there.

    1. In the SpaceLobbyScreen's event handler, we need to explicitly handle Event::Signal by handling any pending avatar_cache updates (since we now use that cache) and then redraw if there were any. Otherwise, the avatars don't actually get shown when they're fetched, they don't show up until you close/re-open the SpaceLobbyScreen.
    2. Just an optimization: once we fetch the avatar from the cache, we should store it locally in the SpaceRoomInfo' room_avatar field.
  2. As with the RoomScreen, we actually need to save the UI state when the SpaceLobbyScreen is destroyed, not just when showing a new one. Currently the state never actually gets saved, so there's nothing to restore.

  3. Truncating a string (e.g., the topic) is an okay approach if you know that you always want it to be limited to, say, 50 characters, but in general it's best to allow the Label to truncate it via flow: Right and wrap: Ellipsis (which is currently broken in makepad but will be fixed shortly).

  4. Displaying Public/Private for each subspace is actually a bit crowded and unnecessary. On second thought, I think we should probably just display that for the top-level main space, similar to what Element does.

Let me know if you want to fix these things in this PR, otherwise I can just merge it as-is and make the improvements soon after.

should be good to merge now after you review new changes..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-author This issue is waiting on the original author for a response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SpaceLobbyScreen improvements post-#663

2 participants