benjamin's review; let's squash this MR

Author: bnjbvr

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

@@ -1040,9 +1040,9 @@ impl Client {
         // rustc win that one fight.
         let room_id: &RoomId = &room_id;
 
-        let sdk_room_preview = self.inner.get_room_preview(room_id.into(), via_servers).await?;
+        let room_preview = self.inner.get_room_preview(room_id.into(), via_servers).await?;
 
-        Ok(Arc::new(RoomPreview::from_sdk(sdk_room_preview)))
+        Ok(Arc::new(RoomPreview::new(self.inner.clone(), room_preview)))
     }
 
     /// Given a room alias, get the preview of a room, to interact with it.
@@ -1057,9 +1057,9 @@ impl Client {
         // rustc win that one fight.
         let room_alias: &RoomAliasId = &room_alias;
 
-        let sdk_room_preview = self.inner.get_room_preview(room_alias.into(), Vec::new()).await?;
+        let room_preview = self.inner.get_room_preview(room_alias.into(), Vec::new()).await?;
 
-        Ok(Arc::new(RoomPreview::from_sdk(sdk_room_preview)))
+        Ok(Arc::new(RoomPreview::new(self.inner.clone(), room_preview)))
     }
 
     /// Waits until an at least partially synced room is received, and returns
@@ -1830,6 +1830,12 @@ pub enum JoinRule {
     /// conditions described in a set of [`AllowRule`]s, or they can request
     /// an invite to the room.
     KnockRestricted { rules: Vec<AllowRule> },
+
+    /// A custom join rule, up for interpretation by the consumer.
+    Custom {
+        /// The string representation for this custom rule.
+        repr: String,
+    },
 }
 
 /// An allow rule which defines a condition that allows joining a room.
@@ -1857,6 +1863,7 @@ impl TryFrom<JoinRule> for ruma::events::room::join_rules::JoinRule {
                 let rules = allow_rules_from(rules)?;
                 Ok(Self::KnockRestricted(ruma::events::room::join_rules::Restricted::new(rules)))
             }
+            JoinRule::Custom { repr } => Ok(serde_json::from_str(&repr)?),
         }
     }
 }

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

@@ -2,8 +2,8 @@ use std::{collections::HashMap, fmt, fmt::Display};
 
 use matrix_sdk::{
     encryption::CryptoStoreError, event_cache::EventCacheError, oidc::OidcError, reqwest,
-    room::edit::EditError, room_preview::WrongRoomPreviewState, send_queue::RoomSendQueueError,
-    HttpError, IdParseError, NotificationSettingsError as SdkNotificationSettingsError,
+    room::edit::EditError, send_queue::RoomSendQueueError, HttpError, IdParseError,
+    NotificationSettingsError as SdkNotificationSettingsError,
     QueueWedgeError as SdkQueueWedgeError, StoreError,
 };
 use matrix_sdk_ui::{encryption_sync_service, notification_client, sync_service, timeline};
@@ -155,12 +155,6 @@ impl From<RoomSendQueueError> for ClientError {
     }
 }
 
-impl From<WrongRoomPreviewState> for ClientError {
-    fn from(e: WrongRoomPreviewState) -> Self {
-        Self::new(e)
-    }
-}
-
 /// Bindings version of the sdk type replacing OwnedUserId/DeviceIds with simple
 /// String.
 ///

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

@@ -18,8 +18,6 @@ mod platform;
 mod room;
 mod room_directory_search;
 mod room_info;
-
-#[allow(deprecated)]
 mod room_list;
 mod room_member;
 mod room_preview;

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

@@ -1,4 +1,12 @@
-use std::{fmt::Debug, mem::MaybeUninit, ptr::addr_of_mut, sync::Arc, time::Duration};
+#![allow(deprecated)]
+
+use std::{
+    fmt::Debug,
+    mem::{ManuallyDrop, MaybeUninit},
+    ptr::addr_of_mut,
+    sync::Arc,
+    time::Duration,
+};
 
 use eyeball_im::VectorDiff;
 use futures_util::{pin_mut, StreamExt, TryFutureExt};
@@ -599,6 +607,13 @@ impl RoomListItem {
     /// An error will be returned if the room is in a state other than invited
     /// or knocked.
     async fn preview_room(&self, via: Vec<String>) -> Result<Arc<RoomPreview>, ClientError> {
+        // Validate parameters first.
+        let server_names: Vec<OwnedServerName> = via
+            .into_iter()
+            .map(|server| ServerName::parse(server).map_err(ClientError::from))
+            .collect::<Result<_, ClientError>>()?;
+
+        // Validate internal room state.
         let membership = self.membership();
         if !matches!(membership, Membership::Invited | Membership::Knocked) {
             return Err(RoomListError::IncorrectRoomMembership {
@@ -608,21 +623,19 @@ impl RoomListItem {
             .into());
         }
 
+        // Do the thing.
         let client = self.inner.client();
         let (room_or_alias_id, server_names) = if let Some(alias) = self.inner.canonical_alias() {
             let room_or_alias_id: OwnedRoomOrAliasId = alias.into();
             (room_or_alias_id, Vec::new())
         } else {
             let room_or_alias_id: OwnedRoomOrAliasId = self.inner.id().to_owned().into();
-            let server_names: Vec<OwnedServerName> = via
-                .into_iter()
-                .map(|server| ServerName::parse(server).map_err(ClientError::from))
-                .collect::<Result<_, ClientError>>()?;
             (room_or_alias_id, server_names)
         };
 
         let room_preview = client.get_room_preview(&room_or_alias_id, server_names).await?;
-        Ok(Arc::new(RoomPreview::from_sdk(room_preview)))
+
+        Ok(Arc::new(RoomPreview::new(ManuallyDrop::new(client), room_preview)))
     }
 
     /// Build a full `Room` FFI object, filling its associated timeline.

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

@@ -1,23 +1,41 @@
-use std::sync::Arc;
+use std::mem::ManuallyDrop;
 
-use matrix_sdk::room_preview::RoomPreview as SdkRoomPreview;
+use anyhow::Context as _;
+use async_compat::TOKIO1 as RUNTIME;
+use matrix_sdk::{room_preview::RoomPreview as SdkRoomPreview, Client};
 use ruma::space::SpaceRoomJoinRule;
+use tracing::warn;
 
 use crate::{client::JoinRule, error::ClientError, room::Membership};
 
 /// A room preview for a room. It's intended to be used to represent rooms that
 /// aren't joined yet.
 #[derive(uniffi::Object)]
 pub struct RoomPreview {
-    inner: Arc<SdkRoomPreview>,
+    inner: SdkRoomPreview,
+    client: ManuallyDrop<Client>,
+}
+
+impl Drop for RoomPreview {
+    fn drop(&mut self) {
+        // Dropping the inner OlmMachine must happen within a tokio context
+        // because deadpool drops sqlite connections in the DB pool on tokio's
+        // blocking threadpool to avoid blocking async worker threads.
+        let _guard = RUNTIME.enter();
+        // SAFETY: self.client is never used again, which is the only requirement
+        //         for ManuallyDrop::drop to be used safely.
+        unsafe {
+            ManuallyDrop::drop(&mut self.client);
+        }
+    }
 }
 
 #[matrix_sdk_ffi_macros::export]
 impl RoomPreview {
     /// Returns the room info the preview contains.
-    pub fn info(&self) -> RoomPreviewInfo {
+    pub fn info(&self) -> Result<RoomPreviewInfo, ClientError> {
         let info = &self.inner;
-        RoomPreviewInfo {
+        Ok(RoomPreviewInfo {
             room_id: info.room_id.to_string(),
             canonical_alias: info.canonical_alias.as_ref().map(|alias| alias.to_string()),
             name: info.name.clone(),
@@ -27,22 +45,28 @@ impl RoomPreview {
             room_type: info.room_type.as_ref().map(|room_type| room_type.to_string()),
             is_history_world_readable: info.is_world_readable,
             membership: info.state.map(|state| state.into()),
-            join_rule: info.join_rule.clone().into(),
-        }
+            join_rule: info
+                .join_rule
+                .clone()
+                .try_into()
+                .map_err(|_| anyhow::anyhow!("unhandled SpaceRoomJoinRule kind"))?,
+        })
     }
 
     /// Leave the room if the room preview state is either joined, invited or
     /// knocked.
     ///
     /// Will return an error otherwise.
     pub async fn leave(&self) -> Result<(), ClientError> {
-        self.inner.leave().await.map_err(Into::into)
+        let room =
+            self.client.get_room(&self.inner.room_id).context("missing room for a room preview")?;
+        room.leave().await.map_err(Into::into)
     }
 }
 
 impl RoomPreview {
-    pub(crate) fn from_sdk(room_preview: SdkRoomPreview) -> Self {
-        Self { inner: Arc::new(room_preview) }
+    pub(crate) fn new(client: ManuallyDrop<Client>, inner: SdkRoomPreview) -> Self {
+        Self { client, inner }
     }
 }
 
@@ -71,22 +95,22 @@ pub struct RoomPreviewInfo {
     pub join_rule: JoinRule,
 }
 
-impl From<SpaceRoomJoinRule> for JoinRule {
-    fn from(join_rule: SpaceRoomJoinRule) -> Self {
-        match join_rule {
+impl TryFrom<SpaceRoomJoinRule> for JoinRule {
+    type Error = ();
+
+    fn try_from(join_rule: SpaceRoomJoinRule) -> Result<Self, ()> {
+        Ok(match join_rule {
             SpaceRoomJoinRule::Invite => JoinRule::Invite,
             SpaceRoomJoinRule::Knock => JoinRule::Knock,
             SpaceRoomJoinRule::Private => JoinRule::Private,
             SpaceRoomJoinRule::Restricted => JoinRule::Restricted { rules: Vec::new() },
             SpaceRoomJoinRule::KnockRestricted => JoinRule::KnockRestricted { rules: Vec::new() },
             SpaceRoomJoinRule::Public => JoinRule::Public,
+            SpaceRoomJoinRule::_Custom(_) => JoinRule::Custom { repr: join_rule.to_string() },
             _ => {
-                // 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.
-                JoinRule::Private
+                warn!("unhandled SpaceRoomJoinRule: {join_rule}");
+                return Err(());
             }
-        }
+        })
     }
 }

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

@@ -54,7 +54,7 @@ use ruma::{
             },
             filter::{create_filter::v3::Request as FilterUploadRequest, FilterDefinition},
             knock::knock_room,
-            membership::{join_room_by_id, join_room_by_id_or_alias, leave_room},
+            membership::{join_room_by_id, join_room_by_id_or_alias},
             room::create_room,
             session::login::v3::DiscoveryInfo,
             sync::sync_events,
@@ -1208,13 +1208,6 @@ impl Client {
         Ok(Room::new(self.clone(), base_room))
     }
 
-    /// Leave a room given its `RoomId`.
-    pub async fn leave_room(&self, room_id: &RoomId) -> Result<()> {
-        let request = leave_room::v3::Request::new(room_id.to_owned());
-        self.send(request, None).await?;
-        self.base_client().room_left(room_id).await.map_err(Into::into)
-    }
-
     /// Search the homeserver's directory of public rooms.
     ///
     /// Sends a request to "_matrix/client/r0/publicRooms", returns

crates/matrix-sdk/src/error.rs

@@ -45,9 +45,7 @@ use serde_json::Error as JsonError;
 use thiserror::Error;
 use url::ParseError as UrlParseError;
 
-use crate::{
-    event_cache::EventCacheError, room_preview::WrongRoomPreviewState, store_locks::LockStoreError,
-};
+use crate::{event_cache::EventCacheError, store_locks::LockStoreError};
 
 /// Result type of the matrix-sdk.
 pub type Result<T, E = Error> = std::result::Result<T, E>;
@@ -350,12 +348,6 @@ pub enum Error {
     #[error("wrong room state: {0}")]
     WrongRoomState(WrongRoomState),
 
-    /// Attempted to call a method on a room preview that requires the user to
-    /// have a specific membership state in the room, but the membership
-    /// state is different or none.
-    #[error("wrong room state: {0}")]
-    WrongRoomPreviewState(WrongRoomPreviewState),
-
     /// Session callbacks have been set multiple times.
     #[error("session callbacks have been set multiple times")]
     MultipleSessionCallbacks,

crates/matrix-sdk/src/room_preview.rs

@@ -29,7 +29,7 @@ use ruma::{
 use tokio::try_join;
 use tracing::{instrument, warn};
 
-use crate::{Client, Error, Room};
+use crate::{Client, Room};
 
 /// The preview of a room, be it invited/joined/left, or not.
 #[derive(Debug, Clone)]
@@ -72,43 +72,20 @@ pub struct RoomPreview {
 
     /// The `m.room.direct` state of the room, if known.
     pub is_direct: Option<bool>,
-
-    /// The [`Client`] that can be used to perform actions given the room
-    /// preview.
-    pub(crate) client: Client,
 }
 
 impl RoomPreview {
-    /// 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),
-        }
-    }
-
     /// Constructs a [`RoomPreview`] from the associated room info.
     ///
     /// Note: not using the room info's state/count of joined members, because
     /// we can do better than that.
-    async fn from_room_info(
-        client: &Client,
+    fn from_room_info(
         room_info: RoomInfo,
+        is_direct: Option<bool>,
         num_joined_members: u64,
         state: Option<RoomState>,
     ) -> Self {
-        let is_direct = if let Some(room) = client.get_room(room_info.room_id()) {
-            room.is_direct().await.ok()
-        } else {
-            None
-        };
         RoomPreview {
-            client: client.clone(),
             room_id: room_info.room_id().to_owned(),
             canonical_alias: room_info.canonical_alias().map(ToOwned::to_owned),
             name: room_info.name().map(ToOwned::to_owned),
@@ -138,13 +115,14 @@ impl RoomPreview {
     /// Create a room preview from a known room (i.e. one we've been invited to,
     /// we've joined or we've left).
     pub(crate) async fn from_known(room: &Room) -> Self {
+        let is_direct = room.is_direct().await.ok();
+
         Self::from_room_info(
-            &room.client,
             room.clone_info(),
+            is_direct,
             room.joined_members_count(),
             Some(room.state()),
         )
-        .await
     }
 
     #[instrument(skip(client))]
@@ -207,7 +185,6 @@ impl RoomPreview {
         };
 
         Ok(RoomPreview {
-            client: client.clone(),
             room_id,
             canonical_alias: response.canonical_alias,
             name: response.name,
@@ -259,23 +236,10 @@ impl RoomPreview {
             room_info.handle_state_event(&ev.into());
         }
 
-        let state = client.get_room(room_id).map(|room| room.state());
-
-        Ok(Self::from_room_info(client, room_info, num_joined_members, state).await)
-    }
-}
-
-/// An unexpected room preview state was found.
-#[derive(Debug, thiserror::Error)]
-#[error("expected: {expected}, got: {got:?}")]
-pub struct WrongRoomPreviewState {
-    expected: &'static str,
-    got: Option<RoomState>,
-}
+        let room = client.get_room(room_id);
+        let state = room.as_ref().map(|room| room.state());
+        let is_direct = if let Some(room) = room { room.is_direct().await.ok() } else { None };
 
-impl WrongRoomPreviewState {
-    /// Instantiate a new
-    pub fn new(expected: &'static str, got: Option<RoomState>) -> Self {
-        Self { expected, got }
+        Ok(Self::from_room_info(room_info, is_direct, num_joined_members, state))
     }
 }