Merge branch 'dkasak-master'

High-level summary of changes:

- Rewrite the disambiguation algorithm to simplify it.
- Fixes to state tracking, e.g. use `state_key` instead of `user_id` when
  determining which member an event is acting on.
- Changes to `RoomMember`:
  * Make `RoomMember` "dumber" and don't let it mutate itself. This came about
    primarily because `update_profile` cannot live on `RoomMember` because it
    needs some information from `Room`. The other few mutating methods then
    looked odd so it seemed best to move them to `Room` so that the room takes
    care of updating its members.
  * Each `RoomMember` now contains all information to calculate its set of
    names:
      + `.name()` (short/ergonomic but potentially ambiguous),
      + `.unique_name()` (unique but may be contain MXID when not necessary),
      + `.disambiguated_name()` (shortest possible while being unique).
- Add some logging using the `tracing` crate.
- Improvements to `EventBuilder`:
  * Add a docstring.
  * Make it clear itself when building a sync response so the same builder can
    be reused for later sync responses.
- A few tests.

matrix_sdk/src/client.rs

@@ -30,8 +30,7 @@ use matrix_sdk_common::uuid::Uuid;
 use futures_timer::Delay as sleep;
 use std::future::Future;
 #[cfg(feature = "encryption")]
-use tracing::{debug, warn};
+use tracing::{debug, error, info, instrument, trace, warn};
-use tracing::{info, instrument, trace};
 
 use http::Method as HttpMethod;
 use http::Response as HttpResponse;
@@ -1311,12 +1310,13 @@ impl Client {
         loop {
             let response = self.sync(sync_settings.clone()).await;
 
-            let response = if let Ok(r) = response {
+            let response = match response {
-                r
+                Ok(r) => r,
-            } else {
+                Err(e) => {
-                sleep::new(Duration::from_secs(1)).await;
+                    error!("Received an invalid response: {}", e);
-
+                    sleep::new(Duration::from_secs(1)).await;
-                continue;
+                    continue;
+                }
             };
 
             // TODO send out to-device messages here

matrix_sdk_base/Cargo.toml

@@ -21,6 +21,7 @@ async-trait = "0.1.36"
 serde = "1.0.114"
 serde_json = "1.0.56"
 zeroize = "1.1.0"
+tracing = "0.1.14"
 
 matrix-sdk-common-macros = { version = "0.1.0", path = "../matrix_sdk_common_macros" }
 matrix-sdk-common = { version = "0.1.0", path = "../matrix_sdk_common" }

matrix_sdk_base/src/client.rs

@@ -743,7 +743,7 @@ impl BaseClient {
                 let mut room = room_lock.write().await;
 
                 if let AnyRoomEventStub::State(AnyStateEventStub::RoomMember(mem_event)) = &mut e {
-                    let changed = room.handle_membership(mem_event);
+                    let (changed, _) = room.handle_membership(mem_event, false);
 
                     // The memberlist of the room changed, invalidate the group session
                     // of the room.
@@ -780,7 +780,7 @@ impl BaseClient {
         let mut room = room_lock.write().await;
 
         if let AnyStateEventStub::RoomMember(e) = event {
-            let changed = room.handle_membership(e);
+            let (changed, _) = room.handle_membership(e, true);
 
             // The memberlist of the room changed, invalidate the group session
             // of the room.
@@ -2157,8 +2157,8 @@ mod test {
         let member = room.joined_members.get(&user_id).unwrap();
         assert_eq!(*member.display_name.as_ref().unwrap(), "changed");
 
-        // The second part tests that the event is emitted correctly. If `prev_content` was
+        // The second part tests that the event is emitted correctly. If `prev_content` were
-        // missing, this bool is reset to false.
+        // missing, this bool would had been flipped.
         assert!(passed.load(Ordering::SeqCst))
     }
 
@@ -2411,7 +2411,8 @@ mod test {
             }
         }
 
-        // `receive_joined_timeline_event` does not save the state to the store so we must
+        // `receive_joined_timeline_event` does not save the state to the store
+        // so we must do it ourselves
         client.store_room_state(&room_id).await.unwrap();
 
         // we load state from the store only

matrix_sdk_base/src/models/message.rs

@@ -216,7 +216,6 @@ mod test {
             serde_json::json!({
                 "!roomid:example.com": {
                     "room_id": "!roomid:example.com",
-                    "disambiguated_display_names": {},
                     "room_name": {
                         "name": null,
                         "canonical_alias": null,
@@ -262,7 +261,6 @@ mod test {
         let json = serde_json::json!({
             "!roomid:example.com": {
                 "room_id": "!roomid:example.com",
-                "disambiguated_display_names": {},
                 "room_name": {
                     "name": null,
                     "canonical_alias": null,

matrix_sdk_base/src/models/room_member.rs

@@ -15,16 +15,14 @@
 
 use std::convert::TryFrom;
 
-use crate::events::presence::{PresenceEvent, PresenceEventContent, PresenceState};
+use crate::events::presence::{PresenceEvent, PresenceState};
-use crate::events::room::{
+use crate::events::room::member::MemberEventContent;
-    member::{MemberEventContent, MembershipChange, MembershipState},
-    power_levels::PowerLevelsEventContent,
-};
 use crate::events::StateEventStub;
 use crate::identifiers::{RoomId, UserId};
 
-use crate::js_int::{int, Int, UInt};
+use crate::js_int::{Int, UInt};
 use serde::{Deserialize, Serialize};
+
 // Notes: if Alice invites Bob into a room we will get an event with the sender as Alice and the state key as Bob.
 
 #[derive(Debug, Serialize, Deserialize, Clone)]
@@ -34,6 +32,9 @@ pub struct RoomMember {
     pub user_id: UserId,
     /// The human readable name of the user.
     pub display_name: Option<String>,
+    /// Whether the member's display name is ambiguous due to being shared with
+    /// other members.
+    pub display_name_ambiguous: bool,
     /// The matrix url of the users avatar.
     pub avatar_url: Option<String>,
     /// The time, in ms, since the user interacted with the server.
@@ -52,10 +53,18 @@ pub struct RoomMember {
     pub power_level: Option<Int>,
     /// The normalized power level of this `RoomMember` (0-100).
     pub power_level_norm: Option<Int>,
-    /// The `MembershipState` of this `RoomMember`.
-    pub membership: MembershipState,
     /// The human readable name of this room member.
     pub name: String,
+    // FIXME: The docstring below is currently a lie since we only store the initial event that
+    // creates the member (the one we pass to RoomMember::new).
+    //
+    // The intent of this field is to keep the last (or last few?) state events related to the room
+    // member cached so we can quickly go back to the previous one in case some of them get
+    // redacted. Keeping all state for each room member is probably too much.
+    //
+    // Needs design.
+    /// The events that created the state of this room member.
+    pub events: Vec<StateEventStub<MemberEventContent>>,
     /// The `PresenceEvent`s connected to this user.
     pub presence_events: Vec<PresenceEvent>,
 }
@@ -67,6 +76,7 @@ impl PartialEq for RoomMember {
             && self.user_id == other.user_id
             && self.name == other.name
             && self.display_name == other.display_name
+            && self.display_name_ambiguous == other.display_name_ambiguous
             && self.avatar_url == other.avatar_url
             && self.last_active_ago == other.last_active_ago
     }
@@ -79,6 +89,7 @@ impl RoomMember {
             room_id: room_id.clone(),
             user_id: UserId::try_from(event.state_key.as_str()).unwrap(),
             display_name: event.content.displayname.clone(),
+            display_name_ambiguous: false,
             avatar_url: event.content.avatar_url.clone(),
             presence: None,
             status_msg: None,
@@ -87,12 +98,13 @@ impl RoomMember {
             typing: None,
             power_level: None,
             power_level_norm: None,
-            membership: event.content.membership,
+            presence_events: Vec::default(),
-            presence_events: vec![],
+            events: vec![event.clone()],
         }
     }
 
-    /// Returns the most ergonomic name available for the member.
+    /// Returns the most ergonomic (but potentially ambiguous/non-unique) name
+    /// available for the member.
     ///
     /// This is the member's display name if it is set, otherwise their MXID.
     pub fn name(&self) -> String {
@@ -101,10 +113,11 @@ impl RoomMember {
             .unwrap_or_else(|| format!("{}", self.user_id))
     }
 
-    /// Returns a name for the member which is guaranteed to be unique.
+    /// Returns a name for the member which is guaranteed to be unique, but not
+    /// necessarily the most ergonomic.
     ///
-    /// This is either of the format "DISPLAY_NAME (MXID)" if the display name is set for the
+    /// This is either a name in the format "DISPLAY_NAME (MXID)" if the
-    /// member, or simply "MXID" if not.
+    /// member's display name is set, or simply "MXID" if not.
     pub fn unique_name(&self) -> String {
         self.display_name
             .clone()
@@ -112,100 +125,28 @@ impl RoomMember {
             .unwrap_or_else(|| format!("{}", self.user_id))
     }
 
-    /// Handle profile updates.
+    /// Get the disambiguated display name for the member which is as ergonomic
-    pub(crate) fn update_profile(&mut self, event: &StateEventStub<MemberEventContent>) -> bool {
+    /// as possible while still guaranteeing it is unique.
-        use MembershipChange::*;
+    ///
-
+    /// If the member's display name is currently ambiguous (i.e. shared by
-        match event.membership_change() {
+    /// other room members), this method will return the same result as
-            // we assume that the profile has changed
+    /// `RoomMember::unique_name`. Otherwise, this method will return the same
-            ProfileChanged { .. } => {
+    /// result as `RoomMember::name`.
-                self.display_name = event.content.displayname.clone();
+    ///
-                self.avatar_url = event.content.avatar_url.clone();
+    /// This is usually the name you want when showing room messages from the
-                true
+    /// member or when showing the member in the member list.
-            }
+    ///
-
+    /// **Warning**: When displaying a room member's display name, clients
-            // We're only interested in profile changes here.
+    /// *must* use a disambiguated name, so they *must not* use
-            _ => false,
+    /// `RoomMember::display_name` directly. Clients *should* use this method to
-        }
+    /// obtain the name, but an acceptable alternative is to use
-    }
+    /// `RoomMember::unique_name` in certain situations.
-
+    pub fn disambiguated_name(&self) -> String {
-    pub fn update_power(
+        if self.display_name_ambiguous {
-        &mut self,
+            self.unique_name().into()
-        event: &StateEventStub<PowerLevelsEventContent>,
-        max_power: Int,
-    ) -> bool {
-        let changed;
-        if let Some(user_power) = event.content.users.get(&self.user_id) {
-            changed = self.power_level != Some(*user_power);
-            self.power_level = Some(*user_power);
         } else {
-            changed = self.power_level != Some(event.content.users_default);
+            self.name().into()
-            self.power_level = Some(event.content.users_default);
         }
-
-        if max_power > int!(0) {
-            self.power_level_norm = Some((self.power_level.unwrap() * int!(100)) / max_power);
-        }
-
-        changed
-    }
-
-    /// If the current `PresenceEvent` updated the state of this `User`.
-    ///
-    /// Returns true if the specific users presence has changed, false otherwise.
-    ///
-    /// # Arguments
-    ///
-    /// * `presence` - The presence event for a this room member.
-    pub fn did_update_presence(&self, presence: &PresenceEvent) -> bool {
-        let PresenceEvent {
-            content:
-                PresenceEventContent {
-                    avatar_url,
-                    currently_active,
-                    displayname,
-                    last_active_ago,
-                    presence,
-                    status_msg,
-                },
-            ..
-        } = presence;
-        self.display_name == *displayname
-            && self.avatar_url == *avatar_url
-            && self.presence.as_ref() == Some(presence)
-            && self.status_msg == *status_msg
-            && self.last_active_ago == *last_active_ago
-            && self.currently_active == *currently_active
-    }
-
-    /// Updates the `User`s presence.
-    ///
-    /// This should only be used if `did_update_presence` was true.
-    ///
-    /// # Arguments
-    ///
-    /// * `presence` - The presence event for a this room member.
-    pub fn update_presence(&mut self, presence_ev: &PresenceEvent) {
-        let PresenceEvent {
-            content:
-                PresenceEventContent {
-                    avatar_url,
-                    currently_active,
-                    displayname,
-                    last_active_ago,
-                    presence,
-                    status_msg,
-                },
-            ..
-        } = presence_ev;
-
-        self.presence_events.push(presence_ev.clone());
-        self.avatar_url = avatar_url.clone();
-        self.currently_active = *currently_active;
-        self.display_name = displayname.clone();
-        self.last_active_ago = *last_active_ago;
-        self.presence = Some(*presence);
-        self.status_msg = status_msg.clone();
     }
 }
 
@@ -234,7 +175,9 @@ mod test {
         client
     }
 
-    fn get_room_id() -> RoomId {
+    // TODO: Move this to EventBuilder since it's a magic room ID used in EventBuilder's example
+    // events.
+    fn test_room_id() -> RoomId {
         RoomId::try_from("!SVkFJHzfwvuaIEawgC:localhost").unwrap()
     }
 
@@ -242,11 +185,11 @@ mod test {
     async fn room_member_events() {
         let client = get_client().await;
 
-        let room_id = get_room_id();
+        let room_id = test_room_id();
 
         let mut response = EventBuilder::default()
-            .add_state_event(EventsJson::Member)
+            .add_room_event(EventsJson::Member)
-            .add_state_event(EventsJson::PowerLevels)
+            .add_room_event(EventsJson::PowerLevels)
             .build_sync_response();
 
         client.receive_sync_response(&mut response).await.unwrap();
@@ -261,15 +204,65 @@ mod test {
         assert_eq!(member.power_level, Some(int!(100)));
     }
 
+    #[async_test]
+    async fn room_member_display_name_change() {
+        let client = get_client().await;
+        let room_id = test_room_id();
+
+        let mut builder = EventBuilder::default();
+        let mut initial_response = builder
+            .add_room_event(EventsJson::Member)
+            .build_sync_response();
+        let mut name_change_response = builder
+            .add_room_event(EventsJson::MemberNameChange)
+            .build_sync_response();
+
+        client
+            .receive_sync_response(&mut initial_response)
+            .await
+            .unwrap();
+
+        let room = client.get_joined_room(&room_id).await.unwrap();
+
+        // Initially, the display name is "example".
+        {
+            let room = room.read().await;
+
+            let member = room
+                .joined_members
+                .get(&UserId::try_from("@example:localhost").unwrap())
+                .unwrap();
+
+            assert_eq!(member.display_name.as_ref().unwrap(), "example");
+        }
+
+        client
+            .receive_sync_response(&mut name_change_response)
+            .await
+            .unwrap();
+
+        // Afterwards, the display name is "changed".
+        {
+            let room = room.read().await;
+
+            let member = room
+                .joined_members
+                .get(&UserId::try_from("@example:localhost").unwrap())
+                .unwrap();
+
+            assert_eq!(member.display_name.as_ref().unwrap(), "changed");
+        }
+    }
+
     #[async_test]
     async fn member_presence_events() {
         let client = get_client().await;
 
-        let room_id = get_room_id();
+        let room_id = test_room_id();
 
         let mut response = EventBuilder::default()
-            .add_state_event(EventsJson::Member)
+            .add_room_event(EventsJson::Member)
-            .add_state_event(EventsJson::PowerLevels)
+            .add_room_event(EventsJson::PowerLevels)
             .add_presence_event(EventsJson::Presence)
             .build_sync_response();
 

matrix_sdk_base/src/state/mod.rs

@@ -159,7 +159,6 @@ mod test {
                     "creator": null,
                     "joined_members": {},
                     "invited_members": {},
-                    "disambiguated_display_names": {},
                     "typing_users": [],
                     "power_levels": null,
                     "encrypted": null,
@@ -176,7 +175,6 @@ mod test {
             serde_json::json!({
                 "!roomid:example.com": {
                     "room_id": "!roomid:example.com",
-                    "disambiguated_display_names": {},
                     "room_name": {
                         "name": null,
                         "canonical_alias": null,

matrix_sdk_test/src/lib.rs

@@ -26,6 +26,7 @@ pub enum EventsJson {
     HistoryVisibility,
     JoinRules,
     Member,
+    MemberNameChange,
     MessageEmote,
     MessageNotice,
     MessageText,
@@ -42,7 +43,40 @@ pub enum EventsJson {
     Typing,
 }
 
-/// Easily create events to stream into either a Client or a `Room` for testing.
+/// The `EventBuilder` struct can be used to easily generate valid sync responses for testing.
+/// These can be then fed into either `Client` or `Room`.
+///
+/// It supports generated a number of canned events, such as a member entering a room, his power
+/// level and display name changing and similar. It also supports insertion of custom events in the
+/// form of `EventsJson` values.
+///
+/// **Important** You *must* use the *same* builder when sending multiple sync responses to
+/// a single client. Otherwise, the subsequent responses will be *ignored* by the client because
+/// the `next_batch` sync token will not be rotated properly.
+///
+/// # Example usage
+///
+/// ```rust
+/// use matrix_sdk_test::{EventBuilder, EventsJson};
+///
+/// let mut builder = EventBuilder::new();
+///
+/// // response1 now contains events that add an example member to the room and change their power
+/// // level
+/// let response1 = builder
+///     .add_room_event(EventsJson::Member)
+///     .add_room_event(EventsJson::PowerLevels)
+///     .build_sync_response();
+///
+/// // response2 is now empty (nothing changed)
+/// let response2 = builder.build_sync_response();
+///
+/// // response3 contains a display name change for member example
+/// let response3 = builder
+///     .add_room_event(EventsJson::MemberNameChange)
+///     .build_sync_response();
+/// ```
+
 #[derive(Default)]
 pub struct EventBuilder {
     /// The events that determine the state of a `Room`.
@@ -97,6 +131,7 @@ impl EventBuilder {
     pub fn add_room_event(&mut self, json: EventsJson) -> &mut Self {
         let val: &JsonValue = match json {
             EventsJson::Member => &test_json::MEMBER,
+            EventsJson::MemberNameChange => &test_json::MEMBER_NAME_CHANGE,
             EventsJson::PowerLevels => &test_json::POWER_LEVELS,
             _ => panic!("unknown room event json {:?}", json),
         };
@@ -181,7 +216,8 @@ impl EventBuilder {
         self
     }
 
-    /// Consumes `ResponseBuilder` and returns `SyncResponse`.
+    /// Builds a `SyncResponse` containing the events we queued so far. The next response returned
+    /// by `build_sync_response` will then be empty if no further events were queued.
     pub fn build_sync_response(&mut self) -> SyncResponse {
         let main_room_id = RoomId::try_from("!SVkFJHzfwvuaIEawgC:localhost").unwrap();
 
@@ -293,12 +329,26 @@ impl EventBuilder {
         let response = Response::builder()
             .body(serde_json::to_vec(&body).unwrap())
             .unwrap();
+
+        // Clear state so that the next sync response will be empty if nothing was added.
+        self.clear();
+
         SyncResponse::try_from(response).unwrap()
     }
 
     fn generate_sync_token(&self) -> String {
         format!("t392-516_47314_0_7_1_1_1_11444_{}", self.batch_counter)
     }
+
+    pub fn clear(&mut self) {
+        self.account_data.clear();
+        self.ephemeral.clear();
+        self.invited_room_events.clear();
+        self.joined_room_events.clear();
+        self.left_room_events.clear();
+        self.presence_events.clear();
+        self.state_events.clear();
+    }
 }
 
 /// Embedded sync reponse files

matrix_sdk_test/src/test_json/events.rs

@@ -208,6 +208,7 @@ lazy_static! {
     });
 }
 
+// TODO: Move `prev_content` into `unsigned` once ruma supports it
 lazy_static! {
     pub static ref MEMBER: JsonValue = json!({
         "content": {
@@ -221,14 +222,40 @@ lazy_static! {
         "sender": "@example:localhost",
         "state_key": "@example:localhost",
         "type": "m.room.member",
+        "prev_content": {
+            "avatar_url": null,
+            "displayname": "example",
+            "membership": "invite"
+        },
         "unsigned": {
             "age": 297036,
-            "replaces_state": "$151800111315tsynI:localhost",
+            "replaces_state": "$151800111315tsynI:localhost"
-            "prev_content": {
+        }
-                "avatar_url": null,
+    });
-                "displayname": "example",
+}
-                "membership": "invite"
+
-            }
+// TODO: Move `prev_content` into `unsigned` once ruma supports it
+lazy_static! {
+    pub static ref MEMBER_NAME_CHANGE: JsonValue = json!({
+        "content": {
+            "avatar_url": null,
+            "displayname": "changed",
+            "membership": "join"
+        },
+        "event_id": "$151800234427abgho:localhost",
+        "membership": "join",
+        "origin_server_ts": 151800152,
+        "sender": "@example:localhost",
+        "state_key": "@example:localhost",
+        "type": "m.room.member",
+        "prev_content": {
+            "avatar_url": null,
+            "displayname": "example",
+            "membership": "join"
+        },
+        "unsigned": {
+            "age": 297032,
+            "replaces_state": "$151800140517rfvjc:localhost"
         }
     });
 }
@@ -552,6 +579,7 @@ lazy_static! {
     });
 }
 
+// TODO: Move `prev_content` into `unsigned` once ruma supports it
 lazy_static! {
     pub static ref TOPIC: JsonValue = json!({
         "content": {
@@ -562,11 +590,11 @@ lazy_static! {
         "sender": "@example:localhost",
         "state_key": "",
         "type": "m.room.topic",
+        "prev_content": {
+            "topic": "test"
+        },
         "unsigned": {
           "age": 1392989,
-          "prev_content": {
-            "topic": "test"
-          },
           "prev_sender": "@example:localhost",
           "replaces_state": "$151957069225EVYKm:localhost"
         }

matrix_sdk_test/src/test_json/mod.rs

@@ -9,8 +9,8 @@ pub mod sync;
 
 pub use events::{
     ALIAS, ALIASES, EVENT_ID, KEYS_QUERY, KEYS_UPLOAD, LOGIN, LOGIN_RESPONSE_ERR, LOGOUT, MEMBER,
-    MESSAGE_EDIT, MESSAGE_TEXT, NAME, POWER_LEVELS, PRESENCE, PUBLIC_ROOMS, REACTION, REDACTED,
+    MEMBER_NAME_CHANGE, MESSAGE_EDIT, MESSAGE_TEXT, NAME, POWER_LEVELS, PRESENCE, PUBLIC_ROOMS,
-    REDACTED_INVALID, REDACTED_STATE, REDACTION, REGISTRATION_RESPONSE_ERR, ROOM_ID, ROOM_MESSAGES,
+    REACTION, REDACTED, REDACTED_INVALID, REDACTED_STATE, REDACTION, REGISTRATION_RESPONSE_ERR,
-    TYPING,
+    ROOM_ID, ROOM_MESSAGES, TYPING,
 };
 pub use sync::{DEFAULT_SYNC_SUMMARY, INVITE_SYNC, LEAVE_SYNC, LEAVE_SYNC_EVENT, MORE_SYNC, SYNC};