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

[TIMOB-15443] Android: Enable proxies to get window/tabgroup activity lifecycle events #6179

Closed
wants to merge 5 commits into from

Conversation

mokesmokes
Copy link
Contributor

Very simple to use:

  1. In the proxy creation dictionary pass lifecycleContainer: windowOrTabGroup
  2. In the proxy set the lifecycle callbacks.

See example in JIRA https://jira.appcelerator.org/browse/TIMOB-15443

@@ -394,9 +394,6 @@ protected void handlePostOpen()
// Prevent any duplicate events from firing by marking
// this group has having focus.
isFocused = true;

// Setup the new tab activity like setting orientation modes.
onWindowActivityCreated();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call to onWindowActivityCreated is redundant since it's already called in TiBaseActivity.onCreate for both Windows and TabGroups. That function is not idempotent thus it's a bug.

@mokesmokes
Copy link
Contributor Author

The TiLifecycle is missing the onCreate method. In order to not break existing proxies, I did not add it...... but I'm not sure why we don't have the full lifecycle states. Can someone please shed some light on this? Perhaps we should bite the bullet and add all the callbacks, fix any issues in the main source, and assume external modules will be fixed by their authors? In any case, at this point this PR is not a breaking change.

@FokkeZB
Copy link
Contributor

FokkeZB commented Sep 29, 2014

@ingo, would love to have this so the new Facebook Android module is more user friendly.

@mokesmokes
Copy link
Contributor Author

@FokkeZB @ingo not just for user friendliness - at this point we don't have a Facebook module without this PR or #6160 or both. This one is more elegant and easier to use, plus since @pingwang2011 expressed concerns about making synchronous calls to Javascript during the activity lifecycle perhaps we should remove those callbacks entirely (as async calls they are often mistimed with the actual events) - and people who need them can just use the API in this PR in their module.

@mokesmokes
Copy link
Contributor Author

To reiterate: I actually think it's important that we add onCreate to the Java lifecycle callbacks. Any proxies that inherit KrollProxy do not need to be modified. It will require only 4 or 5 small modifications in the code base (just adding an empty function does it). It may possibly break external modules that don't inherit from KrollProxy but that bug will show up in compile time and is trivial to fix. I strongly suggest we add onCreate, unless there are good reasons not to.

@mokesmokes
Copy link
Contributor Author

OK.... added onCreate to TiLifecycle. The change broke only 5 files that did not inherit from KrollProxy. This is a breaking change and could affect external modules that don't inherit from KrollProxy and use TiLifecycle. Fixing them is trivial - just import android.os.Bundle and adding an empty onCreate function, but it's important that developers get a heads up and thus please merge this ASAP.

Also note that with this PR and #6008 Titanium code (and modules) will have access to the full Activity lifecycle including onSave/onRestoreInstanceState. This will make porting Android libraries much much easier.

@mokesmokes
Copy link
Contributor Author

The PR has also been updated to include onActivityResult events (this is a backwards compatible change, no effect on existing code). It is possible for modules to use TiActivitySupportHelper for this functionality, but this is a clumsy mechanism if we're porting 3rd party libraries that launch their own activities (e.g. Facebook SDK which launches the Facebook app for authentication, permissions, etc.). Adding this mechanism directly enables easy usage of onActivityResult without needing to hack the 3rd party SDK to use TiActivitySupportHelper.

To use this functionality the proxy can call activity. addOnActivityResultListener(proxy) during the onCreate handler, and of course implement the onActivityResultEvent interface (just the one function of onActivityResult).

@mokesmokes
Copy link
Contributor Author

Added saveInstanceState and restoreInstanceState. This does not affect existing code. This particular addition is already in this PR: #6008 but I needed it here for the Facebook module. Depending upon which PR is merged first I will fix the other one to make it mergeable.

To see all these APIs in action see this file in the new Facebook module: https://github.com/mokesmokes/titanium-android-facebook/blob/master/src/com/ti/facebook/ActivityWorkerProxy.java

@@ -512,6 +513,7 @@ public void onResume(Activity activity)
public void onDestroy(Activity activity) {}
public void onStart(Activity activity) {}
public void onStop(Activity activity) {}
public void onCreate(Activity activity, Bundle savedInstanceState) {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To comply with the updated interface since onCreate was added

@mokesmokes
Copy link
Contributor Author

Added onCreate/PrepareOptionsMenu interfaces. An example of this is when developing the Chromecast module, I need access to onCreateOptionsMenu in the proxy to add the Cast button to the ActionBar.

@mokesmokes
Copy link
Contributor Author

This was merged by #6272 so should be closed....

@ingo
Copy link
Contributor

ingo commented Nov 17, 2014

Closing as requested by @mokesmokes

@ingo ingo changed the title [TIMOB-15443] enable proxies to get window/tabgroup activity lifecycle events [TIMOB-15443] Android: Enable proxies to get window/tabgroup activity lifecycle events Nov 18, 2014
@jonalter
Copy link
Contributor

Closing again. Merged in #6272 and #6275

@jonalter jonalter closed this Nov 18, 2014
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.

None yet

4 participants