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.

Sign up for GitHub

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

Jump to bottom

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

Closed
chrisbobbe opened this issue Dec 2, 2021 · 1 comment · Fixed by #5146
Closed

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

chrisbobbe opened this issue Dec 2, 2021 · 1 comment · Fixed by #5146
Labels
a-Android a-notifications

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Dec 2, 2021 •
edited
Loading

Well, here's one blocking issue [for #5101]:
https://developer.android.com/about/versions/12/behavior-changes-12#notification-trampolines
We'll need to replace our NotificationIntentService, which we use when you tap a notification to open it, with directly opening an Activity. (We learned about this from a lint warning in Android Studio: #5121 (comment))

  • Ideally this would be our MainActivity. We'll need to play with things somewhat to try to get the correct behavior in all three cases of the app being in the foreground, in the background, and not running. (Currently NotificationIntentService has NotifyReact.kt do this work.)
    • The main advice from Android upstream won't work for us verbatim because it assumes the canonical Android model where basically every screen in your app is a distinct activity. That structure isn't even the unanimous advice of Android upstream these days -- some of their voices say you should have one or a few activities and use Fragments for navigation instead. But in any case it's not our structure of things using RN, and we have just one activity where the app's whole UI (apart from the notifications) lives. Hence the need to play with things to find a solution.
  • If we can't make that work, it should certainly be possible to make a trampoline activity, analogous to our ShareToZulipActivity. That sure won't be in the spirit of what they're going for here, though. It may also cause visible jank in the UI, like an extra transition.

Originally posted by @gnprice in #5101 (comment)
@chrisbobbe
Copy link
Contributor Author

I used a handy "reference in new issue" button in the GitHub UI to make this issue, and tweaked the output a bit:

image

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 2, 2021
@chrisbobbe
android notif: Remove NotificationIntentService
2521e1d 
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
@chrisbobbe chrisbobbe mentioned this issue Dec 2, 2021
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 7, 2021
@chrisbobbe
android notif: Remove NotificationIntentService
36d5b75 
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
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 8, 2021
@chrisbobbe
android notif: Remove NotificationIntentService
6c31a6a 
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
@gnprice gnprice closed this as completed in bb2000f Dec 8, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 21, 2021
@chrisbobbe
android: Update target SDK version to 31, i.e., Android 12
c05e11e 
This change will be required in order to upload new releases to the
Play Store, effective probably 2022-11-01. That isn't very soon, but
we cut it kind of close last time, and we don't want to this time.

The change causes Android 12 and later to apply to our app a number
of behavior changes introduced in that version, documented here:
  https://developer.android.com/about/versions/12/behavior-changes-12

Most of the work for this was zulip#5145, "android notif: Replace our
`NotificationIntentService` with directly opening MainActivity".
Earlier in this series of commits, we also fixed zulip#5171 ("Handle
stricter length limit on Android toasts when targeting Android 12")
and handled the "Safer component exporting" change described at
  https://developer.android.com/about/versions/12/behavior-changes-12#exported .

There were a few other things that stood out in that "Behavior
Changes" article, but none that seem to require any specific action
from us; see
  zulip#5101 (comment) .

Fixes: zulip#5101
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 21, 2021
@chrisbobbe
android: Update target SDK version to 31, i.e., Android 12
4bb1a85 
This change will be required in order to upload new releases to the
Play Store, effective probably 2022-11-01. That isn't very soon, but
we cut it kind of close last time, and we don't want to this time.

The change causes Android 12 and later to apply to our app a number
of behavior changes introduced in that version, documented here:
  https://developer.android.com/about/versions/12/behavior-changes-12

Most of the work for this was zulip#5145, "android notif: Replace our
`NotificationIntentService` with directly opening MainActivity".
Earlier in this series of commits, we also fixed zulip#5171 ("Handle
stricter length limit on Android toasts when targeting Android 12")
and handled the "Safer component exporting" change described at
  https://developer.android.com/about/versions/12/behavior-changes-12#exported .

There were a few other things that stood out in that "Behavior
Changes" article, but none that seem to require any specific action
from us; see
  zulip#5101 (comment) .

Fixes: zulip#5101
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 22, 2021
@chrisbobbe @gnprice
android: Update target SDK version to 31, i.e., Android 12
8c2440c 
This change will be required in order to upload new releases to the
Play Store, effective probably 2022-11-01. That isn't very soon, but
we cut it kind of close last time, and we don't want to this time.

The change causes Android 12 and later to apply to our app a number
of behavior changes introduced in that version, documented here:
  https://developer.android.com/about/versions/12/behavior-changes-12

Most of the work for this was zulip#5145, "android notif: Replace our
`NotificationIntentService` with directly opening MainActivity".
Earlier in this series of commits, we also fixed zulip#5171 ("Handle
stricter length limit on Android toasts when targeting Android 12")
and handled the "Safer component exporting" change described at
  https://developer.android.com/about/versions/12/behavior-changes-12#exported .

There were a few other things that stood out in that "Behavior
Changes" article, but none that seem to require any specific action
from us; see
  zulip#5101 (comment) .

Fixes: zulip#5101
sumj25 pushed a commit to sumj25/zulip-mobile that referenced this issue Jan 12, 2022
@chrisbobbe @sumj25
android notif: Remove NotificationIntentService
6f1dd4e 
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
sumj25 pushed a commit to sumj25/zulip-mobile that referenced this issue Jan 12, 2022
@chrisbobbe @sumj25
android: Update target SDK version to 31, i.e., Android 12
744d8b1 
This change will be required in order to upload new releases to the
Play Store, effective probably 2022-11-01. That isn't very soon, but
we cut it kind of close last time, and we don't want to this time.

The change causes Android 12 and later to apply to our app a number
of behavior changes introduced in that version, documented here:
  https://developer.android.com/about/versions/12/behavior-changes-12

Most of the work for this was zulip#5145, "android notif: Replace our
`NotificationIntentService` with directly opening MainActivity".
Earlier in this series of commits, we also fixed zulip#5171 ("Handle
stricter length limit on Android toasts when targeting Android 12")
and handled the "Safer component exporting" change described at
  https://developer.android.com/about/versions/12/behavior-changes-12#exported .

There were a few other things that stood out in that "Behavior
Changes" article, but none that seem to require any specific action
from us; see
  zulip#5101 (comment) .

Fixes: zulip#5101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
a-Android a-notifications
Projects
None yet
Milestone
No milestone
1 participant
@chrisbobbe