From 949305da7254315bba030199356a03df2911d776 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Mon, 6 Jul 2020 15:22:49 +0200 Subject: [PATCH] Clarify comment. --- matrix_sdk_base/src/models/room.rs | 302 +++++++++-------------------- 1 file changed, 91 insertions(+), 211 deletions(-) diff --git a/matrix_sdk_base/src/models/room.rs b/matrix_sdk_base/src/models/room.rs index 61c23a98..0d27cf81 100644 --- a/matrix_sdk_base/src/models/room.rs +++ b/matrix_sdk_base/src/models/room.rs @@ -18,7 +18,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::convert::TryFrom; use serde::{Deserialize, Serialize}; -use tracing::{debug, error, trace}; +use tracing::{debug, instrument}; #[cfg(feature = "messages")] use super::message::MessageQueue; @@ -31,7 +31,7 @@ use crate::events::room::{ aliases::AliasesEvent, canonical_alias::CanonicalAliasEvent, encryption::EncryptionEvent, - member::{MemberEvent, MembershipChange, MembershipState}, + member::{MemberEvent, MembershipChange}, name::NameEvent, power_levels::{NotificationPowerLevels, PowerLevelsEvent, PowerLevelsEventContent}, tombstone::TombstoneEvent, @@ -321,10 +321,8 @@ impl Room { /// Get the disambiguated display name for a member of this room. /// - /// If a member has no display name set, returns the MXID as a fallback. - /// - /// This method never fails, even if user with the supplied MXID is not a member in the room. - /// In this case we still return the supplied MXID as a fallback. + /// If a member has no display name set, returns the MXID as a fallback. Additionally, we + /// return the MXID even if there is no such member in the room. /// /// When displaying a room member's display name, clients *must* use this method to obtain the /// name instead of displaying the `RoomMember::display_name` directly. This is because @@ -340,56 +338,49 @@ impl Room { } else { member.name().into() } - } + }, // Even if there is no such member, we return the MXID that was given to us. None => id.as_ref().into(), } } - /// Process the join or invite event for a new room member. - /// - /// If the user is not already a member, he will be added. Otherwise, his state will be updated - /// to reflect the event's state. + /// Process the member event of an entering user. /// /// Returns a tuple of: /// /// 1. True if the event made changes to the room's state, false otherwise. /// 2. Returns a map of display name disambiguations which tells us which members need to have /// their display names disambiguated and to what. - /// - /// # Arguments - /// - /// * `target_member` - The ID of the member to add. - /// * `event` - The join or invite event for the specified room member. - fn add_member( - &mut self, - target_member: &UserId, - event: &MemberEvent, - ) -> (bool, HashMap) { + #[instrument] + fn add_member(&mut self, event: &MemberEvent) -> (bool, HashMap) { let new_member = RoomMember::new(event); // Perform display name disambiguations, if necessary. - let disambiguations = - self.ambiguity_updates(target_member, None, new_member.display_name.clone()); + let disambiguations = self.disambiguation_updates(&new_member.user_id, None, new_member.display_name.clone()); - debug!("add_member: disambiguations: {:#?}", disambiguations); + debug!( + "add_member: disambiguations: {:#?}", + disambiguations + ); - match event.content.membership { - MembershipState::Join => { + match event.membership_change() { + MembershipChange::Joined => { // Since the member is now joined, he shouldn't be tracked as an invited member any // longer if he was previously tracked as such. - self.invited_members.remove(target_member); + self.invited_members.remove(&new_member.user_id); self.joined_members - .insert(target_member.clone(), new_member.clone()) + .insert(new_member.user_id.clone(), new_member.clone()) } - MembershipState::Invite => self + MembershipChange::Invited => self .invited_members - .insert(target_member.clone(), new_member.clone()), + .insert(new_member.user_id.clone(), new_member.clone()), - _ => panic!("Room::add_member called on event that is neither `join` nor `invite`."), + _ => { + panic!("Room::add_member called on an event that is neither a join nor an invite.") + } }; for (id, is_ambiguous) in disambiguations.iter() { @@ -399,34 +390,29 @@ impl Room { (true, disambiguations) } - /// Process the leaving event for a room member. + /// Process the member event of a leaving user. /// /// Returns a tuple of: /// /// 1. True if the event made changes to the room's state, false otherwise. /// 2. Returns a map of display name disambiguations which tells us which members need to have /// their display names disambiguated and to what. - /// - /// # Arguments - /// - /// * `target_member` - The ID of the member to remove. - /// * `event` - The leaving event for the specified room member. - fn remove_member( - &mut self, - target_member: &UserId, - event: &MemberEvent, - ) -> (bool, HashMap) { + #[instrument] + fn remove_member(&mut self, event: &MemberEvent) -> (bool, HashMap) { let leaving_member = RoomMember::new(event); - if self.get_member(target_member).is_none() { + if self.get_member(&leaving_member.user_id).is_none() { return (false, HashMap::new()); } // Perform display name disambiguations, if necessary. let disambiguations = - self.ambiguity_updates(target_member, leaving_member.display_name.clone(), None); + self.disambiguation_updates(&leaving_member.user_id, leaving_member.display_name.clone(), None); - debug!("remove_member: disambiguations: {:#?}", disambiguations); + debug!( + "remove_member: disambiguations: {:#?}", + disambiguations + ); for (id, is_ambiguous) in disambiguations.iter() { self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; @@ -434,20 +420,12 @@ impl Room { // TODO: factor this out to a method called `remove_member` and rename this method // to something like `process_member_leaving_event`. - self.joined_members - .remove(target_member) - .or_else(|| self.invited_members.remove(target_member)); + self.joined_members.remove(&leaving_member.user_id).or_else(|| + self.invited_members.remove(&leaving_member.user_id)); (true, disambiguations) } - /// Check whether the user with the MXID `user_id` is joined or invited to the room. - /// - /// Returns true if so, false otherwise. - pub fn member_is_tracked(&self, user_id: &UserId) -> bool { - self.invited_members.contains_key(&user_id) || self.joined_members.contains_key(&user_id) - } - /// Get a room member by user ID. /// /// If there is no such member, returns `None`. @@ -463,7 +441,7 @@ impl Room { pub fn get_member_mut(&mut self, user_id: &UserId) -> Option<&mut RoomMember> { match self.joined_members.get_mut(user_id) { None => self.invited_members.get_mut(user_id), - Some(m) => Some(m), + Some(m) => Some(m) } } @@ -476,39 +454,19 @@ impl Room { // Find all other users that share the display name with the joining user. members - .filter(|(_, member)| { - member - .display_name - .as_ref() - .map(|other_name| other_name == name) - .unwrap_or(false) - }) + .filter(|(_, member)| member.display_name.as_ref().map(|other_name| other_name == name).unwrap_or(false)) .map(|(_, member)| member.user_id.clone()) .collect() } - /// Given a change in a member's display name, determine the set of members whose display names - /// would either become newly ambiguous or no longer ambiguous after the change. + /// Given a room member, generate a map of all display name disambiguations which are necessary + /// in order to make that member's display name unique. /// - /// Returns a map of `user` -> `ambiguity` for affected room member. If `ambiguity` is `true`, - /// the member's display name is newly ambiguous after the change. If `false`, their display - /// name is no longer ambiguous (but it was before). + /// The `inclusive` parameter controls whether or not the member for which we are + /// disambiguating should be considered a current member of the room. /// - /// The method *must* be called before any actual display name changes are performed in our - /// model (e.g. the `RoomMember` structs). - /// - /// # Arguments - /// - /// * `member` - The ID of the member changing their display name. - /// * `old_name` - Their old display name, prior to the change. May be `None` if they had no - /// display name in this room (e.g. because it was unset or because they were not a room - /// member). - /// * `new_name` - Their new display name, after the change. May be `None` if they are no - /// longer going to have a display name in this room after the change (e.g. because they are - /// unsetting it or leaving the room). - /// - /// Returns a map from user ID to the new ambiguity status. - fn ambiguity_updates( + /// Returns a map from MXID to disambiguated name. + fn disambiguation_updates( &self, member: &UserId, old_name: Option, @@ -525,7 +483,7 @@ impl Room { n if n > 1 => vec![(member.clone(), false)].into_iter().collect(), 1 => old_name_eq_class.into_iter().map(|m| (m, false)).collect(), 0 => HashMap::new(), - _ => panic!("impossible"), + _ => panic!("impossible") }; // @@ -543,10 +501,7 @@ impl Room { _ => vec![(member.clone(), true)].into_iter().collect(), }; - disambiguate_old - .into_iter() - .chain(disambiguate_new.into_iter()) - .collect() + disambiguate_old.into_iter().chain(disambiguate_new.into_iter()).collect() } /// Add to the list of `RoomAliasId`s. @@ -621,61 +576,45 @@ impl Room { pub fn handle_membership( &mut self, event: &MemberEvent, - state_event: bool, ) -> (bool, HashMap) { use MembershipChange::*; - use MembershipState::*; - trace!( - "Received {} event: {}", - if state_event { "state" } else { "timeline" }, - event.event_id - ); + match event.membership_change() { + Invited | Joined => { + debug!( + "handle_membership: User {} entering room {} ({}) (joined or invited)", + event.state_key, + self.room_id, + self.display_name(), + ); - let target_user = match UserId::try_from(event.state_key.clone()) { - Ok(id) => id, - Err(e) => { - error!("Received a member event with invalid state_key: {}", e); - return (false, HashMap::new()); + self.add_member(event) } - }; + Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { + debug!( + "handle_membership: User {} exiting room {} ({}) (leaving, kicked, banned or invitation rejected)", + event.state_key, + self.room_id, + self.display_name(), + ); - if state_event && !self.member_is_tracked(&target_user) { - debug!( - "handle_membership: User {user_id} is {state} the room {room_id} ({room_name})", - user_id = target_user, - state = event.content.membership.describe(), - room_id = self.room_id, - room_name = self.display_name(), - ); - - match event.content.membership { - Join | Invite => self.add_member(&target_user, event), - - // We are not interested in tracking past members for now - _ => (false, HashMap::new()), + self.remove_member(event) } - } else { - let change = event.membership_change(); + ProfileChanged => { + debug!( + "handle_membership: Profile changed for user {} in room {} ({}) (displayname={:?}, avatar={:?}", + event.state_key, + self.room_id, + self.display_name(), + event.content.displayname, + event.content.avatar_url + ); - debug!( - "handle_membership: User {user_id} {action} the room {room_id} ({room_name})", - user_id = target_user, - action = change.describe(), - room_id = self.room_id, - room_name = self.display_name(), - ); - - match change { - Invited | Joined => self.add_member(&target_user, event), - Kicked | Banned | KickedAndBanned | InvitationRejected | Left => { - self.remove_member(&target_user, event) - } - ProfileChanged => self.update_member_profile(&target_user, event), - - // Not interested in other events. - _ => (false, HashMap::new()), + self.update_member_profile(event) } + + // Not interested in other events. + _ => (false, HashMap::new()), } } @@ -775,7 +714,7 @@ impl Room { pub fn receive_timeline_event(&mut self, event: &RoomEvent) -> bool { match event { // update to the current members of the room - RoomEvent::RoomMember(member) => self.handle_membership(member, false).0, + RoomEvent::RoomMember(member) => self.handle_membership(member).0, // finds all events related to the name of the room for later use RoomEvent::RoomName(name) => self.handle_room_name(name), RoomEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -800,7 +739,7 @@ impl Room { pub fn receive_state_event(&mut self, event: &StateEvent) -> bool { match event { // update to the current members of the room - StateEvent::RoomMember(member) => self.handle_membership(member, true).0, + StateEvent::RoomMember(member) => self.handle_membership(member).0, // finds all events related to the name of the room for later use StateEvent::RoomName(name) => self.handle_room_name(name), StateEvent::RoomCanonicalAlias(c_alias) => self.handle_canonical(c_alias), @@ -889,55 +828,38 @@ impl Room { /// /// # Arguments /// - /// * `target_member` - The ID of the member to update. - /// * `event` - The profile update event for the specified room member. + /// * `event` - The profile update event for a specified room member. + #[instrument] pub fn update_member_profile( &mut self, - target_member: &UserId, event: &MemberEvent, ) -> (bool, HashMap) { - let old_name = self - .get_member(target_member) - .map_or_else(|| None, |m| m.display_name.clone()); + let user_id = if let Ok(id) = UserId::try_from(event.state_key.as_str()) { + id + } else { + return (false, HashMap::new()); + }; + + let old_name = self.get_member(&user_id).map_or_else(|| None, |m| m.display_name.clone()); let new_name = event.content.displayname.clone(); - debug!( - "update_member_profile [{}]: from nick {:#?} to nick {:#?}", - self.room_id, old_name, &new_name - ); - - let disambiguations = - self.ambiguity_updates(target_member, old_name.clone(), new_name.clone()); + let disambiguations = self.disambiguation_updates(&user_id, old_name, new_name); for (id, is_ambiguous) in disambiguations.iter() { - if self.get_member_mut(id).is_none() { - error!("update_member_profile: I'm about to fail for id {}. user_id = {}\nevent = {:#?}", - id, - target_member, - event); - } else { - self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; - } + self.get_member_mut(id).unwrap().display_name_ambiguous = *is_ambiguous; } debug!( - "update_member_profile [{}]: disambiguations: {:#?}", - self.room_id, &disambiguations + "update_member_profile: disambiguations: {:#?}", + &disambiguations ); - let changed = match self.get_member_mut(target_member) { + let changed = match self.get_member_mut(&user_id) { Some(member) => { - member.display_name = new_name; + member.display_name = event.content.displayname.clone(); member.avatar_url = event.content.avatar_url.clone(); true } - None => { - error!( - "update_member_profile [{}]: user {} does not exist", - self.room_id, target_member - ); - - false - } + None => false, }; (changed, disambiguations) @@ -973,48 +895,6 @@ impl Room { } } -trait Describe { - fn describe(&self) -> String; -} - -impl Describe for MembershipState { - fn describe(&self) -> String { - use MembershipState::*; - - match self { - Ban => "is banned in", - Invite => "is invited to", - Join => "is a member of", - Knock => "is requesting access", - Leave => "left", - } - .to_string() - } -} - -impl Describe for MembershipChange { - fn describe(&self) -> String { - use MembershipChange::*; - - match self { - Invited => "got invited to", - Joined => "joined", - Kicked => "got kicked from", - Banned => "got banned from", - Unbanned => "got unbanned from", - KickedAndBanned => "got kicked and banned from", - InvitationRejected => "rejected the invitation to", - InvitationRevoked => "got their invitation revoked from", - Left => "left", - ProfileChanged => "changed their profile", - None => "did nothing in", - NotImplemented => "NOT IMPLEMENTED", - Error => "ERROR", - } - .to_string() - } -} - #[cfg(test)] mod test { use super::*;