From d5f66631c1ceddd5dcdfe94e0b54451c42fe4d33 Mon Sep 17 00:00:00 2001 From: Valentin Brandl Date: Wed, 13 May 2020 16:18:07 +0200 Subject: [PATCH 01/32] Implement display name resolving --- matrix_sdk_base/src/models/room.rs | 65 ++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index a2867b89..2067676b 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -180,6 +180,8 @@ pub struct Room { pub unread_notifications: Option, /// The tombstone state of this room. pub tombstone: Option, + /// The map of display names + display_names: HashMap, } impl RoomName { @@ -278,6 +280,7 @@ impl Room { unread_highlight: None, unread_notifications: None, tombstone: None, + display_names: HashMap::new(), } } @@ -298,6 +301,11 @@ impl Room { self.encrypted.as_ref() } + /// Get the resolved display name for a member of this room. + pub fn resolved_display_name(&self, id: &UserId) -> Option<&str> { + self.display_names.get(id).map(|s| s.as_str()) + } + fn add_member(&mut self, event: &MemberEvent) -> bool { if self .members @@ -308,6 +316,63 @@ impl Room { let member = RoomMember::new(event); + // find all users that share the same display name as the joining user + let users_with_same_name: Vec<_> = self + .display_names + .iter() + .filter(|(_, v)| { + member + .display_name + .as_ref() + .map(|n| &n == v) + .unwrap_or(false) + }) + .map(|(k, _)| k) + .cloned() + .collect(); + + // if there is no other user with the same display name -> just use the display name + if users_with_same_name.is_empty() { + let display_name = member + .display_name + .as_ref() + .map(|s| s.to_string()) + .unwrap_or_else(|| format!("{}", member.user_id)); + self.display_names + .insert(member.user_id.clone(), display_name); + } else { + // else user `display_name (userid)` + let users_with_same_name = users_with_same_name + .into_iter() + .filter_map(|id| { + self.members + .get(&id) + .map(|m| { + m.display_name + .as_ref() + .map(|d| format!("{} ({})", d, m.user_id)) + .unwrap_or_else(|| format!("{}", m.user_id)) + }) + .map(|m| (id, m)) + }) + .collect::>(); + + // update all existing users with same name + for (id, member) in users_with_same_name { + self.display_names.insert(id, member); + } + + // insert new member's display name + self.display_names.insert( + member.user_id.clone(), + member + .display_name + .as_ref() + .map(|n| format!("{} ({})", n, member.user_id)) + .unwrap_or_else(|| format!("{}", member.user_id)), + ); + } + self.members .insert(UserId::try_from(event.state_key.as_str()).unwrap(), member); From 4675a72e6b88063cdbb7fc6b8e3a206f45e189ff Mon Sep 17 00:00:00 2001 From: Valentin Brandl Date: Thu, 14 May 2020 14:53:11 +0200 Subject: [PATCH 02/32] Rename accessor for display name --- 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 2067676b..67c97229 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -302,7 +302,7 @@ impl Room { } /// Get the resolved display name for a member of this room. - pub fn resolved_display_name(&self, id: &UserId) -> Option<&str> { + pub fn member_display_name(&self, id: &UserId) -> Option<&str> { self.display_names.get(id).map(|s| s.as_str()) } From 49e913865d05766cb0d6f49601f36b939609cf91 Mon Sep 17 00:00:00 2001 From: Valentin Brandl Date: Thu, 14 May 2020 15:05:06 +0200 Subject: [PATCH 03/32] Fix failing test --- matrix_sdk_base/src/state/mod.rs | 96 ++++++++++++++++---------------- 1 file changed, 49 insertions(+), 47 deletions(-) diff --git a/matrix_sdk_base/src/state/mod.rs b/matrix_sdk_base/src/state/mod.rs index abf3ed82..dfcf71fd 100644 --- a/matrix_sdk_base/src/state/mod.rs +++ b/matrix_sdk_base/src/state/mod.rs @@ -140,57 +140,59 @@ mod test { #[cfg(not(feature = "messages"))] assert_eq!( - r#"{ - "!roomid:example.com": { - "room_id": "!roomid:example.com", - "room_name": { - "name": null, - "canonical_alias": null, - "aliases": [], - "heroes": [], - "joined_member_count": null, - "invited_member_count": null - }, - "own_user_id": "@example:example.com", - "creator": null, - "members": {}, - "typing_users": [], - "power_levels": null, - "encrypted": null, - "unread_highlight": null, - "unread_notifications": null, - "tombstone": null - } -}"#, - serde_json::to_string_pretty(&joined_rooms).unwrap() + serde_json::json!({ + "!roomid:example.com": { + "room_id": "!roomid:example.com", + "room_name": { + "name": null, + "canonical_alias": null, + "aliases": [], + "heroes": [], + "joined_member_count": null, + "invited_member_count": null + }, + "own_user_id": "@example:example.com", + "creator": null, + "members": {}, + "display_names": {}, + "typing_users": [], + "power_levels": null, + "encrypted": null, + "unread_highlight": null, + "unread_notifications": null, + "tombstone": null + } + }), + serde_json::to_value(&joined_rooms).unwrap() ); #[cfg(feature = "messages")] assert_eq!( - r#"{ - "!roomid:example.com": { - "room_id": "!roomid:example.com", - "room_name": { - "name": null, - "canonical_alias": null, - "aliases": [], - "heroes": [], - "joined_member_count": null, - "invited_member_count": null - }, - "own_user_id": "@example:example.com", - "creator": null, - "members": {}, - "messages": [], - "typing_users": [], - "power_levels": null, - "encrypted": null, - "unread_highlight": null, - "unread_notifications": null, - "tombstone": null - } -}"#, - serde_json::to_string_pretty(&joined_rooms).unwrap() + serde_json::json!({ + "!roomid:example.com": { + "room_id": "!roomid:example.com", + "display_names": {}, + "room_name": { + "name": null, + "canonical_alias": null, + "aliases": [], + "heroes": [], + "joined_member_count": null, + "invited_member_count": null + }, + "own_user_id": "@example:example.com", + "creator": null, + "members": {}, + "messages": [], + "typing_users": [], + "power_levels": null, + "encrypted": null, + "unread_highlight": null, + "unread_notifications": null, + "tombstone": null + } + }), + serde_json::to_value(&joined_rooms).unwrap() ); } From 05503b28b77b447a4be8a93db53ed61cae06b454 Mon Sep 17 00:00:00 2001 From: Valentin Brandl Date: Thu, 14 May 2020 17:09:18 +0200 Subject: [PATCH 04/32] Only add name duplicates to the display name map --- matrix_sdk_base/src/models/room.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 67c97229..eb914347 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -13,6 +13,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::borrow::Cow; use std::collections::{BTreeMap, HashMap}; use std::convert::TryFrom; @@ -302,8 +303,20 @@ impl Room { } /// Get the resolved display name for a member of this room. - pub fn member_display_name(&self, id: &UserId) -> Option<&str> { - self.display_names.get(id).map(|s| s.as_str()) + pub fn member_display_name<'a>(&'a self, id: &UserId) -> Option> { + self.display_names + .get(id) + .map(|s| s.as_str().into()) + .or_else(|| { + self.members.get(id).map(|member| { + member + .display_name + .as_ref() + .map(|s| s.to_string()) + .unwrap_or_else(|| format!("{}", member.user_id)) + .into() + }) + }) } fn add_member(&mut self, event: &MemberEvent) -> bool { @@ -331,17 +344,8 @@ impl Room { .cloned() .collect(); - // if there is no other user with the same display name -> just use the display name - if users_with_same_name.is_empty() { - let display_name = member - .display_name - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_else(|| format!("{}", member.user_id)); - self.display_names - .insert(member.user_id.clone(), display_name); - } else { - // else user `display_name (userid)` + // if there is a other user with the same display name -> use `display_name (userid)` + if !users_with_same_name.is_empty() { let users_with_same_name = users_with_same_name .into_iter() .filter_map(|id| { From 9f34615869a042662b461edd22bb634e8d3083a0 Mon Sep 17 00:00:00 2001 From: Valentin Brandl Date: Thu, 14 May 2020 17:09:30 +0200 Subject: [PATCH 05/32] Add first test for display names --- matrix_sdk_base/src/models/room.rs | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index eb914347..e163c9ff 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -699,6 +699,48 @@ mod test { assert!(room.deref().power_levels.is_some()) } + #[async_test] + async fn test_member_display_name() { + let client = get_client(); + + let client_2 = { + let session = Session { + access_token: "1234".to_owned(), + user_id: UserId::try_from("@example:hostlocal").unwrap(), + device_id: "DEVICEID".to_owned(), + }; + BaseClient::new(Some(session)).unwrap() + }; + let room_id = get_room_id(); + let user_id = UserId::try_from("@example:localhost").unwrap(); + + let mut response = EventBuilder::default() + .add_room_event(EventsFile::Member, RoomEvent::RoomMember) + .build_sync_response(); + + client.receive_sync_response(&mut response).await.unwrap(); + + let room = client.get_joined_room(&room_id).await.unwrap(); + let room = room.read().await; + + let display_name = room.member_display_name(&user_id).unwrap(); + assert_eq!("example", display_name); + + let mut response = EventBuilder::default() + .add_room_event(EventsFile::Member, RoomEvent::RoomMember) + .build_sync_response(); + + client_2.receive_sync_response(&mut response).await.unwrap(); + + let room_2 = client_2.get_joined_room(&room_id).await.unwrap(); + let room_2 = room_2.read().await; + + let display_name_2 = room_2.member_display_name(&user_id).unwrap(); + + // TODO: this should change + assert_eq!("example", display_name_2); + } + #[async_test] async fn room_events() { let client = get_client().await; From e3cb3566bf8a40fba9ef48f9c73f2db819549534 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 11:10:09 +0200 Subject: [PATCH 06/32] Rename display_names -> disambiguated_display_names. --- matrix_sdk_base/src/models/message.rs | 2 ++ matrix_sdk_base/src/models/room.rs | 14 +++++++------- matrix_sdk_base/src/state/mod.rs | 4 ++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/matrix_sdk_base/src/models/message.rs b/matrix_sdk_base/src/models/message.rs index 215293ee..cc07ce59 100644 --- a/matrix_sdk_base/src/models/message.rs +++ b/matrix_sdk_base/src/models/message.rs @@ -176,6 +176,7 @@ mod test { serde_json::json!({ "!roomid:example.com": { "room_id": "!roomid:example.com", + "disambiguated_display_names": {}, "room_name": { "name": null, "canonical_alias": null, @@ -225,6 +226,7 @@ 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 e163c9ff..5757350d 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -181,8 +181,8 @@ pub struct Room { pub unread_notifications: Option, /// The tombstone state of this room. pub tombstone: Option, - /// The map of display names - display_names: HashMap, + /// The map of disambiguated display names for users who have the same display name + disambiguated_display_names: HashMap, } impl RoomName { @@ -281,7 +281,7 @@ impl Room { unread_highlight: None, unread_notifications: None, tombstone: None, - display_names: HashMap::new(), + disambiguated_display_names: HashMap::new(), } } @@ -304,7 +304,7 @@ impl Room { /// Get the resolved display name for a member of this room. pub fn member_display_name<'a>(&'a self, id: &UserId) -> Option> { - self.display_names + self.disambiguated_display_names .get(id) .map(|s| s.as_str().into()) .or_else(|| { @@ -331,7 +331,7 @@ impl Room { // find all users that share the same display name as the joining user let users_with_same_name: Vec<_> = self - .display_names + .disambiguated_display_names .iter() .filter(|(_, v)| { member @@ -363,11 +363,11 @@ impl Room { // update all existing users with same name for (id, member) in users_with_same_name { - self.display_names.insert(id, member); + self.disambiguated_display_names.insert(id, member); } // insert new member's display name - self.display_names.insert( + self.disambiguated_display_names.insert( member.user_id.clone(), member .display_name diff --git a/matrix_sdk_base/src/state/mod.rs b/matrix_sdk_base/src/state/mod.rs index dfcf71fd..cb2ec449 100644 --- a/matrix_sdk_base/src/state/mod.rs +++ b/matrix_sdk_base/src/state/mod.rs @@ -154,7 +154,7 @@ mod test { "own_user_id": "@example:example.com", "creator": null, "members": {}, - "display_names": {}, + "disambiguated_display_names": {}, "typing_users": [], "power_levels": null, "encrypted": null, @@ -171,7 +171,7 @@ mod test { serde_json::json!({ "!roomid:example.com": { "room_id": "!roomid:example.com", - "display_names": {}, + "disambiguated_display_names": {}, "room_name": { "name": null, "canonical_alias": null, From 4df0a839aac8dd5272c9d931a30aeaa9c153b6a1 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 11:34:16 +0200 Subject: [PATCH 07/32] Fix Markdown in doc comment. --- matrix_sdk_test/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/matrix_sdk_test/src/lib.rs b/matrix_sdk_test/src/lib.rs index 41b43ec4..c7294cfa 100644 --- a/matrix_sdk_test/src/lib.rs +++ b/matrix_sdk_test/src/lib.rs @@ -216,7 +216,7 @@ impl EventBuilder { self } - /// Consumes `ResponseBuilder and returns SyncResponse. + /// Consumes `ResponseBuilder` and returns `SyncResponse`. pub fn build_sync_response(mut self) -> SyncResponse { let main_room_id = RoomId::try_from("!SVkFJHzfwvuaIEawgC:localhost").unwrap(); From b6d7939685f62f62bc85f0cb2a487f1d56f0ab2e Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 15:10:41 +0200 Subject: [PATCH 08/32] matrix-sdk: Vary sync token with each EventBuilder::build_sync_response call. This allows us to hold onto an EventBuilder object and use it to build multiple sync responses. Previously this would have resulted in each of the responses having the same next_batch sync token. This would make clients ignore the latter responses if they have already received any of the previous ones. --- matrix_sdk_test/src/lib.rs | 56 ++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/matrix_sdk_test/src/lib.rs b/matrix_sdk_test/src/lib.rs index c7294cfa..85180777 100644 --- a/matrix_sdk_test/src/lib.rs +++ b/matrix_sdk_test/src/lib.rs @@ -61,15 +61,22 @@ pub struct EventBuilder { ephemeral: Vec, /// The account data events that determine the state of a `Room`. account_data: Vec, + /// Internal counter to enable the `prev_batch` and `next_batch` of each sync response to vary. + batch_counter: i64, } impl EventBuilder { + pub fn new() -> Self { + let builder: EventBuilder = Default::default(); + builder + } + /// Add an event to the room events `Vec`. pub fn add_ephemeral( - mut self, + &mut self, file: EventsFile, variant: fn(Ev) -> Event, - ) -> Self { + ) -> &mut Self { let val: &str = match file { EventsFile::Typing => include_str!("../test_data/events/typing.json"), _ => panic!("unknown ephemeral event file {:?}", file), @@ -86,10 +93,10 @@ impl EventBuilder { /// Add an event to the room events `Vec`. #[allow(clippy::match_single_binding, unused)] pub fn add_account( - mut self, + &mut self, file: EventsFile, variant: fn(Ev) -> Event, - ) -> Self { + ) -> &mut Self { let val: &str = match file { _ => panic!("unknown account event file {:?}", file), }; @@ -104,10 +111,10 @@ impl EventBuilder { /// Add an event to the room events `Vec`. pub fn add_room_event( - mut self, + &mut self, file: EventsFile, variant: fn(Ev) -> RoomEvent, - ) -> Self { + ) -> &mut Self { let val = match file { EventsFile::Member => include_str!("../test_data/events/member.json"), EventsFile::PowerLevels => include_str!("../test_data/events/power_levels.json"), @@ -126,11 +133,11 @@ impl EventBuilder { } pub fn add_custom_joined_event( - mut self, + &mut self, room_id: &RoomId, event: serde_json::Value, variant: fn(Ev) -> RoomEvent, - ) -> Self { + ) -> &mut Self { let event = serde_json::from_value::>(event) .unwrap() .deserialize() @@ -147,11 +154,11 @@ impl EventBuilder { } pub fn add_custom_invited_event( - mut self, + &mut self, room_id: &RoomId, event: serde_json::Value, variant: fn(Ev) -> AnyStrippedStateEvent, - ) -> Self { + ) -> &mut Self { let event = serde_json::from_value::>(event) .unwrap() .deserialize() @@ -164,11 +171,11 @@ impl EventBuilder { } pub fn add_custom_left_event( - mut self, + &mut self, room_id: &RoomId, event: serde_json::Value, variant: fn(Ev) -> RoomEvent, - ) -> Self { + ) -> &mut Self { let event = serde_json::from_value::>(event) .unwrap() .deserialize() @@ -182,10 +189,10 @@ impl EventBuilder { /// Add a state event to the state events `Vec`. pub fn add_state_event( - mut self, + &mut self, file: EventsFile, variant: fn(Ev) -> StateEvent, - ) -> Self { + ) -> &mut Self { let val = match file { EventsFile::Alias => include_str!("../test_data/events/alias.json"), EventsFile::Aliases => include_str!("../test_data/events/aliases.json"), @@ -202,7 +209,7 @@ impl EventBuilder { } /// Add an presence event to the presence events `Vec`. - pub fn add_presence_event(mut self, file: EventsFile) -> Self { + pub fn add_presence_event(&mut self, file: EventsFile) -> &mut Self { let val = match file { EventsFile::Presence => include_str!("../test_data/events/presence.json"), _ => panic!("unknown presence event file {:?}", file), @@ -217,9 +224,14 @@ impl EventBuilder { } /// Consumes `ResponseBuilder` and returns `SyncResponse`. - pub fn build_sync_response(mut self) -> SyncResponse { + pub fn build_sync_response(&mut self) -> SyncResponse { let main_room_id = RoomId::try_from("!SVkFJHzfwvuaIEawgC:localhost").unwrap(); + // First time building a sync response, so initialize the `prev_batch` to a default one. + let prev_batch = self.generate_sync_token(); + self.batch_counter += 1; + let next_batch = self.generate_sync_token(); + // TODO generalize this. let joined_room = serde_json::json!({ "summary": {}, @@ -235,7 +247,7 @@ impl EventBuilder { "timeline": { "events": self.joined_room_events.remove(&main_room_id).unwrap_or_default(), "limited": true, - "prev_batch": "t392-516_47314_0_7_1_1_1_11444_1" + "prev_batch": prev_batch }, "unread_notifications": { "highlight_count": 0, @@ -262,7 +274,7 @@ impl EventBuilder { "timeline": { "events": events, "limited": true, - "prev_batch": "t392-516_47314_0_7_1_1_1_11444_1" + "prev_batch": prev_batch }, "unread_notifications": { "highlight_count": 0, @@ -282,7 +294,7 @@ impl EventBuilder { "timeline": { "events": events, "limited": false, - "prev_batch": "t392-516_47314_0_7_1_1_1_11444_1" + "prev_batch": prev_batch }, }); left_rooms.insert(room_id, room); @@ -302,7 +314,7 @@ impl EventBuilder { let body = serde_json::json! { { "device_one_time_keys_count": {}, - "next_batch": "s526_47314_0_7_1_1_1_11444_1", + "next_batch": next_batch, "device_lists": { "changed": [], "left": [] @@ -325,6 +337,10 @@ impl EventBuilder { .unwrap(); SyncResponse::try_from(response).unwrap() } + + fn generate_sync_token(&self) -> String { + format!("t392-516_47314_0_7_1_1_1_11444_{}", self.batch_counter) + } } /// Embedded sync reponse files From 60a43439e5b15f4fb80a1d81788a01e39c59fabf Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 15:17:21 +0200 Subject: [PATCH 09/32] Properly test for display name disambiguation. --- matrix_sdk_base/src/models/room.rs | 87 ++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 27 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 5757350d..34f8819d 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -701,44 +701,77 @@ mod test { #[async_test] async fn test_member_display_name() { - let client = get_client(); + // Initialize - let client_2 = { - let session = Session { - access_token: "1234".to_owned(), - user_id: UserId::try_from("@example:hostlocal").unwrap(), - device_id: "DEVICEID".to_owned(), - }; - BaseClient::new(Some(session)).unwrap() - }; + let client = get_client().await; let room_id = get_room_id(); - let user_id = UserId::try_from("@example:localhost").unwrap(); + let user_id1 = UserId::try_from("@example:localhost").unwrap(); + let user_id2 = UserId::try_from("@someone_else:localhost").unwrap(); - let mut response = EventBuilder::default() + let member2_join_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example", + "membership": "join" + }, + "event_id": "$16345217l517tabbz:localhost", + "membership": "join", + "origin_server_ts": 1455123234, + "sender": "@someone_else:localhost", + "state_key": "@someone_else:localhost", + "type": "m.room.member", + "unsigned": { + "age": 1989321234, + "replaces_state": "$1622a2311315tkjoA:localhost", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "invite" + } + } + }); + + let mut event_builder = EventBuilder::new(); + + let mut member1_join_sync_response = event_builder .add_room_event(EventsFile::Member, RoomEvent::RoomMember) .build_sync_response(); - client.receive_sync_response(&mut response).await.unwrap(); - - let room = client.get_joined_room(&room_id).await.unwrap(); - let room = room.read().await; - - let display_name = room.member_display_name(&user_id).unwrap(); - assert_eq!("example", display_name); - - let mut response = EventBuilder::default() - .add_room_event(EventsFile::Member, RoomEvent::RoomMember) + let mut member2_join_sync_response = event_builder + .add_custom_joined_event(&room_id, member2_join_event, |ev| RoomEvent::RoomMember(ev)) .build_sync_response(); - client_2.receive_sync_response(&mut response).await.unwrap(); + // First member with display name "example" joins + client + .receive_sync_response(&mut member1_join_sync_response) + .await + .unwrap(); - let room_2 = client_2.get_joined_room(&room_id).await.unwrap(); - let room_2 = room_2.read().await; + // First member's disambiguated display name is "example" + { + let room = client.get_joined_room(&room_id).await.unwrap(); + let room = room.read().await; + let display_name1 = room.member_display_name(&user_id1).unwrap(); - let display_name_2 = room_2.member_display_name(&user_id).unwrap(); + assert_eq!("example", display_name1); + } - // TODO: this should change - assert_eq!("example", display_name_2); + // Second member with display name "example" joins + client + .receive_sync_response(&mut member2_join_sync_response) + .await + .unwrap(); + + // Both of their display names are now disambiguated with MXIDs + { + let room = client.get_joined_room(&room_id).await.unwrap(); + let room = room.read().await; + let display_name1 = room.member_display_name(&user_id1).unwrap(); + let display_name2 = room.member_display_name(&user_id2).unwrap(); + + assert_eq!(format!("example ({})", user_id1), display_name1); + assert_eq!(format!("example ({})", user_id2), display_name2); + } } #[async_test] From a9fd63fd4bb811e66aa8f8749657b9879cd96a2b Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 15:17:38 +0200 Subject: [PATCH 10/32] Fix display name disambiguation so it passes the test. --- matrix_sdk_base/src/models/room.rs | 31 +++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 34f8819d..41b823a1 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -327,17 +327,24 @@ impl Room { return false; } - let member = RoomMember::new(event); + let new_member = RoomMember::new(event); // find all users that share the same display name as the joining user let users_with_same_name: Vec<_> = self - .disambiguated_display_names + .members .iter() - .filter(|(_, v)| { - member + .filter(|(_, existing_member)| { + new_member .display_name .as_ref() - .map(|n| &n == v) + .and_then(|new_member_name| { + existing_member + .display_name + .as_ref() + .and_then(|existing_member_name| { + Some(new_member_name == existing_member_name) + }) + }) .unwrap_or(false) }) .map(|(k, _)| k) @@ -368,17 +375,19 @@ impl Room { // insert new member's display name self.disambiguated_display_names.insert( - member.user_id.clone(), - member + new_member.user_id.clone(), + new_member .display_name .as_ref() - .map(|n| format!("{} ({})", n, member.user_id)) - .unwrap_or_else(|| format!("{}", member.user_id)), + .map(|n| format!("{} ({})", n, new_member.user_id)) + .unwrap_or_else(|| format!("{}", new_member.user_id)), ); } - self.members - .insert(UserId::try_from(event.state_key.as_str()).unwrap(), member); + self.members.insert( + UserId::try_from(event.state_key.as_str()).unwrap(), + new_member, + ); true } From 22ba253103cddb20154a1c2d274f3f5119a13ee3 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 15:29:37 +0200 Subject: [PATCH 11/32] Use "disambiguated" instead of "resolved" display name in the doc comment. To match how the C2S spec calls it. --- 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 41b823a1..f853aa9f 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -302,7 +302,7 @@ impl Room { self.encrypted.as_ref() } - /// Get the resolved display name for a member of this room. + /// Get the disambiguated display name for a member of this room. pub fn member_display_name<'a>(&'a self, id: &UserId) -> Option> { self.disambiguated_display_names .get(id) From e6b67e5fa7d83251893412e09e4179359e5805bc Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 15:35:43 +0200 Subject: [PATCH 12/32] Add short explanation to Room::member_display_name. --- matrix_sdk_base/src/models/room.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index f853aa9f..7fdedd76 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -303,6 +303,11 @@ impl Room { } /// Get the disambiguated display name for a member of this 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 + /// multiple users 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: &UserId) -> Option> { self.disambiguated_display_names .get(id) From b93eb0e31827b9fc3e4bea09176e16e1ac33e16f Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 16:11:41 +0200 Subject: [PATCH 13/32] Make Room::member_display_name return MXID as fallback. If there is no display name set. This means the method can now always return something so there is no need to wrap in an `Option`. --- matrix_sdk_base/src/models/room.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 7fdedd76..7c2a0cf6 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -304,15 +304,17 @@ impl Room { /// Get the disambiguated display name for a member of this room. /// + /// If a user has no display name set, returns the 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 users 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: &UserId) -> Option> { + pub fn member_display_name<'a>(&'a self, id: &'a UserId) -> Cow<'a, str> { self.disambiguated_display_names .get(id) .map(|s| s.as_str().into()) - .or_else(|| { + .unwrap_or_else(|| { self.members.get(id).map(|member| { member .display_name @@ -320,7 +322,7 @@ impl Room { .map(|s| s.to_string()) .unwrap_or_else(|| format!("{}", member.user_id)) .into() - }) + }).unwrap_or(id.as_ref().into()) }) } @@ -765,7 +767,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).unwrap(); + let display_name1 = room.member_display_name(&user_id1); assert_eq!("example", display_name1); } @@ -780,8 +782,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).unwrap(); - let display_name2 = room.member_display_name(&user_id2).unwrap(); + 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); From a3c46c6144f9f2443c56fa47bbeac837788d8d02 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 16:41:26 +0200 Subject: [PATCH 14/32] Run cargo fmt. --- matrix_sdk_base/src/models/room.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 7c2a0cf6..a4b211b0 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -315,14 +315,17 @@ impl Room { .get(id) .map(|s| s.as_str().into()) .unwrap_or_else(|| { - self.members.get(id).map(|member| { - member - .display_name - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_else(|| format!("{}", member.user_id)) - .into() - }).unwrap_or(id.as_ref().into()) + self.members + .get(id) + .map(|member| { + member + .display_name + .as_ref() + .map(|s| s.to_string()) + .unwrap_or_else(|| format!("{}", member.user_id)) + .into() + }) + .unwrap_or(id.as_ref().into()) }) } From 098cc1f9f8218e3410296c2a0b41b9364ea1276b Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 19:08:14 +0200 Subject: [PATCH 15/32] Add explicit type annotation. --- 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 a4b211b0..77f55c11 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -340,7 +340,7 @@ impl Room { let new_member = RoomMember::new(event); // find all users that share the same display name as the joining user - let users_with_same_name: Vec<_> = self + let users_with_same_name: Vec = self .members .iter() .filter(|(_, existing_member)| { From 20a8e8e49b3e3719337c4baac1f646d35f708631 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 19:24:00 +0200 Subject: [PATCH 16/32] Fix comment styling. --- matrix_sdk_base/src/models/room.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 77f55c11..73a0e670 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -339,7 +339,7 @@ impl Room { let new_member = RoomMember::new(event); - // find all users that share the same display name as the joining user + // Find all users that share the same display name as the joining user. let users_with_same_name: Vec = self .members .iter() @@ -361,7 +361,8 @@ impl Room { .cloned() .collect(); - // if there is a other user with the same display name -> use `display_name (userid)` + // If there is another user with the same display name, use "DISPLAY_NAME (MXID)" instead + // to disambiguate. if !users_with_same_name.is_empty() { let users_with_same_name = users_with_same_name .into_iter() @@ -378,12 +379,12 @@ impl Room { }) .collect::>(); - // update all existing users with same name + // Update all existing users with same name. for (id, member) in users_with_same_name { self.disambiguated_display_names.insert(id, member); } - // insert new member's display name + // Insert new member's display name. self.disambiguated_display_names.insert( new_member.user_id.clone(), new_member From 82827542b7fc29bad7c00bf359bca731caae6c06 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 19:31:01 +0200 Subject: [PATCH 17/32] fixup: explicit type annotations --- 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 73a0e670..e16f7481 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -377,7 +377,7 @@ impl Room { }) .map(|m| (id, m)) }) - .collect::>(); + .collect::>(); // Update all existing users with same name. for (id, member) in users_with_same_name { From ac069152b9d63d284f1b62cff9d02ae5d2af6b1a Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 22:19:51 +0200 Subject: [PATCH 18/32] Retrieve user id from RoomMember instead of reconstructing. --- matrix_sdk_base/src/models/room.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index e16f7481..35a359fd 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -395,10 +395,7 @@ impl Room { ); } - self.members.insert( - UserId::try_from(event.state_key.as_str()).unwrap(), - new_member, - ); + self.members.insert(new_member.user_id.clone(), new_member); true } From e4977d1d2a200b31dc5d8f54ecc631b98cb5fd31 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 9 Jun 2020 23:02:01 +0200 Subject: [PATCH 19/32] Refactor member_display_name. Make it more readable, add comments. --- matrix_sdk_base/src/models/room.rs | 40 +++++++++++++++++------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 35a359fd..5cf61820 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -304,29 +304,35 @@ impl Room { /// Get the disambiguated display name for a member of this room. /// - /// If a user has no display name set, returns the 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 - /// multiple users can share the same display name in which case the display name has to be + /// 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> { - self.disambiguated_display_names + let disambiguated_name = self + .disambiguated_display_names .get(id) - .map(|s| s.as_str().into()) - .unwrap_or_else(|| { - self.members - .get(id) - .map(|member| { - member - .display_name - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_else(|| format!("{}", member.user_id)) - .into() - }) - .unwrap_or(id.as_ref().into()) - }) + .map(|s| s.as_str().into()); + + 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.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 + .display_name + .as_ref() + .map(|s| s.to_string()) + .unwrap_or_else(|| format!("{}", member.user_id)) + .into() + } else { + // There is no member with the requested MXID in the room. We still return the MXID. + id.as_ref().into() + } } fn add_member(&mut self, event: &MemberEvent) -> bool { From 4c184a30a2e7f8cd81d4f88fb0530302eafe9568 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 00:28:56 +0200 Subject: [PATCH 20/32] Add doc comment to RoomName::calculate_name. --- matrix_sdk_base/src/models/room.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 5cf61820..523d15ac 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -201,9 +201,14 @@ impl RoomName { true } + /// Calculate the canonical display name of a room, taking into account its name, aliases and + /// members. + /// + /// The display name is calculated according to [this algorithm][spec]. + /// + /// [spec]: + /// pub fn calculate_name(&self, members: &HashMap) -> String { - // https://matrix.org/docs/spec/client_server/latest#calculating-the-display-name-for-a-room. - // the order in which we check for a name ^^ if let Some(name) = &self.name { let name = name.trim(); name.to_string() From 5868c7266252f25361c337ad30fbebfd8e579cf3 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 12:01:01 +0200 Subject: [PATCH 21/32] Small refactor so we don't duplicate user_id creation. --- matrix_sdk_base/src/models/room.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 523d15ac..2f25c2a9 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -341,15 +341,12 @@ impl Room { } fn add_member(&mut self, event: &MemberEvent) -> bool { - if self - .members - .contains_key(&UserId::try_from(event.state_key.as_str()).unwrap()) - { + let new_member = RoomMember::new(event); + + if self.members.contains_key(&new_member.user_id) { return false; } - let new_member = RoomMember::new(event); - // Find all users that share the same display name as the joining user. let users_with_same_name: Vec = self .members @@ -487,6 +484,7 @@ impl Room { } else { return false; }; + if let Some(member) = self.members.get_mut(&user) { member.update_member(event) } else { From 3e5b6bb460f0da021fe3b875ceecdb2b604e3061 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 12:04:58 +0200 Subject: [PATCH 22/32] Style fixes. --- matrix_sdk_base/src/models/room.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 2f25c2a9..cfefc66a 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -229,7 +229,7 @@ impl RoomName { invited + joined - one }; - // TODO this should use `self.heroes but it is always empty?? + // TODO: This should use `self.heroes` but it is always empty?? if heroes >= invited_joined { let mut names = members .values() @@ -254,10 +254,11 @@ impl RoomName { }) .collect::>(); names.sort(); - // TODO what length does the spec want us to use here and in the `else` + + // TODO: What length does the spec want us to use here and in the `else`? format!("{}, and {} others", names.join(", "), (joined + invited)) } else { - format!("Empty Room (was {} others)", members.len()) + format!("Empty room (was {} others)", members.len()) } } } @@ -474,8 +475,8 @@ impl Room { /// /// Returns true if the joined member list changed, false otherwise. pub fn handle_membership(&mut self, event: &MemberEvent) -> bool { - // TODO this would not be handled correctly as all the MemberEvents have the `prev_content` - // inside of `unsigned` field + // TODO: This would not be handled correctly as all the MemberEvents have the `prev_content` + // inside of `unsigned` field. match event.membership_change() { MembershipChange::Invited | MembershipChange::Joined => self.add_member(event), _ => { From 241d456a81f21a38d2b410bde55ed45a66ecf51e Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 14:39:12 +0200 Subject: [PATCH 23/32] Add RoomMember::name. Returns the most ergonomic name for the member (either the display name (if set) or the MXID). --- matrix_sdk_base/src/models/room_member.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 337551c7..ede74caa 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -32,7 +32,7 @@ use serde::{Deserialize, Serialize}; /// A Matrix room member. /// pub struct RoomMember { - /// The unique mxid of the user. + /// The unique MXID of the user. pub user_id: UserId, /// The human readable name of the user. pub display_name: Option, @@ -100,6 +100,15 @@ impl RoomMember { } } + /// Returns the most ergonomic name available for the member. + /// + /// This is the member's display name if it is set, otherwise their MXID. + pub fn name(&self) -> String { + self.display_name + .clone() + .unwrap_or_else(|| format!("{}", self.user_id)) + } + pub fn update_member(&mut self, event: &MemberEvent) -> bool { use MembershipChange::*; From a0eaa9c364c7a8e6da57b83ca5e8633cba1c4303 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 14:44:41 +0200 Subject: [PATCH 24/32] Implement RoomMember::unique_name. This gives us a name that is as ergonomic as possible while guaranteeing it is unique. --- matrix_sdk_base/src/models/room_member.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index ede74caa..f3bc7245 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -109,6 +109,17 @@ impl RoomMember { .unwrap_or_else(|| format!("{}", self.user_id)) } + /// Returns a name for the member which is guaranteed to be unique. + /// + /// This is either of the format "DISPLAY_NAME (MXID)" if the display name is set for the + /// member, 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)) + } + pub fn update_member(&mut self, event: &MemberEvent) -> bool { use MembershipChange::*; From 7751605e3726a68a1901edba8e7d980955b17725 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 16:36:51 +0200 Subject: [PATCH 25/32] Nix RoomMember::update_member and tracking membership. After discussing with poljar, we concluded we don't actually need to tracking membership state, since we won't be tracking users that left (banned, kicked, disinvited). The only thing we need to keep track of is the difference between joined and invited users which will be dealt with in a separate commit. --- matrix_sdk/src/client.rs | 4 ---- matrix_sdk_base/src/models/room.rs | 23 +++++++++++----------- matrix_sdk_base/src/models/room_member.rs | 24 ++++++----------------- 3 files changed, 17 insertions(+), 34 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index c88e097f..00fbf580 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -1918,10 +1918,6 @@ mod test { .await; assert_eq!(2, room.members.len()); - for member in room.members.values() { - assert_eq!(MembershipState::Join, member.membership); - } - assert!(room.power_levels.is_some()) } diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index cfefc66a..5730087b 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -360,9 +360,7 @@ impl Room { existing_member .display_name .as_ref() - .and_then(|existing_member_name| { - Some(new_member_name == existing_member_name) - }) + .map(|existing_member_name| new_member_name == existing_member_name) }) .unwrap_or(false) }) @@ -475,23 +473,28 @@ impl Room { /// /// Returns true if the joined member list changed, false otherwise. 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() { - MembershipChange::Invited | MembershipChange::Joined => self.add_member(event), - _ => { - let user = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { + Invited | Joined => self.add_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.members.get_mut(&user) { - member.update_member(event) + if let Some(member) = self.members.get_mut(&user_id) { + member.update_profile(event) } else { false } } + + // Not interested in other events. + _ => false, } } @@ -719,10 +722,6 @@ mod test { .await; assert_eq!(2, room.members.len()); - for member in room.members.values() { - assert_eq!(MembershipState::Join, member.membership); - } - assert!(room.deref().power_levels.is_some()) } diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index f3bc7245..29d43aa9 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, MembershipState}, + member::{MemberEvent, MembershipChange}, power_levels::PowerLevelsEvent, }; use crate::identifiers::UserId; @@ -54,8 +54,6 @@ pub struct RoomMember { pub power_level: Option, /// The normalized power level of this `RoomMember` (0-100). pub power_level_norm: Option, - /// The `MembershipState` of this `RoomMember`. - pub membership: MembershipState, /// The human readable name of this room member. pub name: String, /// The events that created the state of this room member. @@ -75,7 +73,6 @@ impl PartialEq for RoomMember { && self.display_name == other.display_name && self.avatar_url == other.avatar_url && self.last_active_ago == other.last_active_ago - && self.membership == other.membership } } @@ -94,7 +91,6 @@ impl RoomMember { typing: None, power_level: None, power_level_norm: None, - membership: event.content.membership, presence_events: Vec::default(), events: vec![Event::RoomMember(event.clone())], } @@ -120,7 +116,8 @@ impl RoomMember { .unwrap_or_else(|| format!("{}", self.user_id)) } - pub fn update_member(&mut self, event: &MemberEvent) -> bool { + /// Handle profile updates. + pub(crate) fn update_profile(&mut self, event: &MemberEvent) -> bool { use MembershipChange::*; match event.membership_change() { @@ -129,15 +126,9 @@ impl RoomMember { self.avatar_url = event.content.avatar_url.clone(); true } - Banned | Kicked | KickedAndBanned | InvitationRejected | InvitationRevoked | Left - | Unbanned | Joined | Invited => { - self.membership = event.content.membership; - true - } - NotImplemented => false, - None => false, - // we ignore the error here as only a buggy or malicious server would send this - Error => false, + + // We're only interested in profile changes here. + _ => false, } } @@ -222,7 +213,6 @@ mod test { use matrix_sdk_test::{async_test, EventBuilder, EventsFile}; use crate::events::collections::all::RoomEvent; - use crate::events::room::member::MembershipState; use crate::identifiers::{RoomId, UserId}; use crate::{BaseClient, Session}; @@ -268,7 +258,6 @@ mod test { .members .get(&UserId::try_from("@example:localhost").unwrap()) .unwrap(); - assert_eq!(member.membership, MembershipState::Join); assert_eq!(member.power_level, Int::new(100)); } @@ -294,7 +283,6 @@ mod test { .get(&UserId::try_from("@example:localhost").unwrap()) .unwrap(); - assert_eq!(member.membership, MembershipState::Join); assert_eq!(member.power_level, Int::new(100)); assert!(member.avatar_url.is_none()); From 331cb02266613eb7af34a5bff303a8db9588dd0c Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 18:12:27 +0200 Subject: [PATCH 26/32] Split joined/invited users and handle removing users. --- matrix_sdk/examples/login.rs | 8 +- matrix_sdk/src/client.rs | 10 +- matrix_sdk_base/src/client.rs | 8 +- matrix_sdk_base/src/models/message.rs | 6 +- matrix_sdk_base/src/models/room.rs | 199 +++++++++++++++------- matrix_sdk_base/src/models/room_member.rs | 7 +- matrix_sdk_base/src/state/mod.rs | 6 +- 7 files changed, 159 insertions(+), 85 deletions(-) diff --git a/matrix_sdk/examples/login.rs b/matrix_sdk/examples/login.rs index 9fe5880b..10251fb8 100644 --- a/matrix_sdk/examples/login.rs +++ b/matrix_sdk/examples/login.rs @@ -23,12 +23,8 @@ impl EventEmitter for EventCallback { // any reads should be held for the shortest time possible to // avoid dead locks let room = room.read().await; - let member = room.members.get(&sender).unwrap(); - member - .display_name - .as_ref() - .map(ToString::to_string) - .unwrap_or(sender.to_string()) + let member = room.joined_members.get(&sender).unwrap(); + member.name() }; println!("{}: {}", name, msg_body); } diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index 00fbf580..f262c649 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -833,8 +833,11 @@ impl Client { let missing_sessions = { let room = self.base_client.get_joined_room(room_id).await; let room = room.as_ref().unwrap().read().await; - let users = room.members.keys(); - self.base_client.get_missing_sessions(users).await? + let members = room + .joined_members + .keys() + .chain(room.invited_members.keys()); + self.base_client.get_missing_sessions(members).await? }; if !missing_sessions.is_empty() { @@ -1276,7 +1279,6 @@ mod test { }; use super::{Client, ClientConfig, Session, SyncSettings, Url}; use crate::events::collections::all::RoomEvent; - use crate::events::room::member::MembershipState; use crate::events::room::message::TextMessageEventContent; use crate::identifiers::{EventId, RoomId, RoomIdOrAliasId, UserId}; @@ -1917,7 +1919,7 @@ mod test { .read() .await; - assert_eq!(2, room.members.len()); + assert_eq!(2, room.joined_members.len()); assert!(room.power_levels.is_some()) } diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 25f9cb45..92cbf4a7 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -963,7 +963,8 @@ impl BaseClient { // If the room is encrypted, update the tracked users. if room.is_encrypted() { - o.update_tracked_users(room.members.keys()).await; + o.update_tracked_users(room.joined_members.keys()).await; + o.update_tracked_users(room.invited_members.keys()).await; } } } @@ -1241,7 +1242,10 @@ impl BaseClient { match &mut *olm { Some(o) => { let room = room.write().await; - let members = room.members.keys(); + let members = room + .joined_members + .keys() + .chain(room.invited_members.keys()); Ok(o.share_group_session(room_id, members).await?) } None => panic!("Olm machine wasn't started"), diff --git a/matrix_sdk_base/src/models/message.rs b/matrix_sdk_base/src/models/message.rs index cc07ce59..53b8ad73 100644 --- a/matrix_sdk_base/src/models/message.rs +++ b/matrix_sdk_base/src/models/message.rs @@ -187,7 +187,8 @@ mod test { }, "own_user_id": "@example:example.com", "creator": null, - "members": {}, + "joined_members": {}, + "invited_members": {}, "messages": [ message ], "typing_users": [], "power_levels": null, @@ -237,7 +238,8 @@ mod test { }, "own_user_id": "@example:example.com", "creator": null, - "members": {}, + "joined_members": {}, + "invited_members": {}, "messages": [ message ], "typing_users": [], "power_levels": null, diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 5730087b..a4800dec 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -159,8 +159,11 @@ pub struct Room { pub own_user_id: UserId, /// The mxid of the room creator. pub creator: Option, - /// The map of room members. - pub members: HashMap, + // TODO: Track banned members, e.g. for /unban support? + /// The map of invited room members. + pub invited_members: HashMap, + /// The map of joined room members. + pub joined_members: HashMap, /// A queue of messages, holds no more than 10 of the most recent messages. /// /// This is helpful when using a `StateStore` to avoid multiple requests @@ -208,7 +211,11 @@ impl RoomName { /// /// [spec]: /// - pub fn calculate_name(&self, members: &HashMap) -> String { + pub fn calculate_name( + &self, + invited_members: &HashMap, + joined_members: &HashMap, + ) -> String { if let Some(name) = &self.name { let name = name.trim(); name.to_string() @@ -229,10 +236,11 @@ impl RoomName { invited + joined - one }; + let members = joined_members.values().chain(invited_members.values()); + // TODO: This should use `self.heroes` but it is always empty?? if heroes >= invited_joined { let mut names = members - .values() .take(3) .map(|mem| { mem.display_name @@ -245,7 +253,6 @@ impl RoomName { names.join(", ") } else if heroes < invited_joined && invited + joined > one { let mut names = members - .values() .take(3) .map(|mem| { mem.display_name @@ -258,7 +265,7 @@ impl RoomName { // TODO: What length does the spec want us to use here and in the `else`? format!("{}, and {} others", names.join(", "), (joined + invited)) } else { - format!("Empty room (was {} others)", members.len()) + "Empty room".to_string() } } } @@ -278,7 +285,8 @@ impl Room { room_name: RoomName::default(), own_user_id: own_user_id.clone(), creator: None, - members: HashMap::new(), + invited_members: HashMap::new(), + joined_members: HashMap::new(), #[cfg(feature = "messages")] messages: MessageQueue::new(), typing_users: Vec::new(), @@ -293,7 +301,8 @@ impl Room { /// Return the display name of the room. pub fn display_name(&self) -> String { - self.room_name.calculate_name(&self.members) + self.room_name + .calculate_name(&self.invited_members, &self.joined_members) } /// Is the room a encrypted room. @@ -326,34 +335,92 @@ impl Room { 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.members.get(id) { + } 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 - .display_name - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_else(|| format!("{}", member.user_id)) - .into() + 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); - if self.members.contains_key(&new_member.user_id) { + if self.joined_members.contains_key(&new_member.user_id) + || self.invited_members.contains_key(&new_member.user_id) + { return false; } - // Find all users that share the same display name as the joining user. - let users_with_same_name: Vec = self - .members + match event.membership_change() { + MembershipChange::Joined => 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. + if let Some(disambiguations) = self.disambiguate_member(&new_member, true) { + for (id, name) in disambiguations.into_iter() { + self.disambiguated_display_names.insert(id, name); + } + } + + true + } + + /// Process the member event of a leaving user. + /// + /// 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 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 + } + } + + /// Given a room member, generate a map of member display name disambiguations required in + /// order to make everyone's display name unique. + /// + /// The `inclusive` parameter controls whether the member which we are disambiguating should + /// be considered a member of the room or not. + /// + /// Returns either the map of disambiguations or None if no disambiguations are necessary. + fn disambiguate_member( + &self, + member: &RoomMember, + inclusive: bool, + ) -> Option> { + 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. + let users_with_same_name: Vec = members .filter(|(_, existing_member)| { - new_member + member .display_name .as_ref() .and_then(|new_member_name| { @@ -364,47 +431,49 @@ impl Room { }) .unwrap_or(false) }) - .map(|(k, _)| k) + // 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) .cloned() .collect(); - // If there is another user with the same display name, use "DISPLAY_NAME (MXID)" instead - // to disambiguate. + // There is at least one other user with the same display name. if !users_with_same_name.is_empty() { - let users_with_same_name = users_with_same_name - .into_iter() - .filter_map(|id| { - self.members - .get(&id) - .map(|m| { - m.display_name - .as_ref() - .map(|d| format!("{} ({})", d, m.user_id)) - .unwrap_or_else(|| format!("{}", m.user_id)) + if users_with_same_name.len() == 1 { + // The ambiguous set is now of size 1, so we can revert to simply using the display + // name for this user. + Some( + users_with_same_name + .into_iter() + .filter_map(|id| { + self.joined_members + .get(&id) + .or_else(|| self.invited_members.get(&id)) + .map(|m| m.name()) + .map(|m| (id, m)) }) - .map(|m| (id, m)) - }) - .collect::>(); - - // Update all existing users with same name. - for (id, member) in users_with_same_name { - self.disambiguated_display_names.insert(id, member); + .collect::>(), + ) + } else { + // Disambiguate everyone in the list by using "DISPLAY_NAME (MXID)" as their + // display name. + Some( + users_with_same_name + .into_iter() + .filter_map(|id| { + self.joined_members + .get(&id) + .or_else(|| self.invited_members.get(&id)) + .map(|m| m.unique_name()) + .map(|m| (id, m)) + }) + .collect::>(), + ) } - - // Insert new member's display name. - self.disambiguated_display_names.insert( - new_member.user_id.clone(), - new_member - .display_name - .as_ref() - .map(|n| format!("{} ({})", n, new_member.user_id)) - .unwrap_or_else(|| format!("{}", new_member.user_id)), - ); + } else { + // Nothing to disambiguate. + None } - - self.members.insert(new_member.user_id.clone(), new_member); - - true } /// Add to the list of `RoomAliasId`s. @@ -479,6 +548,9 @@ impl Room { // inside of `unsigned` field. match event.membership_change() { 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 @@ -486,7 +558,7 @@ impl Room { return false; }; - if let Some(member) = self.members.get_mut(&user_id) { + if let Some(member) = self.joined_members.get_mut(&user_id) { member.update_profile(event) } else { false @@ -561,7 +633,7 @@ impl Room { } for user in event.content.users.keys() { - if let Some(member) = self.members.get_mut(user) { + if let Some(member) = self.joined_members.get_mut(user) { if member.update_power(event, max_power) { updated = true; } @@ -656,7 +728,7 @@ impl Room { /// /// * `event` - The presence event for a specified room member. pub fn receive_presence_event(&mut self, event: &PresenceEvent) -> bool { - if let Some(member) = self.members.get_mut(&event.sender) { + if let Some(member) = self.joined_members.get_mut(&event.sender) { if member.did_update_presence(event) { false } else { @@ -674,10 +746,7 @@ impl Room { #[cfg(test)] mod test { use super::*; - use crate::events::{ - room::{encryption::EncryptionEventContent, member::MembershipState}, - UnsignedData, - }; + use crate::events::{room::encryption::EncryptionEventContent, UnsignedData}; use crate::identifiers::{EventId, UserId}; use crate::{BaseClient, Session}; use matrix_sdk_test::{async_test, sync_response, EventBuilder, EventsFile, SyncResponseFile}; @@ -721,7 +790,7 @@ mod test { .read() .await; - assert_eq!(2, room.members.len()); + assert_eq!(2, room.joined_members.len()); assert!(room.deref().power_levels.is_some()) } @@ -764,7 +833,7 @@ mod test { .build_sync_response(); let mut member2_join_sync_response = event_builder - .add_custom_joined_event(&room_id, member2_join_event, |ev| RoomEvent::RoomMember(ev)) + .add_custom_joined_event(&room_id, member2_join_event, RoomEvent::RoomMember) .build_sync_response(); // First member with display name "example" joins @@ -816,13 +885,13 @@ mod test { let room = client.get_joined_room(&room_id).await.unwrap(); let room = room.read().await; - assert_eq!(room.members.len(), 1); + assert_eq!(room.joined_members.len(), 1); assert!(room.power_levels.is_some()); assert_eq!( room.power_levels.as_ref().unwrap().kick, crate::js_int::Int::new(50).unwrap() ); - let admin = room.members.get(&user_id).unwrap(); + let admin = room.joined_members.get(&user_id).unwrap(); assert_eq!( admin.power_level.unwrap(), crate::js_int::Int::new(100).unwrap() diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 29d43aa9..fab43f3f 100644 --- a/matrix_sdk_base/src/models/room_member.rs +++ b/matrix_sdk_base/src/models/room_member.rs @@ -27,8 +27,7 @@ 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)] -#[cfg_attr(test, derive(Clone))] +#[derive(Clone, Debug, Serialize, Deserialize)] /// A Matrix room member. /// pub struct RoomMember { @@ -255,7 +254,7 @@ mod test { let room = room.read().await; let member = room - .members + .joined_members .get(&UserId::try_from("@example:localhost").unwrap()) .unwrap(); assert_eq!(member.power_level, Int::new(100)); @@ -279,7 +278,7 @@ mod test { let room = room.read().await; let member = room - .members + .joined_members .get(&UserId::try_from("@example:localhost").unwrap()) .unwrap(); diff --git a/matrix_sdk_base/src/state/mod.rs b/matrix_sdk_base/src/state/mod.rs index cb2ec449..803a9965 100644 --- a/matrix_sdk_base/src/state/mod.rs +++ b/matrix_sdk_base/src/state/mod.rs @@ -153,7 +153,8 @@ mod test { }, "own_user_id": "@example:example.com", "creator": null, - "members": {}, + "joined_members": {}, + "invited_members": {}, "disambiguated_display_names": {}, "typing_users": [], "power_levels": null, @@ -182,7 +183,8 @@ mod test { }, "own_user_id": "@example:example.com", "creator": null, - "members": {}, + "joined_members": {}, + "invited_members": {}, "messages": [], "typing_users": [], "power_levels": null, From 97b1bb600461f51cc65e65351d483a05638a02bd Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Wed, 10 Jun 2020 22:45:08 +0200 Subject: [PATCH 27/32] Must not take our user into account when calculating room name. --- matrix_sdk/src/client.rs | 2 +- matrix_sdk_base/src/models/room.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index f262c649..5c3380a4 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -1952,7 +1952,7 @@ mod test { room_names.push(room.read().await.display_name()) } - assert_eq!(vec!["example, example2"], room_names); + assert_eq!(vec!["example2"], room_names); } #[tokio::test] diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index a4800dec..a2a82260 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -213,6 +213,7 @@ impl RoomName { /// pub fn calculate_name( &self, + own_user_id: &UserId, invited_members: &HashMap, joined_members: &HashMap, ) -> String { @@ -241,6 +242,7 @@ impl RoomName { // TODO: This should use `self.heroes` but it is always empty?? if heroes >= invited_joined { let mut names = members + .filter(|m| m.user_id != *own_user_id) .take(3) .map(|mem| { mem.display_name @@ -253,6 +255,7 @@ impl RoomName { names.join(", ") } else if heroes < invited_joined && invited + joined > one { let mut names = members + .filter(|m| m.user_id != *own_user_id) .take(3) .map(|mem| { mem.display_name @@ -302,7 +305,7 @@ impl Room { /// Return the display name of the room. pub fn display_name(&self) -> String { self.room_name - .calculate_name(&self.invited_members, &self.joined_members) + .calculate_name(&self.own_user_id, &self.invited_members, &self.joined_members) } /// Is the room a encrypted room. @@ -970,7 +973,7 @@ mod test { room_names.push(room.read().await.display_name()) } - assert_eq!(vec!["example, example2"], room_names); + assert_eq!(vec!["example2"], room_names); } #[async_test] From 03e53e991b9ca44087486aab663cc640fe147e7d Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Mon, 15 Jun 2020 17:04:10 +0200 Subject: [PATCH 28/32] Hoist prev_content to top-level in both timeline and state events. Also refactor and document why this hoisting is needed. This change makes the user_presence test fail because the hoisting exposes an error encoded into the test's expected result. Previously, the test expected 2 members in the room at the end. This is incorrect since one of the members in the test data leaves the room. However, since the prev_content of state events was previously not hoisted to the top level, the `membership_change` method would not notice it and thus not realize the member had left the room. The test was corrected to expect only a single member in the room. Another test change was made due to a limitation of EventBuilder: due to the fact that it makes the test data go through a de/ser cycle, it cannot easily hoist prev_content to the top level. Because of this, the tests were change to put prev_content into the top level from the outset. --- matrix_sdk/src/client.rs | 2 +- matrix_sdk_base/src/client.rs | 70 ++++++++++++++++++++++++------ matrix_sdk_base/src/models/room.rs | 2 +- 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/matrix_sdk/src/client.rs b/matrix_sdk/src/client.rs index 5c3380a4..5dd87ff1 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -1919,7 +1919,7 @@ mod test { .read() .await; - assert_eq!(2, room.joined_members.len()); + assert_eq!(1, room.joined_members.len()); assert!(room.power_levels.is_some()) } diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 92cbf4a7..d644235b 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -82,8 +82,15 @@ pub struct AdditionalUnsignedData { pub prev_content: Option>, } -/// If a `prev_content` field is found inside of `unsigned` we move it up to the events `prev_content` field. -fn deserialize_prev_content(event: &EventJson) -> Option> { +/// Transform room event by hoisting `prev_content` field from `unsigned` to the top level. +/// +/// Due to a [bug in synapse][synapse-bug], `prev_content` often ends up in `unsigned` contrary to +/// the C2S spec. Some more discussion can be found [here][discussion]. Until this is fixed in +/// synapse or handled in Ruma, we use this to hoist up `prev_content` to the top level. +/// +/// [synapse-bug]: +/// [discussion]: +fn hoist_room_event_prev_content(event: &mut EventJson) -> Option> { let prev_content = serde_json::from_str::(event.json().get()) .map(|more_unsigned| more_unsigned.unsigned) .map(|additional| additional.prev_content) @@ -93,7 +100,33 @@ fn deserialize_prev_content(event: &EventJson) -> Option { - member.prev_content = prev_content.deserialize().ok(); + if let Some(prev) = prev_content.deserialize().ok() { + member.prev_content = Some(prev) + } + + Some(EventJson::from(ev)) + } + _ => None, + } +} + +/// Transform state event by hoisting `prev_content` field from `unsigned` to the top level. +/// +/// See comment of `hoist_room_event_prev_content`. +fn hoist_state_event_prev_content(event: &EventJson) -> Option> { + let prev_content = serde_json::from_str::(event.json().get()) + .map(|more_unsigned| more_unsigned.unsigned) + .map(|additional| additional.prev_content) + .ok() + .flatten()?; + + let mut ev = event.deserialize().ok()?; + match &mut ev { + StateEvent::RoomMember(ref mut member) if member.prev_content.is_none() => { + if let Some(prev) = prev_content.deserialize().ok() { + member.prev_content = Some(prev) + } + Some(EventJson::from(ev)) } _ => None, @@ -675,13 +708,6 @@ impl BaseClient { room_id: &RoomId, event: &mut EventJson, ) -> Result<(Option>, bool)> { - // if the event is a m.room.member event the server will sometimes - // send the `prev_content` field as part of the unsigned field this extracts and - // places it where everything else expects it. - if let Some(e) = deserialize_prev_content(event) { - *event = e; - } - match event.deserialize() { #[allow(unused_mut)] Ok(mut e) => { @@ -941,7 +967,13 @@ impl BaseClient { let mut updated = false; for (room_id, joined_room) in &mut response.rooms.join { let matrix_room = { - for event in &joined_room.state.events { + for event in &mut joined_room.state.events { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_state_event_prev_content(event) { + *event = e; + } + if let Ok(e) = event.deserialize() { if self.receive_joined_state_event(&room_id, &e).await? { updated = true; @@ -996,7 +1028,9 @@ impl BaseClient { *event = e; } - if let Some(e) = deserialize_prev_content(&event) { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_room_event_prev_content(event) { *event = e; } @@ -1071,7 +1105,13 @@ impl BaseClient { let mut updated = false; for (room_id, left_room) in &mut response.rooms.leave { let matrix_room = { - for event in &left_room.state.events { + for event in &mut left_room.state.events { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_state_event_prev_content(event) { + *event = e; + } + if let Ok(e) = event.deserialize() { if self.receive_left_state_event(&room_id, &e).await? { updated = true; @@ -1090,7 +1130,9 @@ impl BaseClient { } for event in &mut left_room.timeline.events { - if let Some(e) = deserialize_prev_content(&event) { + // XXX: Related to `prev_content` and `unsigned`; see the doc comment of + // `hoist_room_event_prev_content` + if let Some(e) = hoist_room_event_prev_content(event) { *event = e; } diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index a2a82260..c98a9843 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -793,7 +793,7 @@ mod test { .read() .await; - assert_eq!(2, room.joined_members.len()); + assert_eq!(1, room.joined_members.len()); assert!(room.deref().power_levels.is_some()) } From 765487dd9f7b4a1a63d8efba2c5cdedf41ef985e Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Mon, 15 Jun 2020 17:05:10 +0200 Subject: [PATCH 29/32] Fix comment style. --- matrix_sdk_base/src/client.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index d644235b..d7cdf65b 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -1001,13 +1001,13 @@ impl BaseClient { } } - // RoomSummary contains information for calculating room name + // RoomSummary contains information for calculating room name. matrix_room .write() .await .set_room_summary(&joined_room.summary); - // set unread notification count + // Set unread notification count. matrix_room .write() .await From 5bd3c49afc3538d913385316e5005fc36a080c52 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Mon, 15 Jun 2020 17:11:12 +0200 Subject: [PATCH 30/32] Correctly handle disambiguation for exiting members, refactor and test. --- matrix_sdk_base/src/models/room.rs | 286 ++++++++++++++++++++++------- 1 file changed, 219 insertions(+), 67 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index c98a9843..ce09da2a 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -147,6 +147,12 @@ pub struct Tombstone { replacement: RoomId, } +#[derive(Debug, PartialEq, Eq)] +enum MemberDirection { + Entering, + Exiting, +} + #[derive(Debug, PartialEq, Serialize, Deserialize)] #[cfg_attr(test, derive(Clone))] /// A Matrix room. @@ -304,8 +310,11 @@ impl Room { /// Return the display name of the room. pub fn display_name(&self) -> String { - self.room_name - .calculate_name(&self.own_user_id, &self.invited_members, &self.joined_members) + self.room_name.calculate_name( + &self.own_user_id, + &self.invited_members, + &self.joined_members, + ) } /// Is the room a encrypted room. @@ -377,10 +386,12 @@ impl Room { }; // Perform display name disambiguations, if necessary. - if let Some(disambiguations) = self.disambiguate_member(&new_member, true) { - for (id, name) in disambiguations.into_iter() { - self.disambiguated_display_names.insert(id, name); - } + 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 @@ -392,6 +403,16 @@ impl Room { fn remove_member(&mut self, event: &MemberEvent) -> bool { let leaving_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), + }; + } + if self.joined_members.contains_key(&leaving_member.user_id) { self.joined_members.remove(&leaving_member.user_id); true @@ -403,25 +424,18 @@ impl Room { } } - /// Given a room member, generate a map of member display name disambiguations required in - /// order to make everyone's display name unique. + /// Given a room `member`, return the list of members which have the same display name. /// - /// The `inclusive` parameter controls whether the member which we are disambiguating should - /// be considered a member of the room or not. - /// - /// Returns either the map of disambiguations or None if no disambiguations are necessary. - fn disambiguate_member( - &self, - member: &RoomMember, - inclusive: bool, - ) -> Option> { + /// 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 { 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. - let users_with_same_name: Vec = members + members .filter(|(_, existing_member)| { member .display_name @@ -438,47 +452,76 @@ impl Room { .filter(|(id, _)| inclusive || **id != member.user_id) .map(|(id, _)| id) .cloned() - .collect(); + .collect() + } - // There is at least one other user with the same display name. - if !users_with_same_name.is_empty() { - if users_with_same_name.len() == 1 { - // The ambiguous set is now of size 1, so we can revert to simply using the display - // name for this user. - Some( - users_with_same_name - .into_iter() - .filter_map(|id| { - self.joined_members - .get(&id) - .or_else(|| self.invited_members.get(&id)) - .map(|m| m.name()) - .map(|m| (id, m)) - }) - .collect::>(), - ) - } else { - // Disambiguate everyone in the list by using "DISPLAY_NAME (MXID)" as their - // display name. - Some( - users_with_same_name - .into_iter() - .filter_map(|id| { - self.joined_members - .get(&id) - .or_else(|| self.invited_members.get(&id)) - .map(|m| m.unique_name()) - .map(|m| (id, m)) - }) - .collect::>(), - ) - } - } else { - // Nothing to disambiguate. - None + /// 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. + /// + /// 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 from MXID to disambiguated name. + fn member_disambiguations( + &self, + member: &RoomMember, + 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() + .filter_map(|id| { + self.joined_members + .get(&id) + .or_else(|| self.invited_members.get(&id)) + .map(f) + .map(|m| (id, m)) + }) + .collect::>() + }; + + match users_with_same_name.len() { + 0 => HashMap::new(), + 1 => disambiguate_with(users_with_same_name, |m: &RoomMember| m.name()), + _ => disambiguate_with(users_with_same_name, |m: &RoomMember| m.unique_name()), } } + /// 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 res = before; + res.extend(after.clone()); + + res.into_iter() + .map(|(user_id, name)| { + if !after.contains_key(&user_id) { + (user_id, None) + } else { + (user_id, Some(name)) + } + }) + .collect() + } + /// Add to the list of `RoomAliasId`s. fn push_room_alias(&mut self, alias: &RoomAliasId) -> bool { self.room_name.push_alias(alias.clone()); @@ -550,7 +593,9 @@ impl Room { // 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), + Invited | Joined => { + self.add_member(event) + } Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { self.remove_member(event) } @@ -804,7 +849,8 @@ mod test { 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("@someone_else:localhost").unwrap(); + let user_id2 = UserId::try_from("@example2:localhost").unwrap(); + let user_id3 = UserId::try_from("@example3:localhost").unwrap(); let member2_join_event = serde_json::json!({ "content": { @@ -815,17 +861,86 @@ mod test { "event_id": "$16345217l517tabbz:localhost", "membership": "join", "origin_server_ts": 1455123234, - "sender": "@someone_else:localhost", - "state_key": "@someone_else:localhost", + "sender": format!("{}", user_id2), + "state_key": format!("{}", user_id2), "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "invite" + }, "unsigned": { "age": 1989321234, - "replaces_state": "$1622a2311315tkjoA:localhost", - "prev_content": { - "avatar_url": null, - "displayname": "example", - "membership": "invite" - } + "replaces_state": "$1622a2311315tkjoA:localhost" + } + }); + + let member2_leave_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example", + "membership": "leave" + }, + "event_id": "$263452333l22bggbz:localhost", + "membership": "leave", + "origin_server_ts": 1455123228, + "sender": format!("{}", user_id2), + "state_key": format!("{}", user_id2), + "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "join" + }, + "unsigned": { + "age": 1989321221, + "replaces_state": "$16345217l517tabbz:localhost" + } + }); + + let member3_join_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example", + "membership": "join" + }, + "event_id": "$16845287981ktggba:localhost", + "membership": "join", + "origin_server_ts": 1455123244, + "sender": format!("{}", user_id3), + "state_key": format!("{}", user_id3), + "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "invite" + }, + "unsigned": { + "age": 1989321254, + "replaces_state": "$1622l2323445kabrA:localhost" + } + }); + + let member3_leave_event = serde_json::json!({ + "content": { + "avatar_url": null, + "displayname": "example", + "membership": "leave" + }, + "event_id": "$11121987981abfgr:localhost", + "membership": "leave", + "origin_server_ts": 1455123230, + "sender": format!("{}", user_id3), + "state_key": format!("{}", user_id3), + "type": "m.room.member", + "prev_content": { + "avatar_url": null, + "displayname": "example", + "membership": "join" + }, + "unsigned": { + "age": 1989321244, + "replaces_state": "$16845287981ktggba:localhost" } }); @@ -839,6 +954,18 @@ mod test { .add_custom_joined_event(&room_id, member2_join_event, 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 + .add_custom_joined_event(&room_id, member2_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) + .build_sync_response(); + // First member with display name "example" joins client .receive_sync_response(&mut member1_join_sync_response) @@ -854,21 +981,46 @@ mod test { assert_eq!("example", display_name1); } - // Second member with display name "example" joins + // Second and third member with display name "example" join client .receive_sync_response(&mut member2_join_sync_response) .await .unwrap(); + client + .receive_sync_response(&mut member3_join_sync_response) + .await + .unwrap(); - // Both of their display names are now disambiguated with MXIDs + // All of their display names are now disambiguated with MXIDs { 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); assert_eq!(format!("example ({})", user_id1), display_name1); assert_eq!(format!("example ({})", user_id2), display_name2); + assert_eq!(format!("example ({})", user_id3), display_name3); + } + + // 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) + .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); + + assert_eq!("example", display_name1); } } From 733689870ea1f3b00f94e259407503b74001226e Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Sat, 20 Jun 2020 12:51:02 +0200 Subject: [PATCH 31/32] Fix compilation error and remaining test. Ref. for compilation error: https://github.com/rust-lang/rust/issues/64552 --- matrix_sdk_base/src/client.rs | 12 +++++++----- matrix_sdk_base/src/event_emitter/mod.rs | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index d7cdf65b..334e873b 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -1284,11 +1284,13 @@ impl BaseClient { match &mut *olm { Some(o) => { let room = room.write().await; - let members = room - .joined_members - .keys() - .chain(room.invited_members.keys()); - Ok(o.share_group_session(room_id, members).await?) + + // XXX: We construct members in a slightly roundabout way instead of chaining the + // iterators directly because of https://github.com/rust-lang/rust/issues/64552 + let joined_members = room.joined_members.keys(); + let invited_members = room.joined_members.keys(); + let members: Vec<&UserId> = joined_members.chain(invited_members).collect(); + Ok(o.share_group_session(room_id, members.into_iter()).await?) } None => panic!("Olm machine wasn't started"), } diff --git a/matrix_sdk_base/src/event_emitter/mod.rs b/matrix_sdk_base/src/event_emitter/mod.rs index a1ec2a5c..951462e2 100644 --- a/matrix_sdk_base/src/event_emitter/mod.rs +++ b/matrix_sdk_base/src/event_emitter/mod.rs @@ -93,7 +93,7 @@ pub enum CustomOrRawEvent<'c> { /// { /// let name = { /// let room = room.read().await; -/// let member = room.members.get(&sender).unwrap(); +/// let member = room.joined_members.get(&sender).unwrap(); /// member /// .display_name /// .as_ref() From c0c02baffc12e0ac01f4237837a7d40e4c56e1a2 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Sat, 20 Jun 2020 12:57:59 +0200 Subject: [PATCH 32/32] Run cargo fmt and apply clippy lints. --- matrix_sdk_base/src/client.rs | 4 ++-- matrix_sdk_base/src/models/room.rs | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/matrix_sdk_base/src/client.rs b/matrix_sdk_base/src/client.rs index 515ba1b3..21b14609 100644 --- a/matrix_sdk_base/src/client.rs +++ b/matrix_sdk_base/src/client.rs @@ -100,7 +100,7 @@ fn hoist_room_event_prev_content(event: &mut EventJson) -> Option { - if let Some(prev) = prev_content.deserialize().ok() { + if let Ok(prev) = prev_content.deserialize() { member.prev_content = Some(prev) } @@ -123,7 +123,7 @@ fn hoist_state_event_prev_content(event: &EventJson) -> Option { - if let Some(prev) = prev_content.deserialize().ok() { + if let Ok(prev) = prev_content.deserialize() { member.prev_content = Some(prev) } diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index be412840..805ae6c2 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -589,9 +589,7 @@ impl Room { // 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) - } + Invited | Joined => self.add_member(event), Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { self.remove_member(event) }