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

Refactor CallCapabilities #1170

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
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
46 changes: 23 additions & 23 deletions packages/core/src/types/fabric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,34 +19,34 @@ export * from './fabricRoomSession'
export * from './fabricMember'
export * from './fabricLayout'

interface CapabilityOnOffState {
on?: true
off?: true
export interface CapabilityOnOffState {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind moving this to the js/src/fabric folder since it's only required in Fabric and is not associated with any SDK event?

We can probably create a new file here packages/js/src/fabric/interfaces. We also need to export the CallCapabilities type from the JS package.

on: boolean
off: boolean
}

interface MemberCapability {
muteAudio?: CapabilityOnOffState
muteVideo?: CapabilityOnOffState
microphoneVolume?: true
microphoneSensitivity?: true
speakerVolume?: true
deaf?: CapabilityOnOffState
raisehand?: CapabilityOnOffState
position?: true
meta?: true
remove?: true
export interface MemberCapability {
muteAudio: CapabilityOnOffState
iAmmar7 marked this conversation as resolved.
Show resolved Hide resolved
muteVideo: CapabilityOnOffState
microphoneVolume: boolean
microphoneSensitivity: boolean
speakerVolume: boolean
deaf: CapabilityOnOffState
raisehand: CapabilityOnOffState
position: boolean
meta: boolean
remove: boolean
}

export interface CallCapabilities {
self?: MemberCapability
member?: MemberCapability
end?: true
setLayout?: true
sendDigit?: true
vmutedHide?: CapabilityOnOffState
lock?: CapabilityOnOffState
device?: true
screenshare?: true
self: MemberCapability
member: MemberCapability
end: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be not present in the docs, probably a typo?

setLayout: boolean
iAmmar7 marked this conversation as resolved.
Show resolved Hide resolved
sendDigit: boolean
iAmmar7 marked this conversation as resolved.
Show resolved Hide resolved
vmutedHide: CapabilityOnOffState
lock: CapabilityOnOffState
device: boolean
screenshare: boolean
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/types/fabricRoomSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
SwEvent,
CallState,
CallPlay,
CallConnect,
CallConnect
} from '..'

/**
Expand Down Expand Up @@ -506,7 +506,7 @@ export interface CallJoinedEventParams {
member_id: string
node_id?: string
origin_call_id: string
capabilities: string[] // TODO: More stronger type is required through server
capabilities: string[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add back this todo? So that we remember to update this later.

}

export interface CallJoinedEvent extends SwEvent {
Expand Down
300 changes: 198 additions & 102 deletions packages/core/src/utils/eventUtils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { CallCapabilities } from '@signalwire/core'
import {
CallCapabilities,
CapabilityOnOffState,
MemberCapability,
} from '@signalwire/core'

export const stripNamespacePrefix = (
event: string,
Expand All @@ -16,109 +20,201 @@ export const stripNamespacePrefix = (
return event
}

const capabilityStringToObjectMap = {
device: { device: true } as CallCapabilities,
'digit.send': { sendDigit: true } as CallCapabilities,
end: { end: true } as CallCapabilities,
'layout.set': { setLayout: true } as CallCapabilities,
screenshare: { screenshare: true } as CallCapabilities,
'vmuted.hide.on': { vmutedHide: { on: true } } as CallCapabilities,
'vmuted.hide.off': { vmutedHide: { off: true } } as CallCapabilities,
'lock.on': { lock: { on: true } } as CallCapabilities,
'lock.off': { lock: { off: true } } as CallCapabilities,
'member.position.set': { member: { position: true } } as CallCapabilities,
'member.meta': { member: { meta: true } } as CallCapabilities,
'member.remove': { member: { remove: true } } as CallCapabilities,
'member.microphone.volume.set': {
member: { microphoneVolume: true },
} as CallCapabilities,
'member.microphone.sensitivity.set': {
member: { microphoneSensitivity: true },
} as CallCapabilities,
'member.speaker.volume.set': {
member: { speakerVolume: true },
} as CallCapabilities,
'member.deaf.on': { member: { deaf: { on: true } } } as CallCapabilities,
'member.deaf.off': { member: { deaf: { off: true } } } as CallCapabilities,
'member.mute.audio.on': {
member: { muteAudio: { on: true } },
} as CallCapabilities,
'member.mute.audio.off': {
member: { muteAudio: { off: true } },
} as CallCapabilities,
'member.mute.video.on': {
member: { muteVideo: { on: true } },
} as CallCapabilities,
'member.mute.video.off': {
member: { muteVideo: { off: true } },
} as CallCapabilities,
'member.raisehand.on': {
member: { raisehand: { on: true } },
} as CallCapabilities,
'member.raisehand.off': {
member: { raisehand: { off: true } },
} as CallCapabilities,
'self.position.set': { self: { position: true } } as CallCapabilities,
'self.meta': { self: { meta: true } } as CallCapabilities,
'self.remove': { self: { remove: true } } as CallCapabilities,
'self.microphone': {
self: { microphoneVolume: true, microphoneSensitivity: true },
} as CallCapabilities,
'self.microphone.volume.set': {
self: { microphoneVolume: true },
} as CallCapabilities,
'self.microphone.sensitivity.set': {
self: { microphoneSensitivity: true },
} as CallCapabilities,
'self.speaker.volume.set': {
self: { speakerVolume: true },
} as CallCapabilities,
'self.deaf.on': { self: { deaf: { on: true } } } as CallCapabilities,
'self.deaf.off': { self: { deaf: { off: true } } } as CallCapabilities,
'self.mute.audio.on': {
self: { muteAudio: { on: true } },
} as CallCapabilities,
'self.mute.audio.off': {
self: { muteAudio: { off: true } },
} as CallCapabilities,
'self.mute.video.on': {
self: { muteVideo: { on: true } },
} as CallCapabilities,
'self.mute.video.off': {
self: { muteVideo: { off: true } },
} as CallCapabilities,
'self.raisehand.on': {
self: { raisehand: { on: true } },
} as CallCapabilities,
'self.raisehand.off': {
self: { raisehand: { off: true } },
} as CallCapabilities,
class CapabilityOnOffStateImpl implements CapabilityOnOffState {
constructor(private _flags: string[]) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This thing can also be moved to a dedicated CallCapabilities file inside the js/src/fabric.


get on(): boolean {
return this._flags.some((flag) =>
/^(.*\.on|(?:(?!.*\.off$).*))$/.test(flag)
)
}
Comment on lines +27 to +29
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be updated into simpler JS?

Suggested change
return this._flags.some((flag) =>
/^(.*\.on|(?:(?!.*\.off$).*))$/.test(flag)
)
return this._flags.some(flag => !flag.endsWith('.off'));


get off(): boolean {
return this._flags.some((flag) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Also, the return type "boolean" is redundant here considering the class implements a certain TS contract. It will automatically throw errors if the Class does not implement what's mentioned in the Contract.

/^(.*\.off|(?:(?!.*\.on$).*))$/.test(flag)
)
}
}

class MemberCapabilityImpl implements MemberCapability {
private _muteAudio?: CapabilityOnOffStateImpl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using the suffix "Impl", you can create a class with the normal name such as "MemberCapability" and use the suffix "Contract" for the TS interface such as "MemberCapabilityContract".

That's what the SDK does with other classes.

private _muteVideo?: CapabilityOnOffStateImpl
private _deaf?: CapabilityOnOffStateImpl
private _raisehand?: CapabilityOnOffStateImpl

Comment on lines +40 to +43
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here; maybe I am wrong, but I tested with the below approach, and the test seems to be green.

Suggested change
private _muteAudio?: CapabilityOnOffStateImpl
private _muteVideo?: CapabilityOnOffStateImpl
private _deaf?: CapabilityOnOffStateImpl
private _raisehand?: CapabilityOnOffStateImpl
private _muteAudio: CapabilityOnOffStateImpl
private _muteVideo: CapabilityOnOffStateImpl
private _deaf: CapabilityOnOffStateImpl
private _raisehand: CapabilityOnOffStateImpl

constructor(
private _flags: string[],
private _memberType: 'self' | 'member'
) {}

get muteAudio(): CapabilityOnOffState {
this._muteAudio =
this._muteAudio ??
new CapabilityOnOffStateImpl(
this._flags.filter(
(flag) =>
flag === this._memberType ||
flag === `${this._memberType}.mute` ||
flag.startsWith(`${this._memberType}.mute.audio`)
)
)
return this._muteAudio!
}

Comment on lines +50 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Do we need this check "this._muteAudio ??"?

  2. Also, do we need to check flag === this._memberType? The memberType in this class is already either "self" or "member" passed from here: https://github.com/signalwire/signalwire-js/pull/1170/files#diff-5846952bebcbcf210739ba06b086157c8e9f575ee84844a5efe9c80dfecdc85aR164

Maybe we can simplify this?

Suggested change
get muteAudio(): CapabilityOnOffState {
this._muteAudio =
this._muteAudio ??
new CapabilityOnOffStateImpl(
this._flags.filter(
(flag) =>
flag === this._memberType ||
flag === `${this._memberType}.mute` ||
flag.startsWith(`${this._memberType}.mute.audio`)
)
)
return this._muteAudio!
}
get muteAudio() {
this._muteAudio = new CapabilityOnOffStateImpl(
this._flags.filter(
(flag) =>
flag === `${this._memberType}.mute` ||
flag.startsWith(`${this._memberType}.mute.audio`)
)
)
return this._muteAudio
}

get muteVideo(): CapabilityOnOffState {
this._muteVideo =
this._muteVideo ??
new CapabilityOnOffStateImpl(
this._flags.filter(
(flag) =>
flag === this._memberType ||
flag === `${this._memberType}.mute` ||
flag.startsWith(`${this._memberType}.mute.video`)
)
)
return this._muteVideo!
}

get microphoneVolume(): boolean {
return this._flags.some(
(flag) =>
flag === this._memberType ||
flag === `${this._memberType}.microphone` ||
flag.startsWith(`${this._memberType}.microphone.volume`)
)
}

get microphoneSensitivity(): boolean {
return this._flags.some(
(flag) =>
flag === this._memberType ||
flag === `${this._memberType}.microphone` ||
flag.startsWith(`${this._memberType}.microphone.sensitivity`)
)
}

get speakerVolume(): boolean {
return this._flags.some(
(flag) =>
flag === this._memberType ||
flag === `${this._memberType}.speaker` ||
flag.startsWith(`${this._memberType}.speaker.volume`)
)
}

get deaf(): CapabilityOnOffState {
this._deaf =
this._deaf ??
new CapabilityOnOffStateImpl(
this._flags.filter(
(flag) =>
flag === this._memberType ||
flag.startsWith(`${this._memberType}.deaf`)
)
)
return this._deaf!
}

get raisehand(): CapabilityOnOffState {
this._raisehand =
Copy link
Collaborator

Choose a reason for hiding this comment

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

this._raisehand ??
new CapabilityOnOffStateImpl(
this._flags.filter(
(flag) =>
flag === this._memberType ||
flag.startsWith(`${this._memberType}.raisehand`)
)
)
return this._raisehand!
}

get position(): boolean {
return this._flags.some(
(flag) =>
flag === this._memberType ||
flag.startsWith(`${this._memberType}.position`)
)
}

get meta(): boolean {
return this._flags.some(
(flag) =>
flag === this._memberType || flag.startsWith(`${this._memberType}.meta`)
)
}

get remove(): boolean {
return this._flags.some(
Copy link
Collaborator

Choose a reason for hiding this comment

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

No capability for this as well.

(flag) =>
flag === this._memberType ||
flag.startsWith(`${this._memberType}.remove`)
)
}
}

const isObjectGuard = (o: unknown): o is Object => o instanceof Object

const capabilityStringToObjects = (capability: string) =>
Object.entries(capabilityStringToObjectMap)
.filter(([key]) => key.startsWith(capability))
.map(([_, value]) => value)

const mergeCapabilityObjects = <T extends Object>(target: T, source: T): T => {
const result = { ...target }
for (const key in source) {
if (
result[key] &&
isObjectGuard(target[key]) &&
isObjectGuard(source[key])
) {
result[key] = mergeCapabilityObjects(target[key], source[key])
} else {
result[key] = source[key]
}
}
return result
class CallCapabilitiesImpl implements CallCapabilities {
private _self?: MemberCapability
iAmmar7 marked this conversation as resolved.
Show resolved Hide resolved
private _member?: MemberCapability
private _vmutedHide?: CapabilityOnOffStateImpl
private _lock?: CapabilityOnOffStateImpl

Comment on lines +156 to +159
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these need to be undefined? The SDK seems to always return all the capabilities, which is, I guess, a good approach. Considering that, we might not these "?".

Suggested change
private _self?: MemberCapability
private _member?: MemberCapability
private _vmutedHide?: CapabilityOnOffStateImpl
private _lock?: CapabilityOnOffStateImpl
private _self: MemberCapability
private _member: MemberCapability
private _vmutedHide: CapabilityOnOffStateImpl
private _lock: CapabilityOnOffStateImpl

constructor(private _flags: string[]) {}

private _buildMemberCapacity(memberType: 'self' | 'member') {
return new MemberCapabilityImpl(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
private _buildMemberCapacity(memberType: 'self' | 'member') {
private _buildMemberCapability(memberType: 'self' | 'member') {

this._flags.filter((flag) => flag.startsWith(memberType)),
memberType
)
}

get self(): MemberCapability {
this._self = this._self ?? this._buildMemberCapacity('self')
return this._self!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mind sharing why the "this._wathever ??" check is required in this whole capability log? We receive the list of capabilities from the server, and we just map it into an object. Isn't that right? Then why do we need this "??" check? Maybe I am missing some obvious point.

}

get member(): MemberCapability {
this._member = this._member ?? this._buildMemberCapacity('member')
return this._member!
}

get end(): boolean {
return this._flags.some((capability) => capability === 'end')
Copy link
Collaborator

Choose a reason for hiding this comment

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

}

get setLayout(): boolean {
return this._flags.some((capability) => capability.startsWith('layout'))
}

get sendDigit(): boolean {
return this._flags.some((capability) => capability.startsWith('digit'))
}

get vmutedHide(): CapabilityOnOffState {
this._vmutedHide =
this._vmutedHide ??
new CapabilityOnOffStateImpl(
this._flags.filter((flag) => flag.startsWith('vmuted'))
)
return this._vmutedHide!
}

get lock(): CapabilityOnOffState {
this._lock =
this._lock ??
new CapabilityOnOffStateImpl(
this._flags.filter((flag) => flag.startsWith('lock'))
)
return this._lock!
}

get device(): boolean {
return this._flags.some((capability) => capability === 'device')
}

get screenshare(): boolean {
return this._flags.some((capability) => capability === 'screenshare')
}
}

export const mapCapabilityPayload = (capabilities: string[]) =>
capabilities
.flatMap(capabilityStringToObjects)
.reduce<CallCapabilities>(mergeCapabilityObjects, {} as CallCapabilities)
new CallCapabilitiesImpl(capabilities)
Loading
Loading