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 4 #130

Closed
wants to merge 27 commits into from

Conversation

Pagebakers
Copy link
Contributor

Upgraded to the new SDK

I'm not an Android developer, but need our apps to be updated, so here goes nothing :)

Basic functions seem to work.

Haven't implemented the new onRinging/onReconnecting/onReconnecting callbacks
callInvite.getState() is not available anymore, removed this where used, but not sure if this breaks anything.

Need testers! :)

@gitstud
Copy link
Contributor

gitstud commented Oct 24, 2019

Would be happy to review this when I get some time, hopefully next weekend

@Pagebakers
Copy link
Contributor Author

Greatly appreciated!

@gitstud
Copy link
Contributor

gitstud commented Oct 26, 2019

Code looks good! I think the comments should come out where you took out the getState calls. I'll get a chance to run it tomorrow or Sunday and let you know how that went

@aniravi24
Copy link
Contributor

aniravi24 commented Oct 26, 2019

I think the state got moved to another class (https://github.com/twilio/voice-quickstart-android/blob/master/Docs/migration-guide-2.x-3.x.md), and it would be ideal if we could figure out a way to keep the state information because I have an app that relies on it.

@Pagebakers
Copy link
Contributor Author

I've been running this partly successful on our app for 2 months now.

We do get this error every now and then though:

java.lang.NullPointerException · Attempt to invoke interface method 'com.facebook.react.ReactNativeHost com.facebook.react.ReactApplication.getReactNativeHost()' on a null object reference
VoiceFirebaseMessagingService.java:78com.hoxfon.react.RNTwilioVoice.fcm.VoiceFirebaseMessagingService$1$1.run	
Handler.java:873android.os.Handler.handleCallback	
Handler.java:99android.os.Handler.dispatchMessage	
Looper.java:201android.os.Looper.loop	
ActivityThread.java:6806android.app.ActivityThread.main	
Method.java:-2java.lang.reflect.Method.invoke	
RuntimeInit.java:547com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run	
ZygoteInit.java:873com.android.internal.os.ZygoteInit.main

Anyone any idea how to fix this?

@fabriziomoscon fabriziomoscon mentioned this pull request Dec 5, 2019
@jdegger
Copy link
Collaborator

jdegger commented Dec 6, 2019

@Pagebakers We tested this as well, but as we understood it (according to your commits), your changes were Android-only. Is this correct?

@Pagebakers
Copy link
Contributor Author

Thanks for testing, this update is Android only indeed.

There's another PR for the IOS update.

@fabriziomoscon
Copy link
Collaborator

Just to coordinate...
I am awaiting from @Pagebakers @jdegger and @gitstud to complete tests this weekend so on Monday we can confirm that branch feat/v4 could be a stepping stone for this PR and the iOS one. Did I miss something?

@jdegger
Copy link
Collaborator

jdegger commented Dec 6, 2019

@Pagebakers @fabriziomoscon Can the PRs be combined so it testable on both platforms in one go?

@fabriziomoscon
Copy link
Collaborator

I think the merging order should be: feat/v4, android SDK v4, and IoS

@Pagebakers
Copy link
Contributor Author

Pagebakers commented Dec 13, 2019

@fabriziomoscon Merging #108 was pretty painless

No issues running this on first sight.

@fabriziomoscon
Copy link
Collaborator

Thanks @Pagebakers you will need to rebase master to fix the above conflicts. I am testing now

@fabriziomoscon
Copy link
Collaborator

@Pagebakers I noticed that you merged half of my PR. Please check the full list here: https://github.com/hoxfon/react-native-twilio-programmable-voice/pull/108/commits
I am going to open a new branch with all rebasing

@Pagebakers
Copy link
Contributor Author

Oh shit something went wrong then, I'll look into it tomorrow.

@fabriziomoscon
Copy link
Collaborator

@Pagebakers I pushed a branch here containing the latest master feat/v4 and your work in this order.
Could you run a test please?
https://github.com/hoxfon/react-native-twilio-programmable-voice/tree/feature/android-sdk-4 should be ready to be tested

@Pagebakers
Copy link
Contributor Author

Getting a compilation error, but that may be due my bad internet connection. I'll need to try it again on wifi later.

@fabriziomoscon
Copy link
Collaborator

My compilation error was on voice.register() and using twilio 4.5.0 fixed it, what is yours?

@gitstud
Copy link
Contributor

gitstud commented Dec 18, 2019

@fabriziomoscon I'm seeing this as well

@Pagebakers
Copy link
Contributor Author

Getting a lot of cannot find symbol errors on the package in your branch unfortunately. Missing androidx classes.

error: cannot find symbol class NonNull

What React version have you been testing on?

@fabriziomoscon
Copy link
Collaborator

RN 0.60

@Pagebakers
Copy link
Contributor Author

OK I figured, tried to upgrade now getting the same voice.register error, twilio 4.5.0 doesn't seem to fix this though :(

@Pagebakers
Copy link
Contributor Author

Pagebakers commented Dec 18, 2019

Running in too much compilation errors now because of the upgrade, don't have the time for this unfortunately.

0.59.9 was running fine

https://github.com/forwarder/react-native-twilio-programmable-voice/blob/feature/android-sdk-4/android/src/main/java/com/hoxfon/react/RNTwilioVoice/TwilioVoiceModule.java#L576

Should become

Voice.register(accessToken, Voice.RegistrationChannel.FCM, fcmToken, registrationListener);

@fabriziomoscon
Copy link
Collaborator

@Pagebakers I know it is taking me more than 10 working days for me to upgrade to RN0.60, however it is a needed change for my app because of other libraries that have new features only for RN0.60.
Personally I am not interested in compiling for RN below 0.60, so possibly we could partition the PRs work, I will keep working on integrating your changes for RN0.60, feel free to push a PR for 0.59 if you think that is the best for your app.

@Pagebakers
Copy link
Contributor Author

Surething, just go ahead releasing this for 0.60. I'll work it out once I find some more spare time. thanks!

@aniravi24
Copy link
Contributor

aniravi24 commented Dec 18, 2019

You can probably use jetifier to fix the compilation errors if it's looking for androidx stuff. RN 0.60 as well as Android have made some changes to the underlying support libraries. If you're not on RN 0.60 or above, try installing https://www.npmjs.com/package/jetifier and running npx jetify before doing a build. This is run automatically with 0.60 and above, and will need to be done whenever node_modules are modified. We use RN 0.61.5 with a production app with the latest npm version of this package and I'm looking forward to testing some of the new work in these branches as soon as I get a chance!

@Pagebakers
Copy link
Contributor Author

Took me a couple more hours, but the solution is easy
Using the reverse option makes it work for < 0.60

npx jetify -r

Thanks @aniravi24 for getting me to the right direction :)

Still getting an error though

java.lang.RuntimeExceptionTwilioVoiceModule.java:561
Could not invoke RNTwilioVoice.initWithAccessToken

@aniravi24
Copy link
Contributor

aniravi24 commented Jan 5, 2020

I also have #142 on top of the work in feature/android-sdk-4.

@fabriziomoscon
Copy link
Collaborator

These changes have been included in #144

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.

5 participants