benjamin's review; let's squash this MR

Author: bnjbvr

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

@@ -1027,7 +1027,7 @@ impl Client {
         &self,
         room_id: String,
         via_servers: Vec<String>,
-    ) -> Result<Arc<RoomPreview>, ClientError> {
+    ) -> Result<RoomPreview, ClientError> {
         let room_id = RoomId::parse(&room_id).context("room_id is not a valid room id")?;
 
         let via_servers = via_servers
@@ -1040,26 +1040,26 @@ 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(RoomPreview::new(self.inner.clone(), room_preview))
     }
 
     /// Given a room alias, get the preview of a room, to interact with it.
     pub async fn get_room_preview_from_room_alias(
         &self,
         room_alias: String,
-    ) -> Result<Arc<RoomPreview>, ClientError> {
+    ) -> Result<RoomPreview, ClientError> {
         let room_alias =
             RoomAliasId::parse(&room_alias).context("room_alias is not a valid room alias")?;
 
         // The `into()` call below doesn't work if I do `(&room_id).into()`, so I let
         // 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(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

@@ -19,7 +19,6 @@ 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};
@@ -598,7 +606,14 @@ 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> {
+    async fn preview_room(&self, via: Vec<String>) -> Result<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(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,26 @@ 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,