Use ClientBuilder pattern in SDK FFI #739
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,87 @@ | ||||||
| use std::{fs, path::PathBuf, sync::Arc}; | ||||||
|
|
||||||
| use anyhow::Context; | ||||||
| use matrix_sdk::{ | ||||||
| ruma::UserId, store::make_store_config, Client as MatrixClient, | ||||||
| ClientBuilder as MatrixClientBuilder, | ||||||
| }; | ||||||
| use sanitize_filename_reader_friendly::sanitize; | ||||||
|
|
||||||
| use super::{client::Client, ClientState, RUNTIME}; | ||||||
|
|
||||||
| #[derive(Clone)] | ||||||
| pub struct ClientBuilder { | ||||||
| base_path: Option<String>, | ||||||
| username: Option<String>, | ||||||
| homeserver_url: Option<String>, | ||||||
| inner: MatrixClientBuilder, | ||||||
| } | ||||||
|
|
||||||
| impl ClientBuilder { | ||||||
| pub fn new() -> Self { | ||||||
| Self { | ||||||
| base_path: None, | ||||||
| username: None, | ||||||
| homeserver_url: None, | ||||||
| inner: MatrixClient::builder().user_agent("rust-sdk-ios"), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
There was a problem hiding this comment. Docs should probably say that base path must be set. Which let's me wonder if we shouldn't have that as a parameters on There was a problem hiding this comment. I think Andy actually wanted to have it as a However, this point only applies to Kotlin since UniFFI's Swift codegen requires callers to re-state argument names (as in There was a problem hiding this comment. Yea I generally favor API that makes invalid states impossible to represent, which would require passing them to |
||||||
| pub fn base_path(self: Arc<Self>, path: String) -> Arc<Self> { | ||||||
| let mut builder = unwrap_or_clone_arc(self); | ||||||
| builder.base_path = Some(path); | ||||||
| Arc::new(builder) | ||||||
| } | ||||||
|
|
||||||
| pub fn username(self: Arc<Self>, username: String) -> Arc<Self> { | ||||||
| let mut builder = unwrap_or_clone_arc(self); | ||||||
| builder.username = Some(username); | ||||||
|
There was a problem hiding this comment. Why hold an inner builder but still have all parameters twice? Why not just
Suggested change
For the homeserver, too... There was a problem hiding this comment. We can't currently read those fields from the inner builder, but they're needed in |
||||||
| Arc::new(builder) | ||||||
| } | ||||||
|
|
||||||
| pub fn homeserver_url(self: Arc<Self>, url: String) -> Arc<Self> { | ||||||
| let mut builder = unwrap_or_clone_arc(self); | ||||||
| builder.homeserver_url = Some(url); | ||||||
| Arc::new(builder) | ||||||
| } | ||||||
|
|
||||||
| pub fn build(self: Arc<Self>) -> anyhow::Result<Arc<Client>> { | ||||||
| let builder = unwrap_or_clone_arc(self); | ||||||
|
|
||||||
| let base_path = builder.base_path.context("Base path was not set")?; | ||||||
| let username = builder | ||||||
|
There was a problem hiding this comment. This would fail for guest clients... but as this also drops support fir them, I guess that's fine... |
||||||
| .username | ||||||
| .context("Username to determine homeserver and home path was not set")?; | ||||||
|
|
||||||
| // Determine store path | ||||||
| let data_path = PathBuf::from(base_path).join(sanitize(&username)); | ||||||
| fs::create_dir_all(&data_path)?; | ||||||
| let store_config = make_store_config(&data_path, None)?; | ||||||
|
|
||||||
| let mut inner_builder = builder.inner.store_config(store_config); | ||||||
|
|
||||||
| // Determine server either from explicitly set homeserver or from userId | ||||||
| if let Some(homeserver_url) = builder.homeserver_url { | ||||||
| inner_builder = inner_builder.homeserver_url(homeserver_url); | ||||||
| } else { | ||||||
| let user = UserId::parse(username)?; | ||||||
| inner_builder = inner_builder.server_name(user.server_name()); | ||||||
| } | ||||||
|
|
||||||
| RUNTIME.block_on(async move { | ||||||
| let client = inner_builder.build().await?; | ||||||
| let c = Client::new(client, ClientState::default()); | ||||||
| Ok(Arc::new(c)) | ||||||
| }) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl Default for ClientBuilder { | ||||||
| fn default() -> Self { | ||||||
| Self::new() | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+79
to
+83
There was a problem hiding this comment. What is this There was a problem hiding this comment. Ah, that's pretty useless in this context but whatever. |
||||||
|
|
||||||
| fn unwrap_or_clone_arc<T: Clone>(arc: Arc<T>) -> T { | ||||||
| Arc::try_unwrap(arc).unwrap_or_else(|x| (*x).clone()) | ||||||
| } | ||||||
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 use of the new ClientBuilder API highlights that username now has to be passed twice, which is not ideal. The client builder needs username to determine file store path, and potentially homeserver, if not set explicitely. The client needs username to construct user to log in with.
Potentially the client could retrieve the username directly from the builder, but not sure if api like
client.login(password)is very good.