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

Android twilio v3 #144

Merged
merged 19 commits into from
Oct 23, 2020
Merged

Android twilio v3 #144

merged 19 commits into from
Oct 23, 2020

Conversation

fabriziomoscon
Copy link
Collaborator

@fabriziomoscon fabriziomoscon commented Jan 12, 2020

This PR is a stepping stone to upgrade Android to Twilio Voice SDK v5.
On top of the current master (59304db) changes include the following PRs:

  • additional changes needed to complete implementation of Twilio Voice SDK v3.3.0

experiment for Android Twilio Voice SDK v3.3.0

  • add hold() call
  • add ringing state
  • remove state for callInvite
  • handle internal CANCEL_CALL notification
  • add ConnectOptions
  • add AcceptOptions
  • integrate the removal of callInvite of state PENDING

TODO Android

  • implement JS method to send ConnectOptions
  • test Android notification for incoming calls: app running in foreground
  • test Android notification for incoming calls: app running in background
  • test Android notification for incoming calls: app not running
  • test Android outgoing calls: app running in foreground
  • test on JS app
  • add additional feature into README
  • consider Twilio Voice Android SDK 5 #142
  • implement Android v4
  • fix: Android twilio v3 #144 (comment)
  • fix Android twilio v3 #144 (comment)

TODO iOS
Consult this: https://www.twilio.com/docs/voice/voip-sdk/ios/changelog#300

@fabriziomoscon fabriziomoscon force-pushed the android-twilio-v3 branch 6 times, most recently from b5e4965 to c3420da Compare January 14, 2020 02:43
This was referenced Jan 18, 2020
@fabriziomoscon fabriziomoscon force-pushed the android-twilio-v3 branch 3 times, most recently from a5f242b to 2a1cd09 Compare January 18, 2020 14:38
@benminer
Copy link

Since the maintainer of this project cannot be reached out, I think that it's best to fork to a fresh repo. I don't have much experience with native app development but I can definitely contribute to the new project with the React Native side. We use Twilio heavily on the browser but just starting working on the mobile app lately.

Awesome, glad to hear there is some support. However, if I understand it correctly, I think most of the work needed to be done is on the native side itself, specifically the wrapper classes that link the TwilioVoice SDK functions to the RCT Bridge for React Native use. I am decently experienced with iOS and can handle that, but may need assistance on the Android side.

Albeit, this is only if I find this PR to not be fixable due to the issues @Pagebakers has raised. I will be tinkering later tonight, I will keep updates posted here.

@Pagebakers
Copy link
Contributor

I'm running with 0.63.3 now. Got it working with !use_frameworks in the pod file and not using flipper in development, but archiving is failing for IOS.

I expect the upgrade to newer Twilio sdk versions wont be a big issue.

@benminer
Copy link

I'm running with 0.63.3 now. Got it working with !use_frameworks in the pod file and not using flipper in development, but archiving is failing for IOS.

I expect the upgrade to newer Twilio sdk versions wont be a big issue.

Is this PR up to date with your most recent changes? Would love to pull down your state of things and try it out with a newer Twilio SDK.

@mysport12
Copy link

mysport12 commented Oct 21, 2020

I too am using use_frameworks! with a pre-install function for the static libraries.

Podfile we are using:

`require_relative '../node_modules/react-native/scripts/react_native_pods'
require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules'
platform :ios, '10.0'

$dynamic_frameworks = ['TwilioVoice']

pre_install do |installer|
installer.pod_targets.each do |pod|
if !$dynamic_frameworks.include?(pod.name)
puts "Overriding the static_framework? method for #{pod.name}"
def pod.build_type;
Pod::BuildType.static_library
end
end
end
end

target 'APP' do
use_frameworks!
config = use_native_modules!
permissions_path = '../node_modules/react-native-permissions/ios'

pod 'TwilioVoice', '~> 5.5.1'

... other pods here

use_react_native!(:path => config["reactNativePath"])

target 'APPTests' do
inherit! :complete
# Pods for testing
end

use_flipper!('Flipper' => '0.62.0')
post_install do |installer|
flipper_post_install(installer)
end
end

target 'APP-tvOS' do

Pods for App-tvOS

target 'APP-tvOSTests' do
inherit! :search_paths
# Pods for testing
end
end`

@jdegger
Copy link
Collaborator

jdegger commented Oct 21, 2020

Since the maintainer of this project cannot be reached out, I think that it's best to fork to a fresh repo. I don't have much experience with native app development but I can definitely contribute to the new project with the React Native side. We use Twilio heavily on the browser but just starting working on the mobile app lately.

Awesome, glad to hear there is some support. However, if I understand it correctly, I think most of the work needed to be done is on the native side itself, specifically the wrapper classes that link the TwilioVoice SDK functions to the RCT Bridge for React Native use. I am decently experienced with iOS and can handle that, but may need assistance on the Android side.

Albeit, this is only if I find this PR to not be fixable due to the issues @Pagebakers has raised. I will be tinkering later tonight, I will keep updates posted here.

Might I also suggest that any new work could be depending on callkeep for handeling the call invites? This has been working wonders for us on Android.

We know iOS14 is killing apps who do not respond to invites in the correct way as well. Callkeep has this all figured out.

I'm on my phone now, but somewhere in this comments section I posted a link to our current fork or this PR which includes callkeep on Android and fixes a problem I found with the proximity sensor as well.

@jdegger
Copy link
Collaborator

jdegger commented Oct 21, 2020

Ps: we have a working iOS14 build/archive running. This should also be on the same fork. I think it's auto linked.

@Pagebakers
Copy link
Contributor

Pagebakers commented Oct 21, 2020

@benminer Yeah i'm still using the version in this PR, there are no other changes as far as I know.

Also using the latest 5.5.1 sdk version for iOS in your POD file should would with this PR, there shouldn't be any breaking changes. So you can run IOS 14.

I quickly looked into upgrading to Android SDK 5, doesn't seem too hard. I might give it a go.

@mysport12 Thanks, i'll try this tomorrow.

@benminer
Copy link

Ps: we have a working iOS14 build/archive running. This should also be on the same fork. I think it's auto linked.

Where is this fork? Is it your repo?

@jdegger
Copy link
Collaborator

jdegger commented Oct 21, 2020

Ps: we have a working iOS14 build/archive running. This should also be on the same fork. I think it's auto linked.

Where is this fork? Is it your repo?

Our project is closed source. But our fork of this PR is public on the @X-Guard organisation.

Found the link on my phone:

android-twilio-v3...X-Guard:85e4152c91a752e63d9a47682f9f347f00177f46

@benminer
Copy link

benminer commented Oct 21, 2020

Ps: we have a working iOS14 build/archive running. This should also be on the same fork. I think it's auto linked.

Where is this fork? Is it your repo?

Our project is closed source. But our fork of this PR is public on the @X-Guard organisation.

Found the link on my phone:

android-twilio-v3...X-Guard:85e4152c91a752e63d9a47682f9f347f00177f46

Thanks so much! So from what I can gather, this is working as expected on iOS 14 and Android with ^this PR's fixes? iOS needs some mirror of CallKeep like you've done on the android end of things?

@jdegger
Copy link
Collaborator

jdegger commented Oct 21, 2020

Ps: we have a working iOS14 build/archive running. This should also be on the same fork. I think it's auto linked.

Where is this fork? Is it your repo?

Our project is closed source. But our fork of this PR is public on the @X-Guard organisation.

Found the link on my phone:

android-twilio-v3...X-Guard:85e4152c91a752e63d9a47682f9f347f00177f46

Thanks so much! So from what I can gather, this is working as expected on iOS 14 and Android with ^this PR's fixes? iOS needs some mirror of CallKeep like you've done on the android end of things?

Yes, Android and callkeep work find in this version.

iOS builds fine and outgoing calls are great too. Incoming calls still have some problems like missing interfaces. We hope callkeep can resolve this.

If the work we did for Android (which was admittedly taken from suggestions made on this repo too, not all our work!) is mirrored to iOS. It should be fully featured once again.

@benminer
Copy link

Ps: we have a working iOS14 build/archive running. This should also be on the same fork. I think it's auto linked.

Where is this fork? Is it your repo?

Our project is closed source. But our fork of this PR is public on the @X-Guard organisation.

Found the link on my phone:

android-twilio-v3...X-Guard:85e4152c91a752e63d9a47682f9f347f00177f46

Thanks so much! So from what I can gather, this is working as expected on iOS 14 and Android with ^this PR's fixes? iOS needs some mirror of CallKeep like you've done on the android end of things?

Yes, Android and callkeep work find in this version.

iOS builds fine and outgoing calls are great too. Incoming calls still have some problems like missing interfaces. We hope callkeep can resolve this.

If the work we did for Android (which was admittedly taken from suggestions made on this repo too, not all our work!) is mirrored to iOS. It should be fully featured once again.

Sweet, so if I get that working somehow on your private fork, you think X-Guard could publish it to NPM considering this repo is dead?

@jdegger
Copy link
Collaborator

jdegger commented Oct 21, 2020

Sure! If it works we would be very happy too.

It's 10:21 PM here so I'm going to bed now. Let me know what you plan to do and you can also reach out to me privately of course. Thanks for your enthusiasm so far!

@fabriziomoscon
Copy link
Collaborator Author

I don't have time to answer to this right now. But I will try to have a look later on.
The call for maintainers is always open, I simply never had any volunteer.

@fabriziomoscon
Copy link
Collaborator Author

I use this branch in PROD, both iOS and Android

// package.json
"react": "16.9.0",
"react-native": "0.61.5",
"react-native-twilio-programmable-voice": "https://github.com/hoxfon/react-native-twilio-programmable-voice#android-twilio-v3",

@jdegger
Copy link
Collaborator

jdegger commented Oct 22, 2020

@fabriziomoscon We are willing to take on the maintainer roll now. Last time you asked we declined as we are not sufficient iOS and Android developers. However we think that we can help and work moving this repo forward by reviewing and accepting PR's etc.

For example if @benminer sends in a PR for this branch we can test it.

If we take on this role though, are you staying on as the maintainer/owner? How would we decide on a direction of this plugin?

For example: I strongly feel that depending on CallKeep and implementing the CallKeep API in this plugin would be very beneficial. However, this is a rather large change of API. A design decision like that has a big influence on how to proceed.

Let me know. If you don't have to time discuss it, and trust us as it is without any prior discussion you can of course just make me maintainer for now.

Your efforts on this repo are still greatly appreciated.

@Pagebakers
Copy link
Contributor

@jdegger this did the trick, thanks for the help!

$dynamic_frameworks = ['TwilioVoice']

pre_install do |installer|
installer.pod_targets.each do |pod|
if !$dynamic_frameworks.include?(pod.name)
puts "Overriding the static_framework? method for #{pod.name}"
def pod.build_type;
Pod::BuildType.static_library
end
end
end
end

@jdegger
Copy link
Collaborator

jdegger commented Oct 22, 2020

I stumbled upon this new repo as well, might be interesting.

https://github.com/MrHertal/react-native-twilio-phone

@MrHertal

@fabriziomoscon
Copy link
Collaborator Author

Hi @jdegger I propose the following.

  • I will merge this PR, tag and release on npm, as I see that most people got it working and it is a stepping stone for implementing newer APIs.
  • I will contact you privately to setup a call to discuss maintenance going forward.
  • We can open a new issue to discuss how to implement CallKeep

@fabriziomoscon fabriziomoscon merged commit 16e20ff into master Oct 23, 2020
@fabriziomoscon fabriziomoscon deleted the android-twilio-v3 branch October 23, 2020 07:40
@jdegger
Copy link
Collaborator

jdegger commented Oct 23, 2020

@fabriziomoscon great!

@fabriziomoscon
Copy link
Collaborator Author

[email protected]

@jdegger
Copy link
Collaborator

jdegger commented Oct 23, 2020

@MrHertal are you available to discuss merging your package / knowledge into this one? It would be beneficial to the react native ecosystem to see if we can collaborate on the same package and share knowledge instead of 'compete' (not really competing but you catch my drift).

@MrHertal
Copy link

Hi @jdegger, sorry for late reply. To be honest, I thought a lot about what to do before starting that new project. I am currently working on a React Native app that must include VoIP calls with Twilio and I don't have much time left to finish it.

I understand that you are working on transforming that library to use CallKeep. I agree this is the best option. But I also thought it would take more time upgrading this library than starting a new project.

This library is originally an "all-in-one" package inspired by Twilio quick-start projects. It includes notifications handling, calls workflow and also the UI. In my opinion, the best thing to do is to split this package to let CallKeep manage the UI. It should then be split again to let @react-native-firebase/messaging and react-native-voip-push-notification manage the notifications, because those libraries are specifically focusing on this task and are well maintained.

Splitting this package like that requires more work than just starting a new project that is relying on mentioned librairies and that only manage the communication with Twilio (start call, hold call, disconnect call...). That is the reason I made https://github.com/MrHertal/react-native-twilio-phone

Now that it is released I can move on my app. If you want to use it in any ways in this repo, you are free to do it. Hope you understand that this is not about competing, but finding the most efficient way to work.

@benminer
Copy link

@MrHertal This is awesome! Thanks so much for doing this!

@jdegger
Copy link
Collaborator

jdegger commented Oct 26, 2020

@MrHertal No worries and thanks for your elaboration, not that you owed us one!

We fully understand your motivation for the package. I have a call scheduled tonight with @fabriziomoscon to discuss how we are going to proceed with this repo. If by any chance you have time left @MrHertal to help with this library as you obviously have the knowledge on how to do so :), it would be very welcome.

@jdegger
Copy link
Collaborator

jdegger commented Oct 27, 2020

This discussion has been moved to #158, we invite you to engage and read our update 👍

@hoxfon hoxfon locked as resolved and limited conversation to collaborators Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.