android notif: Replace our NotificationIntentService with directly opening MainActivity #5146

chrisbobbe · 2021-12-02T23:14:52Z

chrisbobbe · 2021-12-02T23:16:42Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

@@ -11,6 +11,25 @@ import com.swmansion.gesturehandler.react.RNGestureHandlerEnabledRootView;
 import com.zulipmobile.notifications.*
 import com.zulipmobile.sharing.maybeHandleShareIntent

+/* Returns true just if we did handle the intent. */
+fun maybeHandleOpenNotificationIntent(intent: Intent): Boolean {


I think we probably want this to live somewhere else (the share-to counterpart lives in SharingHelper.kt), and probably have a better name? I haven't settled on anything definite for either yet though.

Yeah. Looking at this function and now looking at the share-to version beside it, I think what I really want is to have those two bits of code next to each other -- and probably cleanest as a single when (intent.action), with sub-conditions as needed (like looking for EXTRA_NOTIFICATION_DATA, though actually the cleanest thing there -- which can be a followup -- is probably to parse the outermost parts of the URL.)

IOW rather than align the name of maybeHandleIntent to reflect where it lives, align where it lives to reflect its name: move it to be a private method on MainActivity, and then later in the branch add the open-notification case to it.

Part of why I'd like them to be next to each other is that they're carving out two different swaths of the same space of possibilities -- if we were to have any two things like this that could overlap in what they covered, then the order we called them in would start mattering, and it'd probably be a bug. So it's helpful to have them next to each other and ideally in the same when statement, so that it's easy to see they're mutually exclusive and keep them that way.

gnprice

Thanks! Glad to have this getting cleaned up. Comments below.

gnprice · 2021-12-03T23:11:19Z

android/app/src/main/java/com/zulipmobile/notifications/NotifyReact.kt

-    when (appStatus) {
-        null, ReactAppStatus.NOT_RUNNING, ReactAppStatus.BACKGROUND ->
-            launchMainActivity(application as Context)
-        ReactAppStatus.FOREGROUND -> Unit


I didn't find any reasons given for the conditional; possibly it's an unquestioned relic of the Wix library we used to use.

Indeed the behavior goes back to the Wix library. Looks like it's basically unchanged since 9eddca8:

android notif: Use standard upstream API for watching RN lifecycle. This has exactly the same effect as the 80 lines of code in the wix library's ReactAppLifecycleFacade.

with the logic a bit refactored in 9b0157d and 0feb940.

I don't think having the conditional is necessarily bad, given that launching isn't going to do anything useful when already in the foreground. But with the changes upcoming in this branch, it becomes useful to do this unconditionally, so it's a good change now.

gnprice · 2021-12-03T23:17:33Z

android/app/src/main/java/com/zulipmobile/notifications/NotifyReact.kt

-    Log.d(TAG, "NotifyReact: launching main activity")
+    Log.d(TAG, "android notif: launching main activity")


android notif: Remove false specificity from a log line Launching MainActivity doesn't really have anything to do with "notifying React", i.e., sending information about the opened notification to the JS environment.

Yeah, the prefix there was really to make this log line identifiably correspond to this line of code -- so it was a reference to the filename, not a description of the substance.

I think the simplest thing is just to delete this line, if you haven't had a use for it. (That's what happens later in the branch anyway.)

(If we were to keep this, I'd drop "android" from the prefix -- this is already going to an Android-specific log system.)

gnprice · 2021-12-03T23:29:51Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

-    override fun onNewIntent(intent: Intent?) {
+    override fun onNewIntent(intent: Intent) {


android notif [nfc]: Make MainActivity.onNewIntent's `intent` arg required ReactActivity's `.onNewIntent`, which we're overriding, requires `intent`.

I think it doesn't. Here's the declaration in ReactActivity.java:

@Override public void onNewIntent(Intent intent) {

That doesn't mention the possibility of the argument being null. But... in Java, all object types are nullable. So Java Intent translates to Kotlin Intent?.

Sometimes there's a Java annotation @NotNull indicating that a given value isn't supposed to be null. But there doesn't seem to be one of those here. So if a caller passes null, no linter would complain either.

If you look in the Kotlin version of the SDK docs:
https://developer.android.com/reference/kotlin/android/app/Activity#onnewintent
this is reflected in the notation Intent!:

protected open fun onNewIntent(intent: Intent!): Unit

The meaning of that ! notation is defined here:
https://kotlinlang.org/docs/java-interop.html#notation-for-platform-types

T! means "T or T?"

Practically, the way I ended up with this version of the definition is probably that I started typing "onNewIntent" and let Android Studio autocomplete an empty override for me.

gnprice · 2021-12-03T23:46:39Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+        && intent.hasExtra(EXTRA_NOTIFICATION_DATA)
+    ) {
+        val data = intent.getBundleExtra(EXTRA_NOTIFICATION_DATA)
+        logNotificationData("notif opened", data!!)


android notif: Move a log line into MainActivity's lifecycle methods

This commit is kind of confusing because it's rather a lot more code than just moving a log line sounds like it should be:
2 files changed, 26 insertions(+), 4 deletions(-)

The added plumbing makes sense because of the real work we have it do in the next commit. I think the clearest thing is probably to squash that one into this. The result is only slightly bigger than this already is:
2 files changed, 26 insertions(+), 6 deletions(-)

and it's easier to understand as changing where we do the real work, with the log line coming along for the ride.

gnprice · 2021-12-03T23:52:07Z

android/app/src/main/java/com/zulipmobile/notifications/NotificationIntentService.kt


 class NotificationIntentService : IntentService("NotificationIntentService") {
    override fun onHandleIntent(intent: Intent?) {
        val context = applicationContext as? MainApplication ?: return
        if (Intent.ACTION_VIEW == intent!!.action) {
            val data = intent.getBundleExtra(EXTRA_NOTIFICATION_DATA)
-            onOpened(context, data!!)
+            Log.d(TAG, "android notif: launching main activity")


This commit and the next move this code around twice:

6fd1a19 android notif [nfc]: Inline NotificationUiManager.onOpened
2 files changed, 23 insertions(+), 26 deletions(-)
2521e1d android notif: Remove NotificationIntentService
2 files changed, 21 insertions(+), 50 deletions(-)

If squashed, the resulting diff is just slightly bigger than one of those:
2 files changed, 21 insertions(+), 53 deletions(-)

That also keeps the move within one file, rather than moving to a different file and back. So I think it's clearest to squash those two together.

With that, the preceding commit:
3cf3564 android notif [nfc]: Rename variables to ease next refactor
2 files changed, 6 insertions(+), 6 deletions(-)

also becomes unnecessary.

gnprice · 2021-12-03T23:59:58Z

(Also just replied to the initial thread above.)

…ally The intent used to start the main activity has the FLAG_ACTIVITY_NEW_TASK flag. The doc on that flag says this [1]: > When using this flag, if a task is already running for the > activity you are now starting, then a new activity will not be > started; instead, the current task will simply be brought to the > front of the screen with the state it was last in. That should mean that there's no hiccup or loss of state if we run our code to launch the main activity from the main activity when it's already foregrounded. Empirically that seems to be the case. I didn't find any history that says the conditional is necessary; looks like it dates back to the old Wix library we used to use here [2]. [1] https://developer.android.com/reference/android/content/Intent#FLAG_ACTIVITY_NEW_TASK [2] zulip#5146 (comment)

chrisbobbe · 2021-12-07T22:23:06Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Looks good -- just small comments below.

gnprice · 2021-12-07T23:10:39Z

android/app/src/main/java/com/zulipmobile/sharing/SharingHelper.kt


 @JvmField
 val TAG = "ShareToZulip"

-/* Returns true just if we did handle the intent. */
-fun maybeHandleIntent(


android notif [nfc]: Make maybeHandleIntent a helper method on MainActivity

nit: s/ notif//

Similarly for the next commit:

android notif [nfc]: Simplify interface of MainActivity's maybeHandleIntent

gnprice · 2021-12-07T23:15:45Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+                if (!intent.hasExtra(EXTRA_NOTIFICATION_DATA)) {
+                    return false
+                }
+
+                val data = intent.getBundleExtra(EXTRA_NOTIFICATION_DATA)
+                logNotificationData("notif opened", data!!)


This can be cleaned up slightly by putting the getBundleExtra first, and then checking if it's null instead of calling hasExtra. Then the !! becomes unnecessary.

That also means that if somehow we get an intent with an EXTRA_NOTIFICATION_DATA extra that isn't a Bundle, then we don't crash at the !! and instead just ignore it here as unhandled.

gnprice · 2021-12-07T23:26:04Z

android/app/src/main/java/com/zulipmobile/notifications/NotifyReact.kt

-    // TODO deduplicate this with handleSend in SharingHelper.kt.
-    // Until then, keep in sync when changing.


We could deduplicate all the way down to `appStatus`, except

Yeah. There's still quite a bit of trickiness here that I'd like to deduplicate: sometimes we set a static/global variable, and sometimes we emit an event, and we need to pick the right one or we're likely to drop or duplicate an event. My ideal version of this and handleSend is that they'd each say: here's a WritableMap, and here's a little value identifying where the event should go, and then all that logic like the when (appStatus) would be in a common function they both invoke.

This change is a helpful step! But let's keep these TODO comments here.

gnprice · 2021-12-07T23:43:35Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+        }
+
+
+        val host = (application as ReactApplication).reactNativeHost


nit: just one blank line

gnprice · 2021-12-07T23:45:17Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+    /* Returns true just if we did handle the intent. */
+    private fun maybeHandleIntent(
+        intent: Intent?,
+        contentResolver: ContentResolver


This parameter can be simplified out in much the same way as application: ReactApplication: the call sites just get it from a getter on this, so this function can do the same thing.

gnprice · 2021-12-07T23:51:25Z

android/app/src/main/java/com/zulipmobile/MainActivity.kt

+        val host = (application as ReactApplication).reactNativeHost
+        val reactContext = host.tryGetReactInstanceManager()?.currentReactContext


Hmm, actually I don't love having these lines run here, above the when (intent.action).

The thing is that that means they run even if they're not needed -- if the intent is something else. They look like they could be cheap and side-effect-free… or could not be. I think there isn't any live case where it'd matter even if they are -- the only intents we handle other than the two cases below are the web-auth intents, where we'll want a React instance/context anyway -- but I'd rather not have to worry about that possibility coming up in the future.

Maybe simplest is I'll just add a commit at the end to address this.

Maybe simplest is I'll just add a commit at the end to address this.

OK, SGTM, thanks!

…ally The intent used to start the main activity has the FLAG_ACTIVITY_NEW_TASK flag. The doc on that flag says this [1]: > When using this flag, if a task is already running for the > activity you are now starting, then a new activity will not be > started; instead, the current task will simply be brought to the > front of the screen with the state it was last in. That should mean that there's no hiccup or loss of state if we run our code to launch the main activity from the main activity when it's already foregrounded. Empirically that seems to be the case. I didn't find any history that says the conditional is necessary; looks like it dates back to the old Wix library we used to use here [2]. [1] https://developer.android.com/reference/android/content/Intent#FLAG_ACTIVITY_NEW_TASK [2] zulip#5146 (comment)

chrisbobbe · 2021-12-08T01:20:41Z

Thanks! Revision pushed.

chrisbobbe · 2021-12-08T01:22:19Z

Thanks! Revision pushed.

Ah, and is there a followup from #5146 (comment) :

actually the cleanest thing there -- which can be a followup -- is probably to parse the outermost parts of the URL.

I'm not really sure what you mean there.

Most of them were already formatted correctly, and they'll still be formatted correctly at the end of this series.

For the reason given in the TODO, but also because we want to try removing the me.leolin.shortcutbadger.ShortcutBadger dependency and seeing if things still work OK. We leave the `connectGlobal` call because the component still uses the `dispatch` prop.

…n open I tested this on the office Android device (a Samsung Galaxy S9 running Android 9), and badge counts still work as I expect, including decreasing as messages are read, and disappearing if it's gone down to zero.

We stopped calling one of the methods from JS in a recent commit.

…ally The intent used to start the main activity has the FLAG_ACTIVITY_NEW_TASK flag. The doc on that flag says this [1]: > When using this flag, if a task is already running for the > activity you are now starting, then a new activity will not be > started; instead, the current task will simply be brought to the > front of the screen with the state it was last in. That should mean that there's no hiccup or loss of state if we run our code to launch the main activity from the main activity when it's already foregrounded. Empirically that seems to be the case. I didn't find any history that says the conditional is necessary; looks like it dates back to the old Wix library we used to use here [2]. [1] https://developer.android.com/reference/android/content/Intent#FLAG_ACTIVITY_NEW_TASK [2] zulip#5146 (comment)

…eact` This should be fine and NFC because `launchMainActivity` is just starting an asynchronous task, without waiting for it to complete.

I think we haven't had any use for this line.

And, while we're at it, move launchMainActivity's definition, too, since it has nothing to do with "notifying React", i.e., sending information about the opened notification to the JS environment.

To plumb the notification data to the new location, we attach the data to the launch-main-activity intent as an extra, copied from the present intent (the one created when the notification was received, that we're handling with NotificationIntentService).

…lication

And replace it with directly opening MainActivity. When we set targetSdkVersion to 31, we will no longer be allowed to use a Service as a "notification trampoline", for performance reasons: https://developer.android.com/about/versions/12/behavior-changes-12#notification-trampolines So, go with their recommended fix, and have the PendingIntent open MainActivity directly. In the preceding commits, we moved everything out of NotificationIntentService except the code that starts MainActivity, to make this commit as small as possible. (The really important bit was the `notifyReact` call, which lets the JS environment know about the notification the user opened.) Fixes: zulip#5145

And link to a comment from Greg on further work toward deduplicating.

gnprice · 2021-12-08T21:57:13Z

Thanks! Looks good -- merging. I made one tweak to cut the SuppressLint import in the commit where it stops being used (because Android Studio did that automatically after I made some other edits there.)

I have a draft for that followup and this one: #5146 (comment), and I'll send those as a separate PR because they're nontrivial refactors.

chrisbobbe · 2021-12-08T22:00:32Z

Great, thanks very much!

gnprice · 2021-12-08T23:18:37Z

#5160

…ally The intent used to start the main activity has the FLAG_ACTIVITY_NEW_TASK flag. The doc on that flag says this [1]: > When using this flag, if a task is already running for the > activity you are now starting, then a new activity will not be > started; instead, the current task will simply be brought to the > front of the screen with the state it was last in. That should mean that there's no hiccup or loss of state if we run our code to launch the main activity from the main activity when it's already foregrounded. Empirically that seems to be the case. I didn't find any history that says the conditional is necessary; looks like it dates back to the old Wix library we used to use here [2]. [1] https://developer.android.com/reference/android/content/Intent#FLAG_ACTIVITY_NEW_TASK [2] zulip#5146 (comment)

chrisbobbe requested a review from gnprice December 2, 2021 23:14

chrisbobbe force-pushed the pr-notif-activity branch from 9990022 to 2521e1d Compare December 2, 2021 23:15

chrisbobbe commented Dec 2, 2021

View reviewed changes

gnprice reviewed Dec 3, 2021

View reviewed changes

chrisbobbe force-pushed the pr-notif-activity branch from 2521e1d to c03ffe8 Compare December 7, 2021 22:22

gnprice reviewed Dec 7, 2021

View reviewed changes

chrisbobbe force-pushed the pr-notif-activity branch from c03ffe8 to dc7d4a7 Compare December 8, 2021 01:20

chrisbobbe added 17 commits December 8, 2021 13:52

android [nfc]: Reformat all Android files we're about to change

3889ac1

Most of them were already formatted correctly, and they'll still be formatted correctly at the end of this series.

android notif [nfc]: Remove unused import

6b3d42b

android [nfc]: Remove now-unused BadgeCountUpdaterModule

457f1a3

We stopped calling one of the methods from JS in a recent commit.

android deps: Remove now-unused ShortcutBadger

7419bb5

android notif [nfc]: Call launchMainActivity at the top of `notifyR…

9a64d05

…eact` This should be fine and NFC because `launchMainActivity` is just starting an asynchronous task, without waiting for it to complete.

android notif: Remove a log line

d241370

I think we haven't had any use for this line.

android notif [nfc]: Move launchMainActivity call out of notifyReact

ef53378

And, while we're at it, move launchMainActivity's definition, too, since it has nothing to do with "notifying React", i.e., sending information about the opened notification to the JS environment.

android notif [nfc]: Inline launchMainActivity

478ee95

android [nfc]: Make maybeHandleIntent a helper method on MainActivity

c069979

android [nfc]: Simplify interface of MainActivity's maybeHandleIntent

23f6e44

android notif [nfc]: Pass ordinary Context object instead of ReactApp…

ee126b9

…lication

android notif [nfc]: Deduplicate a bit of code

fdec74f

And link to a comment from Greg on further work toward deduplicating.

gnprice force-pushed the pr-notif-activity branch from dc7d4a7 to fdec74f Compare December 8, 2021 21:57

gnprice merged commit fdec74f into zulip:main Dec 8, 2021

gnprice mentioned this pull request Dec 8, 2021

android: Small refactors around handling intents #5160

Merged

chrisbobbe deleted the pr-notif-activity branch December 9, 2021 00:38

chrisbobbe mentioned this pull request Dec 17, 2021

Target Android 12 (set targetSdkVersion to 31), by 2022-11 deadline #5101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

android notif: Replace our NotificationIntentService with directly opening MainActivity #5146

android notif: Replace our NotificationIntentService with directly opening MainActivity #5146

chrisbobbe commented Dec 2, 2021

chrisbobbe Dec 2, 2021 •

edited

Loading

gnprice Dec 3, 2021

gnprice left a comment

gnprice Dec 3, 2021

gnprice Dec 3, 2021

gnprice Dec 3, 2021

gnprice Dec 3, 2021

gnprice Dec 3, 2021

gnprice commented Dec 3, 2021

chrisbobbe commented Dec 7, 2021

gnprice left a comment

gnprice Dec 7, 2021

gnprice Dec 7, 2021

gnprice Dec 7, 2021

gnprice Dec 7, 2021

gnprice Dec 7, 2021

gnprice Dec 7, 2021

chrisbobbe Dec 8, 2021

chrisbobbe commented Dec 8, 2021

chrisbobbe commented Dec 8, 2021 •

edited

Loading

gnprice commented Dec 8, 2021

chrisbobbe commented Dec 8, 2021

gnprice commented Dec 8, 2021

		Log.d(TAG, "NotifyReact: launching main activity")
		Log.d(TAG, "android notif: launching main activity")

		override fun onNewIntent(intent: Intent?) {
		override fun onNewIntent(intent: Intent) {

		// TODO deduplicate this with handleSend in SharingHelper.kt.
		// Until then, keep in sync when changing.

		}


		val host = (application as ReactApplication).reactNativeHost

		val host = (application as ReactApplication).reactNativeHost
		val reactContext = host.tryGetReactInstanceManager()?.currentReactContext

android notif: Replace our NotificationIntentService with directly opening MainActivity #5146

android notif: Replace our NotificationIntentService with directly opening MainActivity #5146

Conversation

chrisbobbe commented Dec 2, 2021

chrisbobbe Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Dec 3, 2021

chrisbobbe commented Dec 7, 2021

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Dec 8, 2021

chrisbobbe commented Dec 8, 2021 • edited Loading

gnprice commented Dec 8, 2021

chrisbobbe commented Dec 8, 2021

gnprice commented Dec 8, 2021

chrisbobbe Dec 2, 2021 •

edited

Loading

chrisbobbe commented Dec 8, 2021 •

edited

Loading