-
Notifications
You must be signed in to change notification settings - Fork 33
Handle matrix.to room alias and room ID links. #531
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?
Handle matrix.to room alias and room ID links. #531
Conversation
1. Add support for clicking matrix.to links in room messages to navigate to rooms and resolve room aliases. 2. Implement async room alias resolution with proper error handling and user feedback through popup notifications.
4675675
to
24d8652
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.
Thanks for the PR.
I'm a bit confused about the title of this PR. When you "handle" room aliases and links, what do you mean by "handle"? I would expect the UI to open a new tab (in Desktop mode) and show that room's content if it's joined, or if it hasn't been joined yet, then show a new view with a preview of that room and a prompt asking the user to join it.
And then in Mobile mode, the same thing would happen, but you'd just show the new room in a view that replaces the existing RoomScreen, instead of opening a new tab (since there are no tabs). I was under the impression that we'd need Julian's new stack navigation PR in order to handle this properly, in which we're able to "push" the new room view onto the stack of existing views (such that you can navigate backwards to the rooms you viewed previously). Without that functionality, we can't really offer a good user experience of navigating between multiple rooms.
This PR doesn't really seem to do any of that, so I'm a bit confused about what you intended the scope of this to be.
src/home/room_screen.rs
Outdated
c.rooms().into_iter().find(|room| { | ||
room.canonical_alias().as_ref() == Some(room_alias) || | ||
room.alt_aliases().contains(room_alias) | ||
}) |
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 is very expensive --- we cannot iterate over all rooms in the client on the main UI thread.
You deleted my comments, but those comments clearly stated what we needed to do. We do need to show a loading screen (like how I use the LoadingPane
widget when looking for an old replied-to message) while waiting on a background task to resolve the room alias. This is not optional, and I left the same comment on your previous PR on this topic.
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.
Recall that in general, we want to avoid doing as much work as possible on the main UI thread, and move as many things as possible to a background task/thread.
src/home/room_screen.rs
Outdated
log!("TODO: jump to known room {}", room_id); | ||
if let Some(known_room) = get_client().and_then(|c| c.get_room(room_id)) { | ||
cx.widget_action(uid, &Scope::empty().path, RoomsListAction::Selected( | ||
SelectedRoom::JoinedRoom { |
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.
how do you know the room is a Joined room here? what if it's an invite?
src/home/room_screen.rs
Outdated
// Only handle this action if it was requested by this widget | ||
if *requester_uid == room_screen_widget_uid { |
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.
while it seems like a good idea to use the RoomScreen widget UID for this, we should use the Room ID instead. The reason for this is because if the user changes from Desktop to Mobile view (or simply closes the RoomScreen tab and opens it in a new tab), then the widget UID will change, so this condition will never occur.
We want any RoomScreen that is showing this room's Room ID to be able to handle this resolved room alias action.
src/home/room_screen.rs
Outdated
@@ -995,6 +996,43 @@ impl Widget for RoomScreen { | |||
); | |||
} | |||
} | |||
|
|||
// Handle resolved room alias actions - only for requests from this widget | |||
if let Some(ResolveRoomAliasAction::Resolved { requester_uid, room_alias: _ , room_id, servers: _ }) = action.downcast_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.
you should use a match statement for all ResolveRoomAliasAction
s, not multiple separate if let
blocks.
src/home/room_screen.rs
Outdated
if let Some(known_room) = get_client().and_then(|c| c.get_room(room_id)) { | ||
if known_room.is_space() { | ||
enqueue_popup_notification(PopupItem { | ||
message: format!("Found space {} but it is a space, not a regular room.", 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.
This is fine, but the real reason that we can't do anything here is because we don't yet support showing a preview of a Space itself.
message: format!("Found space {} but it is a space, not a regular room.", room_id), | |
message: String::from("Showing a space's home page is not yet supported."), |
src/home/room_screen.rs
Outdated
}); | ||
} else { | ||
cx.widget_action(room_screen_widget_uid, &scope.path, RoomsListAction::Selected( | ||
SelectedRoom::JoinedRoom { |
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.
why are you assuming this is a joined room?
src/home/room_screen.rs
Outdated
} | ||
} else { | ||
enqueue_popup_notification(PopupItem { | ||
message: format!("Found room {} but you are not joined to it yet.", 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.
i dont understand this error condition. Why is it a problem that the room hasn't been joined yet? Can't you request a preview of non-joined rooms?
(Note that my comment that you removed also mentioned this. I believe that Client::get_room_preview()
will handle all of the cases 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.
The only conditions we'd want to notify the user of are:
- an error if we cannot fetch the room preview
- a notice if the room exists but cannot be previewed and must be joined instead
src/home/room_screen.rs
Outdated
auto_dismissal_duration: Some(3.0) | ||
}); | ||
} else { | ||
cx.widget_action(uid, &Scope::empty().path, RoomsListAction::Selected( |
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 assuming this room has been joined again.
Blocked on Makepad PR see: makepad/makepad#766 |
@kevinaboos Need to update latest Makepad version, because Julian's PR about StackNavigation has been merged makepad/makepad#766, need to use something about that PR. Thanks. now i use my local latest makepad repo. |
Indeed, thanks for mentioning that. We have a PR already in progress that updates to the latest version of Makepad, which is #521. Once that is merged, you can proceed here. |
Blocked on #521 |
PR content
Related PR
matrix.to
links for Rooms #87