From b0c02a0dff6a2a2b8b98a3d4d3b0c52c18dc22c8 Mon Sep 17 00:00:00 2001 From: Andy Bodnar Date: Sat, 17 Jan 2026 22:06:53 -0700 Subject: [PATCH 1/2] Improve SpaceLobbyScreen with avatar caching, more info display, and 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 #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 --- src/home/space_lobby.rs | 143 ++++++++++++++++++++++++++++++++-------- 1 file changed, 116 insertions(+), 27 deletions(-) diff --git a/src/home/space_lobby.rs b/src/home/space_lobby.rs index 2a6f27b6..3cc75d97 100644 --- a/src/home/space_lobby.rs +++ b/src/home/space_lobby.rs @@ -6,7 +6,8 @@ //! that allows the user to click on it to show the `SpaceLobby`. //! -use std::collections::{HashMap, HashSet}; +use std::cell::RefCell; +use std::collections::{BTreeMap, HashMap, HashSet}; use imbl::Vector; use makepad_widgets::*; use matrix_sdk::{RoomState, ruma::OwnedRoomId}; @@ -14,7 +15,11 @@ use matrix_sdk_ui::spaces::SpaceRoom; use ruma::room::JoinRuleSummary; use tokio::sync::mpsc::UnboundedSender; use crate::{ - home::rooms_list::RoomsListRef, shared::avatar::{AvatarState, AvatarWidgetExt, AvatarWidgetRefExt}, space_service_sync::{SpaceRequest, SpaceRoomExt, SpaceRoomListAction}, utils::{self, RoomNameId} + avatar_cache::{self, AvatarCacheEntry}, + home::rooms_list::RoomsListRef, + shared::avatar::{AvatarState, AvatarWidgetExt, AvatarWidgetRefExt}, + space_service_sync::{SpaceRequest, SpaceRoomExt, SpaceRoomListAction}, + utils::{self, RoomNameId}, }; @@ -450,6 +455,22 @@ live_design! { } +thread_local! { + /// A cache of UI states for each SpaceLobbyScreen, keyed by the space's room ID. + /// This allows preserving the expanded/collapsed state of subspaces across screen changes. + static SPACE_LOBBY_STATES: RefCell> = const { + RefCell::new(BTreeMap::new()) + }; +} + +/// The UI-side state of a SpaceLobbyScreen that should persist across hide/show cycles. +#[derive(Default)] +struct SpaceLobbyUiState { + /// The set of space IDs that are currently expanded (showing their children). + expanded_spaces: HashSet, +} + + /// A clickable entry shown in the RoomsList that will show the space lobby when clicked. #[derive(Live, LiveHook, Widget)] pub struct SpaceLobbyEntry { @@ -618,14 +639,10 @@ impl Widget for RoomEntry { struct SpaceRoomInfo { id: OwnedRoomId, name: String, - #[allow(unused)] topic: Option, - #[allow(unused)] room_avatar: AvatarState, num_joined_members: u64, - #[allow(unused)] state: Option, - #[allow(unused)] join_rule: Option, /// If `Some`, this is a space. If `None`, it's a room. children_count: Option, @@ -792,10 +809,23 @@ impl Widget for SpaceLobbyScreen { // Below, draw things that are common to child rooms and subspaces. item.label(ids!(content.name_label)).set_text(cx, &info.name); - // TODO: query (and update) room/space avatar from the avatar_cache + + // Display avatar from cache, or show initials as fallback let avatar_ref = item.avatar(ids!(avatar)); let first_char = utils::user_name_first_letter(&info.name); - avatar_ref.show_text(cx, None, None, first_char.unwrap_or("#")); + let mut drew_avatar = false; + if let AvatarState::Known(Some(ref uri)) = info.room_avatar { + if let AvatarCacheEntry::Loaded(data) = avatar_cache::get_or_fetch_avatar(cx, uri.clone()) { + drew_avatar = avatar_ref.show_image( + cx, + None, + |cx, img| utils::load_png_or_jpg(&img, cx, &data), + ).is_ok(); + } + } + if !drew_avatar { + avatar_ref.show_text(cx, None, None, first_char.unwrap_or("#")); + } if let Some(mut lines) = item.widget(ids!(tree_lines)).borrow_mut::() { lines.draw_bg.level = *level as f32; @@ -804,27 +834,67 @@ impl Widget for SpaceLobbyScreen { lines.draw_bg.indent_width = 44.0; // Hardcoded to match } + // Build the info label with join status, visibility, member count, and topic let info_label = item.label(ids!(content.info_label)); - if let Some(c) = info.children_count && c > 0 { - info_label.set_text(cx, &format!( - "{} {} | ~{} {}", - info.num_joined_members, - match info.num_joined_members { - 1 => "member", - _ => "members", - }, - c, - match c { - 1 => "room", - _ => "rooms", + let mut info_parts = Vec::new(); + + // Add join status for rooms we haven't joined + if let Some(state) = &info.state { + match state { + RoomState::Joined => { /* Don't show "Joined" - it's implied */ } + RoomState::Left => info_parts.push("Left".to_string()), + RoomState::Invited => info_parts.push("Invited".to_string()), + RoomState::Knocked => info_parts.push("Knocked".to_string()), + RoomState::Banned => info_parts.push("Banned".to_string()), + } + } + + // Add public/private indicator + if let Some(rule) = &info.join_rule { + match rule { + JoinRuleSummary::Public => info_parts.push("Public".to_string()), + JoinRuleSummary::Knock | JoinRuleSummary::KnockRestricted(_) => { + info_parts.push("Knock to join".to_string()) } - )); - } else { - match info.num_joined_members { - 1 => info_label.set_text(cx, "1 member"), - n => info_label.set_text(cx, &format!("{n} members")), + JoinRuleSummary::Private | JoinRuleSummary::Invite => { + info_parts.push("Private".to_string()) + } + JoinRuleSummary::Restricted(_) | JoinRuleSummary::_Custom(_) | _ => { } } - }; + } + + // Add member count + info_parts.push(format!( + "{} {}", + info.num_joined_members, + if info.num_joined_members == 1 { "member" } else { "members" } + )); + + // Add children count for spaces + if let Some(c) = info.children_count { + if c > 0 { + info_parts.push(format!( + "~{} {}", + c, + if c == 1 { "room" } else { "rooms" } + )); + } + } + + // Add truncated topic if available + if let Some(topic) = &info.topic { + let topic = topic.trim(); + if !topic.is_empty() { + let truncated = if topic.len() > 50 { + format!("{}...", &topic[..47]) + } else { + topic.to_string() + }; + info_parts.push(truncated); + } + } + + info_label.set_text(cx, &info_parts.join(" | ")); item } @@ -998,6 +1068,18 @@ impl SpaceLobbyScreen { return; } + // Save the current UI state before switching to a new space + if let Some(current_space) = &self.space_name_id { + SPACE_LOBBY_STATES.with_borrow_mut(|states| { + states.insert( + current_space.room_id().clone(), + SpaceLobbyUiState { + expanded_spaces: self.expanded_spaces.clone(), + }, + ); + }); + } + self.space_name_id = Some(space_name_id.clone()); let rooms_list_ref = cx.get_global::(); if let Some(sender) = rooms_list_ref.get_space_request_sender() { @@ -1011,9 +1093,16 @@ impl SpaceLobbyScreen { } self.tree_entries.clear(); - self.expanded_spaces.clear(); self.is_loading = true; + // Restore UI state if we've viewed this space before, otherwise start fresh + self.expanded_spaces = SPACE_LOBBY_STATES.with_borrow(|states| { + states + .get(space_name_id.room_id()) + .map(|state| state.expanded_spaces.clone()) + .unwrap_or_default() + }); + // Set parent avatar let avatar_ref = self.view.avatar(ids!(header.parent_space_row.parent_avatar)); let first_char = utils::user_name_first_letter(&space_name); From 372caa2d8dd19b4de1acfa92672223b365eb9330 Mon Sep 17 00:00:00 2001 From: Andy Bodnar Date: Sun, 18 Jan 2026 14:56:36 -0700 Subject: [PATCH 2/2] Address reviewer feedback for SpaceLobbyScreen - 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 --- src/home/space_lobby.rs | 128 +++++++++++++++++++++++++--------------- 1 file changed, 80 insertions(+), 48 deletions(-) diff --git a/src/home/space_lobby.rs b/src/home/space_lobby.rs index 3cc75d97..d32950b2 100644 --- a/src/home/space_lobby.rs +++ b/src/home/space_lobby.rs @@ -12,12 +12,11 @@ use imbl::Vector; use makepad_widgets::*; use matrix_sdk::{RoomState, ruma::OwnedRoomId}; use matrix_sdk_ui::spaces::SpaceRoom; -use ruma::room::JoinRuleSummary; use tokio::sync::mpsc::UnboundedSender; use crate::{ avatar_cache::{self, AvatarCacheEntry}, home::rooms_list::RoomsListRef, - shared::avatar::{AvatarState, AvatarWidgetExt, AvatarWidgetRefExt}, + shared::avatar::{AvatarWidgetExt, AvatarWidgetRefExt}, space_service_sync::{SpaceRequest, SpaceRoomExt, SpaceRoomListAction}, utils::{self, RoomNameId}, }; @@ -640,10 +639,12 @@ struct SpaceRoomInfo { id: OwnedRoomId, name: String, topic: Option, - room_avatar: AvatarState, + /// The avatar URI, used to fetch from cache. + avatar_uri: Option, + /// Cached avatar image data, stored after fetching from avatar cache. + avatar_data: Option>, num_joined_members: u64, state: Option, - join_rule: Option, /// If `Some`, this is a space. If `None`, it's a room. children_count: Option, } @@ -658,10 +659,10 @@ impl From<&SpaceRoom> for SpaceRoomInfo { id: space_room.room_id.clone(), name: space_room.display_name.clone(), topic: space_room.topic.clone(), - room_avatar: AvatarState::Known(space_room.avatar_url.clone()), + avatar_uri: space_room.avatar_url.clone(), + avatar_data: None, num_joined_members: space_room.num_joined_members, state: space_room.state, - join_rule: space_room.join_rule.clone(), children_count: space_room.is_space().then_some(space_room.children_count), } } @@ -673,10 +674,10 @@ impl From for SpaceRoomInfo { id: space_room.room_id, name: space_room.display_name, topic: space_room.topic, - room_avatar: AvatarState::Known(space_room.avatar_url), + avatar_uri: space_room.avatar_url, + avatar_data: None, num_joined_members: space_room.num_joined_members, state: space_room.state, - join_rule: space_room.join_rule, } } } @@ -736,6 +737,15 @@ impl Widget for SpaceLobbyScreen { fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) { self.view.handle_event(cx, event, scope); + // Handle Signal events for avatar cache updates + if let Event::Signal = event { + // Process any pending avatar updates + avatar_cache::process_avatar_updates(cx); + // Try to fetch and store avatars that we don't have yet, then redraw + self.fetch_pending_avatars(cx); + self.redraw(cx); + } + if let Event::Actions(actions) = event { for action in actions { if let Some(SpaceRoomListAction::DetailedChildren { space_id, children, .. }) = action.downcast_ref() { @@ -810,19 +820,32 @@ impl Widget for SpaceLobbyScreen { // Below, draw things that are common to child rooms and subspaces. item.label(ids!(content.name_label)).set_text(cx, &info.name); - // Display avatar from cache, or show initials as fallback + // Display avatar from stored data, or fetch from cache, or show initials let avatar_ref = item.avatar(ids!(avatar)); let first_char = utils::user_name_first_letter(&info.name); let mut drew_avatar = false; - if let AvatarState::Known(Some(ref uri)) = info.room_avatar { - if let AvatarCacheEntry::Loaded(data) = avatar_cache::get_or_fetch_avatar(cx, uri.clone()) { - drew_avatar = avatar_ref.show_image( - cx, - None, - |cx, img| utils::load_png_or_jpg(&img, cx, &data), - ).is_ok(); + + // First try using stored avatar data + if let Some(ref data) = info.avatar_data { + drew_avatar = avatar_ref.show_image( + cx, + None, + |cx, img| utils::load_png_or_jpg(&img, cx, data), + ).is_ok(); + } + // If no stored data, try fetching from cache + if !drew_avatar { + if let Some(ref uri) = info.avatar_uri { + if let AvatarCacheEntry::Loaded(data) = avatar_cache::get_or_fetch_avatar(cx, uri.clone()) { + drew_avatar = avatar_ref.show_image( + cx, + None, + |cx, img| utils::load_png_or_jpg(&img, cx, &data), + ).is_ok(); + } } } + // Fallback to initials if !drew_avatar { avatar_ref.show_text(cx, None, None, first_char.unwrap_or("#")); } @@ -834,7 +857,8 @@ impl Widget for SpaceLobbyScreen { lines.draw_bg.indent_width = 44.0; // Hardcoded to match } - // Build the info label with join status, visibility, member count, and topic + // Build the info label with join status, member count, and topic + // Note: Public/Private is intentionally not shown per-item to reduce clutter let info_label = item.label(ids!(content.info_label)); let mut info_parts = Vec::new(); @@ -849,20 +873,6 @@ impl Widget for SpaceLobbyScreen { } } - // Add public/private indicator - if let Some(rule) = &info.join_rule { - match rule { - JoinRuleSummary::Public => info_parts.push("Public".to_string()), - JoinRuleSummary::Knock | JoinRuleSummary::KnockRestricted(_) => { - info_parts.push("Knock to join".to_string()) - } - JoinRuleSummary::Private | JoinRuleSummary::Invite => { - info_parts.push("Private".to_string()) - } - JoinRuleSummary::Restricted(_) | JoinRuleSummary::_Custom(_) | _ => { } - } - } - // Add member count info_parts.push(format!( "{} {}", @@ -881,16 +891,11 @@ impl Widget for SpaceLobbyScreen { } } - // Add truncated topic if available + // Add topic if available (Label handles truncation via wrap: Ellipsis) if let Some(topic) = &info.topic { let topic = topic.trim(); if !topic.is_empty() { - let truncated = if topic.len() > 50 { - format!("{}...", &topic[..47]) - } else { - topic.to_string() - }; - info_parts.push(truncated); + info_parts.push(topic.to_string()); } } @@ -1057,18 +1062,24 @@ impl SpaceLobbyScreen { } } - pub fn set_displayed_space(&mut self, cx: &mut Cx, space_name_id: &RoomNameId) { - let space_name = space_name_id.to_string(); - let parent_name = self.view.label(ids!(header.parent_space_row.parent_name)); - parent_name.set_text(cx, &space_name); - - // If this space is already being displayed, then the only thing we may need to do - // is update its name in the top-level header (already done above). - if self.space_name_id.as_ref().is_some_and(|sni| sni.room_id() == space_name_id.room_id()) { - return; + /// Fetches avatars from the cache for any tree entries that don't have avatar data yet. + fn fetch_pending_avatars(&mut self, cx: &mut Cx) { + for entry in &mut self.tree_entries { + if let TreeEntry::Item { info, .. } = entry { + // Only fetch if we have a URI but no data yet + if info.avatar_data.is_none() { + if let Some(ref uri) = info.avatar_uri { + if let AvatarCacheEntry::Loaded(data) = avatar_cache::get_or_fetch_avatar(cx, uri.clone()) { + info.avatar_data = Some(data); + } + } + } + } } + } - // Save the current UI state before switching to a new space + /// Saves the current UI state to the cache. Call this when the screen is being hidden. + pub fn save_current_state(&mut self) { if let Some(current_space) = &self.space_name_id { SPACE_LOBBY_STATES.with_borrow_mut(|states| { states.insert( @@ -1079,6 +1090,21 @@ impl SpaceLobbyScreen { ); }); } + } + + pub fn set_displayed_space(&mut self, cx: &mut Cx, space_name_id: &RoomNameId) { + let space_name = space_name_id.to_string(); + let parent_name = self.view.label(ids!(header.parent_space_row.parent_name)); + parent_name.set_text(cx, &space_name); + + // If this space is already being displayed, then the only thing we may need to do + // is update its name in the top-level header (already done above). + if self.space_name_id.as_ref().is_some_and(|sni| sni.room_id() == space_name_id.room_id()) { + return; + } + + // Save the current UI state before switching to a new space + self.save_current_state(); self.space_name_id = Some(space_name_id.clone()); let rooms_list_ref = cx.get_global::(); @@ -1116,4 +1142,10 @@ impl SpaceLobbyScreenRef { let Some(mut inner) = self.borrow_mut() else { return }; inner.set_displayed_space(cx, space_name_id); } + + /// Saves the current UI state. Call this when the screen is being hidden or destroyed. + pub fn save_current_state(&self) { + let Some(mut inner) = self.borrow_mut() else { return }; + inner.save_current_state(); + } }