From 0cc9ed3ae94a15317ff153edeed7b6e7f6331792 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Commaille?= Date: Fri, 25 Apr 2025 09:50:46 +0200 Subject: [PATCH] user: Refactor and clean up --- src/components/user_page.rs | 4 +- src/session/model/remote/user.rs | 52 +---- src/session/model/user.rs | 216 +++++++++--------- .../view/content/room_details/member_row.ui | 2 +- .../create_direct_chat_dialog/user_list.rs | 2 +- 5 files changed, 123 insertions(+), 153 deletions(-) diff --git a/src/components/user_page.rs b/src/components/user_page.rs index c0ff6aa2..5fa6aedd 100644 --- a/src/components/user_page.rs +++ b/src/components/user_page.rs @@ -133,7 +133,7 @@ mod imp { .build(); let bindings = vec![title_binding, avatar_binding]; - let verified_handler = user.connect_verified_notify(clone!( + let verified_handler = user.connect_is_verified_notify(clone!( #[weak(rename_to = imp)] self, move |_| { @@ -671,7 +671,7 @@ mod imp { return; }; - if user.verified() { + if user.is_verified() { self.verified_row.set_title(&gettext("Identity verified")); self.verified_stack.set_visible_child_name("icon"); self.verify_button.set_sensitive(false); diff --git a/src/session/model/remote/user.rs b/src/session/model/remote/user.rs index a908cee5..d815b5ea 100644 --- a/src/session/model/remote/user.rs +++ b/src/session/model/remote/user.rs @@ -2,14 +2,13 @@ use std::time::{Duration, Instant}; use gtk::{glib, glib::clone, prelude::*, subclass::prelude::*}; use matrix_sdk::ruma::OwnedUserId; -use tracing::error; use crate::{ components::PillSource, prelude::*, session::model::{Session, User}, - spawn, spawn_tokio, - utils::{AbortableHandle, LoadingState}, + spawn, + utils::LoadingState, }; /// The time after which the profile of a user is assumed to be stale. @@ -30,7 +29,6 @@ mod imp { loading_state: Cell, // The time of the last request. last_request_time: Cell>, - request_abort_handle: AbortableHandle, } #[glib::object_subclass] @@ -77,39 +75,6 @@ mod imp { pub(super) fn update_last_request_time(&self) { self.last_request_time.set(Some(Instant::now())); } - - /// Request the profile of this user. - pub(super) async fn load_profile(&self) { - let obj = self.obj(); - - self.set_loading_state(LoadingState::Loading); - - let user_id = obj.user_id(); - - let client = obj.session().client(); - let user_id_clone = user_id.clone(); - let handle = spawn_tokio!(async move { - client.account().fetch_user_profile_of(&user_id_clone).await - }); - - let Some(result) = self.request_abort_handle.await_task(handle).await else { - // The task was aborted, which means that the object was dropped. - return; - }; - - let profile = match result { - Ok(profile) => profile, - Err(error) => { - error!("Could not load profile for user `{user_id}`: {error}"); - self.set_loading_state(LoadingState::Error); - return; - } - }; - - obj.set_name(profile.displayname); - obj.set_avatar_url(profile.avatar_url); - self.set_loading_state(LoadingState::Ready); - } } } @@ -145,10 +110,17 @@ impl RemoteUser { imp.update_last_request_time(); spawn!(clone!( - #[weak] - imp, + #[weak(rename_to = obj)] + self, async move { - imp.load_profile().await; + let imp = obj.imp(); + imp.set_loading_state(LoadingState::Loading); + + let loading_state = match obj.load_profile().await { + Ok(()) => LoadingState::Ready, + Err(()) => LoadingState::Error, + }; + imp.set_loading_state(loading_state); } )); } diff --git a/src/session/model/user.rs b/src/session/model/user.rs index cf2ba091..d5d24f61 100644 --- a/src/session/model/user.rs +++ b/src/session/model/user.rs @@ -1,11 +1,6 @@ use gtk::{glib, glib::clone, prelude::*, subclass::prelude::*}; use matrix_sdk::encryption::identities::UserIdentity; -use ruma::{ - api::client::room::create_room, - assign, - events::{room::encryption::RoomEncryptionEventContent, InitialStateEvent}, - MatrixToUri, OwnedMxcUri, OwnedUserId, -}; +use ruma::{MatrixToUri, OwnedMxcUri, OwnedUserId}; use tracing::{debug, error}; use super::{IdentityVerification, Room, Session}; @@ -38,22 +33,22 @@ mod imp { #[properties(wrapper_type = super::User)] pub struct User { /// The ID of this user. - pub user_id: OnceCell, + user_id: OnceCell, /// The ID of this user, as a string. #[property(get = Self::user_id_string)] - pub user_id_string: PhantomData, + user_id_string: PhantomData, /// The current session. #[property(get, construct_only)] - pub session: OnceCell, + session: OnceCell, /// Whether this user is the same as the session's user. #[property(get)] - pub is_own_user: Cell, + is_own_user: Cell, /// Whether this user has been verified. #[property(get)] - pub verified: Cell, + is_verified: Cell, /// Whether this user is currently ignored. #[property(get)] - pub is_ignored: Cell, + is_ignored: Cell, ignored_handler: RefCell>, } @@ -90,13 +85,23 @@ mod imp { } impl User { + /// The ID of this user. + pub(super) fn user_id(&self) -> &OwnedUserId { + self.user_id.get().expect("user ID should be initialized") + } + /// The ID of this user, as a string. fn user_id_string(&self) -> String { - self.user_id.get().unwrap().to_string() + self.user_id().to_string() + } + + /// The current session. + fn session(&self) -> &Session { + self.session.get().expect("session should be initialized") } /// Set the ID of this user. - pub fn set_user_id(&self, user_id: OwnedUserId) { + pub(crate) fn set_user_id(&self, user_id: OwnedUserId) { let user_id = self.user_id.get_or_init(|| user_id); let obj = self.obj(); @@ -105,7 +110,7 @@ mod imp { .sync_create() .build(); - let session = self.session.get().expect("session is initialized"); + let session = self.session(); self.is_own_user.set(session.user_id() == user_id); let ignored_users = session.ignored_users(); @@ -125,7 +130,71 @@ mod imp { self.is_ignored.set(ignored_users.contains(user_id)); self.ignored_handler.replace(Some(ignored_handler)); - obj.init_is_verified(); + spawn!(clone!( + #[weak(rename_to = imp)] + self, + async move { + imp.init_is_verified().await; + } + )); + } + + /// Get the local cryptographic identity (aka cross-signing identity) of + /// this user. + /// + /// Locally, we should always have the crypto identity of our own user + /// and of users with whom we share an encrypted room. + pub(super) async fn local_crypto_identity(&self) -> Option { + let encryption = self.session().client().encryption(); + let user_id = self.user_id().clone(); + let handle = spawn_tokio!(async move { encryption.get_user_identity(&user_id).await }); + + match handle.await.expect("task was not aborted") { + Ok(identity) => identity, + Err(error) => { + error!("Could not get local crypto identity: {error}"); + None + } + } + } + + /// Load whether this user is verified. + async fn init_is_verified(&self) { + // If a user is verified, we should have their crypto identity locally. + let is_verified = self + .local_crypto_identity() + .await + .is_some_and(|i| i.is_verified()); + + if self.is_verified.get() == is_verified { + return; + } + + self.is_verified.set(is_verified); + self.obj().notify_is_verified(); + } + + /// Create an encrypted direct chat with this user. + pub(super) async fn create_direct_chat(&self) -> Result { + let user_id = self.user_id().clone(); + let client = self.session().client(); + let handle = spawn_tokio!(async move { client.create_dm(&user_id).await }); + + match handle.await.expect("task was not aborted") { + Ok(matrix_room) => { + let room = self + .session() + .room_list() + .get_wait(matrix_room.room_id()) + .await + .expect("The newly created room was not found"); + Ok(room) + } + Err(error) => { + error!("Could not create direct chat: {error}"); + Err(error) + } + } } } } @@ -146,35 +215,13 @@ impl User { obj } - /// Get the local cryptographic identity (aka cross-signing identity) of - /// this user. - /// - /// Locally, we should always have the crypto identity of our own user and - /// of users with whom we share an encrypted room. - /// - /// To get the crypto identity of a user with whom we do not share an - /// encrypted room, use [`Self::ensure_crypto_identity()`]. - pub async fn local_crypto_identity(&self) -> Option { - let encryption = self.session().client().encryption(); - let user_id = self.user_id().clone(); - let handle = spawn_tokio!(async move { encryption.get_user_identity(&user_id).await }); - - match handle.await.unwrap() { - Ok(identity) => identity, - Err(error) => { - error!("Could not get local crypto identity: {error}"); - None - } - } - } - /// Get the cryptographic identity (aka cross-signing identity) of this /// user. /// /// First, we try to get the local crypto identity if we are sure that it is /// up-to-date. If we do not have the crypto identity locally, we request it /// from the homeserver. - pub async fn ensure_crypto_identity(&self) -> Option { + pub(crate) async fn ensure_crypto_identity(&self) -> Option { let session = self.session(); let encryption = session.client().encryption(); let user_id = self.user_id(); @@ -190,7 +237,7 @@ impl User { let encryption_clone = encryption.clone(); let handle = spawn_tokio!(async move { encryption_clone.tracked_users().await }); - match handle.await.unwrap() { + match handle.await.expect("task was not aborted") { Ok(tracked_users) => tracked_users.contains(user_id), Err(error) => { error!("Could not get tracked users: {error}"); @@ -202,7 +249,7 @@ impl User { // Try to get the local crypto identity. if should_have_local { - if let Some(identity) = self.local_crypto_identity().await { + if let Some(identity) = self.imp().local_crypto_identity().await { return Some(identity); } } @@ -212,7 +259,7 @@ impl User { let handle = spawn_tokio!(async move { encryption.request_user_identity(&user_id_clone).await }); - match handle.await.unwrap() { + match handle.await.expect("task was not aborted") { Ok(identity) => identity, Err(error) => { error!("Could not request remote crypto identity: {error}"); @@ -222,79 +269,25 @@ impl User { } /// Start a verification of the identity of this user. - pub async fn verify_identity(&self) -> Result { + pub(crate) async fn verify_identity(&self) -> Result { self.session() .verification_list() .create(Some(self.clone())) .await } - /// Load whether this user is verified. - fn init_is_verified(&self) { - spawn!(clone!( - #[weak(rename_to = obj)] - self, - async move { - // If a user is verified, we should have their crypto identity locally. - let verified = obj - .local_crypto_identity() - .await - .is_some_and(|i| i.is_verified()); - - if verified == obj.verified() { - return; - } - - obj.imp().verified.set(verified); - obj.notify_verified(); - } - )); - } - /// The existing direct chat with this user, if any. /// /// A direct chat is a joined room marked as direct, with only our own user /// and the other user in it. - pub fn direct_chat(&self) -> Option { + pub(crate) fn direct_chat(&self) -> Option { self.session().room_list().direct_chat(self.user_id()) } - /// Create an encrypted direct chat with this user. - async fn create_direct_chat(&self) -> Result { - let request = assign!(create_room::v3::Request::new(), - { - is_direct: true, - invite: vec![self.user_id().clone()], - preset: Some(create_room::v3::RoomPreset::TrustedPrivateChat), - initial_state: vec![ - InitialStateEvent::new(RoomEncryptionEventContent::with_recommended_defaults()).to_raw_any(), - ], - }); - - let client = self.session().client(); - let handle = spawn_tokio!(async move { client.create_room(request).await }); - - match handle.await.unwrap() { - Ok(matrix_room) => { - let room = self - .session() - .room_list() - .get_wait(matrix_room.room_id()) - .await - .expect("The newly created room was not found"); - Ok(room) - } - Err(error) => { - error!("Could not create direct chat: {error}"); - Err(error) - } - } - } - /// Get or create a direct chat with this user. /// /// If there is no existing direct chat, a new one is created. - pub async fn get_or_create_direct_chat(&self) -> Result { + pub(crate) async fn get_or_create_direct_chat(&self) -> Result { let user_id = self.user_id(); if let Some(room) = self.direct_chat() { @@ -303,16 +296,16 @@ impl User { } debug!("Creating direct chat with {user_id}…"); - self.create_direct_chat().await.map_err(|_| ()) + self.imp().create_direct_chat().await.map_err(|_| ()) } /// Ignore this user. - pub async fn ignore(&self) -> Result<(), ()> { + pub(crate) async fn ignore(&self) -> Result<(), ()> { self.session().ignored_users().add(self.user_id()).await } /// Stop ignoring this user. - pub async fn stop_ignoring(&self) -> Result<(), ()> { + pub(crate) async fn stop_ignoring(&self) -> Result<(), ()> { self.session().ignored_users().remove(self.user_id()).await } } @@ -325,7 +318,7 @@ pub trait UserExt: IsA { /// The ID of this user. fn user_id(&self) -> &OwnedUserId { - self.upcast_ref().imp().user_id.get().unwrap() + self.upcast_ref().imp().user_id() } /// Whether this user is the same as the session's user. @@ -351,7 +344,7 @@ pub trait UserExt: IsA { self.upcast_ref() .avatar_data() .image() - .unwrap() + .expect("avatar data should have an image") // User avatars never have information. .set_uri_and_info(uri, None); } @@ -364,7 +357,7 @@ pub trait UserExt: IsA { /// Load the user profile from the homeserver. /// /// This overwrites the already loaded display name and avatar. - async fn load_profile(&self) { + async fn load_profile(&self) -> Result<(), ()> { let user_id = self.user_id(); let client = self.session().client(); @@ -374,15 +367,17 @@ pub trait UserExt: IsA { async move { client.account().fetch_user_profile_of(&user_id_clone).await } ); - match handle.await.unwrap() { + match handle.await.expect("task was not aborted") { Ok(response) => { let user = self.upcast_ref::(); user.set_name(response.displayname); user.set_avatar_url(response.avatar_url); + Ok(()) } Err(error) => { - error!("Could not load user profile for {user_id}: {error}"); + error!("Could not load user profile for `{user_id}`: {error}"); + Err(()) } } } @@ -394,8 +389,11 @@ pub trait UserExt: IsA { /// Connect to the signal emitted when the `is-ignored` property changes. fn connect_is_ignored_notify(&self, f: F) -> glib::SignalHandlerId { - self.upcast_ref() - .connect_is_ignored_notify(move |user| f(user.downcast_ref().unwrap())) + self.upcast_ref().connect_is_ignored_notify(move |user| { + f(user + .downcast_ref() + .expect("downcasting to own type should succeed")); + }) } } diff --git a/src/session/view/content/room_details/member_row.ui b/src/session/view/content/room_details/member_row.ui index 602eb800..a5ded6c0 100644 --- a/src/session/view/content/room_details/member_row.ui +++ b/src/session/view/content/room_details/member_row.ui @@ -44,7 +44,7 @@ verified-symbolic Identity Verified - + ContentMemberRow diff --git a/src/session/view/create_direct_chat_dialog/user_list.rs b/src/session/view/create_direct_chat_dialog/user_list.rs index a23cb5b3..913ab950 100644 --- a/src/session/view/create_direct_chat_dialog/user_list.rs +++ b/src/session/view/create_direct_chat_dialog/user_list.rs @@ -170,7 +170,7 @@ mod imp { #[weak] user, async move { - user.load_profile().await; + let _ = user.load_profile().await; } ));