Bring in Sangoma Fork Changes #64

kylekurz · 2019-07-17T16:49:23Z

We've completed our initial pass of updated the react-native-callkeep dependency to provide a more consistent API across the JS between Android and iOS. We've also extended the Android code to support multiple calls. We don't claim this is perfect, but it's been solid for us for the last month and @danjenkins has been utilizing our fork for some of his projects, so we think it's stable enough to bring into consideration for mainline and just work on feature requests moving forward.

Edit: Thanks to @bhuangy for his help on this too, that got squashed down in my PR.

This is the culmination of the Sangoma fork work to update react-native-callkeep to support more consistent SDK features across both iOS and Android. Specifically, adding multi-call tracking to Android and a more consistent JS interface. This is a major version bump, as updating to this will break existing applications. We've attempted to keep the README updated with the proper instructions, although we did *not* write a transition guide. Happy to help contribute additional work if desired prior to merging, or to be a contributor on the community project to help maintain moving forward and avoid our own fork.

kylekurz · 2019-07-17T16:53:19Z

android/src/main/java/io/wazo/callkeep/VoiceConference.java

@@ -0,0 +1,44 @@
+package io.wazo.callkeep;


For the record, we started implementing conference, but it was removed from our GA requirements, so this isn't finished yet. This file is the framework for where further implementation will happen.

kylekurz · 2019-07-17T16:54:15Z

android/src/main/java/io/wazo/callkeep/VoiceConnectionService.java

@@ -94,132 +139,81 @@ private Boolean canMakeOutgoingCall() {
    }

    private Connection createConnection(ConnectionRequest request) {
-        connection = new Connection() {


This was moved and updated to VoiceConnection.java instead of being inline in the service.

danjenkins · 2019-07-17T19:16:13Z

I can confirm I've been using this fork for a month or so and seems good from a functionality perspective now

kylekurz · 2019-07-17T19:18:01Z

#poorsport over here forgot to thank @danjenkins for his contributions too. ;)

sboily · 2019-07-17T19:34:47Z

Hello, thank you for this contribution! We'll check as soon as possible.

Add updateDisplay function to iOS and fix setActive in Android

Use try catches to handle hashmap null object references

android/src/main/java/io/wazo/callkeep/VoiceConnection.java

Address issue #26 change number to 'handle'

docs/android-installation.md

README.md

Update docs and change number to handle

danjenkins · 2019-07-30T21:15:27Z

@kylekurz just noticed you guys enabled multiple calls on android but havent updated endAllCalls to kill them all

react-native-callkeep/index.js

Line 96 in 884d6a0

endAllCalls = () => {

manuquentin · 2019-08-02T17:37:53Z

Hi !
We're integrating these changes in our product, that's a great work !
I've an issue with multi calls on Android, when I'm on call with a number A, I click on Add a call in the native interface, then didReceiveStartCallAction is triggered to call B.

When B answer, I call setCurrentCallActive with the UUID of the B call, but when doing it, that's the first call (A) that is displayed as active.

I'm double checked the uuid (is JS and Java code) they correspond to the B call.

Do you have this issue too ?
Thanks !

danjenkins · 2019-08-02T17:52:45Z

I haven't tested this scenario yet myself - only tested it by recieving a second call and calling setActive on the second uuid when I answer the call. Can probably test this on Monday if Kyle cant

…

On Fri, 2 Aug 2019, 18:37 Emmanuel Quentin, ***@***.***> wrote: Hi ! We're integrating these changes in our product, that's a great work ! I've an issue with multi calls on Android, when I'm on call with a number A, I click on Add a call in the native interface, then didReceiveStartCallAction is triggered to call B. When B answer, I call setCurrentCallActive with the UUID of the B call, but when doing it, that's the first call (A) that is displayed as active. I'm double checked the uuid (is JS and Java code) they correspond to the B call. Do you have this issue too ? Thanks ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#64?email_source=notifications&email_token=AAB3LLJWMMYVEGQJUDAVDJDQCRWHDA5CNFSM4IESOGT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3OMVLQ#issuecomment-517786286>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB3LLIMXL3ZLZWEAAREM43QCRWHDANCNFSM4IESOGTQ> .

kylekurz · 2019-08-02T17:53:50Z

@manuquentin I'm on vacation, but @bhuangy is going to take a look.

chevonc · 2019-08-05T19:26:53Z

ios/RNCallKeep/RNCallKeep.m

+    }
+}
+
+RCT_EXPORT_METHOD(updateDisplay:(NSString *)uuidString :(NSString *)displayName :(NSString *)uri)


This method looks like a good candidate to merge with
RCT_EXPORT_METHOD(reportUpdatedCall:(NSString *)uuidString...) on line 503 in this file, we can keep the api back-compat by adding the extra arg.

Additionally, I was thinking to make this api would be more flexible we can change uri => handle and allow the caller/client application to also pass handleType, as the owner of the call session. When we displayIncomingCall we specify handleType, which may not always be of type CXHandleTypePhoneNumber.

-displayIncomingCall CXCallUpdate *callUpdate = [[CXCallUpdate alloc] init]; callUpdate.remoteHandle = [[CXHandle alloc] initWithType:_handleType value:handle];

manuquentin · 2019-08-07T17:12:02Z

android/src/main/java/io/wazo/callkeep/RNCallKeepModule.java

+    }
+
+    @ReactMethod
+    public void setMutedCall(String uuid, boolean shouldMute) {


When calling this method. I don't see the mute button updated on the native UI.
Have you ever succeeded to change it ?

Hi Emmanuel, the native UI should update to correspond to the correct mute state, and does on our project. Can you please confirm you have this basic flow for each of your callkeep functions?
Call setMutedCall in RN app -> callkeep tells the native sdk to update audio state -> sends a didPerformSetMutedCallAction event to the RN app -> In event callback perform the actual mute action.

Hi @bhuangy !
Yes I receive the didPerformSetMutedCallAction. You can check it in the example branch I've created.

In this example, you can start a call and mute it in the app (not in the native UI). Then you will seen that the mute button on the native UI is not in the muted state.
It's works well on iOS but not on Android (on my Samsung J3).

Tell me if you want more details.

@manuquentin I see the issue you're describing in your example app. I'm looking at it today. Everything looks reasonable at a glance...

Hello, what's the status of this PR? Thank you :)

@manuquentin I'm stumped on this. Your code looks right enough, but it's definitely not matching up properly. I've been pulled into other things, so I'm not actively looking at this right this second.

@sboily Overall, I still think the PR is a serious improvement to the previous CallKeep release. @manuquentin has pointed out one place where he's seeing an issue that I've not been able to resolve yet and @danjenkins pointed out two small changes that should be addressed today or tomorrow by us.

I think the real decision is "is this better", not "is this perfect", but I'm not the maintainer, I'm the one asking for the merge, ha. If we can get to a point where everyone is happy enough to merge, we can continue pushing additional, smaller, fixes upstream during development or join as contributors (your choice) to keep pushing this forward.

Hello @kylekurz, this is my previous discussion with @manuquentin and that exactly my point. I'm agree, i think we need to merge to master, give example (#77) for people to test this new feature, and release a new version on few weeks. It give chance to another people to contribute on small bug instead a big branch. So if there is no objection, i'm agree to merge it. Thank you for your contribution to enhance callkeep :)

I think we should wait until @bhuangy fixes Dan's issues (he's working on that now, should be done today) before merging.

index.js

Wrap rejectCall and answerIncomingCall for iOS

End all calls android

kylekurz · 2019-08-14T17:56:47Z

As of now, the only known outstanding issue I'm aware of is @manuquentin's problem with mute matching the app's state.

manuquentin · 2019-08-14T21:33:28Z

Thanks @kylekurz, @bhuangy, @danjenkins @chevonc and @sboily for your implication in this PR !
I'm merging it and we'll dig into this minor behaviour later.

Add example (follows #64)

kylekurz and others added 4 commits April 17, 2019 12:59

Fix android functionality

00c49c6

Merge remote-tracking branch 'upstream/master'

8a770b0

Merge upstream

b226d16

kylekurz commented Jul 17, 2019

View reviewed changes

Fix branch stuff

32b7fe5

This was referenced Jul 20, 2019

Improvement - Add callUUID to the mute/unmute event #37

Closed

Android - answerCall and endCall come with no params to decide which call should be terminated #40

Closed

Handle multiple calls #48

Closed

Kyle Kurz and others added 4 commits July 22, 2019 12:22

Merge pull request #30 from sangoma/setActiveIncoming

e0e68bd

Add updateDisplay function to iOS and fix setActive in Android

Use try catches to handle hashmap null object references

750e7a9

Merge pull request #31 from sangoma/HandleNullPointerExceptions

f9c0e86

Use try catches to handle hashmap null object references

Address issue #26 change number to 'handle'

f3e9a14

sboily reviewed Jul 24, 2019

View reviewed changes

android/src/main/java/io/wazo/callkeep/VoiceConnection.java Outdated Show resolved Hide resolved

Kyle Kurz added 2 commits July 24, 2019 08:06

Remove old debug log

00fffd9

Merge pull request #32 from sangoma/changeAndroidNumber

e59f2e6

Address issue #26 change number to 'handle'

manuquentin requested changes Jul 24, 2019

View reviewed changes

docs/android-installation.md Outdated Show resolved Hide resolved

manuquentin requested changes Jul 25, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

Brandon Huang and others added 2 commits July 25, 2019 10:28

Update docs and change number to handle

6bc295c

Merge pull request #33 from sangoma/updateDocsAndHandle

884d6a0

Update docs and change number to handle

Brandon Huang added 2 commits July 31, 2019 13:27

Add endAllCalls method to Android

20dcb9b

Update docs

1138c51

Merge in upstream changes to README

7b9c1cc

chevonc reviewed Aug 5, 2019

View reviewed changes

manuquentin reviewed Aug 7, 2019

View reviewed changes

danjenkins reviewed Aug 14, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

danjenkins reviewed Aug 14, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

Brandon Huang and others added 5 commits August 14, 2019 10:30

Wrap rejectCall and answerIncomingCall for iOS

cafa5cd

Merge pull request #35 from sangoma/defineFunctionsNotOniOS

e2cffa8

Wrap rejectCall and answerIncomingCall for iOS

Merge branch 'master' into endAllCallsAndroid

b5e3ec7

Merge branch 'master' into endAllCallsAndroid

9d73892

Merge pull request #34 from sangoma/endAllCallsAndroid

2b69925

End all calls android

manuquentin approved these changes Aug 14, 2019

View reviewed changes

manuquentin merged commit 724104d into react-native-webrtc:master Aug 14, 2019

manuquentin added a commit that referenced this pull request Aug 14, 2019

Merge pull request #77 from react-native-webrtc/add_example

f95cae6

Add example (follows #64)

codetheweb mentioned this pull request Jun 29, 2020

Known Bug: setMutedCall not working on Android #232

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring in Sangoma Fork Changes #64

Bring in Sangoma Fork Changes #64

kylekurz commented Jul 17, 2019 •

edited

Loading

kylekurz Jul 17, 2019

kylekurz Jul 17, 2019

danjenkins commented Jul 17, 2019

kylekurz commented Jul 17, 2019

sboily commented Jul 17, 2019

danjenkins commented Jul 30, 2019

manuquentin commented Aug 2, 2019

danjenkins commented Aug 2, 2019 via email

kylekurz commented Aug 2, 2019

chevonc Aug 5, 2019 •

edited

Loading

manuquentin Aug 7, 2019 •

edited

Loading

bhuangy Aug 7, 2019

manuquentin Aug 8, 2019 •

edited

Loading

kylekurz Aug 8, 2019

sboily Aug 14, 2019

kylekurz Aug 14, 2019

kylekurz Aug 14, 2019

sboily Aug 14, 2019 •

edited

Loading

kylekurz Aug 14, 2019

kylekurz commented Aug 14, 2019

manuquentin commented Aug 14, 2019

Bring in Sangoma Fork Changes #64

Bring in Sangoma Fork Changes #64

Conversation

kylekurz commented Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjenkins commented Jul 17, 2019

kylekurz commented Jul 17, 2019

sboily commented Jul 17, 2019

danjenkins commented Jul 30, 2019

manuquentin commented Aug 2, 2019

danjenkins commented Aug 2, 2019 via email

kylekurz commented Aug 2, 2019

chevonc Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

manuquentin Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuquentin Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboily Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylekurz commented Aug 14, 2019

manuquentin commented Aug 14, 2019

kylekurz commented Jul 17, 2019 •

edited

Loading

chevonc Aug 5, 2019 •

edited

Loading

manuquentin Aug 7, 2019 •

edited

Loading

manuquentin Aug 8, 2019 •

edited

Loading

sboily Aug 14, 2019 •

edited

Loading