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

adding isSip function to room #2427

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/models/room.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2939,6 +2939,21 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
}
event.applyVisibilityEvent(visibilityChange);
}

/**
* Determines whether the given room is a sip room.
* @returns {boolean} True if the given the given room is a sip room.
*/
public isSip(): boolean {
for (const member of this.getMembers()) {
if(member.userId != this.client.getUserId()){
if(member.userId.startsWith("@_sip_")){
Copy link
Member

Choose a reason for hiding this comment

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

Where is this @_sip_ prefix specced?

Copy link
Author

@chagai95 chagai95 Jun 1, 2022

Choose a reason for hiding this comment

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

I don't think it's specced, how would you purpose we go about this?
Our users are all created with this prefix as per the config on the sip bridge, since it's not possible to change a mxid I think it's reasonable to hard code this for now.
https://gitlab.matrix.org/new-vector/voip/matrix-sip-bridge/-/blob/master/config_sample.yaml#L27

Do you perhaps have a better suggestion? 🙈

Copy link
Member

@t3chguy t3chguy Jun 2, 2022

Choose a reason for hiding this comment

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

That's specific to your config on that sip bridge, other sip bridges may have different config. SIP is also not a concept mentioned in the Matrix spec at all. This really cannot land in its current state, it'd be better done as part of the extensible profiles project matrix-org/matrix-spec-proposals#3795

A config is also just that configurable - so the prefix can change and the js-sdk would then do the wrong thing. Also what stops someone registering @_sip_real_user:matrix.org where now this would wrongly return true?

Copy link
Author

Choose a reason for hiding this comment

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

Ok thx! Can you maybe elaborate on how this is related to extensible profiles? We would like to identify if a room is a sip room or not, couldn't we spec that as a room type or something?

Copy link
Member

Choose a reason for hiding this comment

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

That is an alternative, though keep in mind room types are immutable, this function currently can change depending on the members in a room. Extensible profiles could make the sip bridge users have an is_sip flag

Copy link
Member

Choose a reason for hiding this comment

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

If it was just a bit of dead code probably not, if it was something more generic to hide rooms with types the app doesnt know then probably

Copy link
Author

Choose a reason for hiding this comment

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

Ok thanks! It's about creating a call list out of the sip rooms.

Copy link
Author

Choose a reason for hiding this comment

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

@t3chguy I hope it's ok for me to ping you, I was just wondering why not use m.bridge? I've seen that used in Beeper...

Copy link
Member

Choose a reason for hiding this comment

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

image

Just because one vendor uses it, doesn't make it specced. This is especially bad given the m. prefix which is reserved for specced things only

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thx for clarifying that!

return true;
}
}
}
return false;
}
}

/**
Expand Down