refactor(android): Refactored app resume, intent-filter, and "newintent" handling #10519

jquick-axway · 2018-12-08T06:58:35Z

JIRA:

Summary:

Fixed bug where an intent can create a 2nd app instance stuck at the splash screen if an app instance already exists.
- Typically happens when app is launch via URLs, tapping notifications, etc.
- Fixed by immediately destroying 2nd instance, resuming existing activity, and firing a "newintent" event which simulates Android "singleTask" handling.
Fixed bug where startActivityForResult() can create a 2nd app instance stuck at the splash screen if an app instance already exists.
- Fixed by destroying 1st instance (all windows are closed) and then starting up a new Titanium activity instance.
TIMOB-26636 Removed deprecated "newIntent" event in favor of "newintent".
TIMOB-26637 Added new "rootActivity" property to Ti.Android:
- Improves intent-filter usability since only the root activity will receive the new intent.
- Note that existing "currentActivity" property always references the top-most activity opened by Ti.UI.Window.
TIMOB-26638 Root Ti.Android.Activity "intent" property will now be updated when "newintent" has been fired.
- This "intent" property used to be assigned the intent that originally launched/created the activity and would never change.
- This is needed in case "newintent" event gets fired before the "app.js" has a chance to set up a listener for it. Can happen when receiving a URL intent during a cold start of the app.
- Note that Ti.App.Android already provides a "launchIntent". So, we're not losing anything.
TIMOB-26645 Calling finish() method on root activity should exit app.
- This makes it easier for Titanium apps launched via startActivityForResult() since the app needs to call setResult() and finish() methods on root activity to return a result.
Dropped support for Android bug 2373/5277 "tiapp.xml" properties listed here.
- Titanium no longer shows a restart dialog. (Nobody likes this behavior.)
- App now resumes with a "newintent" event, similar to iOS' "handleurl" support.
Dropped support for for "tiapp.xml" property intent-filter-new-task.
- No longer needed now that Android singleTask like support via "newintent" is always enabled.

New Intent Test:

Create a classic Titanium app.
Set project name to "IntentTest". (This is <name/> in "tiapp.xml".)
Set project's "Application Id" to "com.appc.intent.test". (This is <id> in "tiapp.xml".)
Set up the "app.js" with the below code.
Build and run on Android.
On app startup, notice that a JSON of the launch intent is shown.
Verify that displayed intent's "action" is set to "android.intent.action.MAIN".
Open the Mac "Terminal".
CD (change directory) to: ~/Library/Android/sdk/platform-tools
In the terminal, enter the following...
./adb shell am start -n com.appc.intent.test/.IntenttestActivity -a android.intent.action.VIEW -d https://www.google.com
In the app, verify that displayed intent "action" is set to "https://www.google.com".
On Android, press the "Home" button.
In the terminal, enter the following...
./adb shell am start -n com.appc.intent.test/.IntenttestActivity -a android.intent.action.VIEW -d https://www.appcelerator.com
Verify that the Android app is resumed.
Verify that displayed intent "action" is set to "https://www.appcelerator.com".
On Android, press the "Back" button.
In the terminal, enter the following...
./adb shell am start -n com.appc.intent.test/.IntenttestActivity -a android.intent.action.VIEW -d https://www.google.com
Verify that the app gets launched.
Verify that displayed intent "action" is set to "https://www.google.com".
On Android, press the "Home" button. (Do NOT press the "Back" button.)
Go to the app selection screen and tap the app's icon to resume it.
Verify that displayed intent "action" is still set to "https://www.google.com".
In the terminal, enter the following...
./adb shell am start -n com.appc.intent.test/.IntenttestActivity -a android.intent.action.VIEW -d https://www.appcelerator.com
Verify that displayed intent "action" is set to "https://www.appcelerator.com".
In the terminal, enter the following...
./adb shell am start -n com.appc.intent.test/.IntenttestActivity -a android.intent.action.VIEW -d https://www.google.com
Verify that displayed intent "action" is set to "https://www.google.com".

Ti.Android.rootActivity.addEventListener("newintent", function(e) {
	label.text = "New Intent:\n" + JSON.stringify(e.intent, null, 4);
	Ti.API.info("@@@ rootActivity.intent: " + JSON.stringify(Ti.Android.rootActivity.intent));
});

var window = Ti.UI.createWindow();
var scrollView = Ti.UI.createScrollView({
	width: Ti.UI.FILL,
	height: Ti.UI.FILL,
});
var label = Ti.UI.createLabel({
	text: "Launch Intent:\n" + JSON.stringify(Ti.Android.rootActivity.intent, null, 4),
	width: Ti.UI.FILL,
	height: Ti.UI.SIZE,
});
scrollView.add(label);
window.add(scrollView);
window.open();

Activity For Result Test:

Create a Classic Titanium app.
Set project name to "IntentReturnTest". (This is <name/> in "tiapp.xml".)
Set application ID to "com.appc.intent.returntest". (This is <id> in "tiapp.xml".)
Set up app.js using IntentReturnResultTest.js code attached to TIMOB-26075.
Build and install app on Android.
Back out of the app after launch. (This app will be launched by next created app.)
Create another Classic Titanium app named "IntentRequestTest".
Set up app.js using IntentRequestResultTest.js code attached to TIMOB-26075.
Build and run on Android.
On app start up, tap the "Send Intent" button.
Verify that a new window titled "Returns Result" appears. (This is a window from the "IntentReturnTest" app.)
Verify onscreen JSON has "action" set to "android.intent.action.VIEW".
Verify onscreen JSON has "extras" with "MyString" set to "Hello World", "MyNumber" set to 123.456, and "MyBoolean" set to true.
Press the "Back" button.
Verify that window closes and "Requests Result" window shows "Result Code: 0".
Tap the "Send Intent" button.
Tap the "Set Result" button.
Verify window closes and you have been returned to "Requests Result" window.
Verify onscreen JSON has "action" set to "android.intent.action.SEND".
Verify onscreen JSON has "extras" with "response" set to "My response".
Tap the "Send Intent" button.
Press the Android "Home" button.
Go to the application list screen.
Tap the "IntentReturnTest" to launch the app. (This creates a new instance and destroys the old instance.)
Verify onscreen JSON has "action" set to "android.intent.action.MAIN".
Press the Android "Home" button.
Go to the application list screen.
Tap the "IntentRequestTest" app to resume it.
Verify that window displays "Result Code: 0". (This means the other "IntentReturnTest" window instance was canceled out previously.)
Tap the "Send Intent" button.
Verify that "Returns Result" window was displayed successfully. (This creates a new window instance and destroys the old one launch from app list screen.)

…dling - This commit purposely breaks startActivityOnResult() handling. To be addressed in near future.

- Fixed startActivityForResult() handling. - [TIMOB-26636] Removed deprecated "newIntent" event in favor of "newintent". - [TIMOB-26637] Added "rootActivity" property to Ti.Android module. - [TIMOB-26638] Root Ti.Android.Activity "intent" property is now updated when "newintent" is fired.

build · 2018-12-08T07:14:26Z

	Messages
📖	👍 Hey!, You deleted more code than you added. That's awesome!
📖	💾 Here's the generated SDK zipfile.
📖	✅ All tests are passing Nice one! All 2973 tests are passing.

Generated by 🚫 dangerJS against 73dc3c1

jquick-axway · 2018-12-10T19:53:55Z

Perhaps I should ignore the Intent.FLAG_ACTIVITY_CLEAR_TOP flag. Currently, my code will destroy all the child windows manually when resuming the existing activity to simulate this behavior here...
https://github.com/appcelerator/titanium_mobile/pull/10519/files#diff-ae2da06a2b5bf16e83ace992a20675afR209

The problem with this is that our firebase module sets this flag in the notification. So, tapping on a notification while the app's activity is backgrounded brings you back to the splash which may be confuse app developers.
https://github.com/hansemannn/titanium-firebase-cloud-messaging/blob/master/android/src/firebase/cloudmessaging/TiFirebaseMessagingService.java#L117

Thanks goes to @m1ga for discovering this.

- Modified to ignore "Intent.FLAG_ACTIVITY_CLEAR_TOP". No longer destroys child activites. * Firebase module was setting this flag. Tapping notification caused confusion when child windows disappeared. - Now logs a warning when add event listener for "newIntent". (Must use "newintent" instead.)

jquick-axway · 2018-12-11T06:35:08Z

Updated PR:

Now logs a warning when adding event listener for "newIntent".
- Must use "newintent" (with lowercase 'i') instead.
Now ignores Intent.FLAG_ACTIVITY_CLEAR_TOP upon app resume.
- No longer destroys child activities/windows. (Issue with firebase notification module.)

android/titanium/src/java/org/appcelerator/titanium/TiActivity.java

android/titanium/src/java/org/appcelerator/titanium/TiApplication.java

android/titanium/src/java/org/appcelerator/titanium/TiRootActivity.java

android/titanium/src/java/org/appcelerator/titanium/proxy/ActivityProxy.java

…out of the app

lokeshchdhry · 2018-12-14T23:05:40Z

FR Passed.

The refactor of app resume and newintent handling seems to be working as expected.
Verified that liveview has no issues with these changes.

Studio Ver: 5.1.2.201810301430
SDK Ver: 8.0.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.1
Appc NPM: 4.2.13
Appc CLI: 7.0.9-3
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.4
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Pixel (Android 9)

jquick-axway · 2018-12-15T04:13:45Z

Hmm... there is one more thing we have to watch out for. If you home-out of the app and leave it in the background for 30 minutes, then the Android OS might automatically destroy all of the child activity windows. This means when you resume the app after 30 minutes, you'll be brought back to the splash screen. This is mentioned by the ticket below.
https://jira.appcelerator.org/browse/TIMOB-9229

The above ticket handled it via "tiapp.xml" property "ti.android.root.reappears.restart", which this PR has removed. However, I think there is a better solution. In the "AndroidManifest.xml", we should set the <activity/> attribute alwaysRetainTaskState to "true". This avoids the app restart behavior the old ticket performed and preserves the activity stack.
https://developer.android.com/guide/topics/manifest/activity-element#always

Edit:
I was not able to reproduce this after 30 minutes on my Android 9.0 device. But I'll add the alwaysRetainTaskState attribute to the "AndroidManifest.xml" template anyways.

…"alwaysRetainTaskState" set to true. - Prevents OS from auto-destroying child activities if app is in the background for over 30 minutes. - Replaces/deprecates "tiapp.xml" property: "ti.android.root.reappears.restart"

jquick-axway · 2018-12-18T05:24:52Z

Need to make a couple more updates to this PR for the below shortcut related issues.

Many app developers are wrongly accessing the child activity instead of the root activity for onNewIntent() changes. The old code used to copy the root activity's intent to all of the child activities when they were resumed. I don't agree with this behavior, but I don't want to cause tech-support issues either. I'll replicate this behavior in this PR. It shouldn't cause any harm since a child activity's intent is kind of useless anyways. See ticket TIMOB-20459 for existing code examples that devs are already using. Particularly for shortcuts.

I've also seen code where a shortcut would launch a Java TiJSActivity. Like the above, the expectation is for the currentActivity object's "intent" property to provide the new intent as well as fire the "newintent" event. The most full-proof solution is probably to destroy the JSActivity (it's an outdated concept with this PR) and launch/resume the main launcher activity with that JSActivity's intent passed to it. We need to do this for backward compatibility reasons in case an existing app has already installed a shortcut referencing a JSActivity.

- Modified TiJSActivity derived classes to no longer display its activity. * Now launches/resume main activity with JSActivity's intent. * This feature is now obsolete due to design issues. But kept for backward compatibility. - Added undocumented Ti.App "started" event on all platforms via "ti.main.js". * Will be fired after execution of "app.js". * Currently used on Android to defer execution of JSActivity script until after "app.js" is loaded for Alloy support. - Modified all "Ti.UI.Window" child activities to call their own onNewIntent() when root activity's onNewIntent() gets called. * Maintains legacy behavior where existing Titanium apps expect intent to be copied to all child activities. - Fixed possible KrollProxy.onFireEvent() iteration exception if event listener add/removes a listener. * Was never happening in a real app, but this commit's code changes does this in "TiRootActivity.java".

jquick-axway · 2018-12-20T01:28:01Z

Updated PR:

Restored legacy behavior where root activity's new-intent would be copied to all of its Ti.UI.Window child activities.
Modified TiJSActivity derived classes to no longer show an activity. Will now launch/resume main activity with JSActivity's intent instead.

garymathews · 2018-12-20T21:03:49Z

common/Resources/ti.main.js

+	// This event is to be fired after "app.js" execution. Reasons:
+	// - Allow system to queue startup related events until "app.js" has had a chance to add listeners.
+	// - For Alloy apps, we now know that Alloy has been initialized and its globals were added.
+	Ti.App.fireEvent('started');


Do we want to document this event? If this is an internal event maybe rename to _started ?

I don't have a problem publicly documenting it.

@sgtcoolguy, do you have any objections? The above adds a new Ti.App "started" event on all platforms (Android, iOS, and Windows). It is fired after "app.js" gets executed. For example, this is useful for deferring startup related events until after the "app.js" has had a chance to add listeners for them.

garymathews

CR: PASS (although, see not regarding started event)

NOTE: minor, but there are a few places `final` could be used

This is a behavioral change for JSActivities, as the newintent is fired before the Javascript for the JSActivity is executed. I'm fine with this as it makes sense to fire it when it's received and not wait for the activity. But existing users could be caught out.

lokeshchdhry · 2019-01-02T22:34:20Z

FR Passed.

Studio Ver: 5.1.2.201812191831
SDK Ver: 8.0.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.1
Appc NPM: 4.2.13
Appc CLI: 7.0.9
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.4
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Pixel (Android 9)

jquick-axway added 3 commits December 6, 2018 19:30

[TIMOB-26075] Android: Refactored "intent-filter" and "newintent" han…

c896c6b

…dling - This commit purposely breaks startActivityOnResult() handling. To be addressed in near future.

Merge branch 'master' into TIMOB-26075

59fb9bd

jquick-axway added android feature bug needs docs 🚨 improvement work in progress 🚧 BREAKING CHANGES ⚠️ needs tests 🚨 labels Dec 8, 2018

jquick-axway added this to the 8.0.0 milestone Dec 8, 2018

jquick-axway requested review from garymathews and ypbnv December 8, 2018 06:58

build requested a review from a team December 8, 2018 07:14

m1ga mentioned this pull request Dec 9, 2018

Error on notification click hansemannn/titanium-firebase-cloud-messaging#41

Closed