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

API for saving/restoring plugin and js state #236

Closed
wants to merge 5 commits into from
Closed

API for saving/restoring plugin and js state #236

wants to merge 5 commits into from

Conversation

riknoll
Copy link
Contributor

@riknoll riknoll commented Oct 28, 2015

Here is my proposal for dealing with Activity destruction in Android. The code is still a little rough and might need to be finalized, but I wanted to present it for discussion. This relates to CB-8917 and CB-9189.

Background

The issue at hand is that plugins can make calls to an external Activity and, if the device is low on memory, there is a chance that the CordovaActivity will get killed in the background causing the plugin to lose its state as well as its context for the callback. Activities in Android typically handle this situation by using onSaveInstanceState() and onCreate() methods to save and restore state respectively as the Activity is created and destroyed. This solution exposes that lifecycle to plugins as well as the js, allowing them to save state and have it restored if necessary.

Saving js state

The js is given the ability to save its state in JSON using a new method in navigator.app as part of the CoreAndroid API. An application can pass an object to navigator.app.saveState() to save state in case of Activity destruction. This state is returned to the js as part of the resume event's payload and the js application can use it to properly restore the app. The idea is that app developers can take care of state by subscribing to the pause event to save and have it returned in the corresponding resume event. Plugins are also given the opportunity to add to the JSON to convey any information that is needed for the js to resume properly (see [1] below).

Saving plugin state

A plugin that calls out to an external Activity is given the chance to properly restore its state before handling the result of the Activity by implementing the new onSaveInstanceState() and onRestoreInstanceState() methods that are called as part of the Activity lifecycle. The plugin is given a replacement CallbackContext as part of onRestoreInstanceState that can accept the result and add it to the resume event payload for use in the js (again, see [1] below).

NOTE: When I mention that plugins are given the opportunity to restore state, I want to clarify that this only happens for plugins that are waiting for an external Activity result (I try to emphasize this in the onRestoreInstanceState method's javadoc). This makes the API a little less intuitive, but otherwise we would be conflicting with the accepted behavior that plugins currently get destroyed (i.e. lose all of their state) and are selectively rebuilt whenever a new URL is loaded into the webview. If we restore state on resume, then we can end up with some awkward cases where part of the resuming involves loading a new page so the state gets lost again and so on and so forth. My thinking is that restoring the other state is better left to app developers

Discussion

Benefits:

  • It requires minimal updates to existing plugins (and no updates at all if the plugin doesn't use an external Activity)
  • It is a general solution/pattern that plugins can follow rather than forcing them to include platform specific methods in their APIs
  • It allows the js app to save/restore its state whereas previously the app would just restart with no context

Downsides:

  • It still requires that app developers have platform specific code in their js (unless other platforms adopt this API, but I don't know if they have the corresponding need for it)
  • The resume callback will only ever get received after the initial page loads (potential page flickering)
  • The pending result part of the event object doesn't provide much context (so it puts more responsibility on the app developer to keep state so they know what they're getting)

In the core plugins, this is mostly relevant to the Camera plugin which previously would crash upon receiving the Activity result if the CordovaActivity had been killed by the OS while a picture was being taken/chosen. I also updated the camera plugin to use this new API and the only necessary changes to the existing plugin code were the addition of the onSaveInstanceState and onRestoreInstanceState methods. I can post that code too if there is interest in seeing what the plugin side of this looks like

[1] Anatomy of resume event object:

{
    action: "resume",
    state: <state object passed to app.saveState>,
    plugins: <objects for plugins in the form {serviceName:{pluginState}, ...}>,
    pendingResult: {
        pluginServiceName: <plugin service name e.g. "Camera">,
        pluginStatus: <description of result' status (see PluginResult.java)>,
        result: <argument(s) that would have been given to the callback>
    }
}

@riknoll
Copy link
Contributor Author

riknoll commented Oct 28, 2015

@infil00p @nikhilkh please review. I was also unsure of how to handle cordova.js; do we commit that for every js change? I did not update it in my pr.

@infil00p
Copy link
Member

Can we get an updated Camera plugin branch with the same branch name so that we can work off the same code? Also, an example app for this may be a good idea as well.

@riknoll
Copy link
Contributor Author

riknoll commented Oct 28, 2015

Sure, I'll push the camera branch and work on a good sample app

@riknoll
Copy link
Contributor Author

riknoll commented Oct 28, 2015

@riknoll
Copy link
Contributor Author

riknoll commented Oct 28, 2015

Here's an example index.js and index.html: https://gist.github.com/riknoll/94a40dc147040191fd3e

For anyone hoping to test it out, make sure you check "Don't keep activities" in your phone's developer options.

@infil00p
Copy link
Member

I turned on "Don't keep activities" on both a Moto E (Android 4.4.1) and a Samsung Galaxy S6 Edge (Android 5.1) to test this, and I'm not able to actually get the camera to come back. What device was this tested in, and is there any other variables that we're missing here?

@riknoll
Copy link
Contributor Author

riknoll commented Oct 28, 2015

I am using a Nexus 6 with Marshmallow. Did you update cordova.js? I didn't check it in to the PR because I was unsure of what the protocol was since it's a built file.

@imhotep
Copy link
Contributor

imhotep commented Oct 28, 2015

I believe iOS also has the same issue when transitioning from UIWebView to other native views (although I don't believe app crashes) so I vote for having a common API for this feature. /cc @shazron

@jasongin
Copy link
Contributor

Windows apps also have to deal with saving and restoring state like this in at least some cases. See MSDN - Guidelines for app suspend and resume. Apps can be terminated by the OS while suspended and then should be capable of resuming later based on saved state. However, Windows ensures that won't happen to an app that is specifically waiting on another app for a result, such as capturing a photo.

@nikhilkh
Copy link
Contributor

@riknoll As for cordova.js changes you can follow the instructions here: https://github.com/apache/cordova-js to generate a cordova.js. We generate the file at the time of release. To make it easier to test - you can do the generation here and add a commit to update the file that is present as part of cordova-android.

@riknoll
Copy link
Contributor Author

riknoll commented Nov 6, 2015

So in terms of a common API across platforms, what part are we looking at generalizing? Obviously we should ensure that each platform properly fires its pause and resume events, but is there also a need for allowing plugins to communicate state as I have done here?

@riknoll
Copy link
Contributor Author

riknoll commented Nov 7, 2015

After getting some feedback from @purplecabbage on the dev list and talking this over with @jasongin, it sounds as though we should cut everything from this resume payload besides the pending result (which means we would remove app.saveState as well). The burden will be on the app developer to maintain their state in local storage including the context surrounding whatever plugin calls they have pending. The new resume event payload would look like this:

{
    action: "resume",
    pendingResult: {
        pluginServiceName: <plugin service name e.g. "Camera">,
        pluginStatus: <description of result' status (see PluginResult.java)>,
        result: <the result of the call that would be passed to the callback>
    }
}

I agree that this is a much better idea. We should make sure we communicate the need for saving state well in the Android and plugin docs so that developers are aware of this unique Android "feature".

Are people okay with the proposed approach for allowing plugins that are waiting for external results to bundle up their state on the native side? Alternatively, we can leave plugin developers to be good about maintaining their state (using something like Android preferences). This is technically already possible assuming that we fire events correctly, but a little clunky. The advantage of the approach in this PR is that it exposes the standard Android way of doing things on the native side with the Bundle save/restore API. Does that API make sense? Should we open it up to all plugins instead of just the plugin that is receiving the Activity result?

@riknoll
Copy link
Contributor Author

riknoll commented Nov 11, 2015

I am closing this PR and have submitted a new proposal that incorporates feedback in #239

@riknoll riknoll closed this Nov 11, 2015
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.

5 participants