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

Play disconnect sound android #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gentleShark
Copy link

changes to play disconnect.wav on disconnect(). instantiated soundPoolManager in constructor of TwilioVoiceManager.java since .wav file needs time to be loaded and parsed per Android Dev recommendation. Instantiated soundPoolManager in the constructor also seems to result in better sound quality for other SoundPoolManager related tasks compared to creating an instance at runtime with SoundPoolManager.getInstance(getReactApplicationContext()). I also added a few generated files to .gitignore

…anager in constructor of TwilioVoiceManager.java to since wav file needs time to parsed. use of intantiated soundPoolManager also seems to result in better sound quality for other SoundPoolManager related tasks such as ringing, etc
@fabriziomoscon
Copy link
Collaborator

Nice improvement, I will look at merging this PR after other PR that have precedence, because they implement Twilio new libraries.

@gentleShark
Copy link
Author

Sounds good [pun intended]. 👍

@jdegger
Copy link
Collaborator

jdegger commented Oct 27, 2020

Thank you for your PR and our apologies it has been quiet on this PR for so long. Please read #158 for a general update on what's happening in this repo.


I have a few questions about this PR:

  1. Why would we not just have the react-native application play a sound on disconnect instead of android natively?
  2. Since a large PR has been merged recently, can you resolve the conflicts so we can test this?

Many thanks for your efforts!

@gentleShark
Copy link
Author

It's been so long since I've worked on this that I'm not entirely sure what the best answer is to your first question but it sounds like a fine solution. With my current workload, I don't have time to work on this.

@jdegger
Copy link
Collaborator

jdegger commented Oct 27, 2020

@gentleShark I understand! No worries, for now I will add this PR as a backlog item so it can be merged eventually.

Thanks for your efforts! I will leave this open for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants