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 CallNotifyEvent to support matrixRTC ringing #3878

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Nov 14, 2023

This adds the CallNotify event type and the corresponding ICallNotifyContent

Also this adds a sessionId to matrixRTCSessions. The sessionId is the value of call_id of the first membership. The memberships for a session are filtered by that id. So we can take either membership to calculate the sessionId

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Add CallNotifyEvent to support matrixRTC ringing (#3878). Contributed by @toger5.

@github-actions github-actions bot deployed to PR Documentation Preview November 14, 2023 15:42 Active
@toger5 toger5 marked this pull request as ready for review November 14, 2023 19:10
@toger5 toger5 requested review from a team as code owners November 14, 2023 19:10
@toger5 toger5 requested review from dbkr and florianduros November 14, 2023 19:10
@toger5 toger5 force-pushed the toger5/call-notify-event-ringing branch from 7e76cf4 to e7ac459 Compare November 14, 2023 19:10
@github-actions github-actions bot deployed to PR Documentation Preview November 14, 2023 19:14 Active
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

The bulk of this change seems to be adding the session ID I think. I see the desire to export a common type for the notify events in js-sdk but it seems a bit weird for it to just export the type and then never actually use it?

@@ -78,6 +78,9 @@ export type MatrixRTCSessionEventHandlerMap = {
* This class doesn't deal with media at all, just membership & properties of a session.
*/
export class MatrixRTCSession extends TypedEventEmitter<MatrixRTCSessionEvent, MatrixRTCSessionEventHandlerMap> {
// The session Id of the call, this is the call_id of the call Member event.
Copy link
Member

Choose a reason for hiding this comment

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

I would make this clear that this is only for use within the client and won't necessarily be the same between different clients or the same client restarted. Also probably want docs on the getter as that's where the public-facing API is.

and probably, "the oldest member event at the time the MatrixRTCSession object was created", although having read further I guess this isn't really correct either. "The first membership the MatrixRTCSession sees?"

Copy link
Member

Choose a reason for hiding this comment

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

Actually reading the react-sdk PR for context, isn't this just callId? ie. the discriminator used to identify different user calls in the same room? I would just call it callId rather than making up new terminology for it? The callIds for memberships in a session must be the same by definition, or they would belong in a different session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes exactly this is just the call_id from the matrix rtc session.
Since the MatrixRTCSession object is very generic I felt like having call specifics in here is odd.

@toger5 toger5 requested a review from dbkr November 15, 2023 10:49
@github-actions github-actions bot deployed to PR Documentation Preview November 15, 2023 10:50 Active
@toger5 toger5 added this pull request to the merge queue Nov 15, 2023
Merged via the queue into develop with commit 10cd84a Nov 15, 2023
@toger5 toger5 deleted the toger5/call-notify-event-ringing branch November 15, 2023 12:01
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.

None yet

2 participants