Skip to content

ios: support additional options pass from js#238

Merged
manuquentin merged 6 commits into
react-native-webrtc:masterfrom
zxcpoiu:support_options
Nov 13, 2020
Merged

ios: support additional options pass from js#238
manuquentin merged 6 commits into
react-native-webrtc:masterfrom
zxcpoiu:support_options

Conversation

@zxcpoiu

@zxcpoiu zxcpoiu commented Jul 14, 2020

Copy link
Copy Markdown
Member

Continued from #188

@zxcpoiu zxcpoiu closed this Jul 14, 2020
@zxcpoiu zxcpoiu deleted the support_options branch July 14, 2020 09:46
@zxcpoiu zxcpoiu restored the support_options branch July 14, 2020 09:46
@zxcpoiu zxcpoiu deleted the support_options branch July 14, 2020 09:46
@zxcpoiu zxcpoiu restored the support_options branch July 14, 2020 09:47
@zxcpoiu zxcpoiu reopened this Jul 14, 2020
@zxcpoiu

zxcpoiu commented Jul 14, 2020

Copy link
Copy Markdown
Member Author

I clean/remove the overloading functions of reportNewIncomingCall
There is no overloading api now

function arguments:

+ (void)reportNewIncomingCall:(NSString *)uuidString
                       handle:(NSString *)handle
                   handleType:(NSString *)handleType
                     hasVideo:(BOOL)hasVideo
          localizedCallerName:(NSString * _Nullable)localizedCallerName
              supportsHolding:(BOOL)supportsHolding
                 supportsDTMF:(BOOL)supportsDTMF
             supportsGrouping:(BOOL)supportsGrouping
           supportsUngrouping:(BOOL)supportsUngrouping
                  fromPushKit:(BOOL)fromPushKit
                      payload:(NSDictionary * _Nullable)payload
        withCompletionHandler:(void (^_Nullable)(void))completion
{

Breaking change:

All user invoke this function in AppDelegate.m should now use:

    [RNCallKeep reportNewIncomingCall: uuidString
                               handle: handle
                           handleType: handleType
                             hasVideo: YES
                  localizedCallerName: localizedCallerName
                      supportsHolding: YES
                         supportsDTMF: YES
                     supportsGrouping: YES
                   supportsUngrouping: YES
                          fromPushKit: YES
                              payload: nil
                withCompletionHandler: nil];

or one line version

[RNCallKeep reportNewIncomingCall:uuidString handle:handle handleType:handleType hasVideo:YES localizedCallerName:localizedCallerName supportsHolding:YES supportsDTMF:YES supportsGrouping:YES supportsUngrouping:YES fromPushKit:YES payload:nil withCompletionHandler:nil];

@sboily

sboily commented Jul 29, 2020

Copy link
Copy Markdown
Member

Hello, Android support some of features like iOS did for example video support, hold, maybe grouping is conference (need to check) etc ... I don't know if we update this PR and merge this one and create new one for Android.

@zxcpoiu

zxcpoiu commented Aug 3, 2020

Copy link
Copy Markdown
Member Author

I think if this PR does not conflict with master or other WIP PR, it's better to merge this first, then create a new one for android option, once the android api is nearly finalize.

Since this PR is mainly handling IOS only (add a new param: option), and is no-op for android.

@zxcpoiu

zxcpoiu commented Aug 3, 2020

Copy link
Copy Markdown
Member Author

@sboily

I've checked #251 a bit, you've added a hasVideo in displayIncomingCall for pass to android. And the rest ios only options may now supported on Android.

This is current api for displayIncomingCall in this PR

displayIncomingCall = (uuid, handle, localizedCallerName = '', handleType = 'number', hasVideo = false, options = null)

- `options`: object (optional)
  - `ios`: object
    - `supportsHolding`: boolean (optional, default true)
    - `supportsDTMF`: boolean (optional, default true)
    - `supportsGrouping`: boolean (optional, default true)
    - `supportsUngrouping`: boolean (optional, default true)

So I guess we need to:

  • move ios only handleType to the option ( that's breaking change, but normalize the api )

And for updateDisplay = (uuid, displayName, handle, options = null)

  • hasVideo is already in optional arg options, it's fine.

And for both api:

  • android add options args like in ios, but leave it no-op, when some android features has been implemented, then pass it to the native side

Does this what you mean?

@zxcpoiu

zxcpoiu commented Nov 12, 2020

Copy link
Copy Markdown
Member Author

rebased 😉

@manuquentin

Copy link
Copy Markdown
Contributor

Thanks @zxcpoiu !

@manuquentin manuquentin merged commit c3ade44 into react-native-webrtc:master Nov 13, 2020
@geraintwhite

Copy link
Copy Markdown
Contributor

Any chance we could follow semver when including breaking changes like this in releases? It was included in 3.1.5 but should really have been 4.0.0 due to the breaking reportNewIncomingCall API change.

@geraintwhite

geraintwhite commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Also, on 3.1.5 I am getting a crash on iOS after updating the reportNewIncomingCall call to match the new signature.

      NSString *uuid = [[[NSUUID UUID] UUIDString] lowercaseString];
      [RNCallKeep reportNewIncomingCall:uuid
                                 handle:@""
                             handleType:@"generic"
                               hasVideo:NO
                    localizedCallerName:@""
                        supportsHolding:YES
                           supportsDTMF:YES
                       supportsGrouping:NO
                     supportsUngrouping:NO
                            fromPushKit:YES
                                payload:nil
                  withCompletionHandler:completion];
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '+[RNCallKeep reportNewIncomingCall:handle:handleType:hasVideo:localizedCallerName:supportsHolding:supportsDTMF:supportsGrouping:supportsUngrouping:fromPushKit:payload:withCompletionHandler:]: unrecognized selector sent to class 0x10466ca00'

EDIT:

After a clean and rebuild the crash seems to have gone.

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.

4 participants