diff --git a/matrix_sdk/examples/login.rs b/matrix_sdk/examples/login.rs index 838a9a31..8eb30cb3 100644 --- a/matrix_sdk/examples/login.rs +++ b/matrix_sdk/examples/login.rs @@ -24,12 +24,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 3e4c0bdc..087d3db6 100644 --- a/matrix_sdk/src/client.rs +++ b/matrix_sdk/src/client.rs @@ -872,8 +872,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() { @@ -1398,7 +1401,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}; use crate::RegistrationBuilder; @@ -2079,11 +2081,7 @@ mod test { .read() .await; - assert_eq!(2, room.members.len()); - for member in room.members.values() { - assert_eq!(MembershipState::Join, member.membership); - } - + assert_eq!(1, room.joined_members.len()); assert!(room.power_levels.is_some()) } @@ -2116,7 +2114,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/client.rs b/matrix_sdk_base/src/client.rs index 89819b06..692285fd 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 Ok(prev) = prev_content.deserialize() { + 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 Ok(prev) = prev_content.deserialize() { + 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; @@ -963,18 +995,19 @@ 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; } } } - // 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 @@ -995,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; } @@ -1070,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; @@ -1089,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; } @@ -1241,8 +1284,13 @@ impl BaseClient { match &mut *olm { Some(o) => { let room = room.write().await; - let members = room.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 de42e4ad..e767587d 100644 --- a/matrix_sdk_base/src/event_emitter/mod.rs +++ b/matrix_sdk_base/src/event_emitter/mod.rs @@ -95,7 +95,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() diff --git a/matrix_sdk_base/src/models/message.rs b/matrix_sdk_base/src/models/message.rs index d2029f49..b78ea1a7 100644 --- a/matrix_sdk_base/src/models/message.rs +++ b/matrix_sdk_base/src/models/message.rs @@ -178,6 +178,7 @@ mod test { serde_json::json!({ "!roomid:example.com": { "room_id": "!roomid:example.com", + "disambiguated_display_names": {}, "room_name": { "name": null, "canonical_alias": null, @@ -188,7 +189,8 @@ mod test { }, "own_user_id": "@example:example.com", "creator": null, - "members": {}, + "joined_members": {}, + "invited_members": {}, "messages": [ message ], "typing_users": [], "power_levels": null, @@ -227,6 +229,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, @@ -237,7 +240,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 3eab4542..e7c3f175 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; @@ -143,6 +144,12 @@ 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 { @@ -154,8 +161,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 @@ -176,6 +186,8 @@ 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 { @@ -194,9 +206,19 @@ impl RoomName { true } - 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 ^^ + /// 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, + own_user_id: &UserId, + invited_members: &HashMap, + joined_members: &HashMap, + ) -> String { if let Some(name) = &self.name { let name = name.trim(); name.to_string() @@ -217,10 +239,12 @@ impl RoomName { invited + joined - one }; - // TODO this should use `self.heroes but it is always empty?? + 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() + .filter(|m| m.user_id != *own_user_id) .take(3) .map(|mem| { mem.display_name @@ -233,7 +257,7 @@ impl RoomName { names.join(", ") } else if heroes < invited_joined && invited + joined > one { let mut names = members - .values() + .filter(|m| m.user_id != *own_user_id) .take(3) .map(|mem| { mem.display_name @@ -242,10 +266,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()) + "Empty room".to_string() } } } @@ -265,7 +290,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(), @@ -274,12 +300,17 @@ impl Room { unread_highlight: None, unread_notifications: None, tombstone: None, + disambiguated_display_names: HashMap::new(), } } /// 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.own_user_id, + &self.invited_members, + &self.joined_members, + ) } /// Is the room a encrypted room. @@ -294,22 +325,199 @@ 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. 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 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()); + + 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 { - if self - .members - .contains_key(&UserId::try_from(event.state_key.as_str()).unwrap()) + 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; } - let member = RoomMember::new(event); + 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.") + } + }; - self.members - .insert(UserId::try_from(event.state_key.as_str()).unwrap(), member); + // 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), + }; + } 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); + + // 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 + } 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`, 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 { + 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. + members + .filter(|(_, existing_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) + }) + .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(|(id, _)| id) + .cloned() + .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. + /// + /// 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()); @@ -376,22 +584,31 @@ 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 + 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), + 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.members.get_mut(&user) { - member.update_member(event) + + if let Some(member) = self.joined_members.get_mut(&user_id) { + member.update_profile(event) } else { false } } + + // Not interested in other events. + _ => false, } } @@ -458,7 +675,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; } @@ -553,7 +770,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 { @@ -571,10 +788,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, EventsJson, SyncResponseFile}; @@ -618,12 +832,190 @@ mod test { .read() .await; - assert_eq!(2, room.members.len()); - for member in room.members.values() { - assert_eq!(MembershipState::Join, member.membership); + assert_eq!(1, room.joined_members.len()); + assert!(room.deref().power_levels.is_some()) + } + + #[async_test] + async fn test_member_display_name() { + // Initialize + + 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 user_id3 = UserId::try_from("@example3:localhost").unwrap(); + + 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": 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" + } + }); + + 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" + } + }); + + 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_join_sync_response = event_builder + .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) + .await + .unwrap(); + + // 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); + + assert_eq!("example", display_name1); } - assert!(room.deref().power_levels.is_some()) + // 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(); + + // 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); + } } #[async_test] @@ -642,13 +1034,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() @@ -727,7 +1119,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] diff --git a/matrix_sdk_base/src/models/room_member.rs b/matrix_sdk_base/src/models/room_member.rs index 0bb4db7c..9834b716 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; @@ -31,7 +31,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, @@ -53,8 +53,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. @@ -74,7 +72,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 } } @@ -93,13 +90,33 @@ 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())], } } - pub fn update_member(&mut self, event: &MemberEvent) -> bool { + /// 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)) + } + + /// 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)) + } + + /// Handle profile updates. + pub(crate) fn update_profile(&mut self, event: &MemberEvent) -> bool { use MembershipChange::*; match event.membership_change() { @@ -108,15 +125,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, } } @@ -201,7 +212,6 @@ mod test { use matrix_sdk_test::{async_test, EventBuilder, EventsJson}; use crate::events::collections::all::RoomEvent; - use crate::events::room::member::MembershipState; use crate::identifiers::{RoomId, UserId}; use crate::{BaseClient, Session}; @@ -244,10 +254,9 @@ mod test { let room = room.read().await; let member = room - .members + .joined_members .get(&UserId::try_from("@example:localhost").unwrap()) .unwrap(); - assert_eq!(member.membership, MembershipState::Join); assert_eq!(member.power_level, Int::new(100)); } @@ -269,11 +278,10 @@ mod test { let room = room.read().await; let member = room - .members + .joined_members .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()); diff --git a/matrix_sdk_base/src/state/mod.rs b/matrix_sdk_base/src/state/mod.rs index 0f3da268..6bd18820 100644 --- a/matrix_sdk_base/src/state/mod.rs +++ b/matrix_sdk_base/src/state/mod.rs @@ -144,57 +144,61 @@ 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, + "joined_members": {}, + "invited_members": {}, + "disambiguated_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", + "disambiguated_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, + "joined_members": {}, + "invited_members": {}, + "messages": [], + "typing_users": [], + "power_levels": null, + "encrypted": null, + "unread_highlight": null, + "unread_notifications": null, + "tombstone": null + } + }), + serde_json::to_value(&joined_rooms).unwrap() ); } diff --git a/matrix_sdk_test/src/lib.rs b/matrix_sdk_test/src/lib.rs index 4a567acb..80bba176 100644 --- a/matrix_sdk_test/src/lib.rs +++ b/matrix_sdk_test/src/lib.rs @@ -64,15 +64,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, json: EventsJson, variant: fn(Ev) -> Event, - ) -> Self { + ) -> &mut Self { let val: &JsonValue = match json { EventsJson::Typing => &test_json::TYPING, _ => panic!("unknown ephemeral event {:?}", json), @@ -89,10 +96,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, json: EventsJson, variant: fn(Ev) -> Event, - ) -> Self { + ) -> &mut Self { let val: &JsonValue = match json { _ => panic!("unknown account event {:?}", json), }; @@ -107,10 +114,10 @@ impl EventBuilder { /// Add an event to the room events `Vec`. pub fn add_room_event( - mut self, + &mut self, json: EventsJson, variant: fn(Ev) -> RoomEvent, - ) -> Self { + ) -> &mut Self { let val: &JsonValue = match json { EventsJson::Member => &test_json::MEMBER, EventsJson::PowerLevels => &test_json::POWER_LEVELS, @@ -129,11 +136,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() @@ -150,11 +157,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() @@ -167,11 +174,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() @@ -185,10 +192,10 @@ impl EventBuilder { /// Add a state event to the state events `Vec`. pub fn add_state_event( - mut self, + &mut self, json: EventsJson, variant: fn(Ev) -> StateEvent, - ) -> Self { + ) -> &mut Self { let val: &JsonValue = match json { EventsJson::Alias => &test_json::ALIAS, EventsJson::Aliases => &test_json::ALIASES, @@ -205,7 +212,7 @@ impl EventBuilder { } /// Add an presence event to the presence events `Vec`. - pub fn add_presence_event(mut self, json: EventsJson) -> Self { + pub fn add_presence_event(&mut self, json: EventsJson) -> &mut Self { let val: &JsonValue = match json { EventsJson::Presence => &test_json::PRESENCE, _ => panic!("unknown presence event {:?}", json), @@ -219,10 +226,15 @@ impl EventBuilder { self } - /// Consumes `ResponseBuilder and returns SyncResponse. - pub fn build_sync_response(mut self) -> SyncResponse { + /// Consumes `ResponseBuilder` and returns `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": {}, @@ -238,7 +250,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, @@ -265,7 +277,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, @@ -285,7 +297,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); @@ -305,7 +317,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": [] @@ -328,6 +340,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