Skip to content

Updated to Cordova 2.7, fixed a number of bugs. Cleaned up some code.#5

Merged
bobeasterday merged 2 commits into
phonegap-build:masterfrom
NetMatch:master
Jun 5, 2013
Merged

Updated to Cordova 2.7, fixed a number of bugs. Cleaned up some code.#5
bobeasterday merged 2 commits into
phonegap-build:masterfrom
NetMatch:master

Conversation

@gsmeets
Copy link
Copy Markdown

@gsmeets gsmeets commented May 17, 2013

  • The application now fully support push notifications when the application isn't loaded in the background.
  • Put all the plugin code in the plugin namespace: com.plugin.gcm;
  • Renamed to namespace GCM to gcm, namespaces should be lower case
    according to the java style guidelines;
  • Deleted the google gcm code and added gcm.jar instead;
  • Added CordovaGCMBroadcastReceiver.java to enable GCMIntentService to
    be namespaced in the plugin namespace instead of the client project
    (which is is the default with google's GCMIntentService);
  • Moved all java <--> javascript interop to PushPlugin;
  • Moved all notification code to GCMIntentService (maybe this fits
    better in its own class);
  • Changed the way the plugin detects cold starts in order for it not to
    crash when the back button is used to navigate out of the app;
  • Changed the caching on the filesystem to a memory cache on a static
    variable, which increases performance and simplifies code;
  • Cut up some code into separate methods ;
  • Added code comments;
  • Fixed indentation and whitespaces to use tabs, this causes a lot of changed lines unfortunately;
  • Updated the readme file.

* Put all the plugin code in the plugin namespace: com.plugin.gcm;
* Renamed to namespace GCM to gcm, namespaces should be lower case
according to the java style guidelines;
* Deleted the google gcm code and added gcm.jar instead;
* Added CordovaGCMBroadcastReceiver.java to enable GCMIntentService to
be namespaced in the plugin namespace instead of the client project
(which is is the default with google's GCMIntentService);
* Moved all java <--> javascript interop to PushPlugin;
* Moved all notification code to GCMIntentService (maybe this fits
better in its own class);
* Changed the way the plugin detects cold starts in order for it not to
crash when the back button is used to navigate out of the app;
* Changed the caching on the filesystem to a memory cache on a static
variable, which increases performance and simplifies code;
* Cut up some code into separate methods ;
* Added code comments;
* Updated the readme file.
@bobeasterday
Copy link
Copy Markdown

Thanks so much for your submission. Looks like a lot of work. I do have a few questions before I dig in any deeper.

  1. Did you have to make any changes to your App to accommodate these changes? I want to know that the user can still determine cold start, background, or foreground contexts, without having to change their existing app.

  2. I noticed you changed the base-class from Plugin to CordovaPlugin, for 2.7 compatibility. We are still determining the correct course of action on this since PhoneGap Build still needs to support previous versions of Cordova, and this is one of the plugins that PGB supports.

  3. Can you provide any additional context as to what specific bugs you fixed and what behavior differences can be expected?

Thanks again!

@gsmeets
Copy link
Copy Markdown
Author

gsmeets commented May 17, 2013

1.) There are a few changes in the AndroidManifest due to the namespace renaming I did. I've also updated the plugin config file to reflect these changes. That's the only thing that's changed from the client code's perspective. Cold start, background and foreground contexts still operate the same. Just to be sure we're on the same page:

The cold start flag is set when the application had to recreate the main activity with the cordova plugin. Technically this can happen when the application is still active in the background due to unloading of the activity by the OS.
The background flag is set when the application sent a notification which in turn is clicked by the user to open the app.

Foreground happens when the app is active and no notification is sent.

2.) I have no idea of PBG's infrastructure, but I guess adding support for different plugin versions with different cordova versions seems like a reasonable thing to do?

3.) The main issue was that under some circumstances you could get a "the application stopped working" message when re-entering the application after the main activity (and the PushPlugin) were unloaded. This happens when you click the back button out of your app, or when the app is cleaned up while running in the background to reclaim resources for other apps. This can only occur if you don't unregister your push messages on exit, (which is what you want when you want the application to send you messages even after it exited).

The other bugs are probably just cordova 2.7 issues (although I'm not sure, I didn't use this library beforehand), and bugs caused by my refactoring process.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work 👍

@lholmquist
Copy link
Copy Markdown

@gsmeets why did you change the namespacing of the classes?

if i update just the source, i don't want to have to change the manifest.xml and would think it should just work

@dlamotta
Copy link
Copy Markdown

dlamotta commented Jun 5, 2013

This is awesome work. We like it so much, in fact, that we have a PushNotification.js binding with JSNI in a library of our own (Pilot, version 2.7 is not out yet). And I've been trying to test it, but PushPlugin.java extends from Plugin, which is not found in cordova-2.7.0.jar, and thus I have a compile-time error in my project. I see a CordovaPlugin, but not a plain Plugin class.

I installed this plugin in my Android project using plugman, so I grabbed the latest code from GitHub when I installed it. Other than the project not yet fully compatible with Cordova 2.7, I am not sure what else could be missing.

Thoughts?

Thanks!

@bobeasterday
Copy link
Copy Markdown

@dlamotta This is because Cordova 2.7 changed the name of the base class on which all plugins derive. Why not just change all our plugins to extend CordovaPlugin, you ask (and I'm glad you did)?

PhoneGap Build necessarily supports versions of Cordova older than 2.7. Thus 'fixing' the plugin for 2.7 will break it for all of our customers who still build their apps on older versions of Cordova. We can get away with supporting 2.7 AND older versions on our Build servers because we have control of our own tooling.

For manual builds, however, you will need to either build against an older version or Cordova, OR modify PushPlugin.java to extend "CordovaPlugin" instead of "Plugin".

So instead of;
public class PushPlugin extends Plugin {

You need;
public class PushPlugin extends CordovaPlugin {

for 2.7+

HTH

bobeasterday added a commit that referenced this pull request Jun 5, 2013
Updated to Cordova 2.7, fixed a number of bugs. Cleaned up some code.
@bobeasterday bobeasterday merged commit dd4ca03 into phonegap-build:master Jun 5, 2013
@dlamotta
Copy link
Copy Markdown

dlamotta commented Jun 5, 2013

@bobeast Thanks for the reply. Ok, that's interesting, so manual build folks will run into this problem when using Cordova 2.7.

I started going down the path of replacing 'Plugin' with 'CordovaPlugin', but the changes were deeper than that. I'll take another look at it tonight.

Thanks again.

@bobeasterday
Copy link
Copy Markdown

@dlamotta be sure and re-sync up to master. I am now extending CordovaPlugin since users will increasingly build against 2.7.0 and above. Let me know how it goes.

@dlamotta
Copy link
Copy Markdown

dlamotta commented Jun 5, 2013

@bobeast awesome. Even better! Thanks.

@mattcallen
Copy link
Copy Markdown

Hi,

Please remove me from your mailing list I am getting flooded :)

Thanks,
Matt

On 6/5/2013 3:02 PM, Bob Easterday wrote:

@dlamotta https://github.com/dlamotta be sure and re-sync up to
master. I am now extending CordovaPlugin since users will increasingly
build against 2.7.0 and above. Let me know how it goes.


Reply to this email directly or view it on GitHub
#5 (comment).

gaguirre pushed a commit to gaguirre/phonegap-plugin-push that referenced this pull request Sep 29, 2015
Fix didRegisterForRemoteNotification on iOS8
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.

6 participants