-
Notifications
You must be signed in to change notification settings - Fork 121
Add support for "m.federate" and room name in createRoom API method #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2a5106c
87da7c7
338e46d
bd3ad37
077e538
0b83a11
68ece48
c953681
f097ee9
ee77982
f98937f
d30e8e5
bfd65d8
11bf397
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,13 +164,23 @@ def logout(self): | |
| """ | ||
| return self._send("POST", "/logout") | ||
|
|
||
| def create_room(self, alias=None, is_public=False, invitees=None): | ||
| def create_room( | ||
| self, | ||
| alias=None, | ||
| name=None, | ||
| is_public=False, | ||
| invitees=None, | ||
| federate=None | ||
| ): | ||
| """Perform /createRoom. | ||
|
|
||
| Args: | ||
| alias (str): Optional. The room alias name to set for this room. | ||
| name (str): Optional. Name for new room. | ||
| is_public (bool): Optional. The public/private visibility. | ||
| invitees (list<str>): Optional. The list of user IDs to invite. | ||
| federate (bool): Optional. Сan a room be federated. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please include that this defaults to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already explained for Half-Shot why I left three options:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @non-Jedi included.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry to be anal about this, but the spec says that homeservers default to true on that parameter if it isn't included. It's not a synapse-specific behavior, and we need to document the behavior of a spec-compliant HS. So something like: "Defaults to True if not specified" is what I'm after here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's better?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. That's fine. Thanks @slipeer. I'll merge when I'm at a computer I've set up with ssh to my github profile. |
||
| Default to True. | ||
| """ | ||
| content = { | ||
| "visibility": "public" if is_public else "private" | ||
|
|
@@ -179,6 +189,10 @@ def create_room(self, alias=None, is_public=False, invitees=None): | |
| content["room_alias_name"] = alias | ||
| if invitees: | ||
| content["invite"] = invitees | ||
| if name: | ||
| content["name"] = name | ||
| if federate is not None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wrt above comment:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. feredate can be True, False or None. If it's None we do not add it to json. |
||
| content["creation_content"] = {'m.federate': federate} | ||
| return self._send("POST", "/createRoom", content) | ||
|
|
||
| def join_room(self, room_id_or_alias): | ||
|
|
@@ -233,6 +247,18 @@ def send_state_event(self, room_id, event_type, content, state_key="", | |
| params["ts"] = timestamp | ||
| return self._send("PUT", path, content, query_params=params) | ||
|
|
||
| def get_state_event(self, room_id, event_type): | ||
| """Perform GET /rooms/$room_id/state/$event_type | ||
|
|
||
| Args: | ||
| room_id(str): The room ID. | ||
| event_type (str): The type of the event. | ||
|
|
||
| Raises: | ||
| MatrixRequestError(code=404) if the state event is not found. | ||
| """ | ||
| return self._send("GET", "/rooms/{}/state/{}".format(quote(room_id), event_type)) | ||
|
|
||
| def send_message_event(self, room_id, event_type, content, txn_id=None, | ||
| timestamp=None): | ||
| """Perform PUT /rooms/$room_id/send/$event_type | ||
|
|
@@ -393,7 +419,7 @@ def get_room_name(self, room_id): | |
| Args: | ||
| room_id(str): The room ID | ||
| """ | ||
| return self._send("GET", "/rooms/" + room_id + "/state/m.room.name") | ||
| return self.get_state_event(room_id, "m.room.name") | ||
|
|
||
| def set_room_name(self, room_id, name, timestamp=None): | ||
| """Perform PUT /rooms/$room_id/state/m.room.name | ||
|
|
@@ -412,7 +438,7 @@ def get_room_topic(self, room_id): | |
| Args: | ||
| room_id (str): The room ID | ||
| """ | ||
| return self._send("GET", "/rooms/" + room_id + "/state/m.room.topic") | ||
| return self.get_state_event(room_id, "m.room.topic") | ||
|
|
||
| def set_room_topic(self, room_id, topic, timestamp=None): | ||
| """Perform PUT /rooms/$room_id/state/m.room.topic | ||
|
|
@@ -432,8 +458,7 @@ def get_power_levels(self, room_id): | |
| Args: | ||
| room_id(str): The room ID | ||
| """ | ||
| return self._send("GET", "/rooms/" + quote(room_id) + | ||
| "/state/m.room.power_levels") | ||
| return self.get_state_event(room_id, "m.room.power_levels") | ||
|
|
||
| def set_power_levels(self, room_id, content): | ||
| """Perform PUT /rooms/$room_id/state/m.room.power_levels | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| class OneTimeKeysManager(object): | ||
| """Handles one-time keys accounting for an OlmDevice.""" | ||
|
|
||
| def __init__(self, target_keys_number, signed_keys_proportion, keys_threshold): | ||
| self.target_counts = { | ||
| 'signed_curve25519': int(round(signed_keys_proportion * target_keys_number)), | ||
| 'curve25519': int(round((1 - signed_keys_proportion) * target_keys_number)), | ||
| } | ||
| self._server_counts = {} | ||
| self.to_upload = {} | ||
| self.keys_threshold = keys_threshold | ||
|
|
||
| @property | ||
| def server_counts(self): | ||
| return self._server_counts | ||
|
|
||
| @server_counts.setter | ||
| def server_counts(self, server_counts): | ||
| self._server_counts = server_counts | ||
| self.update_keys_to_upload() | ||
|
|
||
| def update_keys_to_upload(self): | ||
| for key_type, target_number in self.target_counts.items(): | ||
| num_keys = self._server_counts.get(key_type, 0) | ||
| num_to_create = max(target_number - num_keys, 0) | ||
| self.to_upload[key_type] = num_to_create | ||
|
|
||
| def should_upload(self): | ||
| if not self._server_counts: | ||
| return True | ||
| for key_type, target_number in self.target_counts.items(): | ||
| if self._server_counts.get(key_type, 0) < target_number * self.keys_threshold: | ||
| return True | ||
| return False | ||
|
|
||
| @property | ||
| def curve25519_to_upload(self): | ||
| return self.to_upload.get('curve25519', 0) | ||
|
|
||
| @property | ||
| def signed_curve25519_to_upload(self): | ||
| return self.to_upload.get('signed_curve25519', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like leaving booleans as
None. You should set this to true, so that users of the API know that federation is on by default. I'd actually argueNoneis closer toFalse, which is obviously dangerously wrong.https://matrix.org/docs/spec/client_server/unstable.html#m-room-create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my homeservet it's off by default...
I prefer to give a choice of three options: explicitly turn on, explicitly turn off and leave by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default given in the spec is
True. If your homeserver does something different then it's non-conforming?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discuss about SDK implementation or about settings on my homeserver? I gave an example to show that the behavior of the server is not always obvious and it is useful to give the developer the opportunity to explicitly set this parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should specify the matrix spec default in the methods docstring.
Noneis fine as default kwarg; we just need to be explicit that according to matrix spec this defaults toTruein the docstring. If servers have a setting that can change the default value of this and that's considered part of the spec, it's a spec bug that the setting isn't mentioned at the place @Half-Shot linked.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So based on a conversation with @ara4n in #matrix-spec:matrix.org, a server that had a default value other than
Truefor room federation would be non-compliant with the matrix spec. I'm fine with including this distinction betweenNoneandFalseif it's useful to you, but the docstring for this method needs to explicitly say thatTrueis the default.https://matrix.to/#/!YkZelGRiqijtzXZODa:matrix.org/$1531495739346932GaxTh:matrix.org