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

kamalbennani · 2022-10-03T19:54:01Z

The goal of this PR is to answer the "lack of implementation" described in this issue

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

CLIENT-0000

Description

This PR introduces a couple of changes:

Remove enabledSounds responsibility from the Device class and move it close to the AudioHelper
Expose toggleable sounds getters/setters
Use this.audio.outgoing instead of relying on the enabledSounds private property

Burndown

Before review

Updated CHANGELOG.md if necessary
Added unit tests if necessary
Updated affected documentation
Verified locally with npm test
Manually sanity tested running locally
Ready for review

Before merge

Got one or more +1s
Squashed erroneous commits if necessary
Re-tested if necessary

charliesantos · 2022-10-03T22:36:18Z

@mhuynh5757 can you please do a quick pass?

…tters to enable and disable toggeable sounds

mhuynh5757 · 2022-10-26T17:44:49Z

lib/twilio/audiohelper.ts

@@ -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) => {


mhuynh5757 · 2022-10-26T18:13:13Z

lib/twilio/audiohelper.ts

-    if (options.enabledSounds) {
-      this._addEnabledSounds(options.enabledSounds);
-    }
+    this._addEnabledSounds();


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?

mhuynh5757 · 2022-10-26T18:14:51Z

lib/twilio/audiohelper.ts

@@ -21,6 +21,12 @@ const kindAliases: Record<string, string> = {
  audiooutput: 'Audio Output',
 };

+export const defaultEnabledSounds: Record<Device.ToggleableSound, boolean> = {


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

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

mhuynh5757 · 2022-10-26T18:21:03Z

lib/twilio/device.ts

@@ -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,


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.

mhuynh5757 · 2022-10-26T18:27:28Z

@kamalbennani thanks for contributing to the SDK! All feedback is greatly appreciated. I left some comments on some more specific details.

One recommendation over the current implementation that I'd like to see change is moving away from setting EnableSoundFn members on the object and using something less anti-pattern, like indexing an internal map with the Sound enumeration we already have. So the API would look something like

audioHelper.shouldPlaySound(Device.SoundName.Incoming)

or maybe we relegate all "shouldPlay" logic to the AudioHelper entirely and in the Device we just ask the AudioHelper to play it, getting some sort of return value from the AudioHelper if necessary.

What do you think?

charliesantos · 2023-03-29T20:33:27Z

@kamalbennani I'm closing this now. See my comment here #14 (comment)

charliesantos requested a review from mhuynh5757 October 3, 2022 22:35

kamalbennani changed the title ~~move enable sounds to the audio helper class and expose getter and setters to enable and disable toggeable sounds~~ [DRAFT] move enable sounds to the audio helper class and expose getter and setters to enable and disable toggeable sounds Oct 4, 2022

move enable sounds to the audio helper class and expose getter and se…

7fea676

…tters to enable and disable toggeable sounds

kamalbennani force-pushed the task/PH-7239/expose-enable-sound-methods branch from c4cbec3 to 7fea676 Compare October 4, 2022 07:33

mhuynh5757 reviewed Oct 26, 2022

View reviewed changes

charliesantos closed this Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kamalbennani commented Oct 3, 2022

charliesantos commented Oct 3, 2022

mhuynh5757 Oct 26, 2022

mhuynh5757 Oct 26, 2022

mhuynh5757 Oct 26, 2022

mhuynh5757 Oct 26, 2022

mhuynh5757 Oct 26, 2022

mhuynh5757 commented Oct 26, 2022 •

edited

Loading

charliesantos commented Mar 29, 2023

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

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

Conversation

kamalbennani commented Oct 3, 2022

Pull Request Details

JIRA link(s):

Description

Burndown

Before review

Before merge

charliesantos commented Oct 3, 2022

mhuynh5757 Oct 26, 2022

Choose a reason for hiding this comment

mhuynh5757 Oct 26, 2022

Choose a reason for hiding this comment

mhuynh5757 Oct 26, 2022

Choose a reason for hiding this comment

mhuynh5757 Oct 26, 2022

Choose a reason for hiding this comment

mhuynh5757 Oct 26, 2022

Choose a reason for hiding this comment

mhuynh5757 commented Oct 26, 2022 • edited Loading

charliesantos commented Mar 29, 2023

mhuynh5757 commented Oct 26, 2022 •

edited

Loading