Show error screens in group calls #29254
Conversation
| Disconnected = "disconnected", | ||
|
|
||
| Connecting = "connecting", | ||
| Connected = "connected", | ||
| Disconnecting = "disconnecting", |
There was a problem hiding this comment.
why do we drop the connecting state but not the disconnecting?
I think it still could be useful to display the connecting state in the room list tile.
Is there any way we can compute this indirectly?
But I see, that we can still tell if its connected or not by using the presented. (which is the more important state)
There was a problem hiding this comment.
This is not really a review since its still in draft. It is mostly that I wanted to get myself familiar with the general solution. It looks really good to me. Addresses a lot of things that were wrong about the EW call logic.
(Probably the comments are not that helpful since you would address the things anyways.)
| // A call ceases to exist as soon as all participants leave and also the | ||
| // user isn't looking at it (for example, waiting in an empty lobby) |
There was a problem hiding this comment.
This is very helpful to follow the logic
| useTypedEventEmitter( | ||
| call, | ||
| CallEvent.Presentation, | ||
| useCallback( | ||
| (presented: boolean) => { | ||
| if (!presented && mounted.current) onClose(); | ||
| }, | ||
| [onClose], | ||
| ), | ||
| ); |
There was a problem hiding this comment.
I think this can also get comment phrasing out the logic and why just this is enough.
| private onTileLayout = async (ev: CustomEvent<IWidgetApiRequest>): Promise<void> => { | ||
| ev.preventDefault(); | ||
| this.layout = Layout.Tile; | ||
| await this.messaging!.transport.reply(ev.detail, {}); // ack | ||
| this.messaging!.transport.reply(ev.detail, {}); // ack | ||
| }; | ||
|
|
||
| private onSpotlightLayout = async (ev: CustomEvent<IWidgetApiRequest>): Promise<void> => { | ||
| ev.preventDefault(); | ||
| this.layout = Layout.Spotlight; | ||
| await this.messaging!.transport.reply(ev.detail, {}); // ack | ||
| this.messaging!.transport.reply(ev.detail, {}); // ack | ||
| }; | ||
|
|
There was a problem hiding this comment.
Do you think this is the right place to start deprecating those actions?
It seems design and product have moved away from this on all platforms for some time.
(Or do we still use this.layout)
I think a first step could be to deprecate the this.layout field.
src/stores/CallStore.ts
Outdated
| @@ -32,6 +32,7 @@ export class CallStore extends AsyncStoreWithClient<EmptyObject> { | |||
| if (!this._instance) { | |||
| this._instance = new CallStore(); | |||
| this._instance.start(); | |||
| window.robincallstore = this._instance; | |||
There was a problem hiding this comment.
just a reminder that this debugging artifact is still here.
src/stores/RoomViewStore.tsx
Outdated
| if (CallStore.instance.getCall(payload.room_id) === null) | ||
| ElementCall.create(room, false).then(() => { | ||
| const call = CallStore.instance.getCall(payload.room_id!)!; | ||
| call.presented = true; |
There was a problem hiding this comment.
Do we really only need to call this is if CallStore.instance.getCall(payload.room_id) === null
If there is already a rtc session and therefore getCall() !== null we still want to set presented to true + call call.start().
robintown
force-pushed
the
robin/call-errors
branch
from
dbc8d2e to
29145c6
Compare
We often want calls to exist even when no more participants are left in the MatrixRTC session. So, we should avoid destroying calls as long as they're being presented in the UI; this means that the user has an intent to either join the call or continue looking at an error screen, and we shouldn't interrupt that interaction. The RoomViewStore is now what takes care of creating and destroying calls, rather than the CallView. In general it seems kinda impossible to safely create and destroy model objects from React lifecycle hooks, so moving this responsibility to a store seemed appropriate and resolves existing issues with calls in React strict mode.
This creates a distinction between the user hanging up and the widget being ready to close, which is useful for allowing Element Call to show error screens when disconnected from the call, for example.
robintown
force-pushed
the
robin/call-errors
branch
from
29145c6 to
1c21949
Compare
This refactors the lifecycle management of the Call class a little bit to allow the UI to continue presenting a call as long as it wants, even if the MatrixRTC session has ended. This is primarily so we can keep the Element Call widget visible in cases where we've left the call but are still showing an error screen—but it's had other benefits, like fixing the infinite loop encountered in React's strict mode and avoiding a spurious reload of the lobby screen in certain cases.
Closes #29196
Closes element-hq/element-call#2955