From 5ef9a7b924be3dad0812d26c70927da87c740329 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 30 Jun 2020 14:42:22 +0200 Subject: [PATCH 01/28] tests: Rename get_room_id to test_room_id. To make it more obvious it's a special room ID value used in tests. --- matrix_sdk_base/src/models/room_member.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 9834b716..bdfd5721 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -233,7 +233,9 @@ mod test { client } - fn get_room_id() -> RoomId { + // TODO: Move this to EventBuilder since it's a magic room ID used in EventBuilder's example + // events. + fn test_room_id() -> RoomId { RoomId::try_from("!SVkFJHzfwvuaIEawgC:localhost").unwrap() } @@ -241,7 +243,7 @@ mod test { async fn room_member_events() { let client = get_client().await; - let room_id = get_room_id(); + let room_id = test_room_id(); let mut response = EventBuilder::default() .add_room_event(EventsJson::Member, RoomEvent::RoomMember) @@ -264,7 +266,7 @@ mod test { async fn member_presence_events() { let client = get_client().await; - let room_id = get_room_id(); + let room_id = test_room_id(); let mut response = EventBuilder::default() .add_room_event(EventsJson::Member, RoomEvent::RoomMember) From 9bd8699e180046270a7b4bbdc9e9765fb9151945 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 30 Jun 2020 15:01:03 +0200 Subject: [PATCH 02/28] Get rid of match on membership change in RoomMember::update_profile. The calling method already did this when it determined that update_profile should be called so we don't need to repeat it. --- matrix_sdk_base/src/models/room_member.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index bdfd5721..b174bb5d 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -117,18 +117,9 @@ impl RoomMember { /// Handle profile updates. pub(crate) fn update_profile(&mut self, event: &MemberEvent) -> bool { - use MembershipChange::*; - - match event.membership_change() { - ProfileChanged => { - self.display_name = event.content.displayname.clone(); - self.avatar_url = event.content.avatar_url.clone(); - true - } - - // We're only interested in profile changes here. - _ => false, - } + self.display_name = event.content.displayname.clone(); + self.avatar_url = event.content.avatar_url.clone(); + true } pub fn update_power(&mut self, event: &PowerLevelsEvent, max_power: Int) -> bool { From 4561b94f336a039a1d1e319a74b53a28886f5850 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 30 Jun 2020 15:33:38 +0200 Subject: [PATCH 03/28] Remove outdated TODO. --- matrix_sdk_base/src/models/room.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index e7c3f175..ef8d3a1a 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -586,8 +586,6 @@ impl Room { pub fn handle_membership(&mut self, event: &MemberEvent) -> bool { use MembershipChange::*; - // TODO: This would not be handled correctly as all the MemberEvents have the `prev_content` - // inside of `unsigned` field. match event.membership_change() { Invited | Joined => self.add_member(event), Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { From c57f0763759800753a21e852941fa398a01f3860 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 30 Jun 2020 16:16:26 +0200 Subject: [PATCH 04/28] Remove unused import. --- matrix_sdk_base/src/models/room_member.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index b174bb5d..3800fda7 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -18,7 +18,7 @@ use std::convert::TryFrom; use crate::events::collections::all::Event; use crate::events::presence::{PresenceEvent, PresenceEventContent, PresenceState}; use crate::events::room::{ - member::{MemberEvent, MembershipChange}, + member::MemberEvent, power_levels::PowerLevelsEvent, }; use crate::identifiers::UserId; From 84fc66261438a129f197130d59f2012fbfa65d95 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 30 Jun 2020 17:09:10 +0200 Subject: [PATCH 05/28] Document and improve EventBuilder. EventBuilder now clears itself between `build_sync_response` calls so that each subsequent call will return an empty response if nothing was added. This allows reuse of a single EventBuilder instance which is important for correct sync token rotation. --- matrix_sdk_test/src/lib.rs | 53 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/matrix_sdk_test/src/lib.rs b/matrix_sdk_test/src/lib.rs index 80bba176..c07c1d33 100644 --- a/matrix_sdk_test/src/lib.rs +++ b/matrix_sdk_test/src/lib.rs @@ -47,7 +47,41 @@ pub enum EventsJson { Typing, } -/// Easily create events to stream into either a Client or a `Room` for testing. +/// The `EventBuilder` struct can be used to easily generate valid sync responses for testing. +/// These can be then fed into either `Client` or `Room`. +/// +/// It supports generated a number of canned events, such as a member entering a room, his power +/// level and display name changing and similar. It also supports insertion of custom events in the +/// form of `EventsJson` values. +/// +/// **Important** You *must* use the *same* builder when sending multiple sync responses to +/// a single client. Otherwise, the subsequent responses will be *ignored* by the client because +/// the `next_batch` sync token will not be rotated properly. +/// +/// # Example usage +/// +/// ```rust +/// use matrix_sdk_test::{EventBuilder, EventsJson}; +/// use matrix_sdk_common::events::collections::all::RoomEvent; +/// +/// let mut builder = EventBuilder::new(); +/// +/// // response1 now contains events that add an example member to the room and change their power +/// // level +/// let response1 = builder +/// .add_room_event(EventsJson::Member, RoomEvent::RoomMember) +/// .add_room_event(EventsJson::PowerLevels, RoomEvent::RoomPowerLevels) +/// .build_sync_response(); +/// +/// // response2 is now empty (nothing changed) +/// let response2 = builder.build_sync_response(); +/// +/// // response3 contains a display name change for member example +/// let response3 = builder +/// .add_room_event(EventsJson::MemberNameChange, RoomEvent::RoomMember) +/// .build_sync_response(); +/// ``` + #[derive(Default)] pub struct EventBuilder { /// The events that determine the state of a `Room`. @@ -226,7 +260,8 @@ impl EventBuilder { self } - /// Consumes `ResponseBuilder` and returns `SyncResponse`. + /// Builds a `SyncResponse` containing the events we queued so far. The next response returned + /// by `build_sync_response` will then be empty if no further events were queued. pub fn build_sync_response(&mut self) -> SyncResponse { let main_room_id = RoomId::try_from("!SVkFJHzfwvuaIEawgC:localhost").unwrap(); @@ -338,12 +373,26 @@ impl EventBuilder { let response = Response::builder() .body(serde_json::to_vec(&body).unwrap()) .unwrap(); + + // Clear state so that the next sync response will be empty if nothing was added. + self.clear(); + SyncResponse::try_from(response).unwrap() } fn generate_sync_token(&self) -> String { format!("t392-516_47314_0_7_1_1_1_11444_{}", self.batch_counter) } + + pub fn clear(&mut self) { + self.account_data.clear(); + self.ephemeral.clear(); + self.invited_room_events.clear(); + self.joined_room_events.clear(); + self.left_room_events.clear(); + self.presence_events.clear(); + self.state_events.clear(); + } } /// Embedded sync reponse files From f447c55fcbcccf7a4e144e948206b27581c8e767 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 30 Jun 2020 19:02:42 +0200 Subject: [PATCH 06/28] Move prev_content in test data to top level for now. Until Ruma fixes it upstream, see hoist_room_event_prev_content for more information. --- matrix_sdk_test/src/test_json/events.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/matrix_sdk_test/src/test_json/events.rs b/matrix_sdk_test/src/test_json/events.rs index a4ba0ced..c3bcea49 100644 --- a/matrix_sdk_test/src/test_json/events.rs +++ b/matrix_sdk_test/src/test_json/events.rs @@ -208,6 +208,7 @@ lazy_static! { }); } +// TODO: Move `prev_content` into `unsigned` once ruma supports it lazy_static! { pub static ref MEMBER: JsonValue = json!({ "content": { @@ -221,14 +222,14 @@ lazy_static! { "sender": "@example:localhost", "state_key": "@example:localhost", "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "invite" + }, "unsigned": { "age": 297036, - "replaces_state": "$151800111315tsynI:localhost", - "prev_content": { - "avatar_url": null, - "displayname": "example", - "membership": "invite" - } + "replaces_state": "$151800111315tsynI:localhost" } }); } @@ -552,6 +553,7 @@ lazy_static! { }); } +// TODO: Move `prev_content` into `unsigned` once ruma supports it lazy_static! { pub static ref TOPIC: JsonValue = json!({ "content": { @@ -562,11 +564,11 @@ lazy_static! { "sender": "@example:localhost", "state_key": "", "type": "m.room.topic", + "prev_content": { + "topic": "test" + }, "unsigned": { "age": 1392989, - "prev_content": { - "topic": "test" - }, "prev_sender": "@example:localhost", "replaces_state": "$151957069225EVYKm:localhost" } From 2a0c6c6474a130c55ed9d6cbbcde26ef20e42f6b Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 14:50:42 +0200 Subject: [PATCH 07/28] Add test and example event to ensure display name changes work correctly. --- matrix_sdk_base/src/models/room_member.rs | 44 +++++++++++++++++++++++ matrix_sdk_test/src/lib.rs | 2 ++ matrix_sdk_test/src/test_json/events.rs | 26 ++++++++++++++ matrix_sdk_test/src/test_json/mod.rs | 4 +-- 4 files changed, 74 insertions(+), 2 deletions(-) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 3800fda7..0db41cb3 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -253,6 +253,50 @@ mod test { assert_eq!(member.power_level, Int::new(100)); } + #[async_test] + async fn room_member_display_name_change() { + let client = get_client().await; + let room_id = test_room_id(); + + let mut builder = EventBuilder::default(); + let mut initial_response = builder + .add_room_event(EventsJson::Member, RoomEvent::RoomMember) + .build_sync_response(); + let mut name_change_response = builder + .add_room_event(EventsJson::MemberNameChange, RoomEvent::RoomMember) + .build_sync_response(); + + client.receive_sync_response(&mut initial_response).await.unwrap(); + + let room = client.get_joined_room(&room_id).await.unwrap(); + + // Initially, the display name is "example". + { + let room = room.read().await; + + let member = room + .joined_members + .get(&UserId::try_from("@example:localhost").unwrap()) + .unwrap(); + + assert_eq!(member.display_name.as_ref().unwrap(), "example"); + } + + client.receive_sync_response(&mut name_change_response).await.unwrap(); + + // Afterwards, the display name is "changed". + { + let room = room.read().await; + + let member = room + .joined_members + .get(&UserId::try_from("@example:localhost").unwrap()) + .unwrap(); + + assert_eq!(member.display_name.as_ref().unwrap(), "changed"); + } + } + #[async_test] async fn member_presence_events() { let client = get_client().await; diff --git a/matrix_sdk_test/src/lib.rs b/matrix_sdk_test/src/lib.rs index c07c1d33..2cfa88c7 100644 --- a/matrix_sdk_test/src/lib.rs +++ b/matrix_sdk_test/src/lib.rs @@ -31,6 +31,7 @@ pub enum EventsJson { HistoryVisibility, JoinRules, Member, + MemberNameChange, MessageEmote, MessageNotice, MessageText, @@ -154,6 +155,7 @@ impl EventBuilder { ) -> &mut Self { let val: &JsonValue = match json { EventsJson::Member => &test_json::MEMBER, + EventsJson::MemberNameChange => &test_json::MEMBER_NAME_CHANGE, EventsJson::PowerLevels => &test_json::POWER_LEVELS, _ => panic!("unknown room event json {:?}", json), }; diff --git a/matrix_sdk_test/src/test_json/events.rs b/matrix_sdk_test/src/test_json/events.rs index c3bcea49..da3bc5a4 100644 --- a/matrix_sdk_test/src/test_json/events.rs +++ b/matrix_sdk_test/src/test_json/events.rs @@ -234,6 +234,32 @@ lazy_static! { }); } +// TODO: Move `prev_content` into `unsigned` once ruma supports it +lazy_static! { + pub static ref MEMBER_NAME_CHANGE: JsonValue = json!({ + "content": { + "avatar_url": null, + "displayname": "changed", + "membership": "join" + }, + "event_id": "$151800234427abgho:localhost", + "membership": "join", + "origin_server_ts": 151800152, + "sender": "@example:localhost", + "state_key": "@example:localhost", + "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "join" + }, + "unsigned": { + "age": 297032, + "replaces_state": "$151800140517rfvjc:localhost" + } + }); +} + lazy_static! { pub static ref MESSAGE_EDIT: JsonValue = json!({ "content": { diff --git a/matrix_sdk_test/src/test_json/mod.rs b/matrix_sdk_test/src/test_json/mod.rs index 406f2023..d17f13bd 100644 --- a/matrix_sdk_test/src/test_json/mod.rs +++ b/matrix_sdk_test/src/test_json/mod.rs @@ -9,7 +9,7 @@ pub mod sync; pub use events::{ ALIAS, ALIASES, EVENT_ID, KEYS_QUERY, KEYS_UPLOAD, LOGIN, LOGIN_RESPONSE_ERR, LOGOUT, MEMBER, - MESSAGE_EDIT, MESSAGE_TEXT, NAME, POWER_LEVELS, PRESENCE, PUBLIC_ROOMS, REACTION, - REGISTRATION_RESPONSE_ERR, ROOM_ID, ROOM_MESSAGES, TYPING, + MEMBER_NAME_CHANGE, MESSAGE_EDIT, MESSAGE_TEXT, NAME, POWER_LEVELS, PRESENCE, PUBLIC_ROOMS, + REACTION, REGISTRATION_RESPONSE_ERR, ROOM_ID, ROOM_MESSAGES, TYPING, }; pub use sync::{DEFAULT_SYNC_SUMMARY, INVITE_SYNC, LEAVE_SYNC, LEAVE_SYNC_EVENT, MORE_SYNC, SYNC}; From ff5f638b60292cd907390eb5e3f163cda283a1a7 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 14:53:58 +0200 Subject: [PATCH 08/28] Remove member from invited_members when he joins. --- matrix_sdk_base/src/models/room.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index ef8d3a1a..1602c3d5 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -363,19 +363,22 @@ impl Room { fn add_member(&mut self, event: &MemberEvent) -> bool { let new_member = RoomMember::new(event); - if self.joined_members.contains_key(&new_member.user_id) - || self.invited_members.contains_key(&new_member.user_id) - { - return false; - } - match event.membership_change() { - MembershipChange::Joined => self - .joined_members - .insert(new_member.user_id.clone(), new_member.clone()), + MembershipChange::Joined => { + // Since the member is now joined, he shouldn't be tracked as an invited member any + // longer. + if self.invited_members.contains_key(&new_member.user_id) { + self.invited_members.remove(&new_member.user_id); + } + + self.joined_members + .insert(new_member.user_id.clone(), new_member.clone()) + } + MembershipChange::Invited => self .invited_members .insert(new_member.user_id.clone(), new_member.clone()), + _ => { panic!("Room::add_member called on an event that is neither a join nor an invite.") } From 9af48920f66dc545075d734b9e7e4fc13b256039 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 10:39:45 +0200 Subject: [PATCH 09/28] Add some TODOs and FIXMEs. --- matrix_sdk_base/src/models/room_member.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 0db41cb3..3b7639c1 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -25,6 +25,7 @@ use crate::identifiers::UserId; use crate::js_int::{Int, UInt}; use serde::{Deserialize, Serialize}; + // Notes: if Alice invites Bob into a room we will get an event with the sender as Alice and the state key as Bob. #[derive(Debug, Serialize, Deserialize, Clone)] @@ -55,6 +56,14 @@ pub struct RoomMember { pub power_level_norm: Option, /// The human readable name of this room member. pub name: String, + // FIXME: The docstring below is currently a lie since we only store the initial event that + // creates the member (the one we pass to RoomMember::new). + // + // The intent of this field is to keep the last (or last few?) state events related to the room + // member cached so we can quickly go back to the previous one in case some of them get + // redacted. Keeping all state for each room member is probably too much. + // + // Needs design. /// The events that created the state of this room member. #[serde(deserialize_with = "super::event_deser::deserialize_events")] pub events: Vec, @@ -116,6 +125,7 @@ impl RoomMember { } /// Handle profile updates. + // TODO: NEXT: Add disambiguation handling here pub(crate) fn update_profile(&mut self, event: &MemberEvent) -> bool { self.display_name = event.content.displayname.clone(); self.avatar_url = event.content.avatar_url.clone(); From 32bdcede0caf1813a25e2f2ba6f086dc156b07bb Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 12:06:59 +0200 Subject: [PATCH 10/28] Small refactoring to simplify member_disambiguations. --- matrix_sdk_base/src/models/room.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 1602c3d5..b2547f76 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -427,7 +427,7 @@ impl Room { /// /// The `inclusive` parameter controls whether the passed member should be included in the /// list or not. - fn shares_displayname_with(&self, member: &RoomMember, inclusive: bool) -> Vec { + fn shares_displayname_with(&self, member: &RoomMember, inclusive: bool) -> Vec { let members = self .invited_members .iter() @@ -449,7 +449,7 @@ impl Room { }) // If not an inclusive search, do not consider the member for which we are disambiguating. .filter(|(id, _)| inclusive || **id != member.user_id) - .map(|(id, _)| id) + .map(|(_, member)| member) .cloned() .collect() } @@ -467,16 +467,10 @@ impl Room { inclusive: bool, ) -> HashMap { let users_with_same_name = self.shares_displayname_with(member, inclusive); - let disambiguate_with = |members: Vec, f: fn(&RoomMember) -> String| { + let disambiguate_with = |members: Vec, f: fn(&RoomMember) -> String| { members .into_iter() - .filter_map(|id| { - self.joined_members - .get(&id) - .or_else(|| self.invited_members.get(&id)) - .map(f) - .map(|m| (id, m)) - }) + .map(|ref m| (m.user_id.clone(), f(m))) .collect::>() }; From c2ec69cf44eabc236ede959941fbdf8ef3c6a66a Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 13:48:31 +0200 Subject: [PATCH 11/28] Style fixes (comment grammar and correctness, whitespace). --- matrix_sdk_base/src/models/room_member.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 3b7639c1..71e59195 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -129,6 +129,7 @@ impl RoomMember { pub(crate) fn update_profile(&mut self, event: &MemberEvent) -> bool { self.display_name = event.content.displayname.clone(); self.avatar_url = event.content.avatar_url.clone(); + true } @@ -149,13 +150,13 @@ impl RoomMember { changed } - /// If the current `PresenceEvent` updated the state of this `User`. + /// If the current `PresenceEvent` updated the state of this `RoomMember`. /// - /// Returns true if the specific users presence has changed, false otherwise. + /// Returns true if the member's presence has changed, false otherwise. /// /// # Arguments /// - /// * `presence` - The presence event for a this room member. + /// * `presence` - The presence event for this room member. pub fn did_update_presence(&self, presence: &PresenceEvent) -> bool { let PresenceEvent { content: @@ -177,13 +178,13 @@ impl RoomMember { && self.currently_active == *currently_active } - /// Updates the `User`s presence. + /// Updates the `RoomMember`'s presence. /// /// This should only be used if `did_update_presence` was true. /// /// # Arguments /// - /// * `presence` - The presence event for a this room member. + /// * `presence` - The presence event for this room member. pub fn update_presence(&mut self, presence_ev: &PresenceEvent) { let PresenceEvent { content: From 599c1ba98f4f36f939db7a35888b5ee0cdc39fb7 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 13:51:21 +0200 Subject: [PATCH 12/28] Add test to ensure member is only treated as joined or invited, not both. --- matrix_sdk_base/src/models/room.rs | 108 +++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index b2547f76..060ebb2a 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -831,6 +831,114 @@ mod test { assert!(room.deref().power_levels.is_some()) } + #[async_test] + async fn member_is_not_both_invited_and_joined() { + let client = get_client().await; + let room_id = get_room_id(); + let user_id1 = UserId::try_from("@example:localhost").unwrap(); + let user_id2 = UserId::try_from("@example2:localhost").unwrap(); + + let member2_invite_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example2", + "membership": "invite" + }, + "event_id": "$16345217l517tabbz:localhost", + "membership": "join", + "origin_server_ts": 1455123234, + "sender": format!("{}", user_id1), + "state_key": format!("{}", user_id2), + "type": "m.room.member", + "unsigned": { + "age": 1989321234, + "replaces_state": "$1622a2311315tkjoA:localhost" + } + }); + + let member2_join_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example2", + "membership": "join" + }, + "event_id": "$163409224327jkbba:localhost", + "membership": "join", + "origin_server_ts": 1455123238, + "sender": format!("{}", user_id2), + "state_key": format!("{}", user_id2), + "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example2", + "membership": "invite" + }, + "unsigned": { + "age": 1989321214, + "replaces_state": "$16345217l517tabbz:localhost" + } + }); + + let mut event_builder = EventBuilder::new(); + + let mut member1_join_sync_response = event_builder + .add_room_event(EventsJson::Member, RoomEvent::RoomMember) + .build_sync_response(); + + let mut member2_invite_sync_response = event_builder + .add_custom_joined_event(&room_id, member2_invite_event, RoomEvent::RoomMember) + .build_sync_response(); + + let mut member2_join_sync_response = event_builder + .add_custom_joined_event(&room_id, member2_join_event, RoomEvent::RoomMember) + .build_sync_response(); + + + // Test that `user` is either joined or invited to `room` but not both. + async fn invited_or_joined_but_not_both(client: &BaseClient, room: &RoomId, user: &UserId) { + let room = client.get_joined_room(&room).await.unwrap(); + let room = room.read().await; + + assert!( + room.invited_members.get(&user).is_none() + || room.joined_members.get(&user).is_none() + ); + assert!( + room.invited_members.get(&user).is_some() + || room.joined_members.get(&user).is_some() + ); + }; + + // First member joins. + client + .receive_sync_response(&mut member1_join_sync_response) + .await + .unwrap(); + + // The first member is not *both* invited and joined but it *is* one of those. + invited_or_joined_but_not_both(&client, &room_id, &user_id1).await; + + // First member invites second member. + client + .receive_sync_response(&mut member2_invite_sync_response) + .await + .unwrap(); + + // Neither member is *both* invited and joined, but they are both *at least one* of those. + invited_or_joined_but_not_both(&client, &room_id, &user_id1).await; + invited_or_joined_but_not_both(&client, &room_id, &user_id2).await; + + // Second member joins. + client + .receive_sync_response(&mut member2_join_sync_response) + .await + .unwrap(); + + // Repeat the previous test. + invited_or_joined_but_not_both(&client, &room_id, &user_id1).await; + invited_or_joined_but_not_both(&client, &room_id, &user_id2).await; + } + #[async_test] async fn test_member_display_name() { // Initialize From 6cacf836616969ce7016a97abbe012b7c0edb848 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 13:53:10 +0200 Subject: [PATCH 13/28] Add (failing) test for displayname disambiguation on profile updates. --- matrix_sdk_base/src/models/room.rs | 119 ++++++++++++++++++++++++++--- 1 file changed, 110 insertions(+), 9 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 060ebb2a..6bc060a4 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -972,6 +972,47 @@ mod test { } }); + let member1_invites_member2_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example", + "membership": "invite" + }, + "event_id": "$16345217l517tabbz:localhost", + "membership": "invite", + "origin_server_ts": 1455123238, + "sender": format!("{}", user_id1), + "state_key": format!("{}", user_id2), + "type": "m.room.member", + "unsigned": { + "age": 1989321238, + "replaces_state": "$1622a2311315tkjoA:localhost" + } + }); + + let member2_name_change_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "changed", + "membership": "join" + }, + "event_id": "$16345217l517tabbz:localhost", + "membership": "join", + "origin_server_ts": 1455123238, + "sender": format!("{}", user_id2), + "state_key": format!("{}", user_id2), + "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "join" + }, + "unsigned": { + "age": 1989321238, + "replaces_state": "$1622a2311315tkjoA:localhost" + } + }); + let member2_leave_event = serde_json::json!({ "content": { "avatar_url": null, @@ -1048,19 +1089,29 @@ mod test { .build_sync_response(); let mut member2_join_sync_response = event_builder - .add_custom_joined_event(&room_id, member2_join_event, RoomEvent::RoomMember) + .add_custom_joined_event(&room_id, member2_join_event.clone(), RoomEvent::RoomMember) .build_sync_response(); let mut member3_join_sync_response = event_builder .add_custom_joined_event(&room_id, member3_join_event, RoomEvent::RoomMember) .build_sync_response(); - let mut member2_leave_sync_response = event_builder + let mut member2_and_member3_leave_sync_response = event_builder .add_custom_joined_event(&room_id, member2_leave_event, RoomEvent::RoomMember) + .add_custom_joined_event(&room_id, member3_leave_event, RoomEvent::RoomMember) .build_sync_response(); - let mut member3_leave_sync_response = event_builder - .add_custom_joined_event(&room_id, member3_leave_event, RoomEvent::RoomMember) + let mut member2_rejoins_when_invited_sync_response = event_builder + .add_custom_joined_event(&room_id, member1_invites_member2_event, RoomEvent::RoomMember) + .add_custom_joined_event(&room_id, member2_join_event, RoomEvent::RoomMember) + .build_sync_response(); + + let mut member1_name_change_sync_response = event_builder + .add_room_event(EventsJson::MemberNameChange, RoomEvent::RoomMember) + .build_sync_response(); + + let mut member2_name_change_sync_response = event_builder + .add_custom_joined_event(&room_id, member2_name_change_event, RoomEvent::RoomMember) .build_sync_response(); // First member with display name "example" joins @@ -1103,11 +1154,7 @@ mod test { // Second and third member leave. The first's display name is now just "example" again. client - .receive_sync_response(&mut member2_leave_sync_response) - .await - .unwrap(); - client - .receive_sync_response(&mut member3_leave_sync_response) + .receive_sync_response(&mut member2_and_member3_leave_sync_response) .await .unwrap(); @@ -1119,6 +1166,60 @@ mod test { assert_eq!("example", display_name1); } + + // Second member rejoins after being invited by first member. Both of their names are + // disambiguated. + client + .receive_sync_response(&mut member2_rejoins_when_invited_sync_response) + .await + .unwrap(); + + { + let room = client.get_joined_room(&room_id).await.unwrap(); + let room = room.read().await; + + let display_name1 = room.member_display_name(&user_id1); + let display_name2 = room.member_display_name(&user_id2); + + assert_eq!(format!("example ({})", user_id1), display_name1); + assert_eq!(format!("example ({})", user_id2), display_name2); + } + + // First member changes his display name to "changed". None of the display names are + // disambiguated. + client + .receive_sync_response(&mut member1_name_change_sync_response) + .await + .unwrap(); + + { + let room = client.get_joined_room(&room_id).await.unwrap(); + let room = room.read().await; + + let display_name1 = room.member_display_name(&user_id1); + let display_name2 = room.member_display_name(&user_id2); + + assert_eq!("changed", display_name1); + assert_eq!("example", display_name2); + } + + // Second member *also* changes his display name to "changed". Again, both display name are + // disambiguated. + client + .receive_sync_response(&mut member2_name_change_sync_response) + .await + .unwrap(); + + { + let room = client.get_joined_room(&room_id).await.unwrap(); + let room = room.read().await; + + let display_name1 = room.member_display_name(&user_id1); + let display_name2 = room.member_display_name(&user_id2); + + assert_eq!(format!("changed ({})", user_id1), display_name1); + assert_eq!(format!("changed ({})", user_id2), display_name2); + } } #[async_test] From 5f49dab1fa046c42c1cd8de8c2b23dd75cbaff1d Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 14:02:37 +0200 Subject: [PATCH 14/28] Correct docstring. --- matrix_sdk_base/src/models/room.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 6bc060a4..c2181294 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -763,7 +763,7 @@ impl Room { /// /// # Arguments /// - /// * `event` - The presence event for a specified room member. + /// * `event` - The presence event to receive and process. pub fn receive_presence_event(&mut self, event: &PresenceEvent) -> bool { if let Some(member) = self.joined_members.get_mut(&event.sender) { if member.did_update_presence(event) { From eeebb43e321361319733b881060cc9c0d73e8820 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 15:44:44 +0200 Subject: [PATCH 15/28] Move mutating methods from RoomMember to Room. The `update_profile` method cannot live in `RoomMember` since that operation needs information which only exists in `Room` (for instance, it needs other members in order to perform display name disambiguation). Leaving other mutating methods on `RoomMember` (like `update_power` and `update_presence`) then seemed illogical so they were all moved into `Room`. In addition, a small refactoring was done to remove `did_update_presence` and `update_presence` since their existence doesn't make much sense anymore and it saves us from repeating work. Their function is now done in `receive_presence_event`. Also, several docstrings were corrected and reworded. --- matrix_sdk_base/src/models/room.rs | 115 +++++++++++++++++----- matrix_sdk_base/src/models/room_member.rs | 91 +---------------- 2 files changed, 95 insertions(+), 111 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index c2181294..1dae019d 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -23,7 +23,7 @@ use super::RoomMember; use crate::api::r0::sync::sync_events::{RoomSummary, UnreadNotificationsCount}; use crate::events::collections::all::{RoomEvent, StateEvent}; -use crate::events::presence::PresenceEvent; +use crate::events::presence::{PresenceEvent, PresenceEventContent}; use crate::events::room::{ aliases::AliasesEvent, canonical_alias::CanonicalAliasEvent, @@ -584,22 +584,14 @@ impl Room { use MembershipChange::*; match event.membership_change() { - Invited | Joined => self.add_member(event), + Invited | Joined => { + self.add_member(event) + } Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { self.remove_member(event) } ProfileChanged => { - let user_id = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { - id - } else { - return false; - }; - - if let Some(member) = self.joined_members.get_mut(&user_id) { - member.update_profile(event) - } else { - false - } + self.update_member_profile(event) } // Not interested in other events. @@ -671,11 +663,12 @@ impl Room { for user in event.content.users.keys() { if let Some(member) = self.joined_members.get_mut(user) { - if member.update_power(event, max_power) { + if Room::update_member_power(member, event, max_power) { updated = true; } } } + updated } @@ -755,28 +748,106 @@ impl Room { } } - /// Receive a presence event from an `IncomingResponse` and updates the client state. + /// Receive a presence event for a member of the current room. /// - /// This will only update the user if found in the current room looped through - /// by `Client::sync`. - /// Returns true if the specific users presence has changed, false otherwise. + /// Returns true if the event causes a change to the member's presence, false otherwise. /// /// # Arguments /// /// * `event` - The presence event to receive and process. pub fn receive_presence_event(&mut self, event: &PresenceEvent) -> bool { + let PresenceEvent { + content: + PresenceEventContent { + avatar_url, + currently_active, + displayname, + last_active_ago, + presence, + status_msg, + }, + .. + } = event; + if let Some(member) = self.joined_members.get_mut(&event.sender) { - if member.did_update_presence(event) { + if member.display_name == *displayname + && member.avatar_url == *avatar_url + && member.presence.as_ref() == Some(presence) + && member.status_msg == *status_msg + && member.last_active_ago == *last_active_ago + && member.currently_active == *currently_active { + + // Everything is the same, nothing to do. false } else { - member.update_presence(event); + // Something changed, do the update. + + member.presence_events.push(event.clone()); + member.avatar_url = avatar_url.clone(); + member.currently_active = *currently_active; + member.display_name = displayname.clone(); + member.last_active_ago = *last_active_ago; + member.presence = Some(*presence); + member.status_msg = status_msg.clone(); + true } } else { - // this is probably an error as we have a `PresenceEvent` for a user - // we don't know about + // This is probably an error as we have a `PresenceEvent` for a user + // we don't know about. false } + + } + + /// Process an update of a member's profile. + /// + /// # Arguments + /// + /// * `event` - The profile update event for a specified room member. + // TODO: NEXT: Add disambiguation handling here + pub(crate) fn update_member_profile(&mut self, event: &MemberEvent) -> bool { + let user_id = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { + id + } else { + return false; + }; + + if let Some(member) = self.joined_members.get_mut(&user_id) { + member.display_name = event.content.displayname.clone(); + member.avatar_url = event.content.avatar_url.clone(); + true + } else if let Some(member) = self.invited_members.get_mut(&user_id) { + member.display_name = event.content.displayname.clone(); + member.avatar_url = event.content.avatar_url.clone(); + true + } else { + false + } + } + + /// Process an update of a member's power level. + /// + /// # Arguments + /// + /// * `event` - The power level event to process. + /// * `max_power` - Maximum power level allowed. + pub fn update_member_power(member: &mut RoomMember, event: &PowerLevelsEvent, max_power: Int) -> bool { + let changed; + + if let Some(user_power) = event.content.users.get(&member.user_id) { + changed = member.power_level != Some(*user_power); + member.power_level = Some(*user_power); + } else { + changed = member.power_level != Some(event.content.users_default); + member.power_level = Some(event.content.users_default); + } + + if max_power > Int::from(0) { + member.power_level_norm = Some((member.power_level.unwrap() * Int::from(100)) / max_power); + } + + changed } } diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 71e59195..ced14760 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -16,11 +16,8 @@ use std::convert::TryFrom; use crate::events::collections::all::Event; -use crate::events::presence::{PresenceEvent, PresenceEventContent, PresenceState}; -use crate::events::room::{ - member::MemberEvent, - power_levels::PowerLevelsEvent, -}; +use crate::events::presence::{PresenceEvent, PresenceState}; +use crate::events::room::member::MemberEvent; use crate::identifiers::UserId; use crate::js_int::{Int, UInt}; @@ -123,90 +120,6 @@ impl RoomMember { .map(|d| format!("{} ({})", d, self.user_id)) .unwrap_or_else(|| format!("{}", self.user_id)) } - - /// Handle profile updates. - // TODO: NEXT: Add disambiguation handling here - pub(crate) fn update_profile(&mut self, event: &MemberEvent) -> bool { - self.display_name = event.content.displayname.clone(); - self.avatar_url = event.content.avatar_url.clone(); - - true - } - - pub fn update_power(&mut self, event: &PowerLevelsEvent, max_power: Int) -> bool { - let changed; - if let Some(user_power) = event.content.users.get(&self.user_id) { - changed = self.power_level != Some(*user_power); - self.power_level = Some(*user_power); - } else { - changed = self.power_level != Some(event.content.users_default); - self.power_level = Some(event.content.users_default); - } - - if max_power > Int::from(0) { - self.power_level_norm = Some((self.power_level.unwrap() * Int::from(100)) / max_power); - } - - changed - } - - /// If the current `PresenceEvent` updated the state of this `RoomMember`. - /// - /// Returns true if the member's presence has changed, false otherwise. - /// - /// # Arguments - /// - /// * `presence` - The presence event for this room member. - pub fn did_update_presence(&self, presence: &PresenceEvent) -> bool { - let PresenceEvent { - content: - PresenceEventContent { - avatar_url, - currently_active, - displayname, - last_active_ago, - presence, - status_msg, - }, - .. - } = presence; - self.display_name == *displayname - && self.avatar_url == *avatar_url - && self.presence.as_ref() == Some(presence) - && self.status_msg == *status_msg - && self.last_active_ago == *last_active_ago - && self.currently_active == *currently_active - } - - /// Updates the `RoomMember`'s presence. - /// - /// This should only be used if `did_update_presence` was true. - /// - /// # Arguments - /// - /// * `presence` - The presence event for this room member. - pub fn update_presence(&mut self, presence_ev: &PresenceEvent) { - let PresenceEvent { - content: - PresenceEventContent { - avatar_url, - currently_active, - displayname, - last_active_ago, - presence, - status_msg, - }, - .. - } = presence_ev; - - self.presence_events.push(presence_ev.clone()); - self.avatar_url = avatar_url.clone(); - self.currently_active = *currently_active; - self.display_name = displayname.clone(); - self.last_active_ago = *last_active_ago; - self.presence = Some(*presence); - self.status_msg = status_msg.clone(); - } } #[cfg(test)] From 7abdeed449af37889043a81b4ba5463425ab3b0f Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 15:56:29 +0200 Subject: [PATCH 16/28] fix: Don't issue a disambiguation in case of a unique display name. --- matrix_sdk_base/src/models/room.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 1dae019d..ea171660 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -475,8 +475,7 @@ impl Room { }; match users_with_same_name.len() { - 0 => HashMap::new(), - 1 => disambiguate_with(users_with_same_name, |m: &RoomMember| m.name()), + 0 | 1 => HashMap::new(), _ => disambiguate_with(users_with_same_name, |m: &RoomMember| m.unique_name()), } } From 7943baee49c578141ea5d2ff9313ca08d5569672 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 1 Jul 2020 16:08:25 +0200 Subject: [PATCH 17/28] add_member provably always returns true. --- matrix_sdk_base/src/models/room.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index ea171660..e37c0dec 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -358,9 +358,7 @@ impl Room { } /// Process the member event of an entering user. - /// - /// Returns true if this made a change to the room's state, false otherwise. - fn add_member(&mut self, event: &MemberEvent) -> bool { + fn add_member(&mut self, event: &MemberEvent) { let new_member = RoomMember::new(event); match event.membership_change() { @@ -385,15 +383,15 @@ impl Room { }; // Perform display name disambiguations, if necessary. - let disambiguations = self.disambiguation_updates(&new_member, MemberDirection::Entering); + let disambiguations = + self.disambiguation_updates(&new_member, MemberDirection::Entering); + for (id, name) in disambiguations.into_iter() { match name { None => self.disambiguated_display_names.remove(&id), Some(name) => self.disambiguated_display_names.insert(id, name), }; } - - true } /// Process the member event of a leaving user. @@ -584,7 +582,9 @@ impl Room { match event.membership_change() { Invited | Joined => { - self.add_member(event) + self.add_member(event); + + true } Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { self.remove_member(event) From e70929317ae0fa49f987bc1610cc8532846cc08e Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 10 Jul 2020 15:07:17 +0200 Subject: [PATCH 18/28] Revert "add_member provably always returns true." This reverts commit 7943baee49c578141ea5d2ff9313ca08d5569672. --- matrix_sdk_base/src/models/room.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index e37c0dec..ea171660 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -358,7 +358,9 @@ impl Room { } /// Process the member event of an entering user. - fn add_member(&mut self, event: &MemberEvent) { + /// + /// Returns true if this made a change to the room's state, false otherwise. + fn add_member(&mut self, event: &MemberEvent) -> bool { let new_member = RoomMember::new(event); match event.membership_change() { @@ -383,15 +385,15 @@ impl Room { }; // Perform display name disambiguations, if necessary. - let disambiguations = - self.disambiguation_updates(&new_member, MemberDirection::Entering); - + let disambiguations = self.disambiguation_updates(&new_member, MemberDirection::Entering); for (id, name) in disambiguations.into_iter() { match name { None => self.disambiguated_display_names.remove(&id), Some(name) => self.disambiguated_display_names.insert(id, name), }; } + + true } /// Process the member event of a leaving user. @@ -582,9 +584,7 @@ impl Room { match event.membership_change() { Invited | Joined => { - self.add_member(event); - - true + self.add_member(event) } Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { self.remove_member(event) From 24d2aa80784fedbc2354cc7661037eba3d1311af Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 10 Jul 2020 11:14:10 +0200 Subject: [PATCH 19/28] Style (cargo fmt, reordering import). --- matrix_sdk_base/src/models/room.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index ea171660..0ca03459 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -17,6 +17,8 @@ use std::borrow::Cow; use std::collections::{BTreeMap, HashMap}; use std::convert::TryFrom; +use serde::{Deserialize, Serialize}; + #[cfg(feature = "messages")] use super::message::MessageQueue; use super::RoomMember; @@ -42,7 +44,7 @@ use crate::events::room::message::MessageEvent; use crate::identifiers::{RoomAliasId, RoomId, UserId}; use crate::js_int::{Int, UInt}; -use serde::{Deserialize, Serialize}; + #[derive(Debug, Default, PartialEq, Serialize, Deserialize, Clone)] /// `RoomName` allows the calculation of a text room name. pub struct RoomName { @@ -467,6 +469,7 @@ impl Room { inclusive: bool, ) -> HashMap { let users_with_same_name = self.shares_displayname_with(member, inclusive); + let disambiguate_with = |members: Vec, f: fn(&RoomMember) -> String| { members .into_iter() @@ -831,7 +834,11 @@ impl Room { /// /// * `event` - The power level event to process. /// * `max_power` - Maximum power level allowed. - pub fn update_member_power(member: &mut RoomMember, event: &PowerLevelsEvent, max_power: Int) -> bool { + pub fn update_member_power( + member: &mut RoomMember, + event: &PowerLevelsEvent, + max_power: Int, + ) -> bool { let changed; if let Some(user_power) = event.content.users.get(&member.user_id) { @@ -843,7 +850,8 @@ impl Room { } if max_power > Int::from(0) { - member.power_level_norm = Some((member.power_level.unwrap() * Int::from(100)) / max_power); + member.power_level_norm = + Some((member.power_level.unwrap() * Int::from(100)) / max_power); } changed @@ -963,7 +971,6 @@ mod test { .add_custom_joined_event(&room_id, member2_join_event, RoomEvent::RoomMember) .build_sync_response(); - // Test that `user` is either joined or invited to `room` but not both. async fn invited_or_joined_but_not_both(client: &BaseClient, room: &RoomId, user: &UserId) { let room = client.get_joined_room(&room).await.unwrap(); @@ -1172,7 +1179,11 @@ mod test { .build_sync_response(); let mut member2_rejoins_when_invited_sync_response = event_builder - .add_custom_joined_event(&room_id, member1_invites_member2_event, RoomEvent::RoomMember) + .add_custom_joined_event( + &room_id, + member1_invites_member2_event, + RoomEvent::RoomMember, + ) .add_custom_joined_event(&room_id, member2_join_event, RoomEvent::RoomMember) .build_sync_response(); From 559306a33c49cd3bf13e552a6e43231d64ca1481 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 10 Jul 2020 11:14:22 +0200 Subject: [PATCH 20/28] Rewrite disambiguation algorithm to handle profile changes. The new algorithm is simpler. Instead of tracking a list of disambiguated display names in `Room`, we instead track the display name ambiguity status in `RoomMember`. This allows a client to generate the correct name for a member using solely the information available in `RoomMember`. The disambiguation algorithm itself now only calculates the set of members whose ambiguity status had changed instead of producing disambiguated display names for everyone affected. This is called on each room entry (join or invite), room entry and profile change, and the updates are propagated to the affected `RoomMember`s. --- matrix_sdk_base/Cargo.toml | 1 + matrix_sdk_base/src/client.rs | 4 +- matrix_sdk_base/src/models/message.rs | 2 - matrix_sdk_base/src/models/room.rs | 531 ++++++++++++++-------- matrix_sdk_base/src/models/room_member.rs | 4 + matrix_sdk_base/src/state/mod.rs | 2 - 6 files changed, 351 insertions(+), 193 deletions(-) diff --git a/matrix_sdk_base/Cargo.toml b/matrix_sdk_base/Cargo.toml index 1acb56b7..bf884319 100644 --- a/matrix_sdk_base/Cargo.toml +++ b/matrix_sdk_base/Cargo.toml @@ -21,6 +21,7 @@ async-trait = "0.1.31" serde = "1.0.110" serde_json = "1.0.53" zeroize = "1.1.0" +tracing = "0.1.14" matrix-sdk-common-macros = { version = "0.1.0", path = "../matrix_sdk_common_macros" } matrix-sdk-common = { version = "0.1.0", path = "../matrix_sdk_common" } diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 975c7c6d..2e0bad13 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -734,7 +734,7 @@ impl BaseClient { let mut room = room_lock.write().await; if let RoomEvent::RoomMember(mem_event) = &mut e { - let changed = room.handle_membership(mem_event); + let (changed, _) = room.handle_membership(mem_event); // The memberlist of the room changed, invalidate the group session // of the room. @@ -771,7 +771,7 @@ impl BaseClient { let mut room = room_lock.write().await; if let StateEvent::RoomMember(e) = event { - let changed = room.handle_membership(e); + let (changed, _) = room.handle_membership(e); // The memberlist of the room changed, invalidate the group session // of the room. diff --git a/matrix_sdk_base/src/models/message.rs b/matrix_sdk_base/src/models/message.rs index b78ea1a7..db6d3ac5 100644 --- a/matrix_sdk_base/src/models/message.rs +++ b/matrix_sdk_base/src/models/message.rs @@ -178,7 +178,6 @@ mod test { serde_json::json!({ "!roomid:example.com": { "room_id": "!roomid:example.com", - "disambiguated_display_names": {}, "room_name": { "name": null, "canonical_alias": null, @@ -229,7 +228,6 @@ mod test { let json = serde_json::json!({ "!roomid:example.com": { "room_id": "!roomid:example.com", - "disambiguated_display_names": {}, "room_name": { "name": null, "canonical_alias": null, diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 0ca03459..61c23a98 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -14,10 +14,11 @@ // limitations under the License. use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; use serde::{Deserialize, Serialize}; +use tracing::{debug, error, trace}; #[cfg(feature = "messages")] use super::message::MessageQueue; @@ -30,7 +31,7 @@ use crate::events::room::{ aliases::AliasesEvent, canonical_alias::CanonicalAliasEvent, encryption::EncryptionEvent, - member::{MemberEvent, MembershipChange}, + member::{MemberEvent, MembershipChange, MembershipState}, name::NameEvent, power_levels::{NotificationPowerLevels, PowerLevelsEvent, PowerLevelsEventContent}, tombstone::TombstoneEvent, @@ -146,12 +147,6 @@ pub struct Tombstone { replacement: RoomId, } -#[derive(Debug, PartialEq, Eq)] -enum MemberDirection { - Entering, - Exiting, -} - #[derive(Debug, PartialEq, Serialize, Deserialize, Clone)] /// A Matrix room. pub struct Room { @@ -188,8 +183,6 @@ pub struct Room { pub unread_notifications: Option, /// The tombstone state of this room. pub tombstone: Option, - /// The map of disambiguated display names for users who have the same display name - disambiguated_display_names: HashMap, } impl RoomName { @@ -302,7 +295,6 @@ impl Room { unread_highlight: None, unread_notifications: None, tombstone: None, - disambiguated_display_names: HashMap::new(), } } @@ -329,191 +321,231 @@ impl Room { /// Get the disambiguated display name for a member of this room. /// - /// If a member has no display name set, returns the MXID as a fallback. Additionally, we - /// return the MXID even if there is no such member in the room. + /// If a member has no display name set, returns the MXID as a fallback. + /// + /// This method never fails, even if user with the supplied MXID is not a member in the room. + /// In this case we still return the supplied MXID as a fallback. /// /// When displaying a room member's display name, clients *must* use this method to obtain the /// name instead of displaying the `RoomMember::display_name` directly. This is because /// multiple members can share the same display name in which case the display name has to be /// disambiguated. pub fn member_display_name<'a>(&'a self, id: &'a UserId) -> Cow<'a, str> { - let disambiguated_name = self - .disambiguated_display_names - .get(id) - .map(|s| s.as_str().into()); + let member = self.get_member(id); - if let Some(name) = disambiguated_name { - // The display name of the member is non-unique so we return a disambiguated version. - name - } else if let Some(member) = self - .joined_members - .get(id) - .or_else(|| self.invited_members.get(id)) - { - // The display name of the member is unique so we can return it directly if it is set. - // If not, we return his MXID. - member.name().into() - } else { - // There is no member with the requested MXID in the room. We still return the MXID. - id.as_ref().into() - } - } - - /// Process the member event of an entering user. - /// - /// Returns true if this made a change to the room's state, false otherwise. - fn add_member(&mut self, event: &MemberEvent) -> bool { - let new_member = RoomMember::new(event); - - match event.membership_change() { - MembershipChange::Joined => { - // Since the member is now joined, he shouldn't be tracked as an invited member any - // longer. - if self.invited_members.contains_key(&new_member.user_id) { - self.invited_members.remove(&new_member.user_id); + match member { + Some(member) => { + if member.display_name_ambiguous { + member.unique_name().into() + } else { + member.name().into() } - - self.joined_members - .insert(new_member.user_id.clone(), new_member.clone()) } - MembershipChange::Invited => self - .invited_members - .insert(new_member.user_id.clone(), new_member.clone()), - - _ => { - panic!("Room::add_member called on an event that is neither a join nor an invite.") - } - }; - - // Perform display name disambiguations, if necessary. - let disambiguations = self.disambiguation_updates(&new_member, MemberDirection::Entering); - for (id, name) in disambiguations.into_iter() { - match name { - None => self.disambiguated_display_names.remove(&id), - Some(name) => self.disambiguated_display_names.insert(id, name), - }; + // Even if there is no such member, we return the MXID that was given to us. + None => id.as_ref().into(), } - - true } - /// Process the member event of a leaving user. + /// Process the join or invite event for a new room member. /// - /// Returns true if this made a change to the room's state, false otherwise. - fn remove_member(&mut self, event: &MemberEvent) -> bool { - let leaving_member = RoomMember::new(event); + /// If the user is not already a member, he will be added. Otherwise, his state will be updated + /// to reflect the event's state. + /// + /// Returns a tuple of: + /// + /// 1. True if the event made changes to the room's state, false otherwise. + /// 2. Returns a map of display name disambiguations which tells us which members need to have + /// their display names disambiguated and to what. + /// + /// # Arguments + /// + /// * `target_member` - The ID of the member to add. + /// * `event` - The join or invite event for the specified room member. + fn add_member( + &mut self, + target_member: &UserId, + event: &MemberEvent, + ) -> (bool, HashMap) { + let new_member = RoomMember::new(event); // Perform display name disambiguations, if necessary. let disambiguations = - self.disambiguation_updates(&leaving_member, MemberDirection::Exiting); - for (id, name) in disambiguations.into_iter() { - match name { - None => self.disambiguated_display_names.remove(&id), - Some(name) => self.disambiguated_display_names.insert(id, name), - }; + self.ambiguity_updates(target_member, None, new_member.display_name.clone()); + + debug!("add_member: disambiguations: {:#?}", disambiguations); + + match event.content.membership { + MembershipState::Join => { + // Since the member is now joined, he shouldn't be tracked as an invited member any + // longer if he was previously tracked as such. + self.invited_members.remove(target_member); + + self.joined_members + .insert(target_member.clone(), new_member.clone()) + } + + MembershipState::Invite => self + .invited_members + .insert(target_member.clone(), new_member.clone()), + + _ => panic!("Room::add_member called on event that is neither `join` nor `invite`."), + }; + + for (id, is_ambiguous) in disambiguations.iter() { + self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; } - if self.joined_members.contains_key(&leaving_member.user_id) { - self.joined_members.remove(&leaving_member.user_id); - true - } else if self.invited_members.contains_key(&leaving_member.user_id) { - self.invited_members.remove(&leaving_member.user_id); - true - } else { - false + (true, disambiguations) + } + + /// Process the leaving event for a room member. + /// + /// Returns a tuple of: + /// + /// 1. True if the event made changes to the room's state, false otherwise. + /// 2. Returns a map of display name disambiguations which tells us which members need to have + /// their display names disambiguated and to what. + /// + /// # Arguments + /// + /// * `target_member` - The ID of the member to remove. + /// * `event` - The leaving event for the specified room member. + fn remove_member( + &mut self, + target_member: &UserId, + event: &MemberEvent, + ) -> (bool, HashMap) { + let leaving_member = RoomMember::new(event); + + if self.get_member(target_member).is_none() { + return (false, HashMap::new()); + } + + // Perform display name disambiguations, if necessary. + let disambiguations = + self.ambiguity_updates(target_member, leaving_member.display_name.clone(), None); + + debug!("remove_member: disambiguations: {:#?}", disambiguations); + + for (id, is_ambiguous) in disambiguations.iter() { + self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; + } + + // TODO: factor this out to a method called `remove_member` and rename this method + // to something like `process_member_leaving_event`. + self.joined_members + .remove(target_member) + .or_else(|| self.invited_members.remove(target_member)); + + (true, disambiguations) + } + + /// Check whether the user with the MXID `user_id` is joined or invited to the room. + /// + /// Returns true if so, false otherwise. + pub fn member_is_tracked(&self, user_id: &UserId) -> bool { + self.invited_members.contains_key(&user_id) || self.joined_members.contains_key(&user_id) + } + + /// Get a room member by user ID. + /// + /// If there is no such member, returns `None`. + pub fn get_member(&self, user_id: &UserId) -> Option<&RoomMember> { + self.joined_members + .get(user_id) + .or_else(|| self.invited_members.get(user_id)) + } + + /// Get a room member by user ID. + /// + /// If there is no such member, returns `None`. + pub fn get_member_mut(&mut self, user_id: &UserId) -> Option<&mut RoomMember> { + match self.joined_members.get_mut(user_id) { + None => self.invited_members.get_mut(user_id), + Some(m) => Some(m), } } - /// Given a room `member`, return the list of members which have the same display name. - /// - /// The `inclusive` parameter controls whether the passed member should be included in the - /// list or not. - fn shares_displayname_with(&self, member: &RoomMember, inclusive: bool) -> Vec { + /// Given a display name, return the set of members which share it. + fn display_name_equivalence_class(&self, name: &str) -> HashSet { let members = self .invited_members .iter() .chain(self.joined_members.iter()); - // Find all other users that share the same display name as the joining user. + // Find all other users that share the display name with the joining user. members - .filter(|(_, existing_member)| { + .filter(|(_, member)| { member .display_name .as_ref() - .and_then(|new_member_name| { - existing_member - .display_name - .as_ref() - .map(|existing_member_name| new_member_name == existing_member_name) - }) + .map(|other_name| other_name == name) .unwrap_or(false) }) - // If not an inclusive search, do not consider the member for which we are disambiguating. - .filter(|(id, _)| inclusive || **id != member.user_id) - .map(|(_, member)| member) - .cloned() + .map(|(_, member)| member.user_id.clone()) .collect() } - /// Given a room member, generate a map of all display name disambiguations which are necessary - /// in order to make that member's display name unique. + /// Given a change in a member's display name, determine the set of members whose display names + /// would either become newly ambiguous or no longer ambiguous after the change. /// - /// The `inclusive` parameter controls whether or not the member for which we are - /// disambiguating should be considered a current member of the room. + /// Returns a map of `user` -> `ambiguity` for affected room member. If `ambiguity` is `true`, + /// the member's display name is newly ambiguous after the change. If `false`, their display + /// name is no longer ambiguous (but it was before). /// - /// Returns a map from MXID to disambiguated name. - fn member_disambiguations( + /// The method *must* be called before any actual display name changes are performed in our + /// model (e.g. the `RoomMember` structs). + /// + /// # Arguments + /// + /// * `member` - The ID of the member changing their display name. + /// * `old_name` - Their old display name, prior to the change. May be `None` if they had no + /// display name in this room (e.g. because it was unset or because they were not a room + /// member). + /// * `new_name` - Their new display name, after the change. May be `None` if they are no + /// longer going to have a display name in this room after the change (e.g. because they are + /// unsetting it or leaving the room). + /// + /// Returns a map from user ID to the new ambiguity status. + fn ambiguity_updates( &self, - member: &RoomMember, - inclusive: bool, - ) -> HashMap { - let users_with_same_name = self.shares_displayname_with(member, inclusive); + member: &UserId, + old_name: Option, + new_name: Option, + ) -> HashMap { + // Must be called *before* any changes to the model. - let disambiguate_with = |members: Vec, f: fn(&RoomMember) -> String| { - members - .into_iter() - .map(|ref m| (m.user_id.clone(), f(m))) - .collect::>() + let old_name_eq_class = match old_name { + None => HashSet::new(), + Some(name) => self.display_name_equivalence_class(&name), }; - match users_with_same_name.len() { - 0 | 1 => HashMap::new(), - _ => disambiguate_with(users_with_same_name, |m: &RoomMember| m.unique_name()), - } - } + let disambiguate_old = match old_name_eq_class.len().saturating_sub(1) { + n if n > 1 => vec![(member.clone(), false)].into_iter().collect(), + 1 => old_name_eq_class.into_iter().map(|m| (m, false)).collect(), + 0 => HashMap::new(), + _ => panic!("impossible"), + }; - /// Calculate disambiguation updates needed when a room member either enters or exits. - fn disambiguation_updates( - &self, - member: &RoomMember, - when: MemberDirection, - ) -> HashMap> { - let before; - let after; + // - match when { - MemberDirection::Entering => { - before = self.member_disambiguations(member, false); - after = self.member_disambiguations(member, true); - } - MemberDirection::Exiting => { - before = self.member_disambiguations(member, true); - after = self.member_disambiguations(member, false); - } - } + let mut new_name_eq_class = match new_name { + None => HashSet::new(), + Some(name) => self.display_name_equivalence_class(&name), + }; - let mut res = before; - res.extend(after.clone()); + new_name_eq_class.insert(member.clone()); - res.into_iter() - .map(|(user_id, name)| { - if !after.contains_key(&user_id) { - (user_id, None) - } else { - (user_id, Some(name)) - } - }) + let disambiguate_new = match new_name_eq_class.len() { + 1 => HashMap::new(), + 2 => new_name_eq_class.into_iter().map(|m| (m, true)).collect(), + _ => vec![(member.clone(), true)].into_iter().collect(), + }; + + disambiguate_old + .into_iter() + .chain(disambiguate_new.into_iter()) .collect() } @@ -581,23 +613,69 @@ impl Room { /// Handle a room.member updating the room state if necessary. /// - /// Returns true if the joined member list changed, false otherwise. - pub fn handle_membership(&mut self, event: &MemberEvent) -> bool { + /// Returns a tuple of: + /// + /// 1. True if the joined member list changed, false otherwise. + /// 2. A map of display name disambiguations which tells us which members need to have their + /// display names disambiguated and to what. + pub fn handle_membership( + &mut self, + event: &MemberEvent, + state_event: bool, + ) -> (bool, HashMap) { use MembershipChange::*; + use MembershipState::*; - match event.membership_change() { - Invited | Joined => { - self.add_member(event) - } - Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { - self.remove_member(event) - } - ProfileChanged => { - self.update_member_profile(event) - } + trace!( + "Received {} event: {}", + if state_event { "state" } else { "timeline" }, + event.event_id + ); - // Not interested in other events. - _ => false, + let target_user = match UserId::try_from(event.state_key.clone()) { + Ok(id) => id, + Err(e) => { + error!("Received a member event with invalid state_key: {}", e); + return (false, HashMap::new()); + } + }; + + if state_event && !self.member_is_tracked(&target_user) { + debug!( + "handle_membership: User {user_id} is {state} the room {room_id} ({room_name})", + user_id = target_user, + state = event.content.membership.describe(), + room_id = self.room_id, + room_name = self.display_name(), + ); + + match event.content.membership { + Join | Invite => self.add_member(&target_user, event), + + // We are not interested in tracking past members for now + _ => (false, HashMap::new()), + } + } else { + let change = event.membership_change(); + + debug!( + "handle_membership: User {user_id} {action} the room {room_id} ({room_name})", + user_id = target_user, + action = change.describe(), + room_id = self.room_id, + room_name = self.display_name(), + ); + + match change { + Invited | Joined => self.add_member(&target_user, event), + Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { + self.remove_member(&target_user, event) + } + ProfileChanged => self.update_member_profile(&target_user, event), + + // Not interested in other events. + _ => (false, HashMap::new()), + } } } @@ -697,7 +775,7 @@ impl Room { pub fn receive_timeline_event(&mut self, event: &RoomEvent) -> bool { match event { // update to the current members of the room - RoomEvent::RoomMember(member) => self.handle_membership(member), + RoomEvent::RoomMember(member) => self.handle_membership(member, false).0, // finds all events related to the name of the room for later use RoomEvent::RoomName(name) => self.handle_room_name(name), RoomEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -722,7 +800,7 @@ impl Room { pub fn receive_state_event(&mut self, event: &StateEvent) -> bool { match event { // update to the current members of the room - StateEvent::RoomMember(member) => self.handle_membership(member), + StateEvent::RoomMember(member) => self.handle_membership(member, true).0, // finds all events related to the name of the room for later use StateEvent::RoomName(name) => self.handle_room_name(name), StateEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -777,8 +855,8 @@ impl Room { && member.presence.as_ref() == Some(presence) && member.status_msg == *status_msg && member.last_active_ago == *last_active_ago - && member.currently_active == *currently_active { - + && member.currently_active == *currently_active + { // Everything is the same, nothing to do. false } else { @@ -799,33 +877,70 @@ impl Room { // we don't know about. false } - } /// Process an update of a member's profile. /// + /// Returns a tuple of: + /// + /// 1. True if the event made changes to the room's state, false otherwise. + /// 2. A map of display name disambiguations which tells us which members need to have their + /// display names disambiguated and to what. + /// /// # Arguments /// - /// * `event` - The profile update event for a specified room member. - // TODO: NEXT: Add disambiguation handling here - pub(crate) fn update_member_profile(&mut self, event: &MemberEvent) -> bool { - let user_id = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { - id - } else { - return false; + /// * `target_member` - The ID of the member to update. + /// * `event` - The profile update event for the specified room member. + pub fn update_member_profile( + &mut self, + target_member: &UserId, + event: &MemberEvent, + ) -> (bool, HashMap) { + let old_name = self + .get_member(target_member) + .map_or_else(|| None, |m| m.display_name.clone()); + let new_name = event.content.displayname.clone(); + + debug!( + "update_member_profile [{}]: from nick {:#?} to nick {:#?}", + self.room_id, old_name, &new_name + ); + + let disambiguations = + self.ambiguity_updates(target_member, old_name.clone(), new_name.clone()); + for (id, is_ambiguous) in disambiguations.iter() { + if self.get_member_mut(id).is_none() { + error!("update_member_profile: I'm about to fail for id {}. user_id = {}\nevent = {:#?}", + id, + target_member, + event); + } else { + self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; + } + } + + debug!( + "update_member_profile [{}]: disambiguations: {:#?}", + self.room_id, &disambiguations + ); + + let changed = match self.get_member_mut(target_member) { + Some(member) => { + member.display_name = new_name; + member.avatar_url = event.content.avatar_url.clone(); + true + } + None => { + error!( + "update_member_profile [{}]: user {} does not exist", + self.room_id, target_member + ); + + false + } }; - if let Some(member) = self.joined_members.get_mut(&user_id) { - member.display_name = event.content.displayname.clone(); - member.avatar_url = event.content.avatar_url.clone(); - true - } else if let Some(member) = self.invited_members.get_mut(&user_id) { - member.display_name = event.content.displayname.clone(); - member.avatar_url = event.content.avatar_url.clone(); - true - } else { - false - } + (changed, disambiguations) } /// Process an update of a member's power level. @@ -858,6 +973,48 @@ impl Room { } } +trait Describe { + fn describe(&self) -> String; +} + +impl Describe for MembershipState { + fn describe(&self) -> String { + use MembershipState::*; + + match self { + Ban => "is banned in", + Invite => "is invited to", + Join => "is a member of", + Knock => "is requesting access", + Leave => "left", + } + .to_string() + } +} + +impl Describe for MembershipChange { + fn describe(&self) -> String { + use MembershipChange::*; + + match self { + Invited => "got invited to", + Joined => "joined", + Kicked => "got kicked from", + Banned => "got banned from", + Unbanned => "got unbanned from", + KickedAndBanned => "got kicked and banned from", + InvitationRejected => "rejected the invitation to", + InvitationRevoked => "got their invitation revoked from", + Left => "left", + ProfileChanged => "changed their profile", + None => "did nothing in", + NotImplemented => "NOT IMPLEMENTED", + Error => "ERROR", + } + .to_string() + } +} + #[cfg(test)] mod test { use super::*; diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index ced14760..98fc4fd7 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -33,6 +33,8 @@ pub struct RoomMember { pub user_id: UserId, /// The human readable name of the user. pub display_name: Option, + /// Whether the member's display name is ambiguous due to being shared with other members. + pub display_name_ambiguous: bool, /// The matrix url of the users avatar. pub avatar_url: Option, /// The time, in ms, since the user interacted with the server. @@ -76,6 +78,7 @@ impl PartialEq for RoomMember { && self.user_id == other.user_id && self.name == other.name && self.display_name == other.display_name + && self.display_name_ambiguous == other.display_name_ambiguous && self.avatar_url == other.avatar_url && self.last_active_ago == other.last_active_ago } @@ -88,6 +91,7 @@ impl RoomMember { room_id: event.room_id.as_ref().map(|id| id.to_string()), user_id: UserId::try_from(event.state_key.as_str()).unwrap(), display_name: event.content.displayname.clone(), + display_name_ambiguous: false, avatar_url: event.content.avatar_url.clone(), presence: None, status_msg: None, diff --git a/matrix_sdk_base/src/state/mod.rs b/matrix_sdk_base/src/state/mod.rs index 6bd18820..dd901082 100644 --- a/matrix_sdk_base/src/state/mod.rs +++ b/matrix_sdk_base/src/state/mod.rs @@ -159,7 +159,6 @@ mod test { "creator": null, "joined_members": {}, "invited_members": {}, - "disambiguated_display_names": {}, "typing_users": [], "power_levels": null, "encrypted": null, @@ -176,7 +175,6 @@ mod test { serde_json::json!({ "!roomid:example.com": { "room_id": "!roomid:example.com", - "disambiguated_display_names": {}, "room_name": { "name": null, "canonical_alias": null, From 949305da7254315bba030199356a03df2911d776 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Mon, 6 Jul 2020 15:22:49 +0200 Subject: [PATCH 21/28] Clarify comment. --- matrix_sdk_base/src/models/room.rs | 302 +++++++++-------------------- 1 file changed, 91 insertions(+), 211 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 61c23a98..0d27cf81 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -18,7 +18,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; use serde::{Deserialize, Serialize}; -use tracing::{debug, error, trace}; +use tracing::{debug, instrument}; #[cfg(feature = "messages")] use super::message::MessageQueue; @@ -31,7 +31,7 @@ use crate::events::room::{ aliases::AliasesEvent, canonical_alias::CanonicalAliasEvent, encryption::EncryptionEvent, - member::{MemberEvent, MembershipChange, MembershipState}, + member::{MemberEvent, MembershipChange}, name::NameEvent, power_levels::{NotificationPowerLevels, PowerLevelsEvent, PowerLevelsEventContent}, tombstone::TombstoneEvent, @@ -321,10 +321,8 @@ impl Room { /// Get the disambiguated display name for a member of this room. /// - /// If a member has no display name set, returns the MXID as a fallback. - /// - /// This method never fails, even if user with the supplied MXID is not a member in the room. - /// In this case we still return the supplied MXID as a fallback. + /// If a member has no display name set, returns the MXID as a fallback. Additionally, we + /// return the MXID even if there is no such member in the room. /// /// When displaying a room member's display name, clients *must* use this method to obtain the /// name instead of displaying the `RoomMember::display_name` directly. This is because @@ -340,56 +338,49 @@ impl Room { } else { member.name().into() } - } + }, // Even if there is no such member, we return the MXID that was given to us. None => id.as_ref().into(), } } - /// Process the join or invite event for a new room member. - /// - /// If the user is not already a member, he will be added. Otherwise, his state will be updated - /// to reflect the event's state. + /// Process the member event of an entering user. /// /// Returns a tuple of: /// /// 1. True if the event made changes to the room's state, false otherwise. /// 2. Returns a map of display name disambiguations which tells us which members need to have /// their display names disambiguated and to what. - /// - /// # Arguments - /// - /// * `target_member` - The ID of the member to add. - /// * `event` - The join or invite event for the specified room member. - fn add_member( - &mut self, - target_member: &UserId, - event: &MemberEvent, - ) -> (bool, HashMap) { + #[instrument] + fn add_member(&mut self, event: &MemberEvent) -> (bool, HashMap) { let new_member = RoomMember::new(event); // Perform display name disambiguations, if necessary. - let disambiguations = - self.ambiguity_updates(target_member, None, new_member.display_name.clone()); + let disambiguations = self.disambiguation_updates(&new_member.user_id, None, new_member.display_name.clone()); - debug!("add_member: disambiguations: {:#?}", disambiguations); + debug!( + "add_member: disambiguations: {:#?}", + disambiguations + ); - match event.content.membership { - MembershipState::Join => { + match event.membership_change() { + MembershipChange::Joined => { // Since the member is now joined, he shouldn't be tracked as an invited member any // longer if he was previously tracked as such. - self.invited_members.remove(target_member); + self.invited_members.remove(&new_member.user_id); self.joined_members - .insert(target_member.clone(), new_member.clone()) + .insert(new_member.user_id.clone(), new_member.clone()) } - MembershipState::Invite => self + MembershipChange::Invited => self .invited_members - .insert(target_member.clone(), new_member.clone()), + .insert(new_member.user_id.clone(), new_member.clone()), - _ => panic!("Room::add_member called on event that is neither `join` nor `invite`."), + _ => { + panic!("Room::add_member called on an event that is neither a join nor an invite.") + } }; for (id, is_ambiguous) in disambiguations.iter() { @@ -399,34 +390,29 @@ impl Room { (true, disambiguations) } - /// Process the leaving event for a room member. + /// Process the member event of a leaving user. /// /// Returns a tuple of: /// /// 1. True if the event made changes to the room's state, false otherwise. /// 2. Returns a map of display name disambiguations which tells us which members need to have /// their display names disambiguated and to what. - /// - /// # Arguments - /// - /// * `target_member` - The ID of the member to remove. - /// * `event` - The leaving event for the specified room member. - fn remove_member( - &mut self, - target_member: &UserId, - event: &MemberEvent, - ) -> (bool, HashMap) { + #[instrument] + fn remove_member(&mut self, event: &MemberEvent) -> (bool, HashMap) { let leaving_member = RoomMember::new(event); - if self.get_member(target_member).is_none() { + if self.get_member(&leaving_member.user_id).is_none() { return (false, HashMap::new()); } // Perform display name disambiguations, if necessary. let disambiguations = - self.ambiguity_updates(target_member, leaving_member.display_name.clone(), None); + self.disambiguation_updates(&leaving_member.user_id, leaving_member.display_name.clone(), None); - debug!("remove_member: disambiguations: {:#?}", disambiguations); + debug!( + "remove_member: disambiguations: {:#?}", + disambiguations + ); for (id, is_ambiguous) in disambiguations.iter() { self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; @@ -434,20 +420,12 @@ impl Room { // TODO: factor this out to a method called `remove_member` and rename this method // to something like `process_member_leaving_event`. - self.joined_members - .remove(target_member) - .or_else(|| self.invited_members.remove(target_member)); + self.joined_members.remove(&leaving_member.user_id).or_else(|| + self.invited_members.remove(&leaving_member.user_id)); (true, disambiguations) } - /// Check whether the user with the MXID `user_id` is joined or invited to the room. - /// - /// Returns true if so, false otherwise. - pub fn member_is_tracked(&self, user_id: &UserId) -> bool { - self.invited_members.contains_key(&user_id) || self.joined_members.contains_key(&user_id) - } - /// Get a room member by user ID. /// /// If there is no such member, returns `None`. @@ -463,7 +441,7 @@ impl Room { pub fn get_member_mut(&mut self, user_id: &UserId) -> Option<&mut RoomMember> { match self.joined_members.get_mut(user_id) { None => self.invited_members.get_mut(user_id), - Some(m) => Some(m), + Some(m) => Some(m) } } @@ -476,39 +454,19 @@ impl Room { // Find all other users that share the display name with the joining user. members - .filter(|(_, member)| { - member - .display_name - .as_ref() - .map(|other_name| other_name == name) - .unwrap_or(false) - }) + .filter(|(_, member)| member.display_name.as_ref().map(|other_name| other_name == name).unwrap_or(false)) .map(|(_, member)| member.user_id.clone()) .collect() } - /// Given a change in a member's display name, determine the set of members whose display names - /// would either become newly ambiguous or no longer ambiguous after the change. + /// Given a room member, generate a map of all display name disambiguations which are necessary + /// in order to make that member's display name unique. /// - /// Returns a map of `user` -> `ambiguity` for affected room member. If `ambiguity` is `true`, - /// the member's display name is newly ambiguous after the change. If `false`, their display - /// name is no longer ambiguous (but it was before). + /// The `inclusive` parameter controls whether or not the member for which we are + /// disambiguating should be considered a current member of the room. /// - /// The method *must* be called before any actual display name changes are performed in our - /// model (e.g. the `RoomMember` structs). - /// - /// # Arguments - /// - /// * `member` - The ID of the member changing their display name. - /// * `old_name` - Their old display name, prior to the change. May be `None` if they had no - /// display name in this room (e.g. because it was unset or because they were not a room - /// member). - /// * `new_name` - Their new display name, after the change. May be `None` if they are no - /// longer going to have a display name in this room after the change (e.g. because they are - /// unsetting it or leaving the room). - /// - /// Returns a map from user ID to the new ambiguity status. - fn ambiguity_updates( + /// Returns a map from MXID to disambiguated name. + fn disambiguation_updates( &self, member: &UserId, old_name: Option, @@ -525,7 +483,7 @@ impl Room { n if n > 1 => vec![(member.clone(), false)].into_iter().collect(), 1 => old_name_eq_class.into_iter().map(|m| (m, false)).collect(), 0 => HashMap::new(), - _ => panic!("impossible"), + _ => panic!("impossible") }; // @@ -543,10 +501,7 @@ impl Room { _ => vec![(member.clone(), true)].into_iter().collect(), }; - disambiguate_old - .into_iter() - .chain(disambiguate_new.into_iter()) - .collect() + disambiguate_old.into_iter().chain(disambiguate_new.into_iter()).collect() } /// Add to the list of `RoomAliasId`s. @@ -621,61 +576,45 @@ impl Room { pub fn handle_membership( &mut self, event: &MemberEvent, - state_event: bool, ) -> (bool, HashMap) { use MembershipChange::*; - use MembershipState::*; - trace!( - "Received {} event: {}", - if state_event { "state" } else { "timeline" }, - event.event_id - ); + match event.membership_change() { + Invited | Joined => { + debug!( + "handle_membership: User {} entering room {} ({}) (joined or invited)", + event.state_key, + self.room_id, + self.display_name(), + ); - let target_user = match UserId::try_from(event.state_key.clone()) { - Ok(id) => id, - Err(e) => { - error!("Received a member event with invalid state_key: {}", e); - return (false, HashMap::new()); + self.add_member(event) } - }; + Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { + debug!( + "handle_membership: User {} exiting room {} ({}) (leaving, kicked, banned or invitation rejected)", + event.state_key, + self.room_id, + self.display_name(), + ); - if state_event && !self.member_is_tracked(&target_user) { - debug!( - "handle_membership: User {user_id} is {state} the room {room_id} ({room_name})", - user_id = target_user, - state = event.content.membership.describe(), - room_id = self.room_id, - room_name = self.display_name(), - ); - - match event.content.membership { - Join | Invite => self.add_member(&target_user, event), - - // We are not interested in tracking past members for now - _ => (false, HashMap::new()), + self.remove_member(event) } - } else { - let change = event.membership_change(); + ProfileChanged => { + debug!( + "handle_membership: Profile changed for user {} in room {} ({}) (displayname={:?}, avatar={:?}", + event.state_key, + self.room_id, + self.display_name(), + event.content.displayname, + event.content.avatar_url + ); - debug!( - "handle_membership: User {user_id} {action} the room {room_id} ({room_name})", - user_id = target_user, - action = change.describe(), - room_id = self.room_id, - room_name = self.display_name(), - ); - - match change { - Invited | Joined => self.add_member(&target_user, event), - Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { - self.remove_member(&target_user, event) - } - ProfileChanged => self.update_member_profile(&target_user, event), - - // Not interested in other events. - _ => (false, HashMap::new()), + self.update_member_profile(event) } + + // Not interested in other events. + _ => (false, HashMap::new()), } } @@ -775,7 +714,7 @@ impl Room { pub fn receive_timeline_event(&mut self, event: &RoomEvent) -> bool { match event { // update to the current members of the room - RoomEvent::RoomMember(member) => self.handle_membership(member, false).0, + RoomEvent::RoomMember(member) => self.handle_membership(member).0, // finds all events related to the name of the room for later use RoomEvent::RoomName(name) => self.handle_room_name(name), RoomEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -800,7 +739,7 @@ impl Room { pub fn receive_state_event(&mut self, event: &StateEvent) -> bool { match event { // update to the current members of the room - StateEvent::RoomMember(member) => self.handle_membership(member, true).0, + StateEvent::RoomMember(member) => self.handle_membership(member).0, // finds all events related to the name of the room for later use StateEvent::RoomName(name) => self.handle_room_name(name), StateEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -889,55 +828,38 @@ impl Room { /// /// # Arguments /// - /// * `target_member` - The ID of the member to update. - /// * `event` - The profile update event for the specified room member. + /// * `event` - The profile update event for a specified room member. + #[instrument] pub fn update_member_profile( &mut self, - target_member: &UserId, event: &MemberEvent, ) -> (bool, HashMap) { - let old_name = self - .get_member(target_member) - .map_or_else(|| None, |m| m.display_name.clone()); + let user_id = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { + id + } else { + return (false, HashMap::new()); + }; + + let old_name = self.get_member(&user_id).map_or_else(|| None, |m| m.display_name.clone()); let new_name = event.content.displayname.clone(); - debug!( - "update_member_profile [{}]: from nick {:#?} to nick {:#?}", - self.room_id, old_name, &new_name - ); - - let disambiguations = - self.ambiguity_updates(target_member, old_name.clone(), new_name.clone()); + let disambiguations = self.disambiguation_updates(&user_id, old_name, new_name); for (id, is_ambiguous) in disambiguations.iter() { - if self.get_member_mut(id).is_none() { - error!("update_member_profile: I'm about to fail for id {}. user_id = {}\nevent = {:#?}", - id, - target_member, - event); - } else { - self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; - } + self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; } debug!( - "update_member_profile [{}]: disambiguations: {:#?}", - self.room_id, &disambiguations + "update_member_profile: disambiguations: {:#?}", + &disambiguations ); - let changed = match self.get_member_mut(target_member) { + let changed = match self.get_member_mut(&user_id) { Some(member) => { - member.display_name = new_name; + member.display_name = event.content.displayname.clone(); member.avatar_url = event.content.avatar_url.clone(); true } - None => { - error!( - "update_member_profile [{}]: user {} does not exist", - self.room_id, target_member - ); - - false - } + None => false, }; (changed, disambiguations) @@ -973,48 +895,6 @@ impl Room { } } -trait Describe { - fn describe(&self) -> String; -} - -impl Describe for MembershipState { - fn describe(&self) -> String { - use MembershipState::*; - - match self { - Ban => "is banned in", - Invite => "is invited to", - Join => "is a member of", - Knock => "is requesting access", - Leave => "left", - } - .to_string() - } -} - -impl Describe for MembershipChange { - fn describe(&self) -> String { - use MembershipChange::*; - - match self { - Invited => "got invited to", - Joined => "joined", - Kicked => "got kicked from", - Banned => "got banned from", - Unbanned => "got unbanned from", - KickedAndBanned => "got kicked and banned from", - InvitationRejected => "rejected the invitation to", - InvitationRevoked => "got their invitation revoked from", - Left => "left", - ProfileChanged => "changed their profile", - None => "did nothing in", - NotImplemented => "NOT IMPLEMENTED", - Error => "ERROR", - } - .to_string() - } -} - #[cfg(test)] mod test { use super::*; From ec81a5e539913c34fef2768d33a649311954974c Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Thu, 9 Jul 2020 10:09:57 +0200 Subject: [PATCH 22/28] Implement Room::member_is_tracked. --- matrix_sdk_base/src/models/room.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 0d27cf81..648a00cf 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -426,6 +426,13 @@ impl Room { (true, disambiguations) } + /// Check whether the user with the MXID `user_id` is joined or invited to the room. + /// + /// Returns true if so, false otherwise. + pub fn member_is_tracked(&self, user_id: &UserId) -> bool { + self.invited_members.contains_key(&user_id) || self.joined_members.contains_key(&user_id) + } + /// Get a room member by user ID. /// /// If there is no such member, returns `None`. From b16724841d02c6d32775f1c9f46e19d1e92ba4ea Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Thu, 9 Jul 2020 12:22:06 +0200 Subject: [PATCH 23/28] Correct state tracking of room members. - use `state_key` instead of `user_id` to determine which member is affected by the event - assign state directly from the event in `add_member` instead of using `membership_change` - expand/fix docstrings - add some logging --- matrix_sdk_base/src/client.rs | 4 +- matrix_sdk_base/src/models/room.rs | 208 +++++++++++++++++++---------- 2 files changed, 143 insertions(+), 69 deletions(-) diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 2e0bad13..613ad219 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -734,7 +734,7 @@ impl BaseClient { let mut room = room_lock.write().await; if let RoomEvent::RoomMember(mem_event) = &mut e { - let (changed, _) = room.handle_membership(mem_event); + let (changed, _) = room.handle_membership(mem_event, false); // The memberlist of the room changed, invalidate the group session // of the room. @@ -771,7 +771,7 @@ impl BaseClient { let mut room = room_lock.write().await; if let StateEvent::RoomMember(e) = event { - let (changed, _) = room.handle_membership(e); + let (changed, _) = room.handle_membership(e, true); // The memberlist of the room changed, invalidate the group session // of the room. diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 648a00cf..41379b71 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -18,7 +18,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; use serde::{Deserialize, Serialize}; -use tracing::{debug, instrument}; +use tracing::{trace, debug, error}; #[cfg(feature = "messages")] use super::message::MessageQueue; @@ -31,7 +31,7 @@ use crate::events::room::{ aliases::AliasesEvent, canonical_alias::CanonicalAliasEvent, encryption::EncryptionEvent, - member::{MemberEvent, MembershipChange}, + member::{MemberEvent, MembershipState, MembershipChange}, name::NameEvent, power_levels::{NotificationPowerLevels, PowerLevelsEvent, PowerLevelsEventContent}, tombstone::TombstoneEvent, @@ -345,41 +345,47 @@ impl Room { } } - /// Process the member event of an entering user. + /// Process the join or invite event for a new room member. + /// + /// This method should be called only on events which add new members, not on existing ones. /// /// Returns a tuple of: /// /// 1. True if the event made changes to the room's state, false otherwise. /// 2. Returns a map of display name disambiguations which tells us which members need to have /// their display names disambiguated and to what. - #[instrument] - fn add_member(&mut self, event: &MemberEvent) -> (bool, HashMap) { + /// + /// # Arguments + /// + /// * `target_member` - The ID of the member to add. + /// * `event` - The join or invite event for the specified room member. + fn add_member(&mut self, target_member: &UserId, event: &MemberEvent) -> (bool, HashMap) { let new_member = RoomMember::new(event); // Perform display name disambiguations, if necessary. - let disambiguations = self.disambiguation_updates(&new_member.user_id, None, new_member.display_name.clone()); + let disambiguations = self.disambiguation_updates(target_member, None, new_member.display_name.clone()); debug!( "add_member: disambiguations: {:#?}", disambiguations ); - match event.membership_change() { - MembershipChange::Joined => { + match event.content.membership { + MembershipState::Join => { // Since the member is now joined, he shouldn't be tracked as an invited member any // longer if he was previously tracked as such. - self.invited_members.remove(&new_member.user_id); + self.invited_members.remove(target_member); self.joined_members - .insert(new_member.user_id.clone(), new_member.clone()) + .insert(target_member.clone(), new_member.clone()) } - MembershipChange::Invited => self + MembershipState::Invite => self .invited_members - .insert(new_member.user_id.clone(), new_member.clone()), + .insert(target_member.clone(), new_member.clone()), _ => { - panic!("Room::add_member called on an event that is neither a join nor an invite.") + panic!("Room::add_member called on event that is neither `join` nor `invite`.") } }; @@ -390,24 +396,28 @@ impl Room { (true, disambiguations) } - /// Process the member event of a leaving user. + /// Process the leaving event for a room member. /// /// Returns a tuple of: /// /// 1. True if the event made changes to the room's state, false otherwise. /// 2. Returns a map of display name disambiguations which tells us which members need to have /// their display names disambiguated and to what. - #[instrument] - fn remove_member(&mut self, event: &MemberEvent) -> (bool, HashMap) { + /// + /// # Arguments + /// + /// * `target_member` - The ID of the member to remove. + /// * `event` - The leaving event for the specified room member. + fn remove_member(&mut self, target_member: &UserId, event: &MemberEvent) -> (bool, HashMap) { let leaving_member = RoomMember::new(event); - if self.get_member(&leaving_member.user_id).is_none() { + if self.get_member(target_member).is_none() { return (false, HashMap::new()); } // Perform display name disambiguations, if necessary. let disambiguations = - self.disambiguation_updates(&leaving_member.user_id, leaving_member.display_name.clone(), None); + self.disambiguation_updates(target_member, leaving_member.display_name.clone(), None); debug!( "remove_member: disambiguations: {:#?}", @@ -420,8 +430,7 @@ impl Room { // TODO: factor this out to a method called `remove_member` and rename this method // to something like `process_member_leaving_event`. - self.joined_members.remove(&leaving_member.user_id).or_else(|| - self.invited_members.remove(&leaving_member.user_id)); + self.joined_members.remove(target_member).or_else(|| self.invited_members.remove(target_member)); (true, disambiguations) } @@ -583,45 +592,95 @@ impl Room { pub fn handle_membership( &mut self, event: &MemberEvent, + state_event: bool ) -> (bool, HashMap) { + use MembershipState::*; use MembershipChange::*; - match event.membership_change() { - Invited | Joined => { - debug!( - "handle_membership: User {} entering room {} ({}) (joined or invited)", - event.state_key, - self.room_id, - self.display_name(), - ); + trace!("Received {} event: {}", if state_event { "state" } else { "timeline" }, + event.event_id); - self.add_member(event) + let target_user = match UserId::try_from(event.state_key.clone()) { + Ok(id) => id, + Err(e) => { + error!("Received a member event with invalid state_key: {}", e); + return (false, HashMap::new()); } - Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { - debug!( - "handle_membership: User {} exiting room {} ({}) (leaving, kicked, banned or invitation rejected)", - event.state_key, - self.room_id, - self.display_name(), - ); + }; - self.remove_member(event) + if state_event && !self.member_is_tracked(&target_user) { + match event.content.membership { + Join => { + debug!("handle_membership: User {} is now a member of the room {} ({})", + event.state_key, + self.room_id, + self.display_name(), + ); + + self.add_member(&target_user, event) + } + + Invite => { + debug!("handle_membership: User {} is now invited to the room {} ({})", + event.state_key, + self.room_id, + self.display_name(), + ); + + self.add_member(&target_user, event) + } + + // We are not interested in tracking past members for now + _ => (false, HashMap::new()), } - ProfileChanged => { - debug!( - "handle_membership: Profile changed for user {} in room {} ({}) (displayname={:?}, avatar={:?}", - event.state_key, - self.room_id, - self.display_name(), - event.content.displayname, - event.content.avatar_url - ); + } else { + match event.membership_change() { + Invited => { + debug!( + "handle_membership: User {} got invited to the room {} ({})", + event.state_key, + self.room_id, + self.display_name(), + ); - self.update_member_profile(event) + self.add_member(&target_user, event) + } + Joined => { + debug!( + "handle_membership: User {} joins the room {} ({})", + event.state_key, + self.room_id, + self.display_name(), + ); + + self.add_member(&target_user, event) + } + Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { + debug!( + "handle_membership: User {} exiting room {} ({}) (leaving, kicked, banned or invitation rejected)", + event.state_key, + self.room_id, + self.display_name(), + ); + + self.remove_member(&target_user, event) + } + ProfileChanged => { + debug!( + "handle_membership: Profile changed for user {} in room {} ({}) (displayname={:?}, avatar={:?}", + event.state_key, + self.room_id, + self.display_name(), + event.content.displayname, + event.content.avatar_url + ); + + self.update_member_profile(&target_user, event) + } + + // Not interested in other events. + _ => (false, HashMap::new()), } - - // Not interested in other events. - _ => (false, HashMap::new()), } } @@ -721,7 +780,7 @@ impl Room { pub fn receive_timeline_event(&mut self, event: &RoomEvent) -> bool { match event { // update to the current members of the room - RoomEvent::RoomMember(member) => self.handle_membership(member).0, + RoomEvent::RoomMember(member) => self.handle_membership(member, false).0, // finds all events related to the name of the room for later use RoomEvent::RoomName(name) => self.handle_room_name(name), RoomEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -746,7 +805,7 @@ impl Room { pub fn receive_state_event(&mut self, event: &StateEvent) -> bool { match event { // update to the current members of the room - StateEvent::RoomMember(member) => self.handle_membership(member).0, + StateEvent::RoomMember(member) => self.handle_membership(member, true).0, // finds all events related to the name of the room for later use StateEvent::RoomName(name) => self.handle_room_name(name), StateEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -835,38 +894,53 @@ impl Room { /// /// # Arguments /// - /// * `event` - The profile update event for a specified room member. - #[instrument] + /// * `target_member` - The ID of the member to update. + /// * `event` - The profile update event for the specified room member. pub fn update_member_profile( &mut self, + target_member: &UserId, event: &MemberEvent, ) -> (bool, HashMap) { - let user_id = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { - id - } else { - return (false, HashMap::new()); - }; - - let old_name = self.get_member(&user_id).map_or_else(|| None, |m| m.display_name.clone()); + let old_name = self.get_member(target_member).map_or_else(|| None, |m| m.display_name.clone()); let new_name = event.content.displayname.clone(); - let disambiguations = self.disambiguation_updates(&user_id, old_name, new_name); + debug!("update_member_profile [{}]: from nick {:#?} to nick {:#?}", + self.room_id, + old_name, &new_name); + + let disambiguations = self.disambiguation_updates(target_member, old_name.clone(), new_name.clone()); for (id, is_ambiguous) in disambiguations.iter() { - self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; + if self.get_member_mut(id).is_none() { + error!("update_member_profile: I'm about to fail for id {}. user_id = {}\nevent = {:#?}", + id, + target_member, + event); + } else { + self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; + } } debug!( - "update_member_profile: disambiguations: {:#?}", + "update_member_profile [{}]: disambiguations: {:#?}", + self.room_id, &disambiguations ); - let changed = match self.get_member_mut(&user_id) { + let changed = match self.get_member_mut(target_member) { Some(member) => { - member.display_name = event.content.displayname.clone(); + member.display_name = new_name; member.avatar_url = event.content.avatar_url.clone(); true } - None => false, + None => { + error!( + "update_member_profile [{}]: user {} does not exist", + self.room_id, + target_member + ); + + false + }, }; (changed, disambiguations) From 390a1aa12c7b461a2a7869e53e8876da7fa19d4d Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Thu, 9 Jul 2020 12:26:04 +0200 Subject: [PATCH 24/28] Clarify member_display_name docstring. --- matrix_sdk_base/src/models/room.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 41379b71..a96f5c84 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -321,8 +321,10 @@ impl Room { /// Get the disambiguated display name for a member of this room. /// - /// If a member has no display name set, returns the MXID as a fallback. Additionally, we - /// return the MXID even if there is no such member in the room. + /// If a member has no display name set, returns the MXID as a fallback. + /// + /// This method never fails, even if user with the supplied MXID is not a member in the room. + /// In this case we still return the supplied MXID as a fallback. /// /// When displaying a room member's display name, clients *must* use this method to obtain the /// name instead of displaying the `RoomMember::display_name` directly. This is because From a8f24da3ba9f6b09757aa18778eed23a3e986447 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Thu, 9 Jul 2020 14:47:57 +0200 Subject: [PATCH 25/28] cargo fmt --- matrix_sdk_base/src/models/room.rs | 107 +++++++++++++--------- matrix_sdk_base/src/models/room_member.rs | 10 +- 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index a96f5c84..9736ef0b 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -18,7 +18,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; use serde::{Deserialize, Serialize}; -use tracing::{trace, debug, error}; +use tracing::{debug, error, trace}; #[cfg(feature = "messages")] use super::message::MessageQueue; @@ -31,7 +31,7 @@ use crate::events::room::{ aliases::AliasesEvent, canonical_alias::CanonicalAliasEvent, encryption::EncryptionEvent, - member::{MemberEvent, MembershipState, MembershipChange}, + member::{MemberEvent, MembershipChange, MembershipState}, name::NameEvent, power_levels::{NotificationPowerLevels, PowerLevelsEvent, PowerLevelsEventContent}, tombstone::TombstoneEvent, @@ -340,7 +340,7 @@ impl Room { } else { member.name().into() } - }, + } // Even if there is no such member, we return the MXID that was given to us. None => id.as_ref().into(), @@ -361,16 +361,18 @@ impl Room { /// /// * `target_member` - The ID of the member to add. /// * `event` - The join or invite event for the specified room member. - fn add_member(&mut self, target_member: &UserId, event: &MemberEvent) -> (bool, HashMap) { + fn add_member( + &mut self, + target_member: &UserId, + event: &MemberEvent, + ) -> (bool, HashMap) { let new_member = RoomMember::new(event); // Perform display name disambiguations, if necessary. - let disambiguations = self.disambiguation_updates(target_member, None, new_member.display_name.clone()); + let disambiguations = + self.disambiguation_updates(target_member, None, new_member.display_name.clone()); - debug!( - "add_member: disambiguations: {:#?}", - disambiguations - ); + debug!("add_member: disambiguations: {:#?}", disambiguations); match event.content.membership { MembershipState::Join => { @@ -386,9 +388,7 @@ impl Room { .invited_members .insert(target_member.clone(), new_member.clone()), - _ => { - panic!("Room::add_member called on event that is neither `join` nor `invite`.") - } + _ => panic!("Room::add_member called on event that is neither `join` nor `invite`."), }; for (id, is_ambiguous) in disambiguations.iter() { @@ -410,7 +410,11 @@ impl Room { /// /// * `target_member` - The ID of the member to remove. /// * `event` - The leaving event for the specified room member. - fn remove_member(&mut self, target_member: &UserId, event: &MemberEvent) -> (bool, HashMap) { + fn remove_member( + &mut self, + target_member: &UserId, + event: &MemberEvent, + ) -> (bool, HashMap) { let leaving_member = RoomMember::new(event); if self.get_member(target_member).is_none() { @@ -421,10 +425,7 @@ impl Room { let disambiguations = self.disambiguation_updates(target_member, leaving_member.display_name.clone(), None); - debug!( - "remove_member: disambiguations: {:#?}", - disambiguations - ); + debug!("remove_member: disambiguations: {:#?}", disambiguations); for (id, is_ambiguous) in disambiguations.iter() { self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; @@ -432,7 +433,9 @@ impl Room { // TODO: factor this out to a method called `remove_member` and rename this method // to something like `process_member_leaving_event`. - self.joined_members.remove(target_member).or_else(|| self.invited_members.remove(target_member)); + self.joined_members + .remove(target_member) + .or_else(|| self.invited_members.remove(target_member)); (true, disambiguations) } @@ -459,7 +462,7 @@ impl Room { pub fn get_member_mut(&mut self, user_id: &UserId) -> Option<&mut RoomMember> { match self.joined_members.get_mut(user_id) { None => self.invited_members.get_mut(user_id), - Some(m) => Some(m) + Some(m) => Some(m), } } @@ -472,7 +475,13 @@ impl Room { // Find all other users that share the display name with the joining user. members - .filter(|(_, member)| member.display_name.as_ref().map(|other_name| other_name == name).unwrap_or(false)) + .filter(|(_, member)| { + member + .display_name + .as_ref() + .map(|other_name| other_name == name) + .unwrap_or(false) + }) .map(|(_, member)| member.user_id.clone()) .collect() } @@ -501,7 +510,7 @@ impl Room { n if n > 1 => vec![(member.clone(), false)].into_iter().collect(), 1 => old_name_eq_class.into_iter().map(|m| (m, false)).collect(), 0 => HashMap::new(), - _ => panic!("impossible") + _ => panic!("impossible"), }; // @@ -519,7 +528,10 @@ impl Room { _ => vec![(member.clone(), true)].into_iter().collect(), }; - disambiguate_old.into_iter().chain(disambiguate_new.into_iter()).collect() + disambiguate_old + .into_iter() + .chain(disambiguate_new.into_iter()) + .collect() } /// Add to the list of `RoomAliasId`s. @@ -594,13 +606,16 @@ impl Room { pub fn handle_membership( &mut self, event: &MemberEvent, - state_event: bool + state_event: bool, ) -> (bool, HashMap) { - use MembershipState::*; use MembershipChange::*; + use MembershipState::*; - trace!("Received {} event: {}", if state_event { "state" } else { "timeline" }, - event.event_id); + trace!( + "Received {} event: {}", + if state_event { "state" } else { "timeline" }, + event.event_id + ); let target_user = match UserId::try_from(event.state_key.clone()) { Ok(id) => id, @@ -613,20 +628,22 @@ impl Room { if state_event && !self.member_is_tracked(&target_user) { match event.content.membership { Join => { - debug!("handle_membership: User {} is now a member of the room {} ({})", - event.state_key, - self.room_id, - self.display_name(), + debug!( + "handle_membership: User {} is now a member of the room {} ({})", + event.state_key, + self.room_id, + self.display_name(), ); self.add_member(&target_user, event) } Invite => { - debug!("handle_membership: User {} is now invited to the room {} ({})", - event.state_key, - self.room_id, - self.display_name(), + debug!( + "handle_membership: User {} is now invited to the room {} ({})", + event.state_key, + self.room_id, + self.display_name(), ); self.add_member(&target_user, event) @@ -903,14 +920,18 @@ impl Room { target_member: &UserId, event: &MemberEvent, ) -> (bool, HashMap) { - let old_name = self.get_member(target_member).map_or_else(|| None, |m| m.display_name.clone()); + let old_name = self + .get_member(target_member) + .map_or_else(|| None, |m| m.display_name.clone()); let new_name = event.content.displayname.clone(); - debug!("update_member_profile [{}]: from nick {:#?} to nick {:#?}", - self.room_id, - old_name, &new_name); + debug!( + "update_member_profile [{}]: from nick {:#?} to nick {:#?}", + self.room_id, old_name, &new_name + ); - let disambiguations = self.disambiguation_updates(target_member, old_name.clone(), new_name.clone()); + let disambiguations = + self.disambiguation_updates(target_member, old_name.clone(), new_name.clone()); for (id, is_ambiguous) in disambiguations.iter() { if self.get_member_mut(id).is_none() { error!("update_member_profile: I'm about to fail for id {}. user_id = {}\nevent = {:#?}", @@ -924,8 +945,7 @@ impl Room { debug!( "update_member_profile [{}]: disambiguations: {:#?}", - self.room_id, - &disambiguations + self.room_id, &disambiguations ); let changed = match self.get_member_mut(target_member) { @@ -937,12 +957,11 @@ impl Room { None => { error!( "update_member_profile [{}]: user {} does not exist", - self.room_id, - target_member + self.room_id, target_member ); false - }, + } }; (changed, disambiguations) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 98fc4fd7..f508a2b1 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -194,7 +194,10 @@ mod test { .add_room_event(EventsJson::MemberNameChange, RoomEvent::RoomMember) .build_sync_response(); - client.receive_sync_response(&mut initial_response).await.unwrap(); + client + .receive_sync_response(&mut initial_response) + .await + .unwrap(); let room = client.get_joined_room(&room_id).await.unwrap(); @@ -210,7 +213,10 @@ mod test { assert_eq!(member.display_name.as_ref().unwrap(), "example"); } - client.receive_sync_response(&mut name_change_response).await.unwrap(); + client + .receive_sync_response(&mut name_change_response) + .await + .unwrap(); // Afterwards, the display name is "changed". { From 4134ba969a3ea8b4e00b61313edfbf5e51f0ff8c Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Thu, 9 Jul 2020 15:09:55 +0200 Subject: [PATCH 26/28] DRY the membership logging a bit. --- matrix_sdk_base/src/models/room.rs | 121 +++++++++++++++-------------- 1 file changed, 62 insertions(+), 59 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 9736ef0b..d6e9c666 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -626,76 +626,37 @@ impl Room { }; if state_event && !self.member_is_tracked(&target_user) { + debug!( + "handle_membership: User {user_id} is {state} the room {room_id} ({room_name})", + user_id = target_user, + state = event.content.membership.describe(), + room_id = self.room_id, + room_name = self.display_name(), + ); + match event.content.membership { - Join => { - debug!( - "handle_membership: User {} is now a member of the room {} ({})", - event.state_key, - self.room_id, - self.display_name(), - ); - - self.add_member(&target_user, event) - } - - Invite => { - debug!( - "handle_membership: User {} is now invited to the room {} ({})", - event.state_key, - self.room_id, - self.display_name(), - ); - - self.add_member(&target_user, event) - } + Join | Invite => self.add_member(&target_user, event), // We are not interested in tracking past members for now _ => (false, HashMap::new()), } } else { - match event.membership_change() { - Invited => { - debug!( - "handle_membership: User {} got invited to the room {} ({})", - event.state_key, - self.room_id, - self.display_name(), - ); + let change = event.membership_change(); - self.add_member(&target_user, event) - } - Joined => { - debug!( - "handle_membership: User {} joins the room {} ({})", - event.state_key, - self.room_id, - self.display_name(), - ); + debug!( + "handle_membership: User {user_id} {action} the room {room_id} ({room_name})", + user_id = target_user, + action = change.describe(), + room_id = self.room_id, + room_name = self.display_name(), + ); - self.add_member(&target_user, event) - } + match change { + Invited | Joined => self.add_member(&target_user, event), Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { - debug!( - "handle_membership: User {} exiting room {} ({}) (leaving, kicked, banned or invitation rejected)", - event.state_key, - self.room_id, - self.display_name(), - ); - self.remove_member(&target_user, event) } - ProfileChanged => { - debug!( - "handle_membership: Profile changed for user {} in room {} ({}) (displayname={:?}, avatar={:?}", - event.state_key, - self.room_id, - self.display_name(), - event.content.displayname, - event.content.avatar_url - ); - - self.update_member_profile(&target_user, event) - } + ProfileChanged => self.update_member_profile(&target_user, event), // Not interested in other events. _ => (false, HashMap::new()), @@ -997,6 +958,48 @@ impl Room { } } +trait Describe { + fn describe(&self) -> String; +} + +impl Describe for MembershipState { + fn describe(&self) -> String { + use MembershipState::*; + + match self { + Ban => "is banned in", + Invite => "is invited to", + Join => "is a member of", + Knock => "is requesting access", + Leave => "left", + } + .to_string() + } +} + +impl Describe for MembershipChange { + fn describe(&self) -> String { + use MembershipChange::*; + + match self { + Invited => "got invited to", + Joined => "joined", + Kicked => "got kicked from", + Banned => "got banned from", + Unbanned => "got unbanned from", + KickedAndBanned => "got kicked and banned from", + InvitationRejected => "rejected the invitation to", + InvitationRevoked => "got their invitation revoked from", + Left => "left", + ProfileChanged => "changed their profile", + None => "did nothing in", + NotImplemented => "NOT IMPLEMENTED", + Error => "ERROR", + } + .to_string() + } +} + #[cfg(test)] mod test { use super::*; From 8daa12ac56cdb092f53496d49907019606b268ac Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 10 Jul 2020 10:21:41 +0200 Subject: [PATCH 27/28] Print error when receiving invalid response in sync_forever. --- matrix_sdk/src/client.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index be083ad1..f06663ae 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -30,8 +30,7 @@ use matrix_sdk_common::uuid::Uuid; use futures_timer::Delay as sleep; use std::future::Future; #[cfg(feature = "encryption")] -use tracing::{debug, warn}; -use tracing::{info, instrument, trace}; +use tracing::{debug, error, info, instrument, trace, warn}; use http::Method as HttpMethod; use http::Response as HttpResponse; @@ -1294,6 +1293,9 @@ impl Client { let response = if let Ok(r) = response { r } else { + let err = response.unwrap_err(); + error!("Received an invalid response: {}", err); + sleep::new(Duration::from_secs(1)).await; continue; From 05a41d3b4dd0d2170a3dd0aac27cd20e10c05ab9 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Fri, 10 Jul 2020 15:47:11 +0200 Subject: [PATCH 28/28] Move and rename member_display_name to RoomMember::disambiguated_name. This makes more sense as all the required information is now available on `RoomMember`. We also don't have to handle the case of the missing member since now you have to actually get a `RoomMember` before you can ask for their name. --- matrix_sdk_base/src/models/room.rs | 51 +++++------------------ matrix_sdk_base/src/models/room_member.rs | 32 ++++++++++++-- 2 files changed, 39 insertions(+), 44 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index d6e9c666..d2adbe79 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -13,7 +13,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::borrow::Cow; use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; @@ -319,34 +318,6 @@ impl Room { self.encrypted.as_ref() } - /// Get the disambiguated display name for a member of this room. - /// - /// If a member has no display name set, returns the MXID as a fallback. - /// - /// This method never fails, even if user with the supplied MXID is not a member in the room. - /// In this case we still return the supplied MXID as a fallback. - /// - /// When displaying a room member's display name, clients *must* use this method to obtain the - /// name instead of displaying the `RoomMember::display_name` directly. This is because - /// multiple members can share the same display name in which case the display name has to be - /// disambiguated. - pub fn member_display_name<'a>(&'a self, id: &'a UserId) -> Cow<'a, str> { - let member = self.get_member(id); - - match member { - Some(member) => { - if member.display_name_ambiguous { - member.unique_name().into() - } else { - member.name().into() - } - } - - // Even if there is no such member, we return the MXID that was given to us. - None => id.as_ref().into(), - } - } - /// Process the join or invite event for a new room member. /// /// This method should be called only on events which add new members, not on existing ones. @@ -1347,7 +1318,7 @@ mod test { { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - let display_name1 = room.member_display_name(&user_id1); + let display_name1 = room.get_member(&user_id1).unwrap().disambiguated_name(); assert_eq!("example", display_name1); } @@ -1366,9 +1337,9 @@ mod test { { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - let display_name1 = room.member_display_name(&user_id1); - let display_name2 = room.member_display_name(&user_id2); - let display_name3 = room.member_display_name(&user_id3); + let display_name1 = room.get_member(&user_id1).unwrap().disambiguated_name(); + let display_name2 = room.get_member(&user_id2).unwrap().disambiguated_name(); + let display_name3 = room.get_member(&user_id3).unwrap().disambiguated_name(); assert_eq!(format!("example ({})", user_id1), display_name1); assert_eq!(format!("example ({})", user_id2), display_name2); @@ -1385,7 +1356,7 @@ mod test { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - let display_name1 = room.member_display_name(&user_id1); + let display_name1 = room.get_member(&user_id1).unwrap().disambiguated_name(); assert_eq!("example", display_name1); } @@ -1401,8 +1372,8 @@ mod test { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - let display_name1 = room.member_display_name(&user_id1); - let display_name2 = room.member_display_name(&user_id2); + let display_name1 = room.get_member(&user_id1).unwrap().disambiguated_name(); + let display_name2 = room.get_member(&user_id2).unwrap().disambiguated_name(); assert_eq!(format!("example ({})", user_id1), display_name1); assert_eq!(format!("example ({})", user_id2), display_name2); @@ -1419,8 +1390,8 @@ mod test { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - let display_name1 = room.member_display_name(&user_id1); - let display_name2 = room.member_display_name(&user_id2); + let display_name1 = room.get_member(&user_id1).unwrap().disambiguated_name(); + let display_name2 = room.get_member(&user_id2).unwrap().disambiguated_name(); assert_eq!("changed", display_name1); assert_eq!("example", display_name2); @@ -1437,8 +1408,8 @@ mod test { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - let display_name1 = room.member_display_name(&user_id1); - let display_name2 = room.member_display_name(&user_id2); + let display_name1 = room.get_member(&user_id1).unwrap().disambiguated_name(); + let display_name2 = room.get_member(&user_id2).unwrap().disambiguated_name(); assert_eq!(format!("changed ({})", user_id1), display_name1); assert_eq!(format!("changed ({})", user_id2), display_name2); diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index f508a2b1..1be90dfb 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -105,7 +105,8 @@ impl RoomMember { } } - /// Returns the most ergonomic name available for the member. + /// Returns the most ergonomic (but potentially ambiguous/non-unique) name available for the + /// member. /// /// This is the member's display name if it is set, otherwise their MXID. pub fn name(&self) -> String { @@ -114,16 +115,39 @@ impl RoomMember { .unwrap_or_else(|| format!("{}", self.user_id)) } - /// Returns a name for the member which is guaranteed to be unique. + /// Returns a name for the member which is guaranteed to be unique, but not necessarily the + /// most ergonomic. /// - /// This is either of the format "DISPLAY_NAME (MXID)" if the display name is set for the - /// member, or simply "MXID" if not. + /// This is either a name in the format "DISPLAY_NAME (MXID)" if the member's display name is + /// set, or simply "MXID" if not. pub fn unique_name(&self) -> String { self.display_name .clone() .map(|d| format!("{} ({})", d, self.user_id)) .unwrap_or_else(|| format!("{}", self.user_id)) } + + /// Get the disambiguated display name for the member which is as ergonomic as possible while + /// still guaranteeing it is unique. + /// + /// If the member's display name is currently ambiguous (i.e. shared by other room members), + /// this method will return the same result as `RoomMember::unique_name`. Otherwise, this + /// method will return the same result as `RoomMember::name`. + /// + /// This is usually the name you want when showing room messages from the member or when + /// showing the member in the member list. + /// + /// **Warning**: When displaying a room member's display name, clients *must* use + /// a disambiguated name, so they *must not* use `RoomMember::display_name` directly. Clients + /// *should* use this method to obtain the name, but an acceptable alternative is to use + /// `RoomMember::unique_name` in certain situations. + pub fn disambiguated_name(&self) -> String { + if self.display_name_ambiguous { + self.unique_name().into() + } else { + self.name().into() + } + } } #[cfg(test)]