better handling of membership in rooms

matrix_client/client.py

@@ -103,6 +103,13 @@ def room_callback(room, incoming_event):
 
             def global_callback(incoming_event):
                 pass
+
+    Attributes:
+        users (dict): A map from user ID to :class:`.User` object.
+            It is populated automatically while tracking the membership in rooms, and
+            shouldn't be modified directly.
+            A :class:`.User` object in this dict is shared between all :class:`.Room`
+            objects where the corresponding user is joined.
     """
 
     def __init__(self, base_url, token=None, user_id=None,
@@ -144,6 +151,9 @@ def __init__(self, base_url, token=None, user_id=None,
         self.rooms = {
             # room_id: Room
         }
+        self.users = {
+            # user_id: User
+        }
         if token:
             check_user_id(user_id)
             self.user_id = user_id
@@ -626,14 +636,17 @@ def _sync(self, timeout_ms=30000):
                         listener['callback'](event)
 
     def get_user(self, user_id):
-        """ Return a User by their id.
+        """Deprecated. Return a User by their id.
 
-        NOTE: This function only returns a user object, it does not verify
-            the user with the Home Server.
+        This method only instantiate a User, which should be done directly.
+        You can also use :attr:`users` in order to access a User object which
+        was created automatically.
 
         Args:
             user_id (str): The matrix user id of a user.
         """
+        warn("get_user is deprecated. Directly instantiate a User instead.",
+             DeprecationWarning)
         return User(self.api, user_id)
 
     # TODO: move to Room class

matrix_client/room.py

@@ -45,7 +45,10 @@ def __init__(self, client, room_id):
         self.invite_only = None
         self.guest_access = None
         self._prev_batch = None
-        self._members = []
+        self._members = {}
+        self.members_displaynames = {
+            # user_id: displayname
+        }
         self.encrypted = False
 
     def set_user_profile(self,
@@ -83,19 +86,16 @@ def display_name(self):
             return self.canonical_alias
 
         # Member display names without me
-        members = [u.get_display_name() for u in self.get_joined_members() if
+        members = [u.get_display_name(self) for u in self.get_joined_members() if
                    self.client.user_id != u.user_id]
-        first_two = members[:2]
-        if len(first_two) == 1:
-            return first_two[0]
+        members.sort()
+
+        if len(members) == 1:
+            return members[0]
         elif len(members) == 2:
-            return "{0} and {1}".format(
-                first_two[0],
-                first_two[1])
+            return "{0} and {1}".format(members[0], members[1])
         elif len(members) > 2:
-            return "{0} and {1} others".format(
-                first_two[0],
-                len(members) - 1)
+            return "{0} and {1} others".format(members[0], len(members) - 1)
         else:  # len(members) <= 0 or not an integer
             # TODO i18n
             return "Empty room"
@@ -477,23 +477,23 @@ def add_room_alias(self, room_alias):
     def get_joined_members(self):
         """Returns list of joined members (User objects)."""
         if self._members:
-            return self._members
+            return list(self._members.values())
         response = self.client.api.get_room_members(self.room_id)
         for event in response["chunk"]:
             if event["content"]["membership"] == "join":
-                self._mkmembers(
-                    User(self.client.api,
-                         event["state_key"],
-                         event["content"].get("displayname"))
-                )
-        return self._members
-
-    def _mkmembers(self, member):
-        if member.user_id not in [x.user_id for x in self._members]:
-            self._members.append(member)
-
-    def _rmmembers(self, user_id):
-        self._members[:] = [x for x in self._members if x.user_id != user_id]
+                user_id = event["state_key"]
+                self._add_member(user_id, event["content"].get("displayname"))
+        return list(self._members.values())
+
+    def _add_member(self, user_id, displayname=None):
+        self.members_displaynames[user_id] = displayname
+        if user_id in self._members:
+            return
+        if user_id in self.client.users:
+            self._members[user_id] = self.client.users[user_id]
+            return
+        self._members[user_id] = User(self.client.api, user_id, displayname)
+        self.client.users[user_id] = self._members[user_id]
 
     def backfill_previous_messages(self, reverse=False, limit=10):
         """Backfill handling of previous messages.
@@ -660,13 +660,10 @@ def _process_state_event(self, state_event):
             elif etype == "m.room.member" and clevel == clevel.ALL:
                 # tracking room members can be large e.g. #matrix:matrix.org
                 if econtent["membership"] == "join":
-                    self._mkmembers(
-                        User(self.client.api,
-                             state_event["state_key"],
-                             econtent.get("displayname"))
-                    )
+                    user_id = state_event["state_key"]
+                    self._add_member(user_id, econtent.get("displayname"))
                 elif econtent["membership"] in ("leave", "kick", "invite"):
-                    self._rmmembers(state_event["state_key"])
+                    self._members.pop(state_event["state_key"], None)
 
         for listener in self.state_listeners:
             if (

matrix_client/user.py

@@ -12,6 +12,8 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from warnings import warn
+
 from .checks import check_user_id
 
 
@@ -25,20 +27,30 @@ def __init__(self, api, user_id, displayname=None):
         self.displayname = displayname
         self.api = api
 
-    def get_display_name(self):
-        """ Get this users display name.
-            See also get_friendly_name()
+    def get_display_name(self, room=None):
+        """Get this user's display name.
+
+        Args:
+            room (Room): Optional. When specified, return the display name of the user
+                in this room.
 
         Returns:
-            str: Display Name
+            The display name. Defaults to the user ID if not set.
         """
+        if room:
+            try:
+                return room.members_displaynames[self.user_id]
+            except KeyError:
+                return self.user_id
         if not self.displayname:
             self.displayname = self.api.get_display_name(self.user_id)
-        return self.displayname
+        return self.displayname or self.user_id
 
     def get_friendly_name(self):
-        display_name = self.api.get_display_name(self.user_id)
-        return display_name if display_name is not None else self.user_id
+        """Deprecated. Use :meth:`get_display_name` instead."""
+        warn("get_friendly_name is deprecated. Use get_display_name instead.",
+             DeprecationWarning)
+        return self.get_display_name()
 
     def set_display_name(self, display_name):
         """ Set this users display name.

test/client_test.py

@@ -110,7 +110,7 @@ def test_state_event():
     ev["state_key"] = "@stereo:xxx.org"
     room._process_state_event(ev)
     assert len(room._members) == 1
-    assert room._members[0].user_id == "@stereo:xxx.org"
+    assert room._members["@stereo:xxx.org"]
     # test member leave event
     ev["content"]['membership'] = 'leave'
     room._process_state_event(ev)
@@ -216,7 +216,7 @@ def test_get_rooms_display_name():
 
     def add_members(api, room, num):
         for i in range(num):
-            room._mkmembers(User(api, '@frho%s:matrix.org' % i, 'ho%s' % i))
+            room._add_member('@frho%s:matrix.org' % i, 'ho%s' % i)
 
     client = MatrixClient("http://example.com")
     client.user_id = "@frho0:matrix.org"
@@ -428,9 +428,9 @@ def test_cache():
     assert m_some.rooms[room_id].name == room_name
     assert m_all.rooms[room_id].name == room_name
 
-    assert m_none.rooms[room_id]._members == m_some.rooms[room_id]._members == []
+    assert m_none.rooms[room_id]._members == m_some.rooms[room_id]._members == {}
     assert len(m_all.rooms[room_id]._members) == 2
-    assert m_all.rooms[room_id]._members[0].user_id == "@alice:example.com"
+    assert m_all.rooms[room_id]._members["@alice:example.com"]
 
 
 @responses.activate

test/user_test.py

@@ -0,0 +1,52 @@
+import pytest
+import responses
+
+from matrix_client.api import MATRIX_V2_API_PATH
+from matrix_client.client import MatrixClient
+from matrix_client.user import User
+
+HOSTNAME = "http://localhost"
+
+
+class TestUser:
+    cli = MatrixClient(HOSTNAME)
+    user_id = "@test:localhost"
+    room_id = "!test:localhost"
+
+    @pytest.fixture()
+    def user(self):
+        return User(self.cli.api, self.user_id)
+
+    @pytest.fixture()
+    def room(self):
+        return self.cli._mkroom(self.room_id)
+
+    @responses.activate
+    def test_get_display_name(self, user, room):
+        displayname_url = HOSTNAME + MATRIX_V2_API_PATH + \
+            "/profile/{}/displayname".format(user.user_id)
+        displayname = 'test'
+        room_displayname = 'room_test'
+
+        # No displayname
+        assert user.get_display_name(room) == user.user_id
+        responses.add(responses.GET, displayname_url, json={})
+        assert user.get_display_name() == user.user_id
+        assert len(responses.calls) == 1
+
+        # Get global displayname
+        responses.replace(responses.GET, displayname_url,
+                          json={"displayname": displayname})
+        assert user.get_display_name() == displayname
+        assert len(responses.calls) == 2
+
+        # Global displayname already present
+        assert user.get_display_name() == displayname
+        # No new request
+        assert len(responses.calls) == 2
+
+        # Per-room displayname
+        room.members_displaynames[user.user_id] = room_displayname
+        assert user.get_display_name(room) == room_displayname
+        # No new request
+        assert len(responses.calls) == 2