notif: Create summary notification #703

rajveermalviya · 2024-05-23T20:54:44Z

notif-test.mp4

Fixes: #572
Fixes: #569
Fixes: #571

Khader-1

Thanks! This looks good.

sumanthvrao

Feel free to add a small description for each of your commits. It's much easier for a review to get a gist of what's happening in each commit that way, and it's a nice to have feature of any code base.

sumanthvrao · 2024-05-29T17:42:09Z

test/notifications/display_test.dart

@@ -166,7 +188,7 @@ void main() {
      final stream = eg.stream();
      final message = eg.streamMessage(stream: stream);
      await checkNotifications(async, messageFcmMessage(message, streamName: null),
-        expectedTitle: '(unknown stream) > ${message.subject}',
+        expectedTitle: '#(unknown stream) > ${message.subject}',


Do you want to extract this out to the other commit on stream renaming? I'm guessing it isn't necessary for this commit to work?

@sumanthvrao Sorry but I am not sure what's requested here, this change is already in a separate commit and it is the first one in the sequence. Would you want this commit to be after the main commit?

Also this change is fixing #572 which is just adding a # before stream names, the streams -> channel renaming is being done in #707 which includes changes for these notification strings.

Oh, apologies if I am misunderstanding here, but how are the two changes in your summary notif commit that add a # before stream name different from your first commit that does the same (for 2 other strings in different part of the codebase)? I was suggesting you merge the former into the first commit?

Ah, sorry for the misunderstanding, this should definitely belong in the first commit.

sumanthvrao · 2024-05-29T17:44:36Z

lib/notifications/display.dart

@@ -94,9 +94,9 @@ class NotificationDisplayManager {
    final zulipLocalizations = GlobalLocalizations.zulipLocalizations;


A general rule of thumb would be if this is an unrelated commit (e.g., its a nice to have commit, but isn't dependent on the later ones), you could move it earlier in the commit sequence?

sumanthvrao · 2024-05-29T17:45:40Z

test/notifications/display_test.dart

@@ -157,7 +179,7 @@ void main() {
      final stream = eg.stream();
      final message = eg.streamMessage(stream: stream);
      await checkNotifications(async, messageFcmMessage(message, streamName: stream.name),
-        expectedTitle: '${stream.name} > ${message.subject}',
+        expectedTitle: '#${stream.name} > ${message.subject}',


Extract out this one as well?

rajveermalviya · 2024-05-31T10:42:10Z

Thanks for the review @sumanthvrao!
Pushed a new revision, PTAL :)

chrisbobbe · 2024-06-12T19:14:51Z

Thanks @rajveermalviya! Could you please rebase this past the just-merged #730, which upgraded Pigeon?

rajveermalviya · 2024-06-13T03:40:34Z

@chrisbobbe rebased to main, PTAL :)

chrisbobbe

Thanks! Small comments below. Greg may have more detailed feedback on the code; he has more experience with it (and also with the UX of Android notifications).

I don't see the implementation for #571 by reading the code, but it does look like that issue is fixed. I noticed that your video shows the time ("1m") in the summary notification. I guess the platform just takes care of adding the time, so we don't have to do anything special to make it appear?

chrisbobbe · 2024-06-13T17:28:42Z

test/notifications/display_test.dart

  Subject<String?> get contentTitle => has((x) => x.contentTitle, 'contentTitle');
  Subject<Map<String?, String?>?> get extras => has((x) => x.extras, 'extras');
  Subject<String?> get smallIconResourceName => has((x) => x.smallIconResourceName, 'smallIconResourceName');
+  Subject<String?> get groupKey => has((x) => x.groupKey, 'groupKey');
+  Subject<bool?> get isGroupSummary => has((x) => x.isGroupSummary, 'isGroupSummary');
+  Subject<InboxStyle?> get inboxStyle => has((x) => x.inboxStyle, 'inboxStyle');
+  Subject<bool?> get autoCancel => has((x) => x.autoCancel, 'autoCancel');
 }

 extension on Subject<PendingIntent> {
  Subject<int> get requestCode => has((x) => x.requestCode, 'requestCode');
  Subject<String> get intentPayload => has((x) => x.intentPayload, 'intentPayload');
  Subject<int> get flags => has((x) => x.flags, 'flags');
 }
+
+extension on Subject<InboxStyle> {
+  Subject<String> get summaryText => has((x) => x.summaryText, 'summaryText');
+}


notif: Add support group summary notifications to Pigeon bindings

The analyzer flags these as unused at this commit:

Running analyze... Analyzing zulip-flutter... info • The declaration 'groupKey' isn't referenced • test/notifications/display_test.dart:379:24 • unused_element info • The declaration 'isGroupSummary' isn't referenced • test/notifications/display_test.dart:380:22 • unused_element info • The declaration 'inboxStyle' isn't referenced • test/notifications/display_test.dart:381:28 • unused_element info • The declaration 'autoCancel' isn't referenced • test/notifications/display_test.dart:382:22 • unused_element info • The declaration 'summaryText' isn't referenced • test/notifications/display_test.dart:392:23 • unused_element 5 issues found. (ran in 6.7s)

So let's either suppress this output until they get used, or introduce them in the same commit(s) that need them.

A command I find helpful for running our automated checks on all the commits for a PR: git rebase --exec 'tools/check --diff @~'.

Moved their introduction to the later (main) commit: 14d52e4

chrisbobbe · 2024-06-13T19:59:53Z

lib/notifications/display.dart

+      inboxStyle: InboxStyle(
+        // TODO(#570) Show organization name, not URL
+        summaryText: data.realmUri.toString()),


InboxStyle; interesting. 🙂

I'm an iOS user and don't have much experience of Zulip notifications on Android. Is this commit related to #128? Maybe later work on #128 will build on what we're doing here?

tldr: No, #128 will be fixed by #718

What #128 is talking about is actually called "MessagingStyle" notifications on Android, and here's how zulip-mobile implements it.

Whereas the usage here of InboxStyle is just because it's a clunky API to create a "special" type of notification called Group summary notification: https://developer.android.com/develop/ui/views/notifications/group#group-summary
And preconditions for that "special" notification are (as mentioned in the above docs):

recommended to be InboxStyle, with a description (we use summaryText).

distinct groupKeys for different groups, the summary notif and other notifs in the corresponding group should use same groupKey.

isGroupSummary = true for the summary notif.

Also updated (main) commit description with the doc link, even though it was already present in the previous commit.

chrisbobbe · 2024-06-13T20:03:29Z

lib/notifications/display.dart

@@ -139,6 +142,22 @@ class NotificationDisplayManager {
        // TODO this doesn't set the Intent flags we set in zulip-mobile; is that OK?
        //   (This is a legacy of `flutter_local_notifications`.)
        ),
+      autoCancel: true,


notif: Create summary notification

I see that zulip-mobile also does this (setAutoCancel(true)). Is this change coherent with the rest of what this commit is doing to create a summary notification? Or could it stand separately as its own commit? The answer will help me understand the user-facing changes that we're making, and how they're related.

Moved this to a separate commit — it's not really needed for Group summary notifications, but was added just to make the UX better, and match the zulip-mobile implementation.

rajveermalviya · 2024-06-14T16:14:53Z

Thanks for the review @chrisbobbe, addressed your comments in the latest revision.

rajveermalviya · 2024-06-14T16:38:40Z

I don't see the implementation for #571 by reading the code, but it does look like that issue is fixed. I noticed that your video shows the time ("1m") in the summary notification. I guess the platform just takes care of adding the time, so we don't have to do anything special to make it appear?

Yes, and the time it shows is actually when the notification was (received via FCM and) created, not the actual time of the message — these may defer if FCM delays the message delivery for various reasons. There is an API to set a custom timestamp using setWhen & setShowWhen but they're not used currently by zulip-mobile. On the contrary the MessagingStyle notifications which zulip-mobile uses (and will be implemented in #718), does require providing a custom timestamp.

chrisbobbe · 2024-06-15T00:28:31Z

Cool, thanks! This revision LGTM; over to Greg for his review.

gnprice

Thanks @rajveermalviya, and thanks @sumanthvrao and @chrisbobbe for the reviews!

Generally this all looks great — just a few small comments below.

gnprice · 2024-06-17T22:33:35Z

lib/notifications/display.dart

+    await ZulipBinding.instance.androidNotificationHost.notify(
+      id: notificationIdAsHashOf(groupKey),
+      channelId: NotificationChannelManager.kChannelId,
+      tag: groupKey,
+      groupKey: groupKey,
+      isGroupSummary: true,
+      color: kZulipBrandColor.value,


nit: keep the ordering of these the same as the other call above:

Suggested change

await ZulipBinding.instance.androidNotificationHost.notify(

id: notificationIdAsHashOf(groupKey),

channelId: NotificationChannelManager.kChannelId,

tag: groupKey,

groupKey: groupKey,

isGroupSummary: true,

color: kZulipBrandColor.value,

await ZulipBinding.instance.androidNotificationHost.notify(

id: notificationIdAsHashOf(groupKey),

tag: groupKey,

channelId: NotificationChannelManager.kChannelId,

groupKey: groupKey,

isGroupSummary: true,

color: kZulipBrandColor.value,

That's helpful especially because "id" and "tag" and "groupKey" inherently sound so similar, so it's helpful to have them listed in the same order when comparing the two calls.

gnprice · 2024-06-17T22:35:49Z

lib/notifications/display.dart

+      inboxStyle: InboxStyle(
+        // TODO(#570) Show organization name, not URL
+        summaryText: data.realmUri.toString()),
+      autoCancel: true,


Here on the summary notification, the corresponding code in zulip-mobile says (thanks for the handy link!):

// TODO Does this do something useful? There isn't a way to open these summary notifs. setAutoCancel(true)

What if you leave this line out — does any behavior change?

There seems to be no difference in behavior with it added or not, things verified in both conditions:

Tapping on summary header, which does nothing; unless clicked on the collapse icon — which toggles between collapsed state.

Tapping on child notif, opens the app and routes to the correct message, also clears that specific notif from the panel/statusbar (only if child notif had setAutoCancel(true)).

Keep opening (which also clears them) child notifs, and made sure summary notif automatically dismisses when there are no child notifs anymore.

From Stackoverflow, it seems like the summary notification may linger if child notifs are removed programmatically and setAutoCancel(true) on summary notif is not set. So, it may be a problem when #341 is implemented, but this is just a speculation though, will have to test and see there's really a difference while implementing #341.

Interesting, thanks for the investigation.

That issue where the summary lingers reminds me of a bug we briefly had in zulip-mobile:
zulip/zulip-mobile#5119
Looks like that didn't involve a lack of setAutoCancel(true), though — we had that on the summary notification from the beginning (in zulip/zulip-mobile@4229325).

Given that this logically shouldn't do anything (based on the reasoning in that TODO comment), and that we've also tested empirically and can't find it doing anything, let's leave it out. We can always add it back if we later discover after #341 that it's needed as a hack.

In any case, we definitely should not include the line while leaving out the TODO comment that was attached to it. That comment is useful information, which we shouldn't discard.

gnprice · 2024-06-17T22:38:36Z

test/notifications/display_test.dart

+        ..containsInOrder([
+          (Subject<AndroidNotificationHostApiNotifyCall> it) => it


FYI, can also put the type on the list:

Suggested change

..containsInOrder([

(Subject<AndroidNotificationHostApiNotifyCall> it) => it

..containsInOrder(<Condition<AndroidNotificationHostApiNotifyCall>>[

(it) => it

When there's a longer list of shorter conditions, that way feels a lot better. In this case, either way is fine.

gnprice · 2024-06-17T22:39:28Z

test/notifications/display_test.dart

+        ..length.equals(2)
+        ..containsInOrder([


Suggested change

..length.equals(2)

..containsInOrder([

.deepEquals([

That'd be equivalent, right? (Unless there's a subtlety I'm missing.)

gnprice · 2024-06-17T22:41:12Z

test/notifications/display_test.dart

+              ..summaryText.equals(data.realmUri.toString())
+            )


nit: similar to contentIntent above:

Suggested change

..summaryText.equals(data.realmUri.toString())

)

..summaryText.equals(data.realmUri.toString()))

gnprice · 2024-06-17T22:50:01Z

pigeon/notifications.dart

    String? smallIconResourceName,
+    String? groupKey,
+    bool? isGroupSummary,
+    InboxStyle? inboxStyle,
+    bool? autoCancel,
    // NotificationCompat.Builder has lots more methods; add as needed.


For adding parameters here, let's keep the builder parameters alphabetized — that helps make it easy to compare to the upstream docs at
https://developer.android.com/reference/androidx/core/app/NotificationCompat.Builder
because the list is so long.

I'll push a small NFC commit (→ 398749f) that just adds some comments here about that. Then the implementations of the class, and the list in AndroidNotificationHostApiNotifyCall, should follow along with the same order. The call sites in lib/notifications/display.dart can go for a logical order instead, though.

rajveermalviya · 2024-06-18T15:50:33Z

Thanks for the review @gnprice! Pushed a new revision.

gnprice

Thanks for the revision! Just a couple of small comments.

gnprice · 2024-06-18T23:17:41Z

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

            smallIconResourceName?.let { setSmallIcon(context.resources.getIdentifier(
                it, "drawable", context.packageName)) }
+            groupKey?.let { setGroup(it) }


nit: match here the ordering of the parameters above (following up on #703 (comment) )

gnprice · 2024-06-18T23:20:14Z

test/notifications/display_test.dart

+        .deepEquals(<Condition<Object?>>[
+          (it) => it.isA<AndroidNotificationHostApiNotifyCall>()


Hmm, this isA check seems unhelpful. It's statically a fact that each of these items is an AndroidNotificationHostApiNotifyCall, so we shouldn't have to repeat that.

How about the version suggested at #703 (comment) ? Or the version that was in the previous revision — as that comment said, either of those is fine.

The issue is if it's deepEquals(<Condition<AndroidNotificationHostApiNotifyCall>>[]) it doesn't recognize that the child is a Condition, instead it goes on with comparing AndroidNotificationHostApiNotifyCall with Condition<AndroidNotificationHostApiNotifyCall> which obviously fails. This is unlike containsInOrder which correctly checks for the generic type Condition<T>.

So, that's why the use of <Condition<Object?>> here and then asserting AndroidNotificationHostApiNotifyCall in the test.

I see. OK, in that case let's stick with the ..length/..containsInOrder version. It's one line longer, but that's worth it in order to make the types make sense, given that the isA here doesn't logically belong here as part of what the test is checking (because it'll statically always be true).

I did wonder if there was a subtlety I was missing 🙂 in why that version wasn't equivalent to deepEquals: #703 (comment) — this definitely counts as such a subtlety.

In general if there's something that comes up when you're acting on a review comment and it seems like there's an unexpected obstacle or things go in a different direction, please flag that proactively in the thread (either in reply to the previous comment, or as a new GitHub code-review comment), whether or not you end up going ahead and implementing the new direction.

That's one of the things that helps keep code reviews moving forward smoothly, because it saves the reviewer from having to guess about why your revision of that code came out the way it did.

For sure, for this one specifically though (isA), I'd looked for any existing use of deepEquals in the repo after your initial feedback in #703 (comment) and had found this. That gave me the impression that it was an OK change to introduce. :)

Reverted back to ..length/..containsInOrder in latest revision: https://github.com/zulip/zulip-flutter/compare/34fc3a81bd0f23d966819de7f9526d3056a63dab..0081b5cafa03915c8cd1e83300b06644a02a2d25

Cool. Yeah, that's a place where isA is needed anyway, because statically it's a list of Route<dynamic> but we want this element to specifically be a WidgetRoute.

The previous comment that this really goes in a different direction from is the one about where in these checks to put the AndroidNotificationHostApiNotifyCall type: #703 (comment)
So it turned out that the deepEquals suggestion was incompatible with that comment. But a contradiction between different review comments I make is the kind of thing I'd very much like to learn about explicitly.

gnprice · 2024-06-18T23:33:50Z

lib/notifications/display.dart

+      inboxStyle: InboxStyle(
+        // TODO(#570) Show organization name, not URL
+        summaryText: data.realmUri.toString()),
+      autoCancel: true,


Interesting, thanks for the investigation.

That issue where the summary lingers reminds me of a bug we briefly had in zulip-mobile:
zulip/zulip-mobile#5119
Looks like that didn't involve a lack of setAutoCancel(true), though — we had that on the summary notification from the beginning (in zulip/zulip-mobile@4229325).

Given that this logically shouldn't do anything (based on the reasoning in that TODO comment), and that we've also tested empirically and can't find it doing anything, let's leave it out. We can always add it back if we later discover after #341 that it's needed as a hack.

In any case, we definitely should not include the line while leaving out the TODO comment that was attached to it. That comment is useful information, which we shouldn't discard.

rajveermalviya · 2024-06-19T02:57:41Z

Thanks @gnprice for the review! Just pushed the latest revision.

This adds support for creating a group summary notification: https://developer.android.com/develop/ui/views/notifications/group#group-summary

Make use of Group Summary Notifications to group notifications based on different realms and also display the respective group label (which is currently the realm URL). See: https://developer.android.com/develop/ui/views/notifications/group#group-summary This change is a port of implementation in zulip-mobile: https://github.com/zulip/zulip-mobile/blob/6d5d56d175644cd0cdf47f3cd30ffadf6756bbdc/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt#L299-L382 Fixes: zulip#569 Fixes: zulip#571

Set autoCancel flag for notifications, ensuring they automatically disappear from the panel when tapped. See: https://developer.android.com/reference/androidx/core/app/NotificationCompat.Builder#setAutoCancel(boolean) Also, zulip-mobile does this already: https://github.com/zulip/zulip-mobile/blob/e352f563ecf2fa9b09b688d5a65b6bc89b0358bc/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt#L353

Fixes: zulip#572

gnprice · 2024-06-19T03:30:43Z

OK, looks great — merging! Thanks @rajveermalviya for building this, and thanks again @sumanthvrao and @chrisbobbe for the reviews. Looking forward to the improved notifications 😄

rajveermalviya force-pushed the notif-minor-issues branch from eac15b4 to 4eb4ff8 Compare May 23, 2024 21:02

rajveermalviya added the buddy review GSoC buddy review needed. label May 24, 2024

rajveermalviya requested a review from Khader-1 May 24, 2024 04:56

Khader-1 reviewed May 26, 2024

View reviewed changes

rajveermalviya added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels May 27, 2024

rajveermalviya requested a review from sumanthvrao May 27, 2024 08:59

sumanthvrao reviewed May 29, 2024

View reviewed changes

rajveermalviya force-pushed the notif-minor-issues branch from 4eb4ff8 to 06b84ea Compare May 31, 2024 10:38

rajveermalviya requested a review from sumanthvrao May 31, 2024 10:42

rajveermalviya mentioned this pull request Jun 3, 2024

notif: Support messaging-style notifications #718

Merged

gnprice added the maintainer review PR ready for review by Zulip maintainers label Jun 11, 2024

gnprice requested a review from chrisbobbe June 11, 2024 18:02

rajveermalviya force-pushed the notif-minor-issues branch from 06b84ea to 6830e57 Compare June 12, 2024 05:12

rajveermalviya force-pushed the notif-minor-issues branch 3 times, most recently from 1dae196 to 3ea7cb5 Compare June 13, 2024 03:40

chrisbobbe reviewed Jun 13, 2024

View reviewed changes

rajveermalviya force-pushed the notif-minor-issues branch from 3ea7cb5 to 2b737db Compare June 14, 2024 15:39

rajveermalviya requested a review from chrisbobbe June 14, 2024 16:15

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

rajveermalviya requested a review from gnprice June 15, 2024 01:23

gnprice reviewed Jun 17, 2024

View reviewed changes

rajveermalviya force-pushed the notif-minor-issues branch from 2b737db to e2421b8 Compare June 18, 2024 02:40

rajveermalviya force-pushed the notif-minor-issues branch from e2421b8 to 1d0e040 Compare June 18, 2024 15:48

rajveermalviya requested a review from gnprice June 18, 2024 15:50

gnprice reviewed Jun 18, 2024

View reviewed changes

rajveermalviya force-pushed the notif-minor-issues branch 2 times, most recently from 58f1d9d to 34fc3a8 Compare June 19, 2024 02:46

rajveermalviya requested a review from gnprice June 19, 2024 02:57

rajveermalviya force-pushed the notif-minor-issues branch from 34fc3a8 to 0081b5c Compare June 19, 2024 03:26

rajveermalviya added 4 commits June 18, 2024 20:29

notif: Add support group summary notifications to Pigeon bindings

499573c

This adds support for creating a group summary notification: https://developer.android.com/develop/ui/views/notifications/group#group-summary

notif: Prefix stream names with #

365f9da

Fixes: zulip#572

gnprice force-pushed the notif-minor-issues branch from 0081b5c to 365f9da Compare June 19, 2024 03:30

gnprice merged commit 365f9da into zulip:main Jun 19, 2024
1 check passed

rajveermalviya deleted the notif-minor-issues branch June 19, 2024 03:33

gnprice mentioned this pull request Jun 20, 2024

notif: Enable autoCancel flag for summary notification #755

Merged

		@@ -94,9 +94,9 @@ class NotificationDisplayManager {
		final zulipLocalizations = GlobalLocalizations.zulipLocalizations;

		..containsInOrder([
		(Subject<AndroidNotificationHostApiNotifyCall> it) => it

	..summaryText.equals(data.realmUri.toString())
	)
	..summaryText.equals(data.realmUri.toString()))

		.deepEquals(<Condition<Object?>>[
		(it) => it.isA<AndroidNotificationHostApiNotifyCall>()

notif: Create summary notification #703

notif: Create summary notification #703

Conversation

rajveermalviya commented May 23, 2024 • edited Loading

Khader-1 left a comment

Choose a reason for hiding this comment

sumanthvrao 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

rajveermalviya commented May 31, 2024

chrisbobbe commented Jun 12, 2024 • edited Loading

rajveermalviya commented Jun 13, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jun 14, 2024

rajveermalviya commented Jun 14, 2024 • edited Loading

chrisbobbe commented Jun 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 Jun 18, 2024 • 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

gnprice Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

rajveermalviya commented Jun 18, 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

rajveermalviya commented Jun 19, 2024

gnprice commented Jun 19, 2024

rajveermalviya commented May 23, 2024 •

edited

Loading

chrisbobbe commented Jun 12, 2024 •

edited

Loading

rajveermalviya commented Jun 14, 2024 •

edited

Loading

rajveermalviya Jun 18, 2024 •

edited

Loading

gnprice Jun 17, 2024 •

edited

Loading