Defer to Element Call for skipping the lobby when starting or joining a call #30833
Conversation
| room_id: roomId, | ||
| view_call: true, | ||
| skipLobby: "shiftKey" in ev ? ev.shiftKey : false, | ||
| skipLobby: "shiftKey" in ev ? ev.shiftKey : undefined, |
There was a problem hiding this comment.
should we just do true here? Otherwise it might be set to false specificly? Wouldnt the better fallback be the intent?
There was a problem hiding this comment.
The undefined is the fallback to the intent. I.e. this skips if shift is held down, otherwise falls back to the intent (As per the test).
There was a problem hiding this comment.
The undefined part makes sense. I am afraid of ev = {..., shiftKey: false} where we should go back to the intent but acutally overwrite it to false.
So I am wondering if skipLobby: "shiftKey" in ev ? (ev.shiftKey? true: undefined) : undefined, is the better option?
There was a problem hiding this comment.
Or maybe a better way to phrase that?
"shiftKey" in ev && ev.shiftKey ? true:undefined
There was a problem hiding this comment.
OH, I see. Yes, I though that was what it was doing :)
There was a problem hiding this comment.
Can ev.shiftKey ever be false? I dont know what exactly is allowed in the webapi?
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/shiftKey would imply it's always a defined boolean 🤔
There was a problem hiding this comment.
Property 'shiftKey' does not exist on type 'ButtonEvent'.
Property 'shiftKey' does not exist on type 'FormEvent<Element>'.ts(2339)
It's a types thing. I wonder if that's true, but I think it's a harmless test.
src/models/Call.ts
Outdated
| private static generateWidgetUrl(client: MatrixClient, roomId: string): URL { | ||
| const baseUrl = window.location.href; | ||
| let url = new URL("./widgets/element-call/index.html#", baseUrl); // this strips hash fragment from baseUrl | ||
| private static appendCallNotifIntent(params: URLSearchParams, client: MatrixClient, roomId: string): void { |
There was a problem hiding this comment.
rename to what does the notif here mean?
I think just using the word callIntent or appendIntentParam would make more sense?
Notif might be misunderstood as setting sendNotificationType url param
There was a problem hiding this comment.
Naming is hard!
I decided to go for appendRoomParams with the intention being that all params within are specific to the context of the room. It's not ideal, but probably does as expected?
toger5
left a comment
toger5
left a comment
There was a problem hiding this comment.
Looks good. I think the only thing we need is the returnToLobby for video rooms. See comment.
| private onRTCSessionStart = (roomId: string, session: MatrixRTCSession): void => { | ||
| this.updateRoom(session.room); | ||
| }; |
There was a problem hiding this comment.
Without this event handler the app won't advertise the presence of calls started by other users or devices - the telltale sign of this is that when you start a call, the other Element Web user will not see a 'Join' button in the room header or a video icon in the room list. They'll only see the notification toast (if they look at their client within the timeout).
There was a problem hiding this comment.
Got it, so actually this might be a refactor too far. Let's see if I can disable widget creation while doing so.
Also, let's add a proper test to catch this.
| // XXX: @element-hq/element-call-embedded <= 0.15.0 sets the wrong parameter for | ||
| // preload by default so we override here. This can be removed when that package | ||
| // is released and upgraded. |
There was a problem hiding this comment.
As in all four params.append("preload", "false"); lines? Also, why not just one?
|
Replaced by #30848 |
This removes support for Element Call to preload the widget, allowing synchronous setting up of the widget rather than doing so in parallel. This is actually helpful as it now simplifies the call creation logic, and allows us to pass in parameters based on the way the user joins the call. The upshot of all of this is we can now rely on Element Call's intent feature to properly set
skipLobbyrather than hardcoding the value.We discussed the preloading of widgets internally, but came to the conclusion that since the widget is now embedded with Element Web it makes less sense to preload, and causes a lot of problems for us.
Checklist
public/exportedsymbols have accurate TSDoc documentation.