[Android] Improve notification UI. #4842

AkashDhiman · 2021-07-01T14:38:59Z

The first 3 commits are updating some related code.

The next 4 commits are adding/removing some data structures to facilitate building up of this UI.

The last 2 commits are involved in changing the notification structure

first one makes it so that the app shows 1 notifications per conversation.
second one groups the notification.

Summary notification is not yet implemented correctly in this PR, and will show incorrect information (gives summary for all notifications on a summary notification for 1 realm) when API < 24 and user is logged in with multiple realms.

Some methods may no longer be needed, yet are not removed because they may prove useful in implementing other features.

This skips #4633 and fixes part of #2691, part of because summary notification is not yet implemented properly.

some screenshots of the same:

Summary notification:

Summary expanded

Individually Expanded

Message from Another realm

Future Work

Provide a proper summary notification.
Add action buttons to mark as read, mute topic and reply from notification.
Add custom sound.
Remove redundant code.

gnprice · 2021-07-02T07:00:27Z

#4842

Thanks @AkashDhiman! This looks like a lot of good changes. Looking forward to having this implemented.

Caveats: There is currently no good way to construct title for
groupPm type conversation. Ideally it should be the names of all the
people in the conversation, but we only have their UserId in the
message payload.

Yeah. This is an example of where to really do a great job, we need some of the data that the app has in Redux and that we don't currently have a great way to share for the notifications code to use. This is related to #4841, in that it's another motivation for wanting to change our model for storing data.

I wouldn't want to let that block all the other progress this PR is about, though. For this PR, maybe just have it say something like "Group PM"? (And maybe plural if there's more of them? Though getting that right in a translatable way is a bit complicated.) From your first screenshot, it looks like if we do that, then the name of the first sender will come right after that, so when there are several PM threads they'll still be possible to tell apart.

I like the switch to NotificationCompat! In general the "compat" libraries, aka AndroidX, are good to use whenever applicable, for the reason you say.

The fact that it simplifies getNotificationManager to where we can just inline it everywhere is a nice bonus. And I appreciate that you separate that inlining into its own commit 🙂

The new Conversation data class makes a lot of sense.

notificationId […] (usually set to the first messageId of the conversation.)

It looks like this becomes the notification ID we use with the Android SDK. What constraints does that ask for on how we pick and use the notification ID?

I think grouping by realmId won't quite work, though, for a couple of reasons:

For most servers that aren't Zulip Cloud, it'll all be the same value -- the ID you get for the first realm you set up on the server. (Probably 2? I think an internal realm uses realm ID 1.) So if you get notifications from several different self-hosted servers, they'll get grouped together.
If you have several accounts in the same org/realm, the notifications you get for those will be grouped together. This isn't a common use case outside of development -- but it often is very helpful for development, so it's worth spending some effort to make several accounts on the same org work similarly to several accounts in different orgs.

Those two reasons are what the Identity type is there to handle. Using realmUri instead of realmId takes care of the first one. Using userId takes care of the other.

Probably a step that would be helpful for using Identity here is to give it a method that returns a unique string key based on its contents. Mostly it would just take realmUri and userId.toString(), and concatenate them separated by some character that can't appear in a URL.

Relatedly, if we store the conversations in ConversationMap keyed only on things like the stream name, topic, and user IDs, then we have a bug where they can collide if those happen to match. For example, if a user is present on several self-hosted servers, the users they're talking to there are likely to have small user IDs that may well collide. And a lot of stream names like #general are very common, and people fairly often use short topics that are pretty generic and can easily collide.

That's a bug we have today, so it's not exactly a regression -- but it becomes more conspicuous with these improvements to the UI that make it easier to get more information from it 😛 . So that's why the sketch in ByConversationMap is there.

In the first commit:

notif [nfc]: Update deprecated code concerning notification token.

The deprecated code was not causing any noticable bugs, but android
studio was prompting that we should fix it, and its good to have up
to date code regardless.

One benefit of this specific commit is that it removes some code.

This sounds great! I like deleting code 😄 There's a couple of things you can do to make this change easier for me to review and merge:

Is there documentation somewhere you can link to about this API change? I'd hope that the Android Studio notice about it would link to something like that.
There's also a couple of changes in NotificationsModule.java related to FirebaseInstanceId.getInstance().getToken(), and at least one of them looks separate from the change to drop InstanceIDListenerService. Can that (or those) be a separate commit from the InstanceIDListenerService change? That way it can get a separate commit message with its own explanation.

AkashDhiman · 2021-08-04T23:02:30Z

@gnprice Ready for a re-review 🙂, I recommend merging #4938 as this PR has it as its base.
I have implemented this with a new logic where we don't have to rely on conversationMap to store past notifications so its quite different from the past revision.
There is a regression involving handling of RemoveFcmMessage which I am looking into. I have added TODOs wherever I think more work is needed.

AkashDhiman · 2021-08-05T15:46:11Z

pushed again changing 1 small thing in (5e8c31b: notif: Update Notification UI in android.),
call to notify on non summary notification had value of TAG set to groupKey, now changed to conversationKey.
Fixed a typo "naem" -> "name" in a comment.

gnprice

Thanks for the revisions! Inline comments below.

I really like the prospect of having the system effectively track the different messages we're showing notifications for, rather than having to do that ourselves. That is a nice simplification.

I'm guessing that's also the source of the challenge with maintaining the functionality of RemoveFcmMessage, i.e. to remove a notification from the UI when you go and read the relevant message from some other client. In the MessagingStyle doc:
https://developer.android.com/reference/androidx/core/app/NotificationCompat.MessagingStyle
I see there's an addMessage, but nothing with a name like removeMessage.

Here's a couple of ideas -- but I'm curious also what you've looked at so far, and maybe a thread on #mobile-team is the best place for detailed discussion:

There is getMessages, which returns a List. The List interface has List#remove, but it's optional -- some List implementations will throw on it. So possibly we could use that, though it'd be a bit shady because the API of MessagingStyle doesn't really guarantee it'll work (even if empirically it currently does.)
It seems like the sticky case here is when the user has read some but not all of the messages that are in a given UI notification (so, that are all in a given conversation.) Once they read all of the messages in the conversation, we can just clear that whole notification.
- I think it's actually a pretty fine UX if nothing in the notification UI changes until you read all of the relevant messages for a given conversation. So if we accept that, then that leaves the problem of how to know when that's happened.
- I think a pretty acceptable approximation would be: once you read the latest of the messages in a given conversation, we presume you've read all of them, and clear that conversation's UI-notification. That may help keep this implementation nice and simple.
- But I'd also be open to solutions that detect exactly when you've read all of them, instead of the one latest message, if you see a nice way to arrange that.

gnprice · 2021-08-09T21:16:23Z

android/app/src/main/AndroidManifest.xml

-                <action android:name="com.google.firebase.INSTANCE_ID_EVENT" />
-            </intent-filter>
-        </service>
+        <activity android:name="com.facebook.react.devsupport.DevSettingsActivity" />


This looks like a conflict-resolution error; this line was recently deleted (moved to another file), and I'm guessing you didn't intend to add it back.

gnprice · 2021-08-09T21:31:14Z

android/app/src/main/java/com/zulipmobile/notifications/NotificationsModule.java

@@ -36,7 +35,9 @@ static void emitToken(@Nullable ReactContext reactContext) {
     */
    @ReactMethod
    public void getToken(Promise promise) {
-        promise.resolve(FirebaseInstanceId.getInstance().getToken());
+        FirebaseInstanceId.getInstance().getInstanceId()


Thanks for those links to release notes -- those are very helpful!

Something related but not done in this commit is to update firebase-messaging itself, which as of now stands at 22.0.0 .

Yeah. Reading through the entries in that release-notes page for "Instance ID" and "Cloud Messaging", it looks like there are some further changes we'll need to adapt to as we do that upgrade.

gnprice · 2021-08-09T21:34:00Z

android/app/src/main/res/values/strings.xml

@@ -1,10 +1,12 @@
 <resources>
    <string name="app_name">Zulip</string>
-    <string name="notification_channel_name">Notifications</string>
+    <string name="notification_channel_name">Messages</string>
+    <string name="notification_channel_description">All the messages, received in Zulip.</string>


(This comment is a reminder to myself to wordsmith this message when I'm about to merge this change.)

gnprice · 2021-08-09T21:38:16Z

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

+    // activeNotifications are not available in NotificationCompatManager
+    // Hence we have to use instance of NotificationManager.
+    val notificationManager =
+        context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager?


Huh, odd that it isn't available in NotificationCompatManager. Do you know what the story is there?

I'd expect the compat library to cover all the functionality that's there in the latest Android SDK. So when a given method or property isn't there, my first thought is that that's a signal that there's some other preferred way to do the same thing.

Do you know what the story is there?

Not sure about this, I couldn't find any reference for why this is the case.

On a side note it is also possible to get active notification via NotificationListenerService#getActiveNotifications so they could have nicely consolidated this for most of the android versions. This is how one can get activeNotifications in API < 23.

gnprice · 2021-08-09T21:44:49Z

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

+    if (activeStatusBarNotifications != null) {
+        for (statusBarNotification in activeStatusBarNotifications) {
+            val notification = statusBarNotification.notification
+            if (notification.extras.getString("conversationKey") == conversationKey) {


Neat. Can we simplify this a bit more, by using a string "tag" instead of this extra?

That is, use StatusBarNotification#getTag:
https://developer.android.com/reference/android/service/notification/StatusBarNotification#getTag()
and when we're posting the Notification in the first place, use the form of notify that takes a string "tag":
https://developer.android.com/reference/android/app/NotificationManager#notify(java.lang.String,%20int,%20android.app.Notification)

doing it this way in the new revision thanks for that!

gnprice · 2021-08-10T03:29:22Z

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

-    // return Uri.parse("${ContentResolver.SCHEME_ANDROID_RESOURCE}://${context.packageName}/${R.raw.zulip}")
-    return Settings.System.DEFAULT_NOTIFICATION_URI
+        updateNotification(context, fcmMessage)
+    } // TODO handle case for RemoveFcmMessage


This is an important bit! 🙂 Definitely interested in seeing how we can handle this part too, with the new design.

This is the only thing that remains to be implemented properly, I have made a "hacky" version but it has few problems, see 1b9d2e7 for detail.

gnprice · 2021-08-10T03:33:33Z

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

        } else if (ACTION_CLEAR.equals(intent.getAction())) {
-            FCMPushNotifications.onClear(this, conversations);
+            // TODO implement dismiss


We probably actually don't want this action anymore. Its functionality was to clear the whole contents of the one UI notification we currently show (by clearing out all the different notification messages we have in the conversations map); but with the new form of notification, the system provides better UI for dismissing notifications.

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

gnprice · 2021-08-10T03:55:39Z

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

+            fcmMessage.sender.fullName
+        }
+    }
+    val groupKey = "${fcmMessage.identity?.realmUri.toString()};${fcmMessage.identity?.userId}"


Let's pull this line out as its own helper function, taking an Identity and returning a String to be used as the notification-group key.

The reason that's helpful is that it helps put a spotlight on the semantics this is supposed to have:

It should be constant for a given identity.

It should be distinct for distinct identities.

Then the one tweak I'd like to make to this definition, thanks to that framing, is: instead of ; let's use |.

I think using ; does in fact meet those two criteria for the behavior, because it can't appear in the user ID (which is just going to be a decimal integer, because we're stringifying it from an int right here.) But it can perhaps appear in the realm URL -- it can certainly appear in URLs, in particular because it's a "URL code point" in the URL spec's terms -- which makes it a bit more subtle than it could be to see that it works. Using | means it's easier to see, in more ways, that it behaves correctly, because | is not a URL code point and can't appear in the URL (nor in the user ID.)

gnprice · 2021-08-10T03:56:13Z

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

+        }
+    }
+    val groupKey = "${fcmMessage.identity?.realmUri.toString()};${fcmMessage.identity?.userId}"
+    val conversationKey = "$groupKey;$title"


Similarly let's use | here, just to keep it the same delimiter in both places.

And let's pull this out as its own helper function too, this one taking the FcmMessage. This helper's job is:

The result should be constant for a given conversation.

It should be distinct for distinct conversations.

That points at a few improvements we can make, relative to the solution of using the user-facing title as it's defined above:

For type Recipient.Pm, use sender.id instead of sender.fullName. That way (a) it doesn't get messed up if they change their name, (b) there's no possibility of the key getting confused because the delimiter character appeared in the data.

Before relying on sender.id, we should make it non-null, the same way as identity as discussed above; our notes say this also dates to server v1.8.0.

If we didn't reliably have sender.id, then sender.email would be a next-best solution.

For Recipient.GroupPm, recipient.pmUsers is perfect. Best to skip the getString, in fact, so this internal data structure's workings aren't complicated by interactions with a translated user-facing string.

For Recipient.Stream... ideally we'd use the stream ID instead of the stream name, but it looks like we don't currently have that in the data. For now, I guess we can settle for a TODO comment in the code saying to switch to stream ID. And for a delimiter, let's use the character \x00, for the same reason as we do that in keyFromNarrow in narrow.js:
// '\x00' is the one character not allowed in Zulip stream names. // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) topic: (streamName, topic) => `topic:s:${streamName}\x00${topic}`,

Oh also, looking at keyFromNarrow reminds me: for these conversation keys let's also have a different fixed prefix for each of the three types of recipient. That way, each of the three different ways we format a conversation key always has a distinct prefix, so it's easy to be sure that they'll never accidentally collide with each other.

done, using \u0000 instead of \x00, kotlin was not letting me use \x, I hope that's fine

Sure -- that should be another way (and I guess the only way, in Kotlin) of spelling the same thing.

gnprice

(One more comment on a bit I just noticed.)

gnprice · 2021-08-10T04:39:55Z

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

-    val soundUri = getNotificationSoundUri(context)
-    val audioAttr = AudioAttributes.Builder()
-        .setUsage(AudioAttributes.USAGE_NOTIFICATION).build()
-    builder.setSound(soundUri, audioAttr)


This is one chunk that I notice doesn't have a counterpart in the new code.

Looking at the API docs, I see that it has no effect on Android 8 O and up!
https://developer.android.com/reference/androidx/core/app/NotificationCompat.Builder#setSound(android.net.Uri)
So it wasn't doing us a lot of good. But: that's because we're supposed to be calling the corresponding method on the notification channel's builder instead:
https://developer.android.com/reference/androidx/core/app/NotificationChannelCompat.Builder#setSound(android.net.Uri,%20android.media.AudioAttributes)

So it seems like that's something we neglected to take care of when we started using a notification channel a few years ago. But seeing this code and deleting it gives us a nice reminder of how we should be doing it. Would you add a few lines to our createNotificationChannel to fix that?

The very cleanest thing would be to make sure we continue to do it on the notification itself, for the sake of pre-Android 8 devices, as well as on the channel. But at this point those devices are well into the single-digit percent (they were 9.0% almost a year ago), so graceful degradation is okay, and I'd be happy with a version that only does it on the channel.

I have added audioAttr use in channel as defined previously for notification builder.
NotificationCompat.Builder doesn't allow me to call setSound with audioAttr (there is no function with that signature in NotificationCompat.Builder) so i don't think i can easily add it there.

gnprice · 2021-08-10T04:56:04Z

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

+        // value, hence using "SupressLint".
+        val channel =
+            NotificationChannel(CHANNEL_ID, name, NotificationManagerCompat.IMPORTANCE_HIGH).apply {
+                description = context.getString(R.string.notification_channel_description)


Hmm -- can you post a screenshot of how this description appears in the UI? That would help me wordsmith the text for the description.

I'm not actually seeing anything that looks like this when I browse around the notification settings for other apps on my phone, which is making me suspect that my phone (Android 11 on a Pixel) just ignores the description. I also don't see anything that looks like it in the handy guide doc you linked:
https://developer.android.com/training/notify-user/channels

At the very end it is the grayed out message. This is accessible on tapping the notification channel name from the app setting.

Fascinating, thanks.

Do you see this on any other apps you happen to have lying around? I just looked at this part of the UI for several well-known apps -- including Facebook Messenger, which has an extensive set of notification categories, so they've clearly paid attention to this area of the API, and also including Android Messages, which ditto plus it's actually from the Android group at Google -- and I don't see a grayed-out message like that at all. There's the "Advanced" section, and then nothing after that. (Also, just to confirm: what Android version are you on?)

If other app makers, including the Google Android group itself, aren't using this feature, then we clearly don't need to feel obligated to do so, and I'm inclined to skip it. I think the category's name itself can convey all the same information -- perhaps we should rename it "All messages" to be clearer -- and the way the UI ends up presenting this description text feels pretty out-of-context and not likely to be helpful.

I looked again at the settings UI where this name appears, and I think "Messages" works well as the name and "All messages" would be unnecessarily redundant.

A couple of quick comparison points:

FB Messenger has "Chats and calls".

Android Messages has "Incoming messages".

gnprice · 2021-08-10T05:04:12Z

OK, I've merged 6/13 commits from the initial part of this PR!

c2087fa android notif [nfc]: Remove InstanceIDListenerService.java.
78a2b46 android notif [nfc]: Update deprecated code in getToken.
5763242 android notif [nfc]: Use compat version for notification modules.
507475d android notif [nfc]: Remove getNotificationManager.
6320ae3 android notif [nfc]: Remove ByConversationMap.
f666c41 android notif: Better name channel; enable lights and vibration.

I fixed up the conflict-resolution error mentioned at the top of my review today. I also dropped the channel description for now because I'm not sure how that will show up; see comment just above this.

The only other change is just that I tweaked the summary lines to say "android notif". That slightly more specific context is helpful for interpreting summaries like "Use compat version for notification modules." -- aha, it's Android, so that's the Android compat library aka AndroidX -- and it's also helpful just for imagining what kind of code this is touching, because the Android-native code is fairly distinct in some ways from the JS code.

Looking forward to the next revision, and to getting to merge the main changes here soon 😄

specifically: - sender.id - identity - realmUri Context at: - zulip#4842 (comment)

gnprice · 2021-08-19T20:11:28Z

Thanks for the revision!

Two high-level comments on the "remove" case:

    // TODO figure out a way to cancel individual group notifications
    // misbehaves with multiple realm, shows empty notification.
    // This cancels all group Notification when no other notification
    // is present.
    if (Build.VERSION.SDK_INT >= 24) {
        if (getActiveNotification(context)?.all { statusBarNotification -> statusBarNotification.isGroup } == true) {
            NotificationManagerCompat.from(context).cancelAll()
        }
    }

So this is scanning through all the existing notifications and checking whether it's the case that they're all… well, if this works then it's checking that they're all group summaries, I guess. (The docs actually say that isGroup is true "if this notification is part of a group", but it doesn't make totally clear what that's supposed to mean.) If they are, then we must have removed the last of the underlying individual notifications, and we can clear out the groups.

To get the desired behavior, it seems like we want to do something pretty similar, except instead of checking whether there are any individual (i.e., non-group-summary) notifications at all, we want to check whether there are any of those for each group key. So for example we could do a scan through these and build up a Set of group keys to keep, and then another scan to find group-summary notifications that are for empty groups and should be cancelled.

    for (fcmMessageId in fcmMessage.messageIds) {
        val activeNotifications = getActiveNotification(context) ?: return
        for (statusBarNotification in activeNotifications) {
            val notification = statusBarNotification.notification
            val messageId = notification.extras.getInt("zulipMessageId")
            if (fcmMessageId == messageId) {

We can make this linear instead of quadratic by turning messageIds into a Set, and then making just one linear scan where we check membership in that. … Oh hey, actually we already made a Set at parse time 😄 -- fcmMessage.messageIds is a Set<Int>. So we can write it like this:

        val activeNotifications = getActiveNotification(context) ?: return
        for (statusBarNotification in activeNotifications) {
            val notification = statusBarNotification.notification
            val messageId = notification.extras.getInt("zulipMessageId")
            if (fcmMessage.messageIds.contains(messageId)) {

specifically: - sender.id - identity - identity.realmUri Context at: - zulip#4842 (comment)

AkashDhiman · 2021-09-09T14:43:06Z

@gnprice did the required changes, I think the notifications are getting dismissed as expected now, you can review it again.
I have also added the string I thought was most suited for group pm in this revision, please see if that's appropriate.

gnprice · 2021-09-30T06:31:57Z

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

+        // statusBarNotification.groupKey also contains package name
+        // in the beginning and hence doesn't match the groupKey we
+        // set in notification builder; therefore we use endsWith.
+        if (statusBarNotification.groupKey.endsWith(groupKey)) {


Ah interesting.

Can we compute the exact groupKey we expect to see here? Perhaps something like "com.zulipmobile" + extractGroupKey(fcmMessage.identity)? (Except that hardcodes the package name, and we should instead use whatever the appropriate API is to refer to it.) (Also, whatever computation is needed should go in its own tiny helper function, probably right next to extractGroupKey itself, so it can get a name and javadoc to help explain what it means.)

Then we could use == instead of endsWith, which would be cleaner because it's easier to confidently reason about. With endsWith, we have to worry about whether it's possible for one of our groupKey values to just happen to end with another one, so that there'd be an accidental match here. It'd be possible to design the format of the group keys to rule that out, if we have to… but it's cleaner, if we can, to just not have that be a possible problem in the first place.

using statusBarNotification.notification.group == groupKey, the groupKey in statusBarNotification contains "ranking" which may also have a number concatenated at the beginning which I am not sure how to compute.

Huh interesting. Well, glad that solution works.

gnprice · 2021-09-30T06:33:34Z

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

+private fun getActiveNotification(context: Context): Array<StatusBarNotification>? =
+    (context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager?)?.activeNotifications


Let's give this a plural name, getActiveNotifications. That matches the underlying activeNotifications property, as well as the fact that the return value is an array with potentially multiple notifications.

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

gnprice · 2021-09-30T06:55:53Z

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

+    val statusBarNotifications = getActiveNotification(context) ?: return
+    val groupKey = extractGroupKey(fcmMessage.identity)
+    for (statusBarNotification in statusBarNotifications) {
+        val notification = statusBarNotification.notification
+        val messageId = notification.extras.getInt("zulipMessageId")
+        if (fcmMessage.messageIds.contains(messageId)) {
+            NotificationManagerCompat.from(context).cancel(statusBarNotification.tag, statusBarNotification.id)


I believe the key context for what this logic is doing is:

Each StatusBarNotification corresponds to one Zulip conversation.

The zulipMessageId extra corresponds to the last Zulip-message ID in that conversation.

Is that right? If so, I think a couple of one-line comments would help orient the reader to what's meant to be going on here. For example… actually, better. Let's name the extra to help make that clear, like lastZulipMessageId. Then something like:

Suggested change

val statusBarNotifications = getActiveNotification(context) ?: return

val groupKey = extractGroupKey(fcmMessage.identity)

for (statusBarNotification in statusBarNotifications) {

val notification = statusBarNotification.notification

val messageId = notification.extras.getInt("zulipMessageId")

if (fcmMessage.messageIds.contains(messageId)) {

NotificationManagerCompat.from(context).cancel(statusBarNotification.tag, statusBarNotification.id)

val statusBarNotifications = getActiveNotification(context) ?: return

val groupKey = extractGroupKey(fcmMessage.identity)

// Find any conversations we can cancel the notification for.

// The API doesn't lend itself to removing individual messages as

// they're read, so we wait until we're ready to remove the whole

// conversation's notification.

for (statusBarNotification in statusBarNotifications) {

// Each statusBarNotification represents one Zulip conversation.

val notification = statusBarNotification.notification

val lastMessageId = notification.extras.getInt("lastZulipMessageId")

if (fcmMessage.messageIds.contains(lastMessageId)) {

// The latest Zulip message in this conversation was read.

// That's our cue to cancel the notification for the conversation.

NotificationManagerCompat.from(context).cancel(statusBarNotification.tag, statusBarNotification.id)

That's a one-line comment to mention the first point, and some more comments for the "why" of what we're doing with the conversation's last Zulip-message ID.

Oh also, in that first comment a link to #4842 (review) would be helpful, to give a pointer back into the history of our reasoning.

android/app/.project

gnprice · 2021-09-30T07:16:32Z

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

+
+    val builder = NotificationCompat.Builder(context, CHANNEL_ID)
+    builder.apply {
+        color = Color.rgb(100, 146, 254)


Hmm, yeah, good idea! That'd be a great change to make either before or after the commit where you add a new place we use the color.

gnprice

Thanks @AkashDhiman for the revision! This is looking great. Comments below, and above because GitHub does that when I reply on a previous thread, and a few other comments further above in replies on other threads:
#4842 (comment)
#4842 (comment)
#4842 (comment)
#4842 (comment)

I merged #4967 a little while ago, and I think that should mean that when rebasing you'll get to drop this commit:
28dadc9 notif: Make some MessageFcmMessage properties Non Nullable.
because that took care of ensuring we always have those properties.

gnprice · 2021-09-30T07:43:00Z

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

+val audioAttr: AudioAttributes = AudioAttributes.Builder()
+    .setUsage(AudioAttributes.USAGE_NOTIFICATION).build()


This initializer looks like it'll involve executing a bit of code; which I think means that'll happen when this JVM class is loaded, so basically at app startup. Generally it's best to avoid executing code at the top level like this, because it adds up across a codebase to make programs slow to start up. (Also because in some situations it can suffer races, though I don't think that's an issue here.)

Instead, this can be a little helper function instead of a constant. Or, if we're using it in just one place, it could go inside that function.

gnprice · 2021-09-30T07:47:06Z

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

+    val messagingStyle = when (notification) {
+        null -> NotificationCompat.MessagingStyle(selfUser)
+        else -> NotificationCompat.MessagingStyle
+            .extractMessagingStyleFromNotification(notification)
+            ?: NotificationCompat.MessagingStyle(selfUser)
+    }


A fun Kotlin tip to simplify this sort of logic: ?.let. Here:

val messagingStyle = notification?.let { NotificationCompat.MessagingStyle.extractMessagingStyleFromNotification(it) } ?: NotificationCompat.MessagingStyle(selfUser)

That lets you deduplicate the NotificationCompat.MessagingStyle(selfUser) fallback case.

gnprice · 2021-09-30T07:57:21Z

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

+        // here notifications are identified by TAG + notificationId
+        // simply using notificationId is not sufficient and will
+        // fail in case of different account in same realm.
+        notify(groupKey, fcmMessage.identity.realmId, summaryNotification.build())
+        notify(conversationKey, notificationId, builder.build())


Hmm, I'm thinking about this comment. It's true, we may have several notifications here with the same "id", the same second argument to notify, because it's the Zulip message ID. And we've designed the "tag", the first argument, so that it on the other hand is unique.

But we don't actually need both the tag and the ID in order to uniquely identify a notification, do we? I think just the tag alone does it.

Is there a thing this comment is telling the reader to do? It reads like it's warning about a pitfall, where some other code we write needs to be careful not to do something wrong. But after studying it, I think the point is just as an explanation of why we use this three-argument form of notify, with the tag arguments.

Looking again at these notification "id"s, the second argument to each of these notify calls:

Do we ever use the fact that the summaries have the realm ID as their notification ID? I think we don't; and I think if we did, that would probably be a bug, because realm IDs are per-server and in fact are highly likely to collide between any two servers other than Zulip Cloud (because they'll probably both be the only realm on the server.) If we don't, how about just using a constant ID, like 1? That would help make it clear that we're relying entirely on the tags.

Similarly, do we ever use the fact that the notification IDs for individual conversations vary, or that they're Zulip message IDs? I think we don't, in which case it'd similarly be good to just use a constant. It can even be the same constant -- we've designed these keys so they don't collide between the two types, either.

using NOTIFICATION_ID, like the main branch here.

AkashDhiman · 2021-11-04T16:47:26Z

Sorry for the delay on this @gnprice, I have addressed the last review.

2 small things that remain from last time's review:

write the brandColor in colors.xml [Android] Improve notification UI. #4842 (comment)
rename the channel from "Messages" to "All Messages" [Android] Improve notification UI. #4842 (comment)

Between this revision and the previous one, The layout seems to have changed a bit, but I am not sure why, hence before merging I recommend we figure out the reason and try to move back to that layout, It has larger text for heading and a default group conversation icon.
Previous Layout:

New Layout (Expanded Group Conversation)

New Layout (Closed Group Conversation)

This commit allows the notification to automatically dismiss themselves on receiving a message of type `RemoveFcmMessage`. Caveat: * Whole of the conversation (PM, group PM, or stream > topic) is removed from the notif, in case it contains a message with message id that is provided by the RemoveFcmMessage payload. A better behavior is to remove only the specific messages provided in the payload.

gnprice · 2021-11-08T23:45:26Z

Thanks! Glad that was a straightforward fix. (And here in chat is where you debugged it, for cross-reference.)

This looks good; I'll merge.

The remaining open items were:

We'd discussed changing the name of the notification channel, from "Messages" to "All messages": [Android] Improve notification UI. #4842 (comment) I looked back at that just now, and I think it's better as it is.
This change gave us a prompt to drop Android 6 support, bumping the minimum to Android 7: [Android] Improve notification UI. #4842 (comment) We should probably do that, but it sounds like the symptom on Android 6 isn't terrible, so we don't need to do so before merge.
There's a couple of followup items here: [Android] Improve notification UI. #4842 (comment)

Thanks again @AkashDhiman for all your work on this!

gnprice · 2021-11-09T01:49:07Z

OK, and those followups all filed (see backlinks above.) I think this thread is complete!

There's hardly anything left in this file, after zulip#4842 made it unnecessary to maintain our own data structure of conversations because we can use the system's data structures of active notifications. Fold what little is left into the main file that uses it (indeed the only file that uses its one nontrivial item.) Also add some javadoc while we're at it.

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

There's hardly anything left in this file, after zulip#4842 made it unnecessary to maintain our own data structure of conversations because we can use the system's data structures of active notifications. Fold what little is left into the main file that uses it (indeed the only file that uses its one nontrivial item.) Also add some javadoc while we're at it.

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

There's hardly anything left in this file, after zulip#4842 made it unnecessary to maintain our own data structure of conversations because we can use the system's data structures of active notifications. Fold what little is left into the main file that uses it (indeed the only file that uses its one nontrivial item.) Also add some javadoc while we're at it.

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

There's hardly anything left in this file, after zulip#4842 made it unnecessary to maintain our own data structure of conversations because we can use the system's data structures of active notifications. Fold what little is left into the main file that uses it (indeed the only file that uses its one nontrivial item.) Also add some javadoc while we're at it.

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

There's hardly anything left in this file, after zulip#4842 made it unnecessary to maintain our own data structure of conversations because we can use the system's data structures of active notifications. Fold what little is left into the main file that uses it (indeed the only file that uses its one nontrivial item.) Also add some javadoc while we're at it.

AkashDhiman added a-notifications a-Android labels Jul 1, 2021

AkashDhiman requested a review from gnprice July 1, 2021 16:59

AkashDhiman mentioned this pull request Jul 29, 2021

React to notification #4862

Open

AkashDhiman force-pushed the notif branch 3 times, most recently from 3250c0c to 2825477 Compare August 4, 2021 22:57

AkashDhiman force-pushed the notif branch 2 times, most recently from 9e5edc4 to 6575678 Compare August 5, 2021 15:43

gnprice reviewed Aug 10, 2021

View reviewed changes

gnprice mentioned this pull request Aug 10, 2021

[wip] Reply from notification in Android #4945

Draft

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this pull request Aug 19, 2021

notif: Make some MessageFcmMessage properties Non Nullable.

c8e5989

specifically: - sender.id - identity - realmUri Context at: - zulip#4842 (comment)

AkashDhiman force-pushed the notif branch 2 times, most recently from 9de656f to 1b9d2e7 Compare August 19, 2021 15:30

gnprice mentioned this pull request Aug 25, 2021

notif: Require sender_id, server, and realm_uri, rejecting ancient servers' data #4967

Merged

AkashDhiman added a commit to AkashDhiman/zulip-mobile that referenced this pull request Sep 9, 2021

notif: Make some MessageFcmMessage properties Non Nullable.

28dadc9

specifically: - sender.id - identity - identity.realmUri Context at: - zulip#4842 (comment)

AkashDhiman force-pushed the notif branch from 1b9d2e7 to 081669a Compare September 9, 2021 14:32

gnprice reviewed Sep 30, 2021

View reviewed changes

gnprice mentioned this pull request Nov 2, 2021

Clear oldest notifications when you have a giant number of them #5096

Open

AkashDhiman force-pushed the notif branch from 081669a to 0c0dcf0 Compare November 4, 2021 16:21

AkashDhiman added 2 commits November 8, 2021 15:37

gitignore [nfc]: Add ignore for android/app/.project.

c716b6c

gnprice force-pushed the notif branch from f93fbf8 to c716b6c Compare November 8, 2021 23:45

gnprice merged commit c716b6c into zulip:main Nov 8, 2021

This was referenced Nov 8, 2021

Separate Android notification data by account #4633

Closed

Drop Android 6 Marshmallow support #5114

Merged

Use org name rather than URL for heading of Android notif group #5115

Open

Better title group PMs in Android notifications #5116

Open

AkashDhiman deleted the notif branch November 9, 2021 13:47

This was referenced Nov 10, 2021

Notifications can leave a ghostly heading-notification behind #5119

Closed

Reading one conversation can cause another one's notification to disappear #5120

Closed

gnprice mentioned this pull request Nov 10, 2021

Revise Android notifs code after recent overhaul #5121

Merged

gnprice added the P1 high-priority label Nov 10, 2021

gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Nov 11, 2021

android notif [nfc]: Explain the delimiters in our notification tags

895515a

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Nov 16, 2021

android notif [nfc]: Explain the delimiters in our notification tags

5cfacfb

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Nov 16, 2021

android notif [nfc]: Explain the delimiters in our notification tags

820a036

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

chetas411 pushed a commit to chetas411/zulip-mobile that referenced this pull request Nov 28, 2021

android notif [nfc]: Explain the delimiters in our notification tags

f455b45

Original discussion here: zulip#4842 (comment) zulip#4842 (comment)

gnprice mentioned this pull request Jun 17, 2022

[WIP] Show grouped notifications using messaging style #3241

Closed

gnprice mentioned this pull request Aug 10, 2022

notifications: Remove Android summary notification. #5459

Closed

		private fun getActiveNotification(context: Context): Array<StatusBarNotification>? =
		(context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager?)?.activeNotifications

		val audioAttr: AudioAttributes = AudioAttributes.Builder()
		.setUsage(AudioAttributes.USAGE_NOTIFICATION).build()

[Android] Improve notification UI. #4842

[Android] Improve notification UI. #4842

Conversation

AkashDhiman commented Jul 1, 2021

Summary notification:

Summary expanded

Individually Expanded

Message from Another realm

Future Work

gnprice commented Jul 2, 2021

AkashDhiman commented Aug 4, 2021

AkashDhiman commented Aug 5, 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

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

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

gnprice commented Aug 10, 2021

gnprice commented Aug 19, 2021 • edited Loading

AkashDhiman commented Sep 9, 2021

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

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

AkashDhiman commented Nov 4, 2021 • edited Loading

gnprice commented Nov 8, 2021

gnprice commented Nov 9, 2021

gnprice commented Aug 19, 2021 •

edited

Loading

AkashDhiman commented Nov 4, 2021 •

edited

Loading