Skip to content

notif: Create summary notification #703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 19, 2024
Merged

notif: Create summary notification #703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
499573c
notif: Add support group summary notifications to Pigeon bindings
rajveermalviya May 31, 2024
c665d6e
notif: Create group summary notification
rajveermalviya May 31, 2024
c7ee2d3
notif: Enable autoCancel to dismiss notifications when tapped
rajveermalviya Jun 14, 2024
365f9da
notif: Prefix stream names with `#`
rajveermalviya May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions android/app/src/main/kotlin/com/zulip/flutter/Notifications.g.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,31 @@ data class PendingIntent (
)
}
}

/**
* Corresponds to `androidx.core.app.NotificationCompat.InboxStyle`
*
* See: https://developer.android.com/reference/androidx/core/app/NotificationCompat.InboxStyle
*
* Generated class from Pigeon that represents data sent in messages.
*/
data class InboxStyle (
val summaryText: String

) {
companion object {
@Suppress("LocalVariableName")
fun fromList(__pigeon_list: List<Any?>): InboxStyle {
val summaryText = __pigeon_list[0] as String
return InboxStyle(summaryText)
}
}
fun toList(): List<Any?> {
return listOf(
summaryText,
)
}
}
private object NotificationsPigeonCodec : StandardMessageCodec() {
override fun readValueOfType(type: Byte, buffer: ByteBuffer): Any? {
return when (type) {
Expand All @@ -90,6 +115,11 @@ private object NotificationsPigeonCodec : StandardMessageCodec() {
PendingIntent.fromList(it)
}
}
130.toByte() -> {
return (readValue(buffer) as? List<Any?>)?.let {
InboxStyle.fromList(it)
}
}
else -> super.readValueOfType(type, buffer)
}
}
Expand All @@ -99,6 +129,10 @@ private object NotificationsPigeonCodec : StandardMessageCodec() {
stream.write(129)
writeValue(stream, value.toList())
}
is InboxStyle -> {
stream.write(130)
writeValue(stream, value.toList())
}
else -> super.writeValue(stream, value)
}
}
Expand All @@ -125,7 +159,7 @@ interface AndroidNotificationHostApi {
* https://developer.android.com/reference/kotlin/android/app/NotificationManager.html#notify
* https://developer.android.com/reference/androidx/core/app/NotificationCompat.Builder
*/
fun notify(tag: String?, id: Long, channelId: String, color: Long?, contentIntent: PendingIntent?, contentText: String?, contentTitle: String?, extras: Map<String?, String?>?, smallIconResourceName: String?)
fun notify(tag: String?, id: Long, autoCancel: Boolean?, channelId: String, color: Long?, contentIntent: PendingIntent?, contentText: String?, contentTitle: String?, extras: Map<String?, String?>?, groupKey: String?, inboxStyle: InboxStyle?, isGroupSummary: Boolean?, smallIconResourceName: String?)

companion object {
/** The codec used by AndroidNotificationHostApi. */
Expand All @@ -143,15 +177,19 @@ interface AndroidNotificationHostApi {
val args = message as List<Any?>
val tagArg = args[0] as String?
val idArg = args[1].let { num -> if (num is Int) num.toLong() else num as Long }
val channelIdArg = args[2] as String
val colorArg = args[3].let { num -> if (num is Int) num.toLong() else num as Long? }
val contentIntentArg = args[4] as PendingIntent?
val contentTextArg = args[5] as String?
val contentTitleArg = args[6] as String?
val extrasArg = args[7] as Map<String?, String?>?
val smallIconResourceNameArg = args[8] as String?
val autoCancelArg = args[2] as Boolean?
val channelIdArg = args[3] as String
val colorArg = args[4].let { num -> if (num is Int) num.toLong() else num as Long? }
val contentIntentArg = args[5] as PendingIntent?
val contentTextArg = args[6] as String?
val contentTitleArg = args[7] as String?
val extrasArg = args[8] as Map<String?, String?>?
val groupKeyArg = args[9] as String?
val inboxStyleArg = args[10] as InboxStyle?
val isGroupSummaryArg = args[11] as Boolean?
val smallIconResourceNameArg = args[12] as String?
val wrapped: List<Any?> = try {
api.notify(tagArg, idArg, channelIdArg, colorArg, contentIntentArg, contentTextArg, contentTitleArg, extrasArg, smallIconResourceNameArg)
api.notify(tagArg, idArg, autoCancelArg, channelIdArg, colorArg, contentIntentArg, contentTextArg, contentTitleArg, extrasArg, groupKeyArg, inboxStyleArg, isGroupSummaryArg, smallIconResourceNameArg)
listOf(null)
} catch (exception: Throwable) {
wrapError(exception)
Expand Down
11 changes: 11 additions & 0 deletions android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,20 @@ private class AndroidNotificationHost(val context: Context)
override fun notify(
tag: String?,
id: Long,
autoCancel: Boolean?,
channelId: String,
color: Long?,
contentIntent: PendingIntent?,
contentText: String?,
contentTitle: String?,
extras: Map<String?, String?>?,
groupKey: String?,
inboxStyle: InboxStyle?,
isGroupSummary: Boolean?,
smallIconResourceName: String?
) {
val notification = NotificationCompat.Builder(context, channelId).apply {
autoCancel?.let { setAutoCancel(it) }
color?.let { setColor(it.toInt()) }
contentIntent?.let { setContentIntent(
android.app.PendingIntent.getActivity(context,
Expand All @@ -49,6 +54,12 @@ private class AndroidNotificationHost(val context: Context)
contentTitle?.let { setContentTitle(it) }
extras?.let { setExtras(
Bundle().apply { it.forEach { (k, v) -> putString(k, v) } } ) }
groupKey?.let { setGroup(it) }
inboxStyle?.let { setStyle(
NotificationCompat.InboxStyle()
.setSummaryText(it.summaryText)
) }
isGroupSummary?.let { setGroupSummary(it) }
smallIconResourceName?.let { setSmallIcon(context.resources.getIdentifier(
it, "drawable", context.packageName)) }
}.build()
Expand Down
33 changes: 31 additions & 2 deletions lib/host/android_notifications.g.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,30 @@ class PendingIntent {
}
}

/// Corresponds to `androidx.core.app.NotificationCompat.InboxStyle`
///
/// See: https://developer.android.com/reference/androidx/core/app/NotificationCompat.InboxStyle
class InboxStyle {
InboxStyle({
required this.summaryText,
});

String summaryText;

Object encode() {
return <Object?>[
summaryText,
];
}

static InboxStyle decode(Object result) {
result as List<Object?>;
return InboxStyle(
summaryText: result[0]! as String,
);
}
}


class _PigeonCodec extends StandardMessageCodec {
const _PigeonCodec();
Expand All @@ -61,6 +85,9 @@ class _PigeonCodec extends StandardMessageCodec {
if (value is PendingIntent) {
buffer.putUint8(129);
writeValue(buffer, value.encode());
} else if (value is InboxStyle) {
buffer.putUint8(130);
writeValue(buffer, value.encode());
} else {
super.writeValue(buffer, value);
}
Expand All @@ -71,6 +98,8 @@ class _PigeonCodec extends StandardMessageCodec {
switch (type) {
case 129:
return PendingIntent.decode(readValue(buffer)!);
case 130:
return InboxStyle.decode(readValue(buffer)!);
default:
return super.readValueOfType(type, buffer);
}
Expand Down Expand Up @@ -107,15 +136,15 @@ class AndroidNotificationHostApi {
/// See:
/// https://developer.android.com/reference/kotlin/android/app/NotificationManager.html#notify
/// https://developer.android.com/reference/androidx/core/app/NotificationCompat.Builder
Future<void> notify({String? tag, required int id, required String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? smallIconResourceName,}) async {
Future<void> notify({String? tag, required int id, bool? autoCancel, required String channelId, int? color, PendingIntent? contentIntent, String? contentText, String? contentTitle, Map<String?, String?>? extras, String? groupKey, InboxStyle? inboxStyle, bool? isGroupSummary, String? smallIconResourceName,}) async {
final String __pigeon_channelName = 'dev.flutter.pigeon.zulip.AndroidNotificationHostApi.notify$__pigeon_messageChannelSuffix';
final BasicMessageChannel<Object?> __pigeon_channel = BasicMessageChannel<Object?>(
__pigeon_channelName,
pigeonChannelCodec,
binaryMessenger: __pigeon_binaryMessenger,
);
final List<Object?>? __pigeon_replyList =
await __pigeon_channel.send(<Object?>[tag, id, channelId, color, contentIntent, contentText, contentTitle, extras, smallIconResourceName]) as List<Object?>?;
await __pigeon_channel.send(<Object?>[tag, id, autoCancel, channelId, color, contentIntent, contentText, contentTitle, extras, groupKey, inboxStyle, isGroupSummary, smallIconResourceName]) as List<Object?>?;
if (__pigeon_replyList == null) {
throw _createConnectionError(__pigeon_channelName);
} else if (__pigeon_replyList.length > 1) {
Expand Down
32 changes: 25 additions & 7 deletions lib/notifications/display.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,27 +89,30 @@ class NotificationDisplayManager {
}
}

static void _onMessageFcmMessage(MessageFcmMessage data, Map<String, dynamic> dataJson) {
static Future<void> _onMessageFcmMessage(MessageFcmMessage data, Map<String, dynamic> dataJson) async {
assert(debugLog('notif message content: ${data.content}'));
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

final title = switch (data.recipient) {
FcmMessageStreamRecipient(:var streamName?, :var topic) =>
'$streamName > $topic',
'#$streamName > $topic',
FcmMessageStreamRecipient(:var topic) =>
'(unknown channel) > $topic', // TODO get stream name from data
'#(unknown channel) > $topic', // TODO get stream name from data
FcmMessageDmRecipient(:var allRecipientIds) when allRecipientIds.length > 2 =>
zulipLocalizations.notifGroupDmConversationLabel(
data.senderFullName, allRecipientIds.length - 2), // TODO use others' names, from data
FcmMessageDmRecipient() =>
data.senderFullName,
};
final conversationKey = _conversationKey(data);
ZulipBinding.instance.androidNotificationHost.notify(
final groupKey = _groupKey(data);
final conversationKey = _conversationKey(data, groupKey);

await ZulipBinding.instance.androidNotificationHost.notify(
// TODO the notification ID can be constant, instead of matching requestCode
// (This is a legacy of `flutter_local_notifications`.)
id: notificationIdAsHashOf(conversationKey),
tag: conversationKey,
channelId: NotificationChannelManager.kChannelId,
groupKey: groupKey,

contentTitle: title,
contentText: data.content,
Expand Down Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);

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

color: kZulipBrandColor.value,
// TODO vary notification icon for debug
smallIconResourceName: 'zulip_notification', // This name must appear in keep.xml too: https://github.com/zulip/zulip-flutter/issues/528
inboxStyle: InboxStyle(
// TODO(#570) Show organization name, not URL
summaryText: data.realmUri.toString()),
Comment on lines +158 to +160
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
}

Expand All @@ -157,8 +176,7 @@ class NotificationDisplayManager {
| ((bytes[3] & 0x7f) << 24);
}

static String _conversationKey(MessageFcmMessage data) {
final groupKey = _groupKey(data);
static String _conversationKey(MessageFcmMessage data, String groupKey) {
final conversation = switch (data.recipient) {
FcmMessageStreamRecipient(:var streamId, :var topic) => 'stream:$streamId:$topic',
FcmMessageDmRecipient(:var allRecipientIds) => 'dm:${allRecipientIds.join(',')}',
Expand Down
13 changes: 13 additions & 0 deletions pigeon/notifications.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ class PendingIntent {
final int flags;
}

/// Corresponds to `androidx.core.app.NotificationCompat.InboxStyle`
///
/// See: https://developer.android.com/reference/androidx/core/app/NotificationCompat.InboxStyle
class InboxStyle {
InboxStyle({required this.summaryText});

final String summaryText;
}

@HostApi()
abstract class AndroidNotificationHostApi {
/// Corresponds to `android.app.NotificationManager.notify`,
Expand Down Expand Up @@ -54,12 +63,16 @@ abstract class AndroidNotificationHostApi {
required int id,

// The remaining arguments go to method calls on NotificationCompat.Builder.
bool? autoCancel,
required String channelId,
int? color,
PendingIntent? contentIntent,
String? contentText,
String? contentTitle,
Map<String?, String?>? extras,
String? groupKey,
InboxStyle? inboxStyle,
bool? isGroupSummary,
String? smallIconResourceName,
// NotificationCompat.Builder has lots more methods; add as needed.
// Keep them alphabetized, for easy comparison with that class's docs.
Expand Down
12 changes: 12 additions & 0 deletions test/model/binding.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,23 +496,31 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi {
Future<void> notify({
String? tag,
required int id,
bool? autoCancel,
required String channelId,
int? color,
PendingIntent? contentIntent,
String? contentText,
String? contentTitle,
Map<String?, String?>? extras,
String? groupKey,
InboxStyle? inboxStyle,
bool? isGroupSummary,
String? smallIconResourceName,
}) async {
_notifyCalls.add((
tag: tag,
id: id,
autoCancel: autoCancel,
channelId: channelId,
color: color,
contentIntent: contentIntent,
contentText: contentText,
contentTitle: contentTitle,
extras: extras,
groupKey: groupKey,
inboxStyle: inboxStyle,
isGroupSummary: isGroupSummary,
smallIconResourceName: smallIconResourceName,
));
}
Expand All @@ -521,11 +529,15 @@ class FakeAndroidNotificationHostApi implements AndroidNotificationHostApi {
typedef AndroidNotificationHostApiNotifyCall = ({
String? tag,
int id,
bool? autoCancel,
String channelId,
int? color,
PendingIntent? contentIntent,
String? contentText,
String? contentTitle,
Map<String?, String?>? extras,
String? groupKey,
InboxStyle? inboxStyle,
bool? isGroupSummary,
String? smallIconResourceName,
});
Loading
Loading