Skip to content
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

add hooks for m.call.* #201

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add hooks for m.call.* #201

wants to merge 3 commits into from

Conversation

mvgorcum
Copy link

@mvgorcum mvgorcum commented May 6, 2018

Signed-off-by: Mathijs van Gorcum [email protected]

Copy link
Collaborator

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. How difficult would it be to hook up the sdk so that it can speak webrtc natively? If we could do that, it would definitely be worthwhile to define some methods on the Room object.

@@ -621,6 +691,41 @@ def get_text_body(self, text, msgtype="m.text"):
"body": text
}

def get_call_invite_content(self, call_id, sdp, version, lifetime, types="offer"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this dictionary constructed under a separate method rather than just in the method body of send_call_invite?

Stylistically, if kept as a separate method, types kwarg should probably be type.

@@ -254,6 +254,76 @@ def send_message_event(self, room_id, event_type, content, txn_id=None,
params["ts"] = timestamp
return self._send("PUT", path, content, query_params=params)

def send_call_invite(self, room_id, call_id, sdp, version, lifetime,
msgtype="m.call.invite", timestamp=None):
"""Perform PUT /rooms/$room_id/send/m.room.call.invite
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is actually m.call.invite? https://matrix.org/speculator/spec/HEAD/client_server/unstable.html#module-voip

Yep, you've got it right in actual method; just need to fixup docstring.

@@ -254,6 +254,76 @@ def send_message_event(self, room_id, event_type, content, txn_id=None,
params["ts"] = timestamp
return self._send("PUT", path, content, query_params=params)

def send_call_invite(self, room_id, call_id, sdp, version, lifetime,
msgtype="m.call.invite", timestamp=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msgtype kwarg is never used. Probably just shouldn't be included in method signature at all.

Args:
room_id (str): The room ID to send the event in.
call_id (str): Call identifier string.
sdp (dict): Session Description Protocol dict.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From spec, looks like sdp is a string rather than a dict?

@@ -254,6 +254,76 @@ def send_message_event(self, room_id, event_type, content, txn_id=None,
params["ts"] = timestamp
return self._send("PUT", path, content, query_params=params)

def send_call_invite(self, room_id, call_id, sdp, version, lifetime,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version should probably be kwarg with default value of 0.

timestamp=timestamp
)

def send_call_answer(self, room_id, call_id, sdp, version, lifetime,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many comments from above appear to apply to all api methods.

@@ -222,6 +222,50 @@ def send_audio(self, url, name, **audioinfo):
return self.client.api.send_content(self.room_id, url, name, "m.audio",
extra_information=audioinfo)

def send_call_invite(self, call_id, sdp, version, lifetime):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't offer any additional abstraction over methods defined in api class. Until we have a reason to put methods here that make interacting with the api methods easier, probably just delete these methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants