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

Twilio Voice Android SDK 5 #142

Closed

Conversation

aniravi24
Copy link
Contributor

@aniravi24 aniravi24 commented Jan 5, 2020

This builds on top of the migration to Android SDK v4.

Definitely load the diff for the main voice module - that's where most of the changes are. The readme hasn't been updated to indicate the different states that are hardcoded in the module - still haven't totally decided on that but it works! Many of the changes are just Android studio formatting.

I'd also like to update the package version to 5.0.0 - maybe we should keep it the same version as the twilio libraries we use so it's less confusing.

@aniravi24 aniravi24 changed the title Feature/android sdk 5 Twilio Voice Android SDK 5 Jan 5, 2020
Copy link
Collaborator

@fabriziomoscon fabriziomoscon left a comment

Choose a reason for hiding this comment

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

thanks for this.
Having formatting and line swaps into the PR makes it really difficult to understand your changes. Please create a new PR with only changes and next we can do formatting.

@aniravi24
Copy link
Contributor Author

aniravi24 commented Jan 6, 2020

@fabriziomoscon sorry about that! Accidentally let Android studio format everything. Instead of making a whole new PR, mind if I leave GitHub comments in the places where I actually made changes? Would that work for now?

@fabriziomoscon
Copy link
Collaborator

Formatting should always be added as a separate commit. The reason is not limited to code review, since the commit history will be used by users to understand when and what change has been added when they are debugging their app.
All commits should be grouped when the refer the the same logical change, so formatting can be quickly ignored

@aniravi24
Copy link
Contributor Author

Got it - will do another PR when I get a chance with only the changes made.

@fabriziomoscon
Copy link
Collaborator

@aniravi24 thanks for your contributions.
Your fixes are now incorporated into #144
I noticed that there might be functional changed or improvements on the set audio calls, but I would advise to add such changes on top of the final merge which I plan to land as v4.0.0 of this library.

@aniravi24
Copy link
Contributor Author

@fabriziomoscon sorry for the delay! Thank you for doing that. That code comes straight from the Twilio quickstart for Android.

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

Successfully merging this pull request may close these issues.

2 participants