notif: Support messaging-style notifications #718

rajveermalviya · 2024-06-03T15:58:24Z

after.mp4

Fixes: #128

rajveermalviya · 2024-06-25T18:38:24Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+
+fun toPigeonPerson(person: androidx.core.app.Person): Person {
+    return Person(
+        null, // TODO: Serialize icon maybe using content URI, mData is not exposed.


Currently, we don't return Person's IconData back when returning active notification (via getActiveNotificationMessagingStyleByTag) because we don't seem to have an API to get data bytes back.

Fortunately though it doesn't seem to be necessary because in my local testing, the avatar is correctly visibile even when same sender has multiple messages, which makes sense because here's what ends up happening:

MessagingStyle( user: Person('You') messages: [ MessagingStyleMessage(content: '...', person: Person(key: id1, name: 'John', iconData: null)), // Old, got from active notif MessagingStyleMessage(content: '...', person: Person(key: id1, name: 'John', iconData: null)), // Old, got from active notif MessagingStyleMessage(content: '...', person: Person(key: id1, name: 'John', iconData: [...])), // Latest message, newly added ], ... )

So, Android seems to be okay with older messages not having iconData for a specific Person.

Interesting.

If (as it sounds like it is) this is something it turns out we don't actually need, and we're OK with not having, then instead of a TODO comment this should have a comment explaining why we're OK with it.

I think in fact I'm much happier if we can indeed avoid serializing these icons back. They seem likely potentially a significant amount of data — much more than the message text, timestamp, sender name, etc., that makes up the whole rest of the notification info — so they have the potential to be a performance problem. Especially so as there's quadratic growth here (with each call's data growing linearly as there are more messages in the history).

In your example above, it's the same sender each time. What happens if some of the old senders are different from the one that's latest and has the icon data?

In your example above, it's the same sender each time. What happens if some of the old senders are different from the one that's latest and has the icon data?

It works for multiple senders too, (also visible in the PR description video):

Khader-1

Thank you, @rajveermalviya! The code looks good to me. The manual test as well as the attached video demonstrate that the behavior described in the issue has been correctly addressed.

chrisbobbe

Thanks! Small comments below, and marking for Greg's review. He can take a closer look than me because he's more familiar with Android notifications logic.

chrisbobbe · 2024-07-03T08:13:06Z

lib/notifications/display.dart

+    final MessagingStyle messagingStyle = oldMessagingStyle != null
+      ? MessagingStyle(
+          user: oldMessagingStyle.user,
+          messages: oldMessagingStyle.messages?.toList() ?? [], // Clone a fixed-length list
+          isGroupConversation: oldMessagingStyle.isGroupConversation)


Is there a reason to create a new MessagingStyle based on oldMessagingStyle (but with conversationTitle omitted)? What if we just used oldMessagingStyle?

I did get a test failure when I tried this, but it's pretty opaque to me:

00:18 +770 ~6 -1: /Users/chrisbobbe/dev/zulip-flutter/test/notifications/display_test.dart: NotificationDisplayManager show multiple stream messages [E] Expected: a List<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})> that: has length that: equals <6> contains, in order: [Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void, Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void, Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void, Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void, Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void, Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void] Actual: [(autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: Instance of 'PendingIntent', contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1084, id: 1266374773, inboxStyle: null, isGroupSummary: null, messagingStyle: Instance of 'MessagingStyle', number: 1, smallIconResourceName: zulip_notification, tag: https://chat.example/|1084|stream:1:example topic), (autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: null, contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1084, id: 712478168, inboxStyle: Instance of 'InboxStyle', isGroupSummary: true, messagingStyle: null, number: null, smallIconResourceName: zulip_notification, tag: https://chat.example/|1084), (autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: Instance of 'PendingIntent', contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1084, id: 1266374773, inboxStyle: null, isGroupSummary: null, messagingStyle: Instance of 'MessagingStyle', number: 2, smallIconResourceName: zulip_notification, tag: https://chat.example/|1084|stream:1:example topic), (autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: null, contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1084, id: 712478168, inboxStyle: Instance of 'InboxStyle', isGroupSummary: true, messagingStyle: null, number: null, smallIconResourceName: zulip_notification, tag: https://chat.example/|1084), (autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: Instance of 'PendingIntent', contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1084, id: 1266374773, inboxStyle: null, isGroupSummary: null, messagingStyle: Instance of 'MessagingStyle', number: 3, smallIconResourceName: zulip_notification, tag: https://chat.example/|1084|stream:1:example topic), (autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: null, contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1084, id: 712478168, inboxStyle: Instance of 'InboxStyle', isGroupSummary: true, messagingStyle: null, number: null, smallIconResourceName: zulip_notification, tag: https://chat.example/|1084)] Which: did not have an element matching the expectation at index 0 <Closure: (Subject<({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag})>) => void> package:checks/src/checks.dart 85:9 check.<fn> package:checks/src/checks.dart 708:12 _TestContext.expect package:checks/src/extensions/iterable.dart 93:13 IterableChecks.containsInOrder test/notifications/display_test.dart 188:11 main.<fn>.checkNotification test/notifications/display_test.dart 205:7 main.<fn>.checkNotifications test/notifications/display_test.dart 241:13 main.<fn>.<fn>.<fn> To run this test again: /Users/chrisbobbe/.local/lib/flutter/bin/cache/dart-sdk/bin/dart test /Users/chrisbobbe/dev/zulip-flutter/test/notifications/display_test.dart -p vm --plain-name 'NotificationDisplayManager show multiple stream messages' 00:43 +1192 ~168 -1: Some tests failed.

chrisbobbe · 2024-07-03T08:18:03Z

lib/notifications/display.dart

+            FcmMessageStreamRecipient() => true,
+            FcmMessageDmRecipient(:var allRecipientIds) when allRecipientIds.length > 2 => true,
+            FcmMessageDmRecipient() => false,
+        });


Suggested change

});

});

chrisbobbe · 2024-07-03T08:24:01Z

test/notifications/display_test.dart

+      final message2 = eg.streamMessage(id: 102, stream: stream);
+      final messageData2 = messageFcmMessage(message2, streamName: stream.name);
+      final message3 = eg.streamMessage(id: 102, stream: stream);
+      final messageData3 = messageFcmMessage(message3, streamName: stream.name);


Are message2 and message3 intended to have the same ID?

chrisbobbe · 2024-07-03T08:30:53Z

lib/notifications/display.dart

+  static Future<Uint8List?> _fetchBitmap(Uri url) async {
+    try {
+      final resp = await http.get(url);
+      return resp.bodyBytes;
+    } catch (e) {
+      // TODO(log)
+      return null;
+    }
+  }


I wonder how long this request could sit unresolved, and if we want to time out after some short-ish amount of time. Users will want to see their notifications as soon as we can reasonably show them, even if they're missing an avatar image, I think.

No need to block, this could be a TODO; we haven't implemented this in zulip-mobile.

chrisbobbe · 2024-07-03T08:33:40Z

lib/notifications/display.dart

+      text: data.content,
+      timestampMs: data.time * 1000,
+      person: Person(
+        key: data.senderId.toString(),


Should we add the org URL to the key? There could be users on different realms with the same ID.

gnprice

Thanks @rajveermalviya, and thanks @Khader-1 and @chrisbobbe for the previous reviews! Excited to see this feature.

Comments below; I've read through all but the tests of the main commit. There are also Chris's comments above.

(And in a future round of review I'll also want to do some manual testing myself, to supplement what you and @Khader-1 have done already. I didn't yet in this review.)

gnprice · 2024-07-03T19:56:00Z

pigeon/notifications.dart

+    required this.iconData,
+    required this.name,
+    required this.key,


nit: order the same way as the fields are declared

gnprice · 2024-07-03T20:02:38Z

pigeon/notifications.dart

    String? smallIconResourceName,
    // NotificationCompat.Builder has lots more methods; add as needed.
    // Keep them alphabetized, for easy comparison with that class's docs.
  });
+
+  MessagingStyle? getActiveNotificationMessagingStyleByTag(String tag);


This should get a doc comment saying what it corresponds to in the Android SDK or AndroidX (or more generally the API that's being wrapped). Compare the notify method above.

gnprice · 2024-07-03T20:10:53Z

pigeon/notifications.dart

+  // TODO: Pigeon doesn't support DateTime yet:
+  //  https://github.com/flutter/flutter/issues/115979
+  final int timestampMs;


It looks like the underlying API wants an integer milliseconds-since-epoch timestamp anyway. So that's the same thing I'd want this Pigeon-based wrapper API to take, even if Pigeon did support DateTime.

(If we passed a DateTime, then our own code would be converting from that on the Kotlin side, after our own code constructed a DateTime on the Dart side from the timestamp the server gave it. Better to skip all those conversions, and worrying about if one of them accidentally involves the time zone etc.)

gnprice · 2024-07-03T20:11:50Z

pigeon/notifications.dart

+
+  final Person user;
+  final String? conversationTitle;
+  final List<MessagingStyleMessage?>? messages;


Suggested change

final List<MessagingStyleMessage?>? messages;

final List<MessagingStyleMessage>? messages;

Or is there a meaning encoded by having null as an element of the list?

Currently pigeon doesn't support non-null type arguments, and throws this error:

Error: pigeon/notifications.dart:98: Generic type parameters must be nullable in field "messages" in class "MessagingStyle".

flutter/flutter#97848

Added a TODO(pigeon) comment explaining it.

Ah great, thanks. And in fact following that link, I see I'd already upvoted that Pigeon issue 🙂

The nullability here doesn't cause much of a problem directly, as long as the reader knows about it — it's mostly just confusing, when the reader isn't aware of that Pigeon issue and so expects that the nullability would be a choice by the author of this API and therefore should mean something. So a comment giving that context is perfect.

gnprice · 2024-07-03T20:16:45Z

pigeon/notifications.dart

+    required this.key,
+  });
+
+  final Uint8List? iconData;


I'm not sure what data this is meant to be exactly — it's evidently not Flutter's IconData.

So that should be explained with a bit of dartdoc on the field. From the implementation on the Kotlin side, it looks like it's the bytes of a bitmap. But a bitmap in what format (or formats)? A link to the relevant Android docs would be good.

(Most of these fields don't have docs because they correspond directly to fields in the underlying Android class, which the class's dartdoc points to. But this one has another wrinkle between it and the setIcon method, so there's more explanation needed.)

gnprice · 2024-07-03T21:01:22Z

lib/notifications/display.dart

+      ? MessagingStyle(
+          user: oldMessagingStyle.user,
+          messages: oldMessagingStyle.messages?.toList() ?? [], // Clone a fixed-length list


Instead of copying oldMessagingStyle, we can mutate it.

(I guess this overlaps with Chris's question #718 (comment) .)

gnprice · 2024-07-03T21:03:19Z

lib/notifications/display.dart

+          user: Person(
+            key: data.userId.toString(),


Hmm, I wonder what the scope of these Person keys is. Do they apply just within a conversation, or across all the app's notifications?

Unless we find clear confirmation of the former, let's conservatively assume the latter. That means we should include the realm URL in this key, as well as the user ID.

… Ha and I see Chris already asked the same question 😄: #718 (comment)

I wasn't able to find much info if a Person is scoped to a certain groupKey. So for now, updated it to include realm URL in the person key, just in case.

gnprice · 2024-07-03T21:08:36Z

lib/notifications/display.dart

+            FcmMessageDmRecipient() => false,
+        });
+
+    messagingStyle.conversationTitle = switch (data.recipient) {


There's a comment above this in the corresponding zulip-mobile code, about the title changing between messages in a conversation, which I think is still relevant.

gnprice · 2024-07-03T21:11:09Z

lib/notifications/display.dart

-    final groupKey = _groupKey(data);
-    final conversationKey = _conversationKey(data, groupKey);
+
+    messagingStyle.messages?.add(MessagingStyleMessage(


Suggested change

messagingStyle.messages?.add(MessagingStyleMessage(

messagingStyle.messages!.add(MessagingStyleMessage(

If messages here were somehow null, it'd be wrong for us to skip adding this new message.

So if it actually could be null, this should be like (messagingStyle.messages ??= []).add(…. But I think in fact it can't.

That also might be a sign that the wrapper API should just take a list here, rather than a nullable list. There's no distinction being expressed by null vs. an empty list, right?

gnprice · 2024-07-03T21:18:10Z

lib/notifications/display.dart

+      messagingStyle: messagingStyle,
+      number: messagingStyle.messages?.length,


nit: put these after the color and icon, like in the corresponding zulip-mobile code

The icon and its color are the first thing the user sees, before unfurling the notification. So they logically come before the contents.

(That would have been clever in the old code too, oh well.)

Also there's a TODO comment down there which this commit can delete 🙂

rajveermalviya · 2024-07-07T19:58:20Z

Thanks for the reviews @chrisbobbe and @gnprice, pushed a new revision PTAL.

gnprice

Thanks for the revision! Comments below. For this round I focused only on the areas I reviewed last time.

gnprice · 2024-07-08T04:27:18Z

pigeon/notifications.dart

+  /// The `tag` is used to find a notification that matches the
+  /// same tag from the active notifications list.
+  ///
+  /// Returns null if active notifications list is empty or none
+  /// of the notification matches the `tag`, else returns messaging
+  /// style information of the matching active notification.


Suggested change

/// The `tag` is used to find a notification that matches the

/// same tag from the active notifications list.

///

/// Returns null if active notifications list is empty or none

/// of the notification matches the `tag`, else returns messaging

/// style information of the matching active notification.

/// Returns the messaging style, if any, of an active notification

/// that has tag `tag`. If there are several such notifications,

/// an arbitrary one of them is used.

/// Returns null if there are no such notifications.

In particular "list is empty" can be subsumed into "none of them match" — if the list is empty, then it has no element that matches (because it has no element at all).

gnprice · 2024-07-08T04:29:20Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

+                val style = NotificationCompat.MessagingStyle(toAndroidPerson(messagingStyle.user))
+                    .setConversationTitle(messagingStyle.conversationTitle)
+                    .setGroupConversation(messagingStyle.isGroupConversation)
+                messagingStyle.messages?.forEach { it?.let {


Suggested change

messagingStyle.messages?.forEach { it?.let {

messagingStyle.messages.forEach { it?.let {

Else Android Studio gives me a warning about this line.

(When we set up CI for the Android build, we should include the Kotlin linter too, to be sure to catch this sort of thing.)

gnprice · 2024-07-08T04:38:49Z

pigeon/notifications.dart

+  /// Wraps `androidx.core.graphics.drawable.IconCompat.createWithData`
+  /// which generates an `IconCompat` from compressed image data.
+  ///
+  /// It uses `android.graphics.BitmapFactory` to decode the image data,
+  /// and the `android.graphics.Bitmap.CompressFormat` enum lists the
+  /// supported image compression formats (JPEG, PNG, WEBP).
+  ///
+  /// See:
+  ///  https://developer.android.com/reference/androidx/core/graphics/drawable/IconCompat#createWithData(byte[],int,int)
+  ///  https://developer.android.com/reference/android/graphics/BitmapFactory.html
+  ///  https://developer.android.com/reference/android/graphics/Bitmap.CompressFormat.html


Suggested change

/// Wraps `androidx.core.graphics.drawable.IconCompat.createWithData`

/// which generates an `IconCompat` from compressed image data.

///

/// It uses `android.graphics.BitmapFactory` to decode the image data,

/// and the `android.graphics.Bitmap.CompressFormat` enum lists the

/// supported image compression formats (JPEG, PNG, WEBP).

///

/// See:

/// https://developer.android.com/reference/androidx/core/graphics/drawable/IconCompat#createWithData(byte[],int,int)

/// https://developer.android.com/reference/android/graphics/BitmapFactory.html

/// https://developer.android.com/reference/android/graphics/Bitmap.CompressFormat.html

/// This should be compressed image data, in a format to be passed

/// to `androidx.core.graphics.drawable.IconCompat.createWithData`.

/// Supported formats include JPEG, PNG, and WEBP.

///

/// See:

/// https://developer.android.com/reference/androidx/core/graphics/drawable/IconCompat#createWithData(byte[],int,int)

The createWithData doc links to the other two and explains their relevance, so the reader who wants to dig in can follow links from there. Including the list of three known formats is a helpful shortcut, though.

This field is data that gets fed to that function, so I wouldn't say it "wraps" the function — something that wraps the function would be another function, one that you could feed the same sort of data into and it'd in turn feed it to that underlying function.

gnprice · 2024-07-08T05:06:25Z

test/model/binding.dart

@@ -492,6 +492,11 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi {
  }
  List<AndroidNotificationHostApiNotifyCall> _notifyCalls = [];

+  void clearActiveNotifications() {


This should get a bit of dartdoc explaining what it means. See takeNotifyCalls, and methods on other classes in this file, for examples.

Also it should go after the field, and be separated by blank lines. The case of takeNotifyCalls above is unusual because it's formatted analogously to a getter for a private field.

In particular, unlike takeNotifyCalls and the similar "take" methods elsewhere in this file, this method has an important effect on the behavior of the code under test, not only on the records that are kept for the use of the test code.

The "take" methods are very close to behaving like proper getters — they do have a side effect, but the side effect only affects a later call to the very same method. They have no effect on code that isn't also calling the same method.

gnprice · 2024-07-08T05:15:03Z

test/notifications/display_test.dart

+        ..length.equals(messages.length * 2)
+        ..containsInOrder(notifyCallsChecks);


Well, in a previous thread we concluded that this was better than deepEquals because it allows the types around the condition callbacks to most fully make sense:
#703 (comment)

But just now I was trying to work out why clearActiveNotifications was needed, so I commented out the line below and saw the tests duly fail… and I discovered a strong reason to go with deepEquals after all. Namely, one gets much better error messages that way on failure.

Specifically, with deepEquals when it fails, one gets a failure message that goes recursively into the condition to say exactly why the element in question didn't match. With containsInOrder one doesn't get that — which is pretty fundamental, not just a bug in containsInOrder, because the interface of containsInOrder means that its implementation can't know which element was supposed to have matched the condition, only that none of the available elements did match it.

As a result containsInOrder can be fine when each condition is so simple that just knowing it didn't match tells you all you really need to know, but is no longer adequate when the conditions are themselves complex, as these are.

So let's use deepEquals here, and just accept that the conditions will have a should-be-redundant preamble of .isA<AndroidNotificationHostApiNotifyCall>().

Concretely, with deepEquals, after commenting out the clearActiveNotifications call, I got an error like this:

Which: has an element at [<0>] that: is a ({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag}) has id that: equals <403715830> … has messagingStyle that: is not null … has messages that: has length that: Actual: <2> which are not equal

so that I know the problem is that .messagingStyle.messages has length 2, instead of (… scrolling up in the error message …) 1.

From that, I immediately realized why the clearActiveNotifications was needed.

With the containsInOrder, I instead had:

Actual: [(autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: Instance of 'PendingIntent', contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1089, id: 1399477876, inboxStyle: null, isGroupSummary: null, messagingStyle: Instance of 'MessagingStyle', number: 2, smallIconResourceName: zulip_notification, tag: https://chat.example/|1089|dm:1089), (autoCancel: true, channelId: messages-1, color: 4284781310, contentIntent: null, contentText: null, contentTitle: null, extras: null, groupKey: https://chat.example/|1089, id: 300287668, inboxStyle: Instance of 'InboxStyle', isGroupSummary: true, messagingStyle: null, number: null, smallIconResourceName: zulip_notification, tag: https://chat.example/|1089)] Which: did not have an element matching the expectation at index 0 <A value that: is a ({bool? autoCancel, String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, int id, InboxStyle? inboxStyle, bool? isGroupSummary, MessagingStyle? messagingStyle, int? number, String? smallIconResourceName, String? tag}) has id that: equals <1399477876> has tag that: equals 'https://chat.example/|1089|dm:1089' has channelId that: equals 'messages-1' has contentTitle that: is null has contentText that: is null has messagingStyle that: is not null has user that: is not null has iconBitmap that: is null has key that: equals 'https://chat.example/|1089' has name that: equals 'You' has isGroupConversation that: equals <false> has conversationTitle that: equals 'Self User' has messages that: has length that: equals <1> contains, in order: [Closure: (Subject<MessagingStyleMessage?>) => void] has number that: equals <1> has color that: equals <4284781310> has smallIconResourceName that: equals 'zulip_notification' has extras that: is null has groupKey that: equals 'https://chat.example/|1089' has isGroupSummary that: is null has inboxStyle that: is null has autoCancel that: equals <true> has contentIntent that: is not null has requestCode that: equals <1399477876> has flags that: equals <201326592> has intentPayload that: equals '{"server":"zulip.example.cloud","realm_id":"4","realm_uri":"https://chat.example/","user_id":"1089","event":"message","sender_id":"1089","sender_avatar_url":"https://chat.example/avatar/1089.jpeg","sender_full_name":"Self User","zulip_message_id":"1338","time":"1678139636","content":"<p>This is an example message.</p>"}'>

which was completely uninformative.

gnprice · 2024-07-08T06:28:23Z

test/model/binding.dart

+    if (tag != null && messagingStyle != null) {
+      _activeNotificationsMessagingStyle[tag] = MessagingStyle(


If messagingStyle is null, then this should probably still update the map, right? The current notification with this tag will become a notification that has no messaging style.

gnprice · 2024-07-08T06:37:33Z

lib/notifications/display.dart

+        });
+    }
+
+    // For stream messages, the title remains constant. For DMs, it may


Hmm, why do you say it remains constant? It can change if the channel/stream gets renamed, right?

I think the comment in zulip-mobile is accurate here too. It just needs "PM" modernized to read "DM", and the reference to zulip/zulip-mobile#5116 can refer to the existing TODO below… or better yet, file a zulip-flutter issue for better titling group DM threads in notifications, referring back to zulip/zulip-mobile#5116 .

file a zulip-flutter issue for better titling group DM threads in notifications

Filed — #794

gnprice · 2024-07-08T06:39:04Z

lib/notifications/display.dart

+    // For stream messages, the title remains constant. For DMs, it may
+    // change if the DM sender's display name updates. For group DMs,


(And I'm not sure what "if the DM sender's display name updates" means. A conversation may have several DM senders.)

gnprice · 2024-07-08T06:41:18Z

lib/notifications/display.dart

      smallIconResourceName: 'zulip_notification', // This name must appear in keep.xml too: https://github.com/zulip/zulip-flutter/issues/528
-      // TODO(#128) inbox-style
+      messagingStyle: messagingStyle,


nit: blank line to separate these

rajveermalviya · 2024-07-13T06:42:32Z

Thanks for the review, @gnprice. I've pushed a revision that addresses your comments and includes the checkNotification/checkNotifications refactor.

Next, I'll send a follow-up PR that will include all the test cases mentioned, as well as the HTTP client test mentioned in one of the comments.

gnprice · 2024-07-14T22:40:54Z

test/model/binding.dart

+              .map<MessagingStyleMessage?>((message) => message != null
+                ? MessagingStyleMessage(


two nits:

Suggested change

.map<MessagingStyleMessage?>((message) => message != null

? MessagingStyleMessage(

.map((message) => message == null

? null

: MessagingStyleMessage(

the type on map is redundant, because that's already the type this would return, right?

generally prefer foo == null ? null : /* … long expression … */ over foo != null ? /* … long expression … */ : null, in order to keep the handling of the side case close to the condition for it.

But actually in this case, can we say this instead?

.map((message) => MessagingStyleMessage( text: message!.text,

That is, I believe we never actually intend to put a null value into this list — the type allows it only because of that Pigeon limitation. So we can just use the ! operator to throw if a null does somehow appear; if it does, that'll mean there's something wrong with the test.

gnprice · 2024-07-14T22:46:06Z

test/notifications/display_test.dart

+      testBinding.androidNotificationHost.clearActiveNotifications();
+    }));


Suggested change

testBinding.androidNotificationHost.clearActiveNotifications();

}));

}));

At the end of the test case, the whole notification state gets reset by the testBinding.reset that the init call at the top of the test case set up. So that means that, conveniently, we don't need another step to clear the active notifications at the end of the test.

gnprice · 2024-07-14T22:47:07Z

test/notifications/display_test.dart

+      testBinding.androidNotificationHost.clearActiveNotifications();
+    }


Similarly:

Suggested change

testBinding.androidNotificationHost.clearActiveNotifications();

}

}

(The call above this, on the other hand, is still needed, in order to clear things before the testBinding.firebaseMessaging.onBackgroundMessage.add call that follows it.)

gnprice · 2024-07-14T22:48:37Z

test/notifications/display_test.dart

+      final expectedTag =
+        '${data.realmUri}|${data.userId}|$expectedTagComponent';


nit:

Suggested change

final expectedTag =

'${data.realmUri}|${data.userId}|$expectedTagComponent';

final expectedTag = '${data.realmUri}|${data.userId}|$expectedTagComponent';

just to reduce the diff, since this line isn't otherwise changing

gnprice · 2024-07-14T22:49:26Z

test/notifications/display_test.dart

+      const expectedIntentFlags =
+        PendingIntentFlag.immutable | PendingIntentFlag.updateCurrent;


nit: similarly, this can be left in the order it was, in order to eliminate it from the diff

gnprice · 2024-07-14T22:50:46Z

test/notifications/display_test.dart

+      // List of all the checks for messages on each notify calls.
+      final messageStyleMessagesChecks = messageStyleMessages.mapIndexed<Condition<Object?>>((i, data) {


nit:

Suggested change

// List of all the checks for messages on each notify calls.

final messageStyleMessagesChecks = messageStyleMessages.mapIndexed<Condition<Object?>>((i, data) {

final messageStyleMessagesChecks = messageStyleMessages.mapIndexed<Condition<Object?>>((i, data) {

I think the code explains this already as well as the comment does. (Just look at the one place this variable is used.)

gnprice · 2024-07-14T22:57:01Z

test/notifications/display_test.dart

+      final message1 = eg.streamMessage(topic: 'topic 1', stream: stream, timestamp: 1);
+      final data1 = messageFcmMessage(message1, streamName: stream.name);
+      final message2 = eg.streamMessage(topic: 'topic 1', stream: stream, timestamp: 2);
+      final data2 = messageFcmMessage(message2, streamName: stream.name);
+      final message3 = eg.streamMessage(topic: 'topic 1', stream: stream, timestamp: 3);


Do the timestamps matter to what this test is testing? I think they don't. If not, then similar to #718 (comment) , let's remove them to help the reader focus on the things the test is about:

Suggested change

final message1 = eg.streamMessage(topic: 'topic 1', stream: stream, timestamp: 1);

final data1 = messageFcmMessage(message1, streamName: stream.name);

final message2 = eg.streamMessage(topic: 'topic 1', stream: stream, timestamp: 2);

final data2 = messageFcmMessage(message2, streamName: stream.name);

final message3 = eg.streamMessage(topic: 'topic 1', stream: stream, timestamp: 3);

final message1 = eg.streamMessage(topic: 'topic 1', stream: stream);

final data1 = messageFcmMessage(message1, streamName: stream.name);

final message2 = eg.streamMessage(topic: 'topic 1', stream: stream);

final data2 = messageFcmMessage(message2, streamName: stream.name);

final message3 = eg.streamMessage(topic: 'topic 1', stream: stream);

gnprice · 2024-07-14T23:00:17Z

test/notifications/display_test.dart

+        expectedTitle: '#${stream.name} > ${message3.topic}',
+        expectedTagComponent: 'stream:${message3.streamId}:${message3.topic}');


Similar to #718 (comment) : this test is relying on all three of these messages being in the same stream and topic. (If they weren't, then the messageStyleMessages: [data1, data2, data3] above wouldn't be the right expectation.) So let's make that explicit in the structure of the test:

Suggested change

expectedTitle: '#${stream.name} > ${message3.topic}',

expectedTagComponent: 'stream:${message3.streamId}:${message3.topic}');

expectedTitle: '#${stream.name} > $topic',

expectedTagComponent: 'stream:${stream.streamId}:$topic');

by expressing the topic and stream ID the same way each time.

Or probably better yet: pull out locals for expectedTitle and expectedTagComponent and compute them just once.

gnprice · 2024-07-14T23:03:12Z

test/notifications/display_test.dart

+        final expectedTag =
+          '${data.realmUri}|${data.userId}|$expectedTagComponent';
+        final expectedGroupKey = '${data.realmUri}|${data.userId}';
+        final expectedId =
+          NotificationDisplayManager.notificationIdAsHashOf(expectedTag);


bump on this part:

Then above that it should have an assert that verifies the assumption.

(I.e., the assert should verify that realmUri and userId are the same in all the given messages.)

gnprice · 2024-07-14T23:04:23Z

Thanks @rajveermalviya for the revision! This is very close to merge now. Comments above, and looking forward to that follow-up PR with more tests too.

rajveermalviya · 2024-07-15T07:00:19Z

Thanks for the review @gnprice, pushed a new revision PTAL.

rajveermalviya · 2024-07-15T07:43:42Z

Had a rebase conflict; fixed now.

Add methods and types for creating messaging style notifications: https://developer.android.com/develop/ui/views/notifications/build-notification#messaging-style

Use messaging style notifications to display messages with sender's name and avatars, along with support for displaying multiple messages from a specific topic by updating existing notification from notifications panel. See: https://developer.android.com/develop/ui/views/notifications/build-notification#messaging-style This change is similar to existing implementation in zulip-mobile: https://github.com/zulip/zulip-mobile/blob/e352f563ecf2fa9b09b688d5a65b6bc89b0358bc/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt#L177-L309 Fixes: zulip#128

gnprice

Thanks @rajveermalviya for building this and for all the revisions! Looks great — merging. And I see you've just sent #813 as a draft of that followup 😃

(I did end up testing this end-to-end, mainly by having this PR in the builds I put on my phone the last couple of weeks. Glad to have the feature.)

gnprice · 2024-07-15T19:53:52Z

test/notifications/display_test.dart

+          assert(messageData.realmUri == data.realmUri);
+          assert(messageData.userId == data.userId);


This works fine. FWIW what I was imagining was an assert or two like

assert(messageStyleMessages.every(…));

at the top of checkNotification.

Added this as an nfc in #813

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya force-pushed the messaging-style-notif branch from 0e3d32c to b61ad25 Compare June 12, 2024 05:18

rajveermalviya mentioned this pull request Jun 14, 2024

notif: Create summary notification #703

Merged

rajveermalviya force-pushed the messaging-style-notif branch 4 times, most recently from cbab765 to 042a89e Compare June 25, 2024 18:15

rajveermalviya marked this pull request as ready for review June 25, 2024 18:20

rajveermalviya commented Jun 25, 2024

View reviewed changes

rajveermalviya requested a review from Khader-1 June 25, 2024 18:39

rajveermalviya added the buddy review GSoC buddy review needed. label Jun 25, 2024

Khader-1 approved these changes Jun 26, 2024

View reviewed changes

Khader-1 assigned sumanthvrao Jun 26, 2024

Khader-1 added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jun 26, 2024

gnprice requested a review from sumanthvrao June 26, 2024 16:10

rajveermalviya force-pushed the messaging-style-notif branch 2 times, most recently from 1998fb1 to 3285d57 Compare June 30, 2024 19:31

Khader-1 unassigned sumanthvrao Jun 30, 2024

gnprice added the maintainer review PR ready for review by Zulip maintainers label Jul 3, 2024

gnprice requested a review from chrisbobbe July 3, 2024 04:25

chrisbobbe reviewed Jul 3, 2024

View reviewed changes

chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jul 3, 2024

gnprice reviewed Jul 3, 2024

View reviewed changes

rajveermalviya force-pushed the messaging-style-notif branch 3 times, most recently from a1e1858 to a7f2e86 Compare July 7, 2024 19:51

rajveermalviya requested review from chrisbobbe and gnprice July 7, 2024 19:58

gnprice reviewed Jul 8, 2024

View reviewed changes

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Jul 12, 2024

rajveermalviya force-pushed the messaging-style-notif branch 2 times, most recently from d9a7c22 to f463551 Compare July 13, 2024 06:32

rajveermalviya requested a review from gnprice July 13, 2024 06:42

gnprice reviewed Jul 14, 2024

View reviewed changes

rajveermalviya force-pushed the messaging-style-notif branch from f463551 to a55c35c Compare July 15, 2024 06:53

rajveermalviya requested a review from gnprice July 15, 2024 07:00

rajveermalviya force-pushed the messaging-style-notif branch from a55c35c to 994a96d Compare July 15, 2024 07:42

rajveermalviya force-pushed the messaging-style-notif branch from 994a96d to d6a0f22 Compare July 15, 2024 13:59

rajveermalviya added 3 commits July 15, 2024 12:55

notif: Add messaging-style notifications support to Pigeon bindings

4e3e0a4

Add methods and types for creating messaging style notifications: https://developer.android.com/develop/ui/views/notifications/build-notification#messaging-style

notif [nfc]: Use localization for self user display name

2393a19

rajveermalviya mentioned this pull request Jul 15, 2024

notif: Add more tests for messaging style notif and fix for fetchBitmap #813

Merged

gnprice reviewed Jul 15, 2024

View reviewed changes

gnprice force-pushed the messaging-style-notif branch from d6a0f22 to 2393a19 Compare July 15, 2024 20:06

gnprice merged commit 2393a19 into zulip:main Jul 15, 2024

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 15, 2024

notif test: Expand test coverage for messaging style notif

62ac84f

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya deleted the messaging-style-notif branch July 15, 2024 21:48

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 16, 2024

notif test: Expand test coverage for messaging style notif

4fab4cd

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 16, 2024

notif test: Add more tests for messaging style notif

215b889

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 28, 2024

notif test: Add more tests for messaging style notif

ad011a3

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 28, 2024

notif test: Add more tests for messaging style notif

a1ca6f5

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 30, 2024

notif test: Add more tests for messaging style notif

92a44c4

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 30, 2024

notif test: Add more tests for messaging style notif

a4d2a7b

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

gnprice pushed a commit to rajveermalviya/zulip-flutter that referenced this pull request Jul 30, 2024

notif test: Add more tests for messaging style notif

2c2775a

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

chrisbobbe mentioned this pull request Dec 26, 2024

android: Notification grouping should treat topics case-insensitively #1205

Closed

	final List<MessagingStyleMessage?>? messages;
	final List<MessagingStyleMessage>? messages;

	messagingStyle.messages?.add(MessagingStyleMessage(
	messagingStyle.messages!.add(MessagingStyleMessage(

		messagingStyle: messagingStyle,
		number: messagingStyle.messages?.length,

	messagingStyle.messages?.forEach { it?.let {
	messagingStyle.messages.forEach { it?.let {

		..length.equals(messages.length * 2)
		..containsInOrder(notifyCallsChecks);

		if (tag != null && messagingStyle != null) {
		_activeNotificationsMessagingStyle[tag] = MessagingStyle(

		// For stream messages, the title remains constant. For DMs, it may
		// change if the DM sender's display name updates. For group DMs,

		.map<MessagingStyleMessage?>((message) => message != null
		? MessagingStyleMessage(

		testBinding.androidNotificationHost.clearActiveNotifications();
		}));

		final expectedTag =
		'${data.realmUri}\|${data.userId}\|$expectedTagComponent';

	final expectedTag =
	'${data.realmUri}\|${data.userId}\|$expectedTagComponent';
	final expectedTag = '${data.realmUri}\|${data.userId}\|$expectedTagComponent';

		const expectedIntentFlags =
		PendingIntentFlag.immutable \| PendingIntentFlag.updateCurrent;

		// List of all the checks for messages on each notify calls.
		final messageStyleMessagesChecks = messageStyleMessages.mapIndexed<Condition<Object?>>((i, data) {

		expectedTitle: '#${stream.name} > ${message3.topic}',
		expectedTagComponent: 'stream:${message3.streamId}:${message3.topic}');

		assert(messageData.realmUri == data.realmUri);
		assert(messageData.userId == data.userId);

notif: Support messaging-style notifications #718

notif: Support messaging-style notifications #718

Conversation

rajveermalviya commented Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

Khader-1 left a comment

Choose a reason for hiding this comment

chrisbobbe 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 left a comment • edited Loading

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

rajveermalviya commented Jul 7, 2024

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

rajveermalviya Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jul 13, 2024

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 commented Jul 14, 2024

rajveermalviya commented Jul 15, 2024

rajveermalviya commented Jul 15, 2024

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

rajveermalviya commented Jun 3, 2024 •

edited

Loading

rajveermalviya Jul 7, 2024 •

edited

Loading

gnprice left a comment •

edited

Loading

rajveermalviya Jul 8, 2024 •

edited

Loading