search

stevegraham / twilio_client_phonegap

Phonegap plugins for the Twilio Client iOS and Android SDKs
MIT License
36 stars 73 forks source link

Updated plugin for use with Cordova 2.9. Added UILocalNotification Support #8

Closed lylepratt closed 8 years ago

lylepratt commented 11 years ago

Updated plugin for use with Cordova 2.9. Added UILocalNotification Support for displaying a UILocalNotification to the user in the event of an incoming call while the app is in a backgrounded state. Other miscellaneous bug fixes that I found, like the connection not getting set properly in the incoming connection delegate.

lylepratt commented 11 years ago

More info concerning the showNotification and cancelNotification functions: Jeff and I have been discussing this and we feel that almost anyone using this plugin will need the ability to show the user that a call is ringing when the app is backgrounded. Why have a VOIP app with background capability if you can't tell them the call is ringing?

We also looked through the existing Cordova Local Notification plugins, but there was none that was simple, cross platform compatible. This added to our decision to include simple support for displaying a local notification.

I've attached an image of what Twilio.Connection.showNotification("Incoming Call: Lyle Pratt (-----)", "incoming.wav") looks like when used: local_notification_twilio_plugin

lylepratt commented 11 years ago

@kwhinnery @stevegraham thoughts? Jeff is planning on also adding similar notification support to the Android plugin.

stevegraham commented 11 years ago

+1. You're absolutely right. I always intended to add that but this library was something I hacked out one afternoon while working at Twilio, because there was no appetite from engineering to create this library. Since I left Twilio and they suffer from NIH very badly it has been a low priority for me to work on it.

Thanks very much for the PRs. Great work. Will code review to merge in a few.

Sent from my iPhone

On 19 Jul 2013, at 19:03, Lyle Pratt notifications@github.com wrote:

@kwhinnery @stevegraham thoughts? Jeff is planning on also adding similar notification support to the Android plugin.

— Reply to this email directly or view it on GitHub.

kwhinnery commented 11 years ago

This would be a great addition as a high level helper in the library. Good work.

lylepratt commented 11 years ago

Lol...cordova 3.0 launched today. Guess I should go through and update this.

kwhinnery commented 11 years ago

Will definitely be worth adopting plugman for 3.0 as well. Much better install experience for end users. Better for module devs as well since you can declare the static library, system framework, and compiler flag dependencies in the new plugin.xml. IIRC you can also inject XML into the app's Info.plist to configure background modes and the like (haven't looked much at that bit yet).

On Fri, Jul 19, 2013 at 2:46 PM, Lyle Pratt notifications@github.comwrote:

Lol...cordova 3.0 launched today. Guess I should go through and update this.

— Reply to this email directly or view it on GitHubhttps://github.com/stevegraham/twilio_client_phonegap/pull/8#issuecomment-21272873 .

Kevin Whinnery @kevinwhinnery http://kevinwhinnery.com

lylepratt commented 11 years ago

Hmm, after looking at the new plugin docs, I think that I'm going to hold off on updating it until after we get this app pushed out. But I'll definitely plan on helping get these updated to 3.0.

jefflinwood commented 11 years ago

There seems to be a lot of work involved with packaging it for plugman. I haven't tested either of these plugins out with 3.0 yet.

lylepratt commented 11 years ago

I added another helper method to turn the device's speaker phone on or off during a phone call. @jefflinwood I've done something similar on Android in the past, so it shouldn't be hard to also do in the Android version. Let me know if you'd like to see the code.

stevegraham commented 11 years ago

I've made some comments. I quite determined to keep the API compatible with with the TC iOS SDK, anything that requires extensions to that API is indicative that it shouldn't be there IMO.

lylepratt commented 11 years ago

I agree. 100% compatibility is vital. However, I think both the UILocalNotification and the speaker sound output should be included. Anyone implementing this plugin will probably need both of those things. Neither are necessary in the Twilio Client JS library because it isn't made to run on mobile devices, but that doesn't mean they shouldn't be included here if they will obviously be necessary to create a fully functional VOIP app using the plugin.

kwhinnery commented 11 years ago

After mulling it over a bit, I think I probably agree w/ Stevie on this one. I think it would be common for VoIP apps to want to use local notifications, but not strictly necessary. I have seen Twilio Client use cases that would not make use of this functionality (kiosk type apps, collaboration apps). And local notifications are themselves not germane to the functionality of the Twilio Client SDK. Also, there seem to be a few other plugins that do this:

https://github.com/phonegap/phonegap-plugins/tree/master/iPhone/LocalNotification

What might be cool to explore is a second module with common utilities for VoIP apps?

I am not strongly against including these other ancillary helpers, though - so long as the core VoIP APIs remain the same cross-platform.

On Sun, Jul 21, 2013 at 7:19 PM, Lyle Pratt notifications@github.comwrote:

I agree. 100% capability is vital. However, I think both the UILocalNotification and the speaker sound output should be included. Anyone implementing this plugin will probably need both of those things. Neither are necessary in the Twilio Client JS library because it isn't made to run on mobile devices, but that doesn't mean they shouldn't be included here if they will obviously be necessary to create a fully functional VOIP app using the plugin.

— Reply to this email directly or view it on GitHubhttps://github.com/stevegraham/twilio_client_phonegap/pull/8#issuecomment-21319871 .

Kevin Whinnery @kevinwhinnery http://kevinwhinnery.com

lylepratt commented 11 years ago

We looked at all of the UILocalNotification apps out there and none are have JS syntax that carries over to other platforms and none seem to be maintained. That added to the decision to just include it.

kwhinnery commented 11 years ago

You're the one putting sweat equity into actually cranking this out, so I am happy to smother my architectural purity concerns for this one :)

On Sun, Jul 21, 2013 at 8:46 PM, Lyle Pratt notifications@github.comwrote:

We looked at all of the UILocalNotification apps out there and none are have JS syntax that carries over to other platforms and none seem to be maintained. That added to the decision to just include it.

— Reply to this email directly or view it on GitHubhttps://github.com/stevegraham/twilio_client_phonegap/pull/8#issuecomment-21321272 .

Kevin Whinnery @kevinwhinnery http://kevinwhinnery.com

lylepratt commented 11 years ago

No way! This is Steve's repo, so he can do w/e he wants. :) I'll maintain a fork that has my changes if I need to.

lylepratt commented 11 years ago

Howdy @stevegraham! In the interim, Jeff has pushed his android changes over to my fork. I'm happy to add/remove/reorganize things to get changes you'd like to pull in moved to a different branch (and the ones you don't want included removed). Just let me know what you'd like for me to do.