chore(room_preview): add `RoomListItem::preview_room` #4152

jmartinesp · 2024-10-18T11:19:42Z

This method will return a RoomPreview for the provided room id.

Also added fn RoomPreview::leave() action to be able to decline invites or cancel knocks, since there wasn't a Client::leave_room_by_id counterpart as there is for join.

The PR also deprecates RoomListItem::invited_room, since we have a better alternative now.

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-10-18T11:33:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.86%. Comparing base (31e9600) to head (c7f5638).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4152   +/-   ##
=======================================
  Coverage   84.85%   84.86%           
=======================================
  Files         269      269           
  Lines       28916    28924    +8     
=======================================
+ Hits        24536    24545    +9     
+ Misses       4380     4379    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bindings/matrix-sdk-ffi/src/room_list.rs

bnjbvr · 2024-10-21T09:49:45Z

bindings/matrix-sdk-ffi/src/room_list.rs

+    ///
+    /// ⚠️ Holding on to this room instance after it has been joined is not
+    /// safe. Use `full_room` instead.
+    fn room_without_timeline(&self) -> Result<Arc<Room>, RoomListError> {


Really not fond of this name, I see one of three ways to go from here (there can be more!):

Either rename this preview_room() or stripped_room(), as it's really a very basic stripped view of a room.

Rename fn room() to fn room_with_timeline(), and this one can be fn room() later.

(is kind of orthogonal, and could be in addition to (1) or (2)) It might be nice to return a StrippedRoom or PreviewRoom type instead of Room, from this function, to make it clear which functions are available and which are not. In fact, this might even get rid of the unused internal timeline instance.

What do you think?

preview_room sounds fine to me. As for having a separate struct backing those, maybe we can create one, once we know all the values we need to expose through the FFI. I can try to get this working.

bnjbvr · 2024-10-21T09:51:18Z

bindings/matrix-sdk-ffi/src/lib.rs

+#[allow(deprecated)]
 mod room_list;


How many uses of the deprecated functions are there in this module? Can we add the guard on them one by one instead, or would that be too many? If too many, can we put the allow guard in the module itself, instead of here?

When I tried using the allow guard in the function it didn't take effect (I still had the same compiler warning message, which clippy turned into an error), I had to place it in the module for it to work. I suspect it has something to do with the FFI generated code, but I'm not sure:

error: use of deprecated method `room_list::RoomListItem::invited_room`: Please use `room_without_timeline` instead. --> bindings/matrix-sdk-ffi/src/room_list.rs:594:8 | 594 | fn invited_room(&self) -> Result<Arc<Room>, RoomListError> { | ^^^^^^^^^^^^ | = note: `-D deprecated` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(deprecated)]`

Ah yeah, expanding the macro could tell us whether it's possible to put it on a slightly more specialized location, e.g. the impl block itself, or if it's using it in code generated elsewhere.

Adding it to the impl block fails too. I've only been able to fix it by adding it in the mod declaration.

When I add it to the function, the expanded macro looks like this:

#[doc = " Builds a `Room` FFI from an invited room without initializing its"] #[doc = " internal timeline."] #[doc = ""] #[doc = " An error will be returned if the room is a state different than invited."] #[doc = ""] #[doc = " ⚠\u{65039} Holding on to this room instance after it has been joined is not"] #[doc = " safe. Use `full_room` instead."] #[allow(deprecated)] #[deprecated(note = "Please use `preview_room` instead.")] fn invited_room(&self) -> Result<Arc<Room>, RoomListError> { if !match (self.membership()) { Membership::Invited => true, _ => false } { return Err(RoomListError::IncorrectRoomMembership { expected: (<[_]>::into_vec( #[rustc_box] ::alloc::boxed::Box::new([Membership::Invited]) )), actual: self.membership(), }); } Ok(Arc::new(Room::new(self.inner.inner_room().clone()))) }

And if I add it to the impl block it looks like:

#[uniffi::export(async_runtime = "tokio", )] #[allow(deprecated)] impl RoomListItem { ...

But clippy still complains in both cases.

Yeah, it's the call sites that need the allow(deprecated) guard, not the function itself; but it might be that it's in code generated by uniffi, that's outside our control, so we might need to resort to the mod-level guard indeed.

Forgot to comment here: it's indeed an unmangled free-function generated in the same module, that's calling the invited_room method, so welp 🤷 Module-level guard it is. I wondered if we could get rid of the entire invited_room method altogether, but I don't want to add more API changes to the other side of this PR.

Hywan · 2024-10-22T08:19:01Z

bindings/matrix-sdk-ffi/src/room_list.rs

+        if !matches!(membership, Membership::Invited)
+            && !matches!(self.membership(), Membership::Knocked)


It can be a single matches!.

Suggested change

if !matches!(membership, Membership::Invited)

&& !matches!(self.membership(), Membership::Knocked)

if !matches(membership, Membership::Invited | Membership::Knocked)

… both an invited or a knocked room. This deprecates `RoomListItem::invited_room`.

This allows us to get `RoomPreview` from a room list item when the membership is either invited or knocked. Also, modify `RoomPreview` so it contains both the room info needed for the preview as well as 2 methods, for joining the room or leaving it (rejecting the invite/rescinding the knock).

jmartinesp · 2024-10-23T13:38:07Z

I ended up creating a RoomPreviewActions struct in the FFI layer so when you request a RoomListItem::preview_room you get a RoomPreview record that contains RoomPreviewInfo (also a record) and RoomPreviewActions, which are either join, leave or knock and check the membership state of the room preview before running. So from a client, you'd do something like:

val preview = roomListItem.previewRoom()
if (preview.info.membership == Membership.Invite) {
    preview.actions.join()
}

Also, the idea with the different JoinRoomAction, LeaveRoomAction and KnockRoomAction is that are a kind of 'use cases' that could be reused both by Room and RoomPreview if we want them to, so we have the same logic on both places. However, the code for Room isn't implemented yet, as I wasn't sure if we wanted to take this approach.

bnjbvr

I think we don't need all the preview actions data structure at the SDK level; these need to exist at the FFI layer because of… reasons (that I'm detailed below; let me know if that's wrong). In the current shape I am strongly not in favor of merging this, let's identify what's the minimal set of changes that we could have to unblock us, and maybe consider doing some refactoring later.

bnjbvr · 2024-10-24T10:06:10Z

crates/matrix-sdk/src/room_preview.rs

    }
 }
+
+pub enum RoomPreviewActions {


In this entire file you need to comment each struct, enum, variant, field, function. I'll enable a new lint for this 😈

This was fixed in the following commit if I'm not mistaken.

bnjbvr · 2024-10-24T10:08:30Z

crates/matrix-sdk/src/room_preview.rs

+    #[error(transparent)]
+    Generic(#[from] Box<dyn std::error::Error + Send + Sync>)


I really don't like this; having an exceptional generic error type like that should be limited at the strict minimum, i.e. matrix_sdk::Error only. Let's avoid it entirely by not making a custom error, but instead use an matrix_sdk::Error instead of this error type, there's already WrongRoomState which describes the same error.

Removed in 0fcfd84.

bnjbvr · 2024-10-24T10:11:34Z

bindings/matrix-sdk-ffi/src/room_list.rs

+    /// Builds a `RoomPreview` from a room list item. This is intended for rooms
+    /// with [`Membership::Invite`] or [`Membership::Knocked`],


nit: this sentence ends with a comma, and it doesn't really translate to proper English IMO: maybe This is intended for invited or knocked rooms.?

f8110d2 should fix this.

bnjbvr · 2024-10-24T10:16:47Z

bindings/matrix-sdk-ffi/src/room_preview.rs

+struct
+
+impl RoomPreview {


Seems like this is not an atomic commit… (It's fine but next time to make the review easier, feel free to squash them altogether, since I know that another commit will now change these three lines.)

bnjbvr · 2024-10-24T10:17:21Z

bindings/matrix-sdk-ffi/src/room_preview.rs

 #[derive(uniffi::Record)]
 pub struct RoomPreview {
+    pub info: RoomPreviewInfo,


Why do we need the intermediary RoomPreviewInfo data structure?

So we didn't need to move everything inside RoomPreview into a Record if we made it an Object (as is the case now since I'm removing the separate actions part).

bnjbvr · 2024-10-24T10:21:56Z

bindings/matrix-sdk-ffi/src/room_list.rs

@@ -624,7 +622,7 @@ impl RoomListItem {
        };

        let room_preview = client.get_room_preview(&room_or_alias_id, server_names).await?;
-        Ok(Arc::new(RoomPreview::try_from_sdk(room_preview, client)))
+        RoomPreview::try_from_sdk(room_preview, client)


This fixes stuff from the previous commit, right? 😔

bnjbvr · 2024-10-24T10:26:59Z

crates/matrix-sdk/src/room_preview.rs

+    pub fn new(client: Client, room_preview: RoomPreview) -> Self {
+        let room_id = room_preview.room_id;
+        let room_state = room_preview.state;
+        let is_direct = room_preview.is_direct.unwrap_or_default();


That's a lie, let's keep an Option<bool> for is_direct ;)

This code was removed.

bnjbvr · 2024-10-24T10:28:13Z

crates/matrix-sdk/src/room_preview.rs

-    Knocked { leave: LeaveRoomPreviewAction },
+/// A set of actions that can be performed in a [`RoomPreview`].
+#[derive(Debug)]
+pub struct RoomPreviewActions {


Overall, I don't think this data structure and the other actions are required at the SDK level; maybe they're required at the FFI layer because we need some kind of "lazy provider"-like object for the Record requirements, but I don't think they make sense for the RoomPreview of the SDK, considering there are many other methods that already exist providing the same functionality, so removing it would avoid blowing up the number of abstractions, code duplication, etc.

I added it to the SDK so they could be tested and didn't have to duplicate code, i.e. the Room::join contents mentioned below, which could be just put into one of these actions and then reused inside Room::join instead:

impl Room { async fn join(&self) -> Result<_, _> { self.actions.join() }

But if that's not something we want, we can just remove the actions, leaving a needed RoomPreview::leave method that's needed (Client has code for joining and knocking, but it doesn't have one for leaving a room given its id, so a RoomPreview with invite/knock state can never be left like that).

Actions removed in 0fcfd84

bnjbvr · 2024-10-24T10:29:20Z

crates/matrix-sdk/src/room_preview.rs

+            match self.client.join_room_by_id(&self.room_id).await {
                Ok(room) => {
-                    if room_preview.is_direct.unwrap_or(false) {
+                    if self.is_direct {
                        if let Some(e) = room.set_is_direct(true).await.err() {
                            warn!("error when setting room as direct: {e}");
                        }


This code sequence is duplicated from somewhere else, right? If so, that would be bad, because now we'd need to keep both code locations in sync without each code location knowing about the other :/

testing/matrix-sdk-test/src/sync_builder/mod.rs

jmartinesp · 2024-10-24T14:24:25Z

I removed the Room*Actions in 0fcfd84.
I fixed the RoomPreview contents and added a fn RoomPreview::leave() that was needed in 7bfb1bd.
The doc comment using direct references to types was fixed in f8110d2.
I tried to explain why the JoinRule::_Custom case can't be handled and we default to Private in 69ee5c2.
And 1f08885 should fix the bindings 🤞 .

bnjbvr

Sorry, it's still too many unneeded changes for me. Let me try to hack on this, see if I can get it into the shape I'm expecting.

bnjbvr · 2024-10-25T10:30:44Z

crates/matrix-sdk/src/client/mod.rs

@@ -1208,6 +1208,13 @@ impl Client {
        Ok(Room::new(self.clone(), base_room))
    }

+    /// Leave a room given its `RoomId`.


We don't need this method, it's the same as Room::leave(), and we can only leave a room we do know about (i.e. we can get it with Client::get_room first).

I think we're going in circles... We don't have anything in the ffi::Client to get a Room, and even if we had it, it would be a 'not safe to hold onto' Room as we had in RoomListItem::invited_room (see #4152 (comment)). That's the main reason I split Room and RoomPreview here, and I think it's what we agreed to do.

So now I'm not sure what to do anymore:

If we're ok with returning a Room with no timeline as we did before then I'll just remove everything from RoomPreview and just return that Room in RoomListItem::preview_room instead.

If we're not ok with that, then we need a way to be able to leave that room (aka decline the invite, cancel the knock).

bnjbvr · 2024-10-25T10:35:11Z

crates/matrix-sdk/src/room_preview.rs

+    /// Leave a room using its [`RoomPreview`].
+    pub async fn leave(&self) -> Result<(), Error> {
+        match self.state {
+            Some(RoomState::Left) | None => {
+                Err(Error::WrongRoomPreviewState(WrongRoomPreviewState {
+                    expected: "Invited, Knocked or Joined",
+                    got: self.state,
+                }))
+            }
+            Some(_) => self.client.leave_room(&self.room_id).await.map_err(Into::into),
+        }
+    }


Sorry, I still think that RoomPreview ought to be a purely descriptive data record (i.e. no action methods); can the caller use the room id that's always around, and call leave_room by themselves? Then, can we get rid of the Client stored and address the other comment I had on from_room_info?

bnjbvr · 2024-10-25T10:48:53Z

bindings/matrix-sdk-ffi/src/room_preview.rs

+                // Warning! If new join rules are introduced we may be mapping wrong values
+                // here, but there's no way to match
+                // `SpaceRoomJoinRule::_Custom(_)` and have an exhaustive match.
+                JoinRule::Private


I'd do it differently:

for custom, add a JoinRule::Custom(String) variant that's built with join_rule.to_string() (SpaceRoomJoinRule implements StringEnum which derives a Display impl)?

for _, log a warning and return None. Yes, let's turn this into TryInto 😇

bnjbvr · 2024-10-25T10:52:11Z

bindings/matrix-sdk-ffi/src/client.rs

@@ -1027,7 +1027,7 @@ impl Client {
        &self,
        room_id: String,
        via_servers: Vec<String>,
-    ) -> Result<RoomPreview, ClientError> {
+    ) -> Result<Arc<RoomPreview>, ClientError> {


Do we still need all the Arcs here? (I'm not very sure about it myself, but I suspect we don't since RoomPreview is now a uniffi object)

bnjbvr · 2024-10-25T10:53:10Z

bindings/matrix-sdk-ffi/src/room_list.rs

+            let server_names: Vec<OwnedServerName> = via
+                .into_iter()
+                .map(|server| ServerName::parse(server).map_err(ClientError::from))
+                .collect::<Result<_, ClientError>>()?;


It's a bit weird that if there's a canonical alias, the via routes will not be validated 🤔 Should we validate them ahead, even if we're discarding them before?

crates/matrix-sdk/tests/integration/room_preview.rs

bnjbvr · 2024-10-25T10:56:54Z

bindings/matrix-sdk-ffi/src/lib.rs

+#[allow(deprecated)]
 mod room_list;


Yeah, it's the call sites that need the allow(deprecated) guard, not the function itself; but it might be that it's in code generated by uniffi, that's outside our control, so we might need to resort to the mod-level guard indeed.

jmartinesp · 2024-10-25T11:42:29Z

bindings/matrix-sdk-ffi/src/room_preview.rs

+                // Since we have no way to get the _Custom contents, assume it's private.
+                // Warning! If new join rules are introduced we may be mapping wrong values
+                // here, but there's no way to match
+                // `SpaceRoomJoinRule::_Custom(_)` and have an exhaustive match.


Maybe remove/fix this comment?

bnjbvr · 2024-10-25T12:33:02Z

Jorge confirmed this was good enough for the EX apps, so merging. Thanks @jmartinesp for all the work done here, and sorry about the few back-and-forths.

jmartinesp requested a review from a team as a code owner October 18, 2024 11:19

jmartinesp requested review from bnjbvr and removed request for a team October 18, 2024 11:19

This was referenced Oct 18, 2024

[Story] Request to join a room element-hq/element-meta#2577

Open

[Story] Cancel a request to join element-hq/element-meta#2578

Open

bnjbvr requested changes Oct 21, 2024

View reviewed changes

Hywan reviewed Oct 22, 2024

View reviewed changes

Velin92 mentioned this pull request Oct 22, 2024

Knocked Preview implementation element-hq/element-x-ios#3426

Merged

jmartinesp added 2 commits October 23, 2024 13:45

chore(ffi): add RoomListItem::room_without_timeline that can return…

084e00b

… both an invited or a knocked room. This deprecates `RoomListItem::invited_room`.

jmartinesp force-pushed the chore/add-room-list-item-room-without-timeline branch from 1127a48 to 26958b5 Compare October 23, 2024 12:20

jmartinesp requested a review from bnjbvr October 23, 2024 13:40

bnjbvr requested changes Oct 24, 2024

View reviewed changes

jmartinesp requested a review from bnjbvr October 24, 2024 14:28

jmartinesp changed the title ~~chore(ffi): add RoomListItem::room_without_timeline~~ chore(room_preview): add RoomListItem::preview_room Oct 24, 2024

bnjbvr requested changes Oct 25, 2024

View reviewed changes

chore(room_preview): Add knocking action to the RoomPreviewActions too

d0048e7

bnjbvr force-pushed the chore/add-room-list-item-room-without-timeline branch from 7a8668a to f59d47f Compare October 25, 2024 11:39

jmartinesp commented Oct 25, 2024

View reviewed changes

bnjbvr force-pushed the chore/add-room-list-item-room-without-timeline branch from 05e449d to 8aaaf23 Compare October 25, 2024 11:45

bnjbvr approved these changes Oct 25, 2024

View reviewed changes

bnjbvr mentioned this pull request Oct 25, 2024

chore(ffi): introduce AsyncRuntimeDropped helper #4168

Merged

benjamin's review; let's squash this MR

c7f5638

bnjbvr force-pushed the chore/add-room-list-item-room-without-timeline branch from 8aaaf23 to c7f5638 Compare October 25, 2024 12:16

bnjbvr merged commit 40f4fc1 into main Oct 25, 2024
40 checks passed

bnjbvr deleted the chore/add-room-list-item-room-without-timeline branch October 25, 2024 12:33

		if !matches!(membership, Membership::Invited)
		&& !matches!(self.membership(), Membership::Knocked)

	if !matches!(membership, Membership::Invited)
	&& !matches!(self.membership(), Membership::Knocked)
	if !matches(membership, Membership::Invited \| Membership::Knocked)

		#[error(transparent)]
		Generic(#[from] Box<dyn std::error::Error + Send + Sync>)

		/// Builds a `RoomPreview` from a room list item. This is intended for rooms
		/// with [`Membership::Invite`] or [`Membership::Knocked`],

chore(room_preview): add RoomListItem::preview_room #4152

chore(room_preview): add RoomListItem::preview_room #4152

Conversation

jmartinesp commented Oct 18, 2024 • edited Loading

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Oct 23, 2024 • edited Loading

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Oct 24, 2024 • edited Loading

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented Oct 25, 2024

chore(room_preview): add `RoomListItem::preview_room` #4152

chore(room_preview): add `RoomListItem::preview_room` #4152

jmartinesp commented Oct 18, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

jmartinesp Oct 21, 2024 •

edited

Loading

jmartinesp commented Oct 23, 2024 •

edited

Loading

jmartinesp Oct 24, 2024 •

edited

Loading

jmartinesp commented Oct 24, 2024 •

edited

Loading

jmartinesp Oct 25, 2024 •

edited

Loading