From 7d062387b7a82d63277b24f4a026bfa6f6e73b09 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 29 Mar 2023 14:27:49 +0100 Subject: [PATCH] Revert "Add the call object to Call events" --- spec/test-utils/webrtc.ts | 7 -- spec/unit/webrtc/call.spec.ts | 62 +-------------- spec/unit/webrtc/callFeed.spec.ts | 2 +- spec/unit/webrtc/groupCall.spec.ts | 6 +- src/webrtc/call.ts | 116 ++++++++++------------------- src/webrtc/groupCall.ts | 7 +- 6 files changed, 48 insertions(+), 152 deletions(-) diff --git a/spec/test-utils/webrtc.ts b/spec/test-utils/webrtc.ts index 46b90b7a51f..c1a2c2436c8 100644 --- a/spec/test-utils/webrtc.ts +++ b/spec/test-utils/webrtc.ts @@ -123,7 +123,6 @@ export class MockRTCPeerConnection { public iceCandidateListener?: (e: RTCPeerConnectionIceEvent) => void; public iceConnectionStateChangeListener?: () => void; public onTrackListener?: (e: RTCTrackEvent) => void; - public onDataChannelListener?: (ev: RTCDataChannelEvent) => void; public needsNegotiation = false; public readyToNegotiate: Promise; private onReadyToNegotiate?: () => void; @@ -169,8 +168,6 @@ export class MockRTCPeerConnection { this.iceConnectionStateChangeListener = listener; } else if (type == "track") { this.onTrackListener = listener; - } else if (type == "datachannel") { - this.onDataChannelListener = listener; } } public createDataChannel(label: string, opts: RTCDataChannelInit) { @@ -235,10 +232,6 @@ export class MockRTCPeerConnection { this.negotiationNeededListener(); } } - - public triggerIncomingDataChannel(): void { - this.onDataChannelListener?.({ channel: {} } as RTCDataChannelEvent); - } } export class MockRTCRtpSender { diff --git a/spec/unit/webrtc/call.spec.ts b/spec/unit/webrtc/call.spec.ts index db84ee54004..51798d441af 100644 --- a/spec/unit/webrtc/call.spec.ts +++ b/spec/unit/webrtc/call.spec.ts @@ -431,33 +431,6 @@ describe("Call", function () { expect(transceivers.get("m.usermedia:video")!.sender.track!.id).toBe("usermedia_video_track"); }); - it("should handle error on call upgrade", async () => { - const onError = jest.fn(); - call.on(CallEvent.Error, onError); - - await startVoiceCall(client, call); - - await call.onAnswerReceived( - makeMockEvent("@test:foo", { - version: 1, - call_id: call.callId, - party_id: "party_id", - answer: { - sdp: DUMMY_SDP, - }, - [SDPStreamMetadataKey]: {}, - }), - ); - - const mockGetUserMediaStream = jest.fn().mockRejectedValue(new Error("Test error")); - client.client.getMediaHandler().getUserMediaStream = mockGetUserMediaStream; - - // then unmute which should cause an upgrade - await call.setLocalVideoMuted(false); - - expect(onError).toHaveBeenCalled(); - }); - it("should unmute video after upgrading to video call", async () => { // Regression test for https://github.com/vector-im/element-call/issues/925 await startVoiceCall(client, call); @@ -764,22 +737,11 @@ describe("Call", function () { const dataChannel = call.createDataChannel("data_channel_label", { id: 123 }); - expect(dataChannelCallback).toHaveBeenCalledWith(dataChannel, call); + expect(dataChannelCallback).toHaveBeenCalledWith(dataChannel); expect(dataChannel.label).toBe("data_channel_label"); expect(dataChannel.id).toBe(123); }); - it("should emit a data channel event when the other side adds a data channel", async () => { - await startVoiceCall(client, call); - - const dataChannelCallback = jest.fn(); - call.on(CallEvent.DataChannel, dataChannelCallback); - - (call.peerConn as unknown as MockRTCPeerConnection).triggerIncomingDataChannel(); - - expect(dataChannelCallback).toHaveBeenCalled(); - }); - describe("supportsMatrixCall", () => { it("should return true when the environment is right", () => { expect(supportsMatrixCall()).toBe(true); @@ -1642,7 +1604,7 @@ describe("Call", function () { hasAdvancedBy += advanceBy; expect(lengthChangedListener).toHaveBeenCalledTimes(hasAdvancedBy); - expect(lengthChangedListener).toHaveBeenCalledWith(hasAdvancedBy, call); + expect(lengthChangedListener).toHaveBeenCalledWith(hasAdvancedBy); } }); @@ -1672,24 +1634,4 @@ describe("Call", function () { expect(call.hangup).not.toHaveBeenCalled(); }); }); - - describe("Call replace", () => { - it("Fires event when call replaced", async () => { - const onReplace = jest.fn(); - call.on(CallEvent.Replaced, onReplace); - - await call.placeVoiceCall(); - - const call2 = new MatrixCall({ - client: client.client, - roomId: FAKE_ROOM_ID, - }); - call2.on(CallEvent.Error, errorListener); - await fakeIncomingCall(client, call2); - - call.replacedBy(call2); - - expect(onReplace).toHaveBeenCalled(); - }); - }); }); diff --git a/spec/unit/webrtc/callFeed.spec.ts b/spec/unit/webrtc/callFeed.spec.ts index ea80d5267f2..803e4648ed6 100644 --- a/spec/unit/webrtc/callFeed.spec.ts +++ b/spec/unit/webrtc/callFeed.spec.ts @@ -102,7 +102,7 @@ describe("CallFeed", () => { [CallState.Connected, true], [CallState.Connecting, false], ])("should react to call state, when !isLocal()", (state: CallState, expected: Boolean) => { - call.emit(CallEvent.State, state, CallState.InviteSent, call.typed()); + call.emit(CallEvent.State, state); expect(feed.connected).toBe(expected); }); diff --git a/spec/unit/webrtc/groupCall.spec.ts b/spec/unit/webrtc/groupCall.spec.ts index 22af26e0ca6..031f8eef321 100644 --- a/spec/unit/webrtc/groupCall.spec.ts +++ b/spec/unit/webrtc/groupCall.spec.ts @@ -794,7 +794,7 @@ describe("Group Call", function () { call.isLocalVideoMuted = jest.fn().mockReturnValue(true); call.setLocalVideoMuted = jest.fn(); - call.emit(CallEvent.State, CallState.Connected, CallState.InviteSent, call); + call.emit(CallEvent.State, CallState.Connected); expect(call.setMicrophoneMuted).toHaveBeenCalledWith(false); expect(call.setLocalVideoMuted).toHaveBeenCalledWith(false); @@ -1154,7 +1154,7 @@ describe("Group Call", function () { }); it("handles regular case", () => { - oldMockCall.emit(CallEvent.Replaced, newMockCall.typed(), oldMockCall.typed()); + oldMockCall.emit(CallEvent.Replaced, newMockCall.typed()); expect(oldMockCall.hangup).toHaveBeenCalled(); expect(callChangedListener).toHaveBeenCalledWith(newCallsMap); @@ -1165,7 +1165,7 @@ describe("Group Call", function () { it("handles case where call is missing from the calls map", () => { // @ts-ignore groupCall.calls = new Map(); - oldMockCall.emit(CallEvent.Replaced, newMockCall.typed(), oldMockCall.typed()); + oldMockCall.emit(CallEvent.Replaced, newMockCall.typed()); expect(oldMockCall.hangup).toHaveBeenCalled(); expect(callChangedListener).toHaveBeenCalledWith(newCallsMap); diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index 80851909fa8..1c75e92f0b0 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -296,34 +296,20 @@ export interface VoipEvent { content: Record; } -/** - * These now all have the call object as an argument. Why? Well, to know which call a given event is - * about you have three options: - * 1. Use a closure as the callback that remembers what call it's listening to. This can be - * a pain because you need to pass the listener function again when you remove the listener, - * which might be somewhere else. - * 2. Use not-very-well-known fact that EventEmitter sets 'this' to the emitter object in the - * callback. This doesn't really play well with modern Typescript and eslint and doesn't work - * with our pattern of re-emitting events. - * 3. Pass the object in question as an argument to the callback. - * - * Now that we have group calls which have to deal with multiple call objects, this will - * become more important, and I think methods 1 and 2 are just going to cause issues. - */ export type CallEventHandlerMap = { - [CallEvent.DataChannel]: (channel: RTCDataChannel, call: MatrixCall) => void; - [CallEvent.FeedsChanged]: (feeds: CallFeed[], call: MatrixCall) => void; - [CallEvent.Replaced]: (newCall: MatrixCall, oldCall: MatrixCall) => void; - [CallEvent.Error]: (error: CallError, call: MatrixCall) => void; - [CallEvent.RemoteHoldUnhold]: (onHold: boolean, call: MatrixCall) => void; - [CallEvent.LocalHoldUnhold]: (onHold: boolean, call: MatrixCall) => void; - [CallEvent.LengthChanged]: (length: number, call: MatrixCall) => void; - [CallEvent.State]: (state: CallState, oldState: CallState, call: MatrixCall) => void; + [CallEvent.DataChannel]: (channel: RTCDataChannel) => void; + [CallEvent.FeedsChanged]: (feeds: CallFeed[]) => void; + [CallEvent.Replaced]: (newCall: MatrixCall) => void; + [CallEvent.Error]: (error: CallError) => void; + [CallEvent.RemoteHoldUnhold]: (onHold: boolean) => void; + [CallEvent.LocalHoldUnhold]: (onHold: boolean) => void; + [CallEvent.LengthChanged]: (length: number) => void; + [CallEvent.State]: (state: CallState, oldState?: CallState) => void; [CallEvent.Hangup]: (call: MatrixCall) => void; - [CallEvent.AssertedIdentityChanged]: (call: MatrixCall) => void; + [CallEvent.AssertedIdentityChanged]: () => void; /* @deprecated */ [CallEvent.HoldUnhold]: (onHold: boolean) => void; - [CallEvent.SendVoipEvent]: (event: VoipEvent, call: MatrixCall) => void; + [CallEvent.SendVoipEvent]: (event: VoipEvent) => void; }; // The key of the transceiver map (purpose + media type, separated by ':') @@ -473,7 +459,7 @@ export class MatrixCall extends TypedEventEmittererror), - this, ); } } @@ -1528,7 +1513,7 @@ export class MatrixCall extends TypedEventEmittererror), this); + this.emit(CallEvent.Error, new CallError(code, message, error)); throw error; } @@ -2002,7 +1987,7 @@ export class MatrixCall extends TypedEventEmitter { this.makingOffer = true; try { - // XXX: in what situations do we believe gotLocalOffer actually throws? It appears - // to handle most of its exceptions itself and terminate the call. I'm not entirely - // sure it would ever throw, so I can't add a test for these lines. - // Also the tense is different between "gotLocalOffer" and "getLocalOfferFailed" so - // it's not entirely clear whether getLocalOfferFailed is just misnamed or whether - // they've been cross-polinated somehow at some point. await this.gotLocalOffer(); } catch (e) { this.getLocalOfferFailed(e as Error); @@ -2155,7 +2134,7 @@ export class MatrixCall extends TypedEventEmittererror), this); + this.emit(CallEvent.Error, new CallError(code, message, error)); this.terminate(CallParty.Local, code, false); // no need to carry on & send the candidate queue, but we also @@ -2179,11 +2158,7 @@ export class MatrixCall extends TypedEventEmitter { logger.error(`Call ${this.callId} getLocalOfferFailed() running`, err); - this.emit( - CallEvent.Error, - new CallError(CallErrorCode.LocalOfferFailed, "Failed to get local offer!", err), - this, - ); + this.emit(CallEvent.Error, new CallError(CallErrorCode.LocalOfferFailed, "Failed to get local offer!", err)); this.terminate(CallParty.Local, CallErrorCode.LocalOfferFailed, false); }; @@ -2202,7 +2177,6 @@ export class MatrixCall extends TypedEventEmitter { - this.emit(CallEvent.LengthChanged, Math.round((Date.now() - this.callStartTime!) / 1000), this); + this.emit(CallEvent.LengthChanged, Math.round((Date.now() - this.callStartTime!) / 1000)); }, CALL_LENGTH_INTERVAL); } } else if (this.peerConn?.iceConnectionState == "failed") { @@ -2293,7 +2267,7 @@ export class MatrixCall extends TypedEventEmitter { - this.emit(CallEvent.DataChannel, ev.channel, this); + this.emit(CallEvent.DataChannel, ev.channel); }; /** @@ -2406,17 +2380,13 @@ export class MatrixCall extends TypedEventEmittererror), this); + this.emit(CallEvent.Error, new CallError(code, message, error)); this.hangup(code, false); return; diff --git a/src/webrtc/groupCall.ts b/src/webrtc/groupCall.ts index d7bf15e99a6..adae5cac6bf 100644 --- a/src/webrtc/groupCall.ts +++ b/src/webrtc/groupCall.ts @@ -40,11 +40,6 @@ export enum GroupCallTerminationReason { CallEnded = "call_ended", } -/** - * Because event names are just strings, they do need - * to be unique over all event types of event emitter. - * Some objects could emit more then one set of events. - */ export enum GroupCallEvent { GroupCallStateChanged = "group_call_state_changed", ActiveSpeakerChanged = "active_speaker_changed", @@ -54,7 +49,7 @@ export enum GroupCallEvent { LocalScreenshareStateChanged = "local_screenshare_state_changed", LocalMuteStateChanged = "local_mute_state_changed", ParticipantsChanged = "participants_changed", - Error = "group_call_error", + Error = "error", } export type GroupCallEventHandlerMap = {