better handling of membership in rooms #259

Zil0 · 2018-07-22T15:44:52Z

This should allow for better performance, by using appropriate data structures, and avoiding to create multiple User objects for the same user.

I need this for E2E because when verifying the devices of a user, their device list needs to be the same across rooms.

Signed-off-by: Valentin Deniaud <valentin.deniaud@inpt.fr>

non-Jedi · 2018-07-29T00:58:35Z

matrix_client/client.py

        Args:
            user_id (str): The matrix user id of a user.
        """
+        warn("get_user is deprecated. Directly instanciate a User instead.",


spelling. "instantiate"

non-Jedi

Thanks for this. Couple of points of concern. Also feel like I might be missing something else on this one because I'm tired, so I'll make sure I take another look at it another time.

non-Jedi · 2018-07-29T01:02:33Z

matrix_client/room.py

        """Returns list of joined members (User objects)."""
        if self._members:
-            return self._members
+            return list(self._members.values())


Doesn't need to be changed in this PR, but is there any reason to not deprecate this method and just change Room._members to Room.members? Can't think of a situation where it'd be a good idea to get the displayname of a room without first having called sync.

Sidenote: thanks for changing this list to the much more appropriate dict. Definitely a helpful change that's been bugging me for a bit.

Can't think of it either.
I don't think we should allow modifying the members of a room though, so we should keep Room._members and add Room.members as a property.
Also it would be better to return a dict than a list, since it's easy for the caller to use dict.values() if they want a list, and I can see the check user_id in Room.members having its use.

non-Jedi · 2018-07-29T01:12:00Z

matrix_client/client.py

        self.rooms = {
            # room_id: Room
        }
+        self.users = {


Problem: exposing this dict like this implies that it should be fine for a user to modify it as well as read from it. This needs to be read-only in the class's public api I think.

Sure. Shouldn't Client.rooms be the same?

In fact, not 100% sure how you want class attributes to be read-only (maybe only an indication in docstring would have been enough?). I implemented this with a property, but to me it fixes half of the problem since the internal dict can still be modified. I'd prefer returning a copy, but a copy is expensive.

Ya. Lack of richness in python type system makes expressing non-mutability of this with anything other than copy difficult to impossible. Note in a docstring is probably best bet. If anybody has written a python dict replacement that doesn't allow mutation, we could also look at using that and creating new dicts as things change (allowing old to be gced). Given python, performance characteristics of that solution would probably be bad, too. Docstring note is probably best bet for now.

Settled for docstring note + property then.

tbh, with still mutable dicts, I don't think changing it to a "property" really gains us much. :/ Definitely thanks for the docstring update though.

Absolutely agree, just wasn't sure what you expected. Should be good now!

non-Jedi · 2018-07-29T01:14:24Z

matrix_client/room.py

-
-    def _rmmembers(self, user_id):
-        self._members[:] = [x for x in self._members if x.user_id != user_id]
+                self._add_member(event["state_key"], event["content"].get("displayname"))


Since these user objects are now global rather than per-room, the displayname attached to each needs to be the global displayname rather than the per-room one. This gets the displayname in a particular room, right?

Yeah. I doesn't look like we have access to the global displayname here though.
So the fix would be to maintain a map from room IDs to displaynames in User, and to change the signature of User.get_displayname(self) to User.get_displayname(self, room), knowing that the first user.get_displayname() will always trigger a request to fetch the global displayname we don't have. Not super nice, what do you think?
Also the map of displaynames might get used as a hack to know in which rooms a particular user is, so we may want to implement something that allows that cleanly beforehand.

Waiting on your approval on this to implement this not so satisfying solution + a new attribute of User which contains the Rooms it is a member of.

Sorry on the delay here. I think the map mxid->displayname should live on the room object rather than the user object since that's how it's implemented in the protocol (per-room displaynames are just another state event). This also makes the hack about which rooms a user is in a bit more difficult, so we can kick that can down the road a bit.

This should allow for better performance, by using appropriate data structures, and avoiding to create multiple User objects for the same user.

Also some style improvements.

non-Jedi

This is awesome, and I'm excited to merge as soon as comments are addressed (completed full review this time).

non-Jedi · 2018-08-08T17:36:38Z

matrix_client/room.py

            return self.canonical_alias

        # Member display names without me
        members = [u.get_display_name() for u in self.get_joined_members() if


This should be self.members_displaynames[u.user_id] or the equivalent with appropriate default value (the way I read the spec anyway: https://matrix.org/speculator/spec/HEAD/client_server/unstable.html#calculating-the-display-name-for-a-room).

Good catch. Although I think it should be u.get_display_name(self) if you agree with what I wrote below, as this method will take care of the default value.

non-Jedi · 2018-08-08T17:45:35Z

matrix_client/user.py

+        See also get_friendly_name().
+
+        Args:
+            room (Room): Optional. If absent, return the global displayname for the user.


Docstring needs to note that this is a room object rather than a room id.

Does it? I see how someone reading the doc too fast would pass a str were it's very clearly written Room, but for me they'd deserve their AttributeError. Also, in case you didn't notice, Sphinx is smart enough to automatically add a link (clicking Room brings you to the room class), so there is really 0 ambiguity.
Another concern is that for consistency we'd have to add those notes to every methods that take an attribute room (Room) or user (User), which has a very low appeal to me...

You're right. This was me forgetting about the sphinx convention that already tells you the type of an argument. No need to change anything.

non-Jedi · 2018-08-08T17:47:51Z

matrix_client/user.py

+            try:
+                return room.members_displaynames[self.user_id]
+            except KeyError:
+                pass


If KeyError here, we should raise the same error that gets raised if self.api.get_display_name fails. If a user doesn't have a displayname on their m.room.member, we shouldn't give a displayname in the context of a particular room.

Would be great if you could write a quick test-case for this particular scenario.

Yep, defaulting to the global display name seems like a bad idea after all.

self.api.get_display_name doesn't raise anything and just returns None (which sure is unexpected according to the docstring of User.get_display_name), so here the try/except clause would just be replaced by return room.members_displaynames.get(self.user_id).

However, it seems like get_display_name should indeed raise an exception, while if someone doesn't want that they should call get_friendly_name which ideally wraps around get_display_name, returning the user id if an exception was raised.

While we're at it, I don't think this is a good design. The spec doesn't say anything that suggests having two methods (to me it says that only get_friendly_name should exist, and should be called get_display_name). Also I can't see any use of having an exception raised in this kind of method.

So my proposal would be:

User.get_display_name() gives the global display name, or the user id if not set.

User.get_display_name(room) gives the display name corresponding to the room, or the user id if not set.

User.get_friendly_name() is deprecated and returns User.get_display_name().

How's all that?

Sounds great to me! I think that really cleans up the displayname part of the client sdk api. Good work!

Zil0 · 2018-08-09T14:31:00Z

Should be better now!
Somehow I forgot that I had already written tests, and me-now didn't put them in the same place than me-2-days-before. Since I'm consistent with my present self, I'd say they are better where they are now (new user_test.py rather than client_test.py).

non-Jedi · 2018-08-09T16:16:37Z

test/user_test.py

+        return self.cli._mkroom(self.room_id)
+
+    @responses.activate
+    def test_get_display_name(self, user, room):


In future, please try to break tests that are testing multiple things into multiple methods.

non-Jedi reviewed Jul 29, 2018

View reviewed changes

Zil0 added 2 commits August 6, 2018 10:26

better handling of membership in rooms

3e79193

This should allow for better performance, by using appropriate data structures, and avoiding to create multiple User objects for the same user.

add lexicographic sort to room displayname calculation

7f821d6

Also some style improvements.

non-Jedi requested changes Aug 8, 2018

View reviewed changes

proper support of global and per-room display names

fdcd726

non-Jedi reviewed Aug 9, 2018

View reviewed changes

non-Jedi merged commit fdcd726 into matrix-org:master Aug 9, 2018

This was referenced Aug 9, 2018

user.get_display_name returns 404 Error #257

Closed

Fixed issue #257. #258

Closed

better handling of membership in rooms #259

better handling of membership in rooms #259

Uh oh!

Conversation

Zil0 commented Jul 22, 2018

Uh oh!

non-Jedi Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non-Jedi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non-Jedi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

non-Jedi Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zil0 commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

non-Jedi Jul 29, 2018 •

edited

Loading

non-Jedi Aug 8, 2018 •

edited

Loading

Zil0 commented Aug 9, 2018 •

edited

Loading