-
Notifications
You must be signed in to change notification settings - Fork 37
Use RoomDisplayName everywhere to ensure that room names display correctly
#590
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
base: main
Are you sure you want to change the base?
Use RoomDisplayName everywhere to ensure that room names display correctly
#590
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
As I said on WeChat, this problem is not just with Invited rooms, but also other newly-joined or newly-added rooms within Robrix.
I think instead of trying to do string comparisons (like room_name == "Empty Room"), we should compare with the actual RoomDisplayName type.
In fact, we should go one step further: instead of sending room name updates as Option<String> values, we should send the actual RoomDisplayName type because it conveys much more information. By comparison, the Option<String> value is worse because it cannot distinguish between a room that has no name vs. a room whose name is not yet known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to carefully consider all the ways a room name can change over time. My comments below highlight some potential issues in your recent changeset that might cause certain name updates to be missed.
src/home/room_preview.rs
Outdated
| // Handle room name display, including Empty/EmptyWas placeholders | ||
| let room_name_text = match &room_info.room_name { | ||
| Some(name) => match name { | ||
| RoomDisplayName::Empty | RoomDisplayName::EmptyWas(_) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not override EmptyWas variant, that is important.
If you want to, you can override Empty to print something like "Empty (Alias )" but that is not strictly necessary either. Remember, the goal here is to update the room names properly, not just override the way they're displayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to keep EmptyWas unchanged and only fallback for Empty/None.
src/home/room_preview.rs
Outdated
| // For None (invited rooms that haven't loaded yet), use canonical alias or default | ||
| room_info.canonical_alias | ||
| .as_ref() | ||
| .map(|alias| alias.to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, if the room name doesn't exist, you should use other values in this order:
- canonical alias
- The first alias in the list of aliases
- Room ID
Using "Room" as a default is not helpful.
You can also do this for Empty room (above) if you want, though it's not strictly necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to keep EmptyWas unchanged and only fallback for Empty/None.
src/sliding_sync.rs
Outdated
| // Always update the room name if it's not a placeholder. | ||
| // We can't reliably compare old vs new because old_room.room is a reference | ||
| // to the SDK's internal object which may have already been updated. | ||
| let should_update = !matches!(new_room_name, RoomDisplayName::Empty | RoomDisplayName::EmptyWas(_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your new change here, what happens when everyone leaves the room, and the room name changes to Empty or WasEmpty? You'll miss those updates.
The proper way to deal with this is to store the previous room name in the RoomListServiceRoomInfo and then compare the new name with that. We want to send an update any time the room name actually changes, not just when it's non-empty. Your comment on L2099-2101 is exactly why that struct exists.
(and then, don't forget to update the room_name in RoomListServiceRoomInfo whenever it has actually changed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
|
Is this ready? If so (and in the future), please re-request a review from me and add the |
|
@TigerInYourDream I just happened to make some changes to the |
This PR addresses room name display issues in three scenarios: 1. Room invitations showing "Empty Room" instead of actual names 2. Tombstoned room replacements not showing correct names without restart 3. Rooms joined from other clients not displaying names correctly Root cause: The comparison logic in `update_room()` was comparing references to SDK's internal state rather than cached snapshots, causing UI updates to be skipped when room names changed. Key changes: - Use `RoomDisplayName` enum instead of `Option<String>` for type safety - Properly preserve `EmptyWas` variant to maintain previous name semantics - Implement name fallback order: canonical_alias → alt_alias → room_id - Update invited room display list when names change - Store and compare cached display names to detect all changes - Only send updates when names actually change (not on every event) Addresses all review comments from project-robius#590. Signed-off-by: Alvin <[email protected]>
70b98f8 to
612a10e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where invited room names were not displaying correctly in the Robrix UI, appearing as "Invite to unnamed room" instead of showing the actual room name. The fix involves changing how room names are stored, retrieved, and displayed throughout the application.
- Refactored room name handling to use
RoomDisplayNametype instead ofOption<String> - Added utility functions for consistent room name fallback logic across the codebase
- Updated room name display to properly handle the Matrix SDK's "Empty Room" placeholder
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.rs | Added preferred_room_name() and display_name_with_fallback() utilities for consistent room name handling with fallback to aliases or room ID |
| src/sliding_sync.rs | Modified room name handling to use RoomDisplayName and preferred_room_name() for proper display name conversion |
| src/room/room_display_filter.rs | Updated FilterableRoom implementations to use display_name_with_fallback() for consistent room name display |
| src/home/rooms_list.rs | Changed room_name fields from Option<String> to RoomDisplayName, updated all room name usage sites to use display_name_with_fallback(), and added invited room handling in UpdateRoomName |
| src/home/room_preview.rs | Updated room preview display to use display_name_with_fallback() for both joined and invited rooms |
| src/home/invite_screen.rs | Modified invite details to use display_name_with_fallback() when displaying room information |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/utils.rs
Outdated
| pub fn preferred_room_name(display_name: &RoomDisplayName) -> Option<String> { | ||
| match display_name { | ||
| RoomDisplayName::Empty => None, | ||
| _ => Some(display_name.to_string()), | ||
| } | ||
| } |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function documentation states it 'Returns None when the display name is explicitly empty' but doesn't explain the purpose of the other variants. Add documentation explaining what happens for other RoomDisplayName variants (e.g., Calculated, Named, Aliased) and why they all return Some(display_name.to_string()).
| if let Some(room) = self.all_joined_rooms.get_mut(&room_id) { | ||
| let was_displayed = (self.display_filter)(room); | ||
| room.room_name = new_room_name; | ||
| // Keep the full RoomDisplayName to preserve EmptyWas semantics |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'EmptyWas' to 'Empty' (likely meant to say 'Empty semantics' or reference a specific variant).
| // Keep the full RoomDisplayName to preserve EmptyWas semantics | |
| // Keep the full RoomDisplayName to preserve Empty semantics |
| } | ||
| } | ||
| } else { | ||
| warning!("Warning: couldn't find invited room {} to update room name", room_id); |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message contains redundant 'Warning:' prefix since it's already logged using the warning! macro. Remove the 'Warning:' prefix from the message.
| warning!("Warning: couldn't find invited room {} to update room name", room_id); | |
| warning!("couldn't find invited room {} to update room name", room_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're duplicating a lot of functionality that is already offered by the SDK — that's the whole point of the RoomDisplayName enum. See my comments on the code.
At a high level, you should simply modify the existing room_name_or_id() function to accept a RoomDisplayName instead of an Option<String>. Then, you can use that function throughout Robrix once you change the rest of the code base to use RoomDisplayName instead of String/Option<String> for a room name.
The only possible modification we might want is to augment the RoomDisplayName::Empty variant with additional info, but the only fallback is the room ID. So, you should do something like this:
/// Returns a string representation of the room name or ID.
pub fn room_name_or_id(
room_name: &RoomDisplayName,
room_id: impl AsRef<RoomId>,
) -> String {
match room_name {
RoomDisplayName::Empty => format!("Empty (ID {})", room_id.as_ref()),
other => other.to_string(),
}
}that's all you really need to do, let's keep it simple.
src/utils.rs
Outdated
| } | ||
|
|
||
| /// Returns the room name that should be shown to the user, falling back to aliases or the room ID. | ||
| pub fn display_name_with_fallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is completely unnecessary; the SDK already offers this functionality within the RoomDisplayName type itself. For ex, the Aliased variant is exactly what you've implemented here with canonical_alias and alt_aliases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated the PR so room_name_or_id now takes RoomDisplayName directly and formats RoomDisplayName::Empty as Empty (ID …), and I’ve switched the relevant call sites to use it—no extra helper needed.
src/utils.rs
Outdated
| /// | ||
| /// Returns `None` when the display name is explicitly empty so that callers can | ||
| /// fall back to aliases or the room ID. | ||
| pub fn preferred_room_name(display_name: &RoomDisplayName) -> Option<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also don't need this function at all.
You should combine this functionality into the existing utility function room_name_or_id(), but change room_name_or_id() to accept a RoomDisplayName instead of an Option<String>. Then, change the rest of the code base to use RoomDisplayName instead of String/Option<String> for a room name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this helper and updated all call sites to rely on the revised room_name_or_id
RoomDisplayName everywhere to ensure that room names display correctly
914f395 to
af47d4b
Compare
Signed-off-by: Alvin <[email protected]>
af47d4b to
b7dbaef
Compare
This PR addresses room name display issues in three scenarios: 1. Room invitations showing "Empty Room" instead of actual names 2. Tombstoned room replacements not showing correct names without restart 3. Rooms joined from other clients not displaying names correctly Root cause: The comparison logic in `update_room()` was comparing references to SDK's internal state rather than cached snapshots, causing UI updates to be skipped when room names changed. Key changes: - Use `RoomDisplayName` enum instead of `Option<String>` for type safety - Properly preserve `EmptyWas` variant to maintain previous name semantics - Implement name fallback order: canonical_alias → alt_alias → room_id - Update invited room display list when names change - Store and compare cached display names to detect all changes - Only send updates when names actually change (not on every event) Addresses all review comments from project-robius#590. Signed-off-by: Alvin <[email protected]>
9d8b442 to
b7dbaef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed changes are generally good, but you can vastly simplify a lot of things by introducing a new RoomName newtype wrapper for RoomDisplayName, and then implementing a lot of traits for that.
Please disable your auto-formatter. There are tons of unrelated whitespace changes that make my job as a reviewer harder.
src/utils.rs
Outdated
| /// Converts a `RoomDisplayName` to the text we prefer to show in the Rooms list. | ||
| /// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment seems out of place...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/utils.rs
Outdated
| |name| name.into(), | ||
| ) | ||
| match room_name { | ||
| RoomDisplayName::Empty => format!("Empty (ID {})", room_id.as_ref()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about this more, I think using the word "Empty" will be confusing to the user. It makes it sound like the room itself is empty (meaning there are no joined members in the room), but what it actually means is that the room's name is empty.
Instead, we should format it as "Unnamed (ID {})", which does not incorrectly imply that the room has no members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated the fallback so RoomDisplayName::Empty renders as Unnamed (ID …), which avoids implying the room itself is empty.
src/utils.rs
Outdated
| /// Returns a string representation of the room name or ID. | ||
| pub fn room_name_or_id( | ||
| room_name: Option<impl Into<String>>, | ||
| room_name: &RoomDisplayName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you still use the impl Into<...> trait argument here, you won't need to make so many changes to all the call sites of this function. I counted over 40 places where this is called, so it's worth it to make it simple to use.
Perhaps something like impl Into<RoomDisplayName> or impl Into<AsRef<RoomDisplayName>> would work, but you probably need to create a newtype instead such that you can implement traits for it. I would suggest something like struct RoomName(RoomDisplayName), which can then impl Into/From, Display, Default, Deref, Clone, etc.
The good thing is that RoomName is not used anywhere in the Matrix SDK, so we can use that safely without confusing anyone.
Whatever you decide, the goal is to make it as easy as possible to call this function (hence why I previously used room_name: Option<impl Into<String>>, because that argument type is quite flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I introduced a RoomName newtype together with an IntoRoomName helper trait so the API still accepts the usual inputs (RoomDisplayName, String/&str, Option, etc.). This kept most of
the existing call sites intact—only a handful needed small adjustments when they were constructing strings manually.
src/utils.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct RoomDisplayNameRon(pub RoomDisplayName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding my other comment, you can use this type for the wrapper (RoomName), and also impl SerRon/DeRon for it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update to RoomDisplayNameRon to RoomName and impl some trait
src/sliding_sync.rs
Outdated
| let room_name_for_avatar = match &room_display_name { | ||
| RoomDisplayName::Empty => None, | ||
| RoomDisplayName::EmptyWas(name) | ||
| | RoomDisplayName::Named(name) | ||
| | RoomDisplayName::Aliased(name) | ||
| | RoomDisplayName::Calculated(name) => Some(name.as_str()), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract this match statement into a function on your new RoomName type; don't copy & paste the same code in multiple places.
Also, I don't think it's correct to stringify EmptyWas(name) as just the bare "name". Instead, that variant should be stringified as format!("Empty Room (was \"{name}\")"), which is how the SDK does it internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
src/sliding_sync.rs
Outdated
| let room_display_name = new_room | ||
| .display_name | ||
| .clone() | ||
| .unwrap_or(RoomDisplayName::Empty); | ||
| let room_name_for_avatar = match &room_display_name { | ||
| RoomDisplayName::Empty => None, | ||
| RoomDisplayName::EmptyWas(name) | ||
| | RoomDisplayName::Named(name) | ||
| | RoomDisplayName::Aliased(name) | ||
| | RoomDisplayName::Calculated(name) => Some(name.as_str()), | ||
| }; | ||
| let room_avatar = room_avatar(&new_room.room, room_name_for_avatar).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of all this effort, just change the room_avatar() function to accept a RoomDisplayName (or your new wrapper type, RoomName).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/home/room_screen.rs
Outdated
| fn current_room_label(&self) -> String { | ||
| match self.room_id.as_ref() { | ||
| Some(room_id) => room_name_or_id(&self.room_name, room_id), | ||
| None => self.room_name.to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this function. Instead, use the RoomName type throughout the entire codebase instead of passing in a String or Option<String>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
src/home/room_screen.rs
Outdated
| if is_first_time_being_loaded { | ||
| if !tl_state.fully_paginated { | ||
| log!("Sending a first-time backwards pagination request for room \"{}\" {}", self.room_name, room_id); | ||
| let room_label = self.current_room_label(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this, it's unnecessary. Once you add the new RoomName newtype struct, just impl Display for it, and you can use that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/home/room_screen.rs
Outdated
| let mut restore_status_view = self.view.restore_status_view(ids!(restore_status_view)); | ||
| restore_status_view.set_content(cx, self.all_rooms_loaded, &self.room_name); | ||
| let room_label = self.current_room_label(); | ||
| restore_status_view.set_content(cx, self.all_rooms_loaded, &room_label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use the same RoomName struct here too. You can pass it directly to set_content() and change the function signature of set_content() to accept a RoomName parameter.
src/home/room_screen.rs
Outdated
| error!("Pagination error ({direction}) in room \"{}\", {}: {error:?}", self.room_name, tl.room_id); | ||
| error!("Pagination error ({direction}) in room \"{}\", {}: {error:?}", room_label_cached, tl.room_id); | ||
| enqueue_popup_notification(PopupItem { | ||
| message: utils::stringify_pagination_error(&error, &self.room_name), | ||
| message: utils::stringify_pagination_error(&error, room_label_cached.as_str()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here: remove current_room_label() and just make the new RoomName wrapper struct implement the Display trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. RestoreStatusView::set_content now takes a RoomName plus an optional RoomId, so the view itself handles the fallback text (Unnamed (ID …) when the name is empty). The RoomScreen call site just does
set_content(cx, self.all_rooms_loaded, RoomName::from(self.room_name.clone()), self.room_id.as_deref()), so we no longer build that label manually.
Bug Fix: Invited Room Names Not Displaying Correctly
Problem Description
When receiving room invitations in Robrix, the room names displayed as "Invite to unnamed room" instead of showing the actual room name (e.g., "room0", "room1").
Root Cause Analysis
1. Initial Problem: UpdateRoomName Not Handling Invited Rooms
The
RoomsListUpdate::UpdateRoomNamehandler only searched inall_joined_rooms, but invited rooms are stored in a separate collectioninvited_rooms.Location:
src/home/rooms_list.rs:458-4922. Matrix Protocol Background: "Empty Room" Placeholder
According to the Matrix Client-Server API Specification:
The Matrix Rust SDK implements this specification in
Room::display_name()and returns "Empty Room" as a placeholder when no room information is available yet.3. Timing Issue with Sliding Sync
For invited rooms, there's a timing sequence:
Initial sync: Robrix receives basic invitation info via sliding sync
room.display_name()returnsSome("Empty Room")(Matrix SDK's placeholder)Subscription: We call
room_list_service.subscribe_to_rooms(&[&room_id])State events arrive: Room name, alias, and member information arrive
room.display_name()updates to return the actual room nameupdate_room()is called with the new name4. SDK Internal Architecture Issue
The
RoomListServiceRoomInfostruct stores a reference to the SDK's Room object:When
update_room()is called:Both
old_room.roomandnew_roomreference the same internal object in the Matrix SDK (viaArc), socached_display_name()always returns the latest value, making it impossible to detect changes using this approach.Solution
Change 1: Handle Invited Rooms in UpdateRoomName Handler
File:
src/home/rooms_list.rsChange 2: Subscribe to Invited Rooms for State Updates
File:
src/sliding_sync.rs:2116Change 3: Handle "Empty Room" Placeholder
File:
src/sliding_sync.rs:2079-2085Rationale: "Empty Room" is a placeholder indicating missing data, not an actual room name. Displaying "Invite to unnamed room" provides better user experience while waiting for the real name to arrive.
Change 4: Special Handling for Invited Room Name Updates
File:
src/sliding_sync.rs:1984-2003Trade-off: This may send duplicate updates (e.g., "room4" → "room4") during subsequent syncs. However, this is acceptable because:
Testing
Test Case 1: Fresh Invitation
Test Case 2: Restored Invitations
Test Results
✅ Tested with room0 through room9
✅ All room names display correctly
✅ No "Invite to unnamed room" for named rooms
Impact
Files Modified
src/home/rooms_list.rs(2 lines added)src/sliding_sync.rs(multiple changes)Behavior Changes
Performance Impact
Related Issues
Matrix SDK Behavior
Room::display_name()returns "Empty Room" as per Matrix specificationArcinternally, makingRooma reference to shared stateold_room.room.cached_display_name()always returns the latest valueAlternative Approaches Considered
Track sent names in a separate Map
Store sent name in RoomListServiceRoomInfo
Current approach: Accept duplicate updates
References
Room::display_name()implementationDate: 2025-09-30
PR Branch:
fix/invited-room-name-display