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.

Sign up for GitHub

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

[DRAFT] move enable sounds to the audio helper class and expose getter and setters to enable and disable toggeable sounds #115

Closed
wants to merge 1 commit into from
Closed

[DRAFT] move enable sounds to the audio helper class and expose getter and setters to enable and disable toggeable sounds #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 37 additions & 7 deletions lib/twilio/audiohelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ const kindAliases: Record<string, string> = {
audiooutput: 'Audio Output',
};

export const defaultEnabledSounds: Record<Device.ToggleableSound, boolean> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can remove these defaults from device if we want the AudioHelper class to have more control over the enablement of sounds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, didn't see the device changes, please ignore this comment! 😓

[Device.SoundName.Disconnect]: true,
[Device.SoundName.Incoming]: true,
[Device.SoundName.Outgoing]: true,
};

/**
* Provides input and output audio-based functionality in one convenient class.
* @publicapi
Expand All @@ -31,6 +37,23 @@ class AudioHelper extends EventEmitter {
*/
get audioConstraints(): MediaTrackConstraints | null { return this._audioConstraints; }

/**
* If no value is passed, it returns a boolean indicating whether the disconnect sound
* is enabled or disabled otherwise it enables or disables the disconnect event sound.
*/
[Device.SoundName.Disconnect]: EnableSoundFn;

/**
* If no value is passed, it returns a boolean indicating whether the incoming sound
* is enabled or disabled otherwise it enables or disables the incoming event sound.
*/
[Device.SoundName.Incoming]: EnableSoundFn;
/**
* If no value is passed, it returns a boolean indicating whether the outgoing sound
* is enabled or disabled otherwise it enables or disables the outgoing event sound.
*/
[Device.SoundName.Outgoing]: EnableSoundFn;

/**
* A Map of all audio input devices currently available to the browser by their device ID.
*/
Expand Down Expand Up @@ -90,6 +113,11 @@ class AudioHelper extends EventEmitter {
*/
private _audioContext?: AudioContext;

/**
* Whether each sound is enabled.
*/
private _enabledSounds: Record<Device.ToggleableSound, boolean>;

/**
* The `getUserMedia()` function to use.
*/
Expand Down Expand Up @@ -164,6 +192,7 @@ class AudioHelper extends EventEmitter {

this._getUserMedia = getUserMedia;
this._mediaDevices = options.mediaDevices || defaultMediaDevices;
this._enabledSounds = options.enabledSounds || defaultEnabledSounds;
this._onActiveInputChanged = onActiveInputChanged;

const isAudioContextSupported: boolean = !!(options.AudioContext || options.audioContext);
Expand All @@ -172,9 +201,7 @@ class AudioHelper extends EventEmitter {
this.isOutputSelectionSupported = isEnumerationSupported && isSetSinkSupported;
this.isVolumeSupported = isAudioContextSupported;

if (options.enabledSounds) {
this._addEnabledSounds(options.enabledSounds);
}
this._addEnabledSounds();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we only call this once at instantiation, and therefore we only add sounds that are enabled at construction time. It's not immediately clear to me that we can enable sounds post-construction, perhaps we need some mechanism for that?


if (this.isVolumeSupported) {
this._audioContext = options.audioContext || options.AudioContext && new options.AudioContext();
Expand Down Expand Up @@ -347,10 +374,11 @@ class AudioHelper extends EventEmitter {
* Merge the passed enabledSounds into {@link AudioHelper}. Currently used to merge the deprecated
* Device.sounds object onto the new {@link AudioHelper} interface. Mutates
* by reference, sharing state between {@link Device} and {@link AudioHelper}.
* @param enabledSounds - The initial sound settings to merge.
* @private
*/
private _addEnabledSounds(enabledSounds: { [name: string]: boolean }) {
private _addEnabledSounds() {
const enabledSounds = this._enabledSounds;

function setValue(key: Device.ToggleableSound, value: boolean) {
if (typeof value !== 'undefined') {
enabledSounds[key] = value;
Expand All @@ -359,8 +387,8 @@ class AudioHelper extends EventEmitter {
return enabledSounds[key];
}

Object.keys(enabledSounds).forEach(key => {
(this as any)[key] = setValue.bind(null, key);
Object.keys(enabledSounds).forEach((key: Device.ToggleableSound) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

this[key] = setValue.bind(null, key);
});
}

Expand Down Expand Up @@ -621,6 +649,8 @@ class AudioHelper extends EventEmitter {
}
}

export type EnableSoundFn = (value?: boolean) => boolean;

namespace AudioHelper {
/**
* Emitted when the available set of Devices changes.
Expand Down
21 changes: 7 additions & 14 deletions lib/twilio/device.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import { EventEmitter } from 'events';
import { levels as LogLevels, LogLevelDesc } from 'loglevel';
import AudioHelper from './audiohelper';
import AudioHelper, { defaultEnabledSounds } from './audiohelper';
import Call from './call';
import DialtonePlayer from './dialtonePlayer';
import {
Expand Down Expand Up @@ -347,15 +347,6 @@ class Device extends EventEmitter {
*/
private _edge: string | null = null;

/**
* Whether each sound is enabled.
*/
private _enabledSounds: Record<Device.ToggleableSound, boolean> = {
[Device.SoundName.Disconnect]: true,
[Device.SoundName.Incoming]: true,
[Device.SoundName.Outgoing]: true,
};

/**
* The name of the home region the {@link Device} is connected to.
*/
Expand Down Expand Up @@ -972,7 +963,7 @@ class Device extends EventEmitter {
maxAverageBitrate: this._options.maxAverageBitrate,
preflight: this._options.preflight,
rtcConstraints: this._options.rtcConstraints,
shouldPlayDisconnect: () => this._enabledSounds.disconnect,
shouldPlayDisconnect: () => this._audio ? this._audio.disconnect() : defaultEnabledSounds.disconnect,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check if these members are defined, i.e. this._audio.disconnect might be undefined depending on the state of the AudioHelper and we might try to invoke it.

twimlParams,
}, options);

Expand All @@ -992,7 +983,8 @@ class Device extends EventEmitter {
this._audio._maybeStartPollingVolume();
}

if (call.direction === Call.CallDirection.Outgoing && this._enabledSounds.outgoing) {
const shouldPlayOutgoing = this._audio ? this._audio.outgoing() : defaultEnabledSounds.outgoing;
if (call.direction === Call.CallDirection.Outgoing && shouldPlayOutgoing) {
this._soundcache.get(Device.SoundName.Outgoing).play();
}

Expand Down Expand Up @@ -1198,7 +1190,9 @@ class Device extends EventEmitter {
this._publishNetworkChange();
});

const play = (this._enabledSounds.incoming && !wasBusy)
const shouldPlayIncoming = this._audio ? this._audio.incoming() : defaultEnabledSounds.incoming;

const play = (shouldPlayIncoming && !wasBusy)
? () => this._soundcache.get(Device.SoundName.Incoming).play()
: () => Promise.resolve();

Expand Down Expand Up @@ -1307,7 +1301,6 @@ class Device extends EventEmitter {
getUserMedia,
{
audioContext: Device.audioContext,
enabledSounds: this._enabledSounds,
},
);

Expand Down