From 5b872a1dfb53a1ab223b8d2366ec6182e4106a97 Mon Sep 17 00:00:00 2001 From: Lakoja Date: Sat, 4 Jan 2025 13:00:06 +0100 Subject: [PATCH] Properly summarize all notifications --- .../NotificationFetcher.kt | 73 ++---- .../NotificationHelper.java | 237 ++++++++---------- .../keylesspalace/tusky/MainActivityTest.kt | 3 +- 3 files changed, 124 insertions(+), 189 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt b/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt index 13ea0d9090..5f9b0c5908 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt @@ -16,7 +16,6 @@ import com.keylesspalace.tusky.util.HttpHeaderLink import com.keylesspalace.tusky.util.isLessThan import dagger.hilt.android.qualifiers.ApplicationContext import javax.inject.Inject -import kotlin.math.min import kotlin.time.Duration.Companion.milliseconds import kotlinx.coroutines.delay @@ -66,74 +65,41 @@ class NotificationFetcher @Inject constructor( .sortedWith( compareBy({ it.id.length }, { it.id }) ) // oldest notifications first - .toMutableList() // TODO do this before filter above? But one could argue that (for example) a tab badge is also a notification // (and should therefore adhere to the notification config). eventHub.dispatch(NewNotificationsEvent(account.accountId, notifications)) - // There's a maximum limit on the number of notifications an Android app - // can display. If the total number of notifications (current notifications, - // plus new ones) exceeds this then some newer notifications will be dropped. - // - // Err on the side of removing *older* notifications to make room for newer - // notifications. - val currentAndroidNotifications = notificationManager.activeNotifications - .sortedWith( - compareBy({ it.tag.length }, { it.tag }) - ) // oldest notifications first - - // Check to see if any notifications need to be removed - val toRemove = currentAndroidNotifications.size + notifications.size - MAX_NOTIFICATIONS - if (toRemove > 0) { - // Prefer to cancel old notifications first - currentAndroidNotifications.subList( - 0, - min(toRemove, currentAndroidNotifications.size) + val notificationsByType: Map> = notifications.groupBy { it.type } + + notificationsByType.forEach { notificationsGroup: Map.Entry> -> + // NOTE: Enqueue the summary first: Only this way one can avoid running into notification rate limits; + // which also would prohibit the group summary to be shown and thus proper grouping! + NotificationHelper.showSummaryNotification( + context, + notificationManager, + account, + notificationsGroup.key, + notificationsGroup.value ) - .forEach { notificationManager.cancel(it.tag, it.id) } - - // Still got notifications to remove? Trim the list of new notifications, - // starting with the oldest. - while (notifications.size > MAX_NOTIFICATIONS) { - notifications.removeAt(0) - } - } - - val notificationsByType = notifications.groupBy { it.type } - // Make and send the new notifications - // TODO: Use the batch notification API available in NotificationManagerCompat - // 1.11 and up (https://developer.android.com/jetpack/androidx/releases/core#1.11.0-alpha01) - // when it is released. - - notificationsByType.forEach { notificationsGroup -> notificationsGroup.value.forEach { notification -> val androidNotification = NotificationHelper.make( context, notificationManager, notification, - account, - notificationsGroup.value.size == 1 + account ) notificationManager.notify( notification.id, account.id.toInt(), androidNotification ) - - // Android will rate limit / drop notifications if they're posted too - // quickly. There is no indication to the user that this happened. - // See https://github.com/tuskyapp/Tusky/pull/3626#discussion_r1192963664 - delay(1000.milliseconds) } - } - NotificationHelper.updateSummaryNotifications( - context, - notificationManager, - account - ) + // NOTE this can schedule multiple (summary) notifications in short succession (normally 1-3). + // They can "collapse" to only one audible one if fast enough. + } accountManager.saveAccount(account) } catch (e: Exception) { @@ -187,6 +153,8 @@ class NotificationFetcher @Inject constructor( Log.d(TAG, " localMarkerId: $localMarkerId") Log.d(TAG, " readingPosition: $readingPosition") + minId = "1405569" + Log.d(TAG, "getting Notifications for ${account.fullName}, min_id: $minId") // Fetch all outstanding notifications @@ -246,12 +214,5 @@ class NotificationFetcher @Inject constructor( companion object { private const val TAG = "NotificationFetcher" - - // There's a system limit on the maximum number of notifications an app - // can show, NotificationManagerService.MAX_PACKAGE_NOTIFICATIONS. Unfortunately - // that's not available to client code or via the NotificationManager API. - // The current value in the Android source code is 50, set 40 here to both - // be conservative, and allow some headroom for summary notifications. - private const val MAX_NOTIFICATIONS = 40 } } diff --git a/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationHelper.java b/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationHelper.java index 4a574db9be..5451a3c614 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationHelper.java +++ b/app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationHelper.java @@ -68,10 +68,8 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -149,7 +147,7 @@ public class NotificationHelper { * @return the new notification */ @NonNull - public static android.app.Notification make(final @NonNull Context context, @NonNull NotificationManager notificationManager, @NonNull Notification body, @NonNull AccountEntity account, boolean isOnlyOneInGroup) { + public static android.app.Notification make(final @NonNull Context context, @NonNull NotificationManager notificationManager, @NonNull Notification body, @NonNull AccountEntity account) { body = body.rewriteToStatusTypeIfNeeded(account.getAccountId()); String mastodonNotificationId = body.getId(); int accountId = (int) account.getId(); @@ -163,9 +161,6 @@ public static android.app.Notification make(final @NonNull Context context, @Non } } - // Notification group member - // ========================= - notificationId++; // Create the notification -- either create a new one, or use the existing one. NotificationCompat.Builder builder; @@ -240,128 +235,102 @@ public static android.app.Notification make(final @NonNull Context context, @Non extras.putString(EXTRA_NOTIFICATION_TYPE, body.getType().name()); builder.addExtras(extras); - // Only alert for the first notification of a batch to avoid multiple alerts at once - if(!isOnlyOneInGroup) { - builder.setGroupAlertBehavior(NotificationCompat.GROUP_ALERT_SUMMARY); - } + // Only ever alert for the summary notification + builder.setGroupAlertBehavior(NotificationCompat.GROUP_ALERT_SUMMARY); return builder.build(); } /** - * Updates the summary notifications for each notification group. + * Updates the summary notifications for each notification type. *

- * Notifications are sent to channels. Within each channel they may be grouped, and the group - * may have a summary. + * Notifications are sent to channels. Within each channel they are grouped and the group has a summary. *

* Tusky uses N notification channels for each account, each channel corresponds to a type - * of notification (follow, reblog, mention, etc). Therefore each channel also has exactly - * 0 or 1 summary notifications along with its regular notifications. + * of notification (follow, reblog, mention, etc). Each channel also has a + * summary notifications along with its regular notifications. *

* The group key is the same as the channel ID. - *

- * Regnerates the summary notifications for all active Tusky notifications for `account`. - * This may delete the summary notification if there are no active notifications for that - * account in a group. * * @see Create a * notification group - * @param context to access application preferences and services - * @param notificationManager the system's NotificationManager - * @param account the account for which the notification should be shown */ - public static void updateSummaryNotifications(@NonNull Context context, @NonNull NotificationManager notificationManager, @NonNull AccountEntity account) { - // Map from the channel ID to a list of notifications in that channel. Those are the - // notifications that will be summarised. - Map> channelGroups = new HashMap<>(); + public static void showSummaryNotification(@NonNull Context context, @NonNull NotificationManager notificationManager, @NonNull AccountEntity account, @NonNull Notification.Type type, @NonNull List additionalNotifications) { int accountId = (int) account.getId(); - // Initialise the map with all channel IDs. - for (Notification.Type ty : Notification.Type.getEntries()) { - channelGroups.put(getChannelId(account, ty), new ArrayList<>()); - } + String typeChannelId = getChannelId(account, type); - // Fetch all existing notifications. Add them to the map, ignoring notifications that: - // - belong to a different account - // - are summary notifications - for (StatusBarNotification sn : notificationManager.getActiveNotifications()) { - if (sn.getId() != accountId) continue; - - String channelId = sn.getNotification().getGroup(); - String summaryTag = GROUP_SUMMARY_TAG + "." + channelId; - if (summaryTag.equals(sn.getTag())) continue; - - // TODO: API 26 supports getting the channel ID directly (sn.getNotification().getChannelId()). - // This works here because the channelId and the groupKey are the same. - List members = channelGroups.get(channelId); - if (members == null) { // can't happen, but just in case... - Log.e(TAG, "members == null for channel ID " + channelId); - continue; - } - members.add(sn); + if (typeChannelId == null) { + return; } - // Create, update, or cancel the summary notifications for each group. - for (Map.Entry> channelGroup : channelGroups.entrySet()) { - String channelId = channelGroup.getKey(); - List members = channelGroup.getValue(); - String summaryTag = GROUP_SUMMARY_TAG + "." + channelId; - - // If there are 0-1 notifications in this group then the additional summary - // notification is not needed and can be cancelled. - if (members.size() <= 1) { - notificationManager.cancel(summaryTag, accountId); - continue; - } + // Create a notification that summarises the other notifications in this group + // + // NOTE: We always create a summary notification (even for activeNotificationsForType.size() == 1): + // - No need to especially track the grouping + // - No need to change an existing single notification when there arrives another one of its group + // - Only the summary one will get announced + TaskStackBuilder summaryStackBuilder = TaskStackBuilder.create(context); + summaryStackBuilder.addParentStack(MainActivity.class); + Intent summaryResultIntent = MainActivity.openNotificationIntent(context, accountId, type); + summaryStackBuilder.addNextIntent(summaryResultIntent); - // Create a notification that summarises the other notifications in this group + PendingIntent summaryResultPendingIntent = summaryStackBuilder.getPendingIntent((int) (notificationId + account.getId() * 10000), + pendingIntentFlags(false)); - // All notifications in this group have the same type, so get it from the first. - String typeName = members.get(0).getNotification().extras.getString(EXTRA_NOTIFICATION_TYPE, Notification.Type.UNKNOWN.name()); - Notification.Type notificationType = Notification.Type.valueOf(typeName); + List activeNotifications = getActiveNotifications(notificationManager.getActiveNotifications(), accountId, typeChannelId); - Intent summaryResultIntent = MainActivity.openNotificationIntent(context, accountId, notificationType); + int notificationCount = activeNotifications.size() + additionalNotifications.size(); - TaskStackBuilder summaryStackBuilder = TaskStackBuilder.create(context); - summaryStackBuilder.addParentStack(MainActivity.class); - summaryStackBuilder.addNextIntent(summaryResultIntent); + String title = context.getResources().getQuantityString(R.plurals.notification_title_summary, notificationCount, notificationCount); + String text = joinNames(context, activeNotifications, additionalNotifications); - PendingIntent summaryResultPendingIntent = summaryStackBuilder.getPendingIntent((int) (notificationId + account.getId() * 10000), - pendingIntentFlags(false)); + NotificationCompat.Builder summaryBuilder = new NotificationCompat.Builder(context, typeChannelId) + .setSmallIcon(R.drawable.ic_notify) + .setContentIntent(summaryResultPendingIntent) + .setColor(context.getColor(R.color.notification_color)) + .setAutoCancel(true) + .setShortcutId(Long.toString(account.getId())) + .setContentTitle(title) + .setContentText(text) + .setSubText(account.getFullName()) + .setVisibility(NotificationCompat.VISIBILITY_PRIVATE) + .setCategory(NotificationCompat.CATEGORY_SOCIAL) + .setGroup(typeChannelId) + .setGroupSummary(true) + .setGroupAlertBehavior(NotificationCompat.GROUP_ALERT_SUMMARY) + ; + + setSoundVibrationLight(account, summaryBuilder); + + String summaryTag = GROUP_SUMMARY_TAG + "." + typeChannelId; + notificationManager.notify(summaryTag, accountId, summaryBuilder.build()); + } - String title = context.getResources().getQuantityString(R.plurals.notification_title_summary, members.size(), members.size()); - String text = joinNames(context, members); + private static List getActiveNotifications(StatusBarNotification[] allNotifications, int accountId, String typeChannelId) { + // Return all active notifications, ignoring notifications that: + // - belong to a different account + // - belong to a different type + // - are summary notifications + List activeNotificationsForType = new ArrayList<>(); + for (StatusBarNotification sn : allNotifications) { + if (sn.getId() != accountId) + continue; - NotificationCompat.Builder summaryBuilder = new NotificationCompat.Builder(context, channelId) - .setSmallIcon(R.drawable.ic_notify) - .setContentIntent(summaryResultPendingIntent) - .setColor(context.getColor(R.color.notification_color)) - .setAutoCancel(true) - .setShortcutId(Long.toString(account.getId())) - .setDefaults(0) // So it doesn't ring twice, notify only in Target callback - .setContentTitle(title) - .setContentText(text) - .setSubText(account.getFullName()) - .setVisibility(NotificationCompat.VISIBILITY_PRIVATE) - .setCategory(NotificationCompat.CATEGORY_SOCIAL) - .setOnlyAlertOnce(true) - .setGroup(channelId) - .setGroupSummary(true); + String channelId = sn.getNotification().getGroup(); - setSoundVibrationLight(account, summaryBuilder); + if (!channelId.equals(typeChannelId)) + continue; - // TODO: Use the batch notification API available in NotificationManagerCompat - // 1.11 and up (https://developer.android.com/jetpack/androidx/releases/core#1.11.0-alpha01) - // when it is released. - notificationManager.notify(summaryTag, accountId, summaryBuilder.build()); + String summaryTag = GROUP_SUMMARY_TAG + "." + channelId; + if (summaryTag.equals(sn.getTag())) + continue; - // Android will rate limit / drop notifications if they're posted too - // quickly. There is no indication to the user that this happened. - // See https://github.com/tuskyapp/Tusky/pull/3626#discussion_r1192963664 - try { Thread.sleep(1000); } catch (InterruptedException ignored) { } + activeNotificationsForType.add(sn); } - } + return activeNotificationsForType; + } private static NotificationCompat.Builder newAndroidNotification(Context context, Notification body, AccountEntity account) { @@ -747,29 +716,41 @@ private static void setSoundVibrationLight(AccountEntity account, NotificationCo } } - private static String wrapItemAt(StatusBarNotification notification) { - return StringUtils.unicodeWrap(notification.getNotification().extras.getString(EXTRA_ACCOUNT_NAME));//getAccount().getName()); + private static String joinNames(Context context, List notifications1, List notifications2) { + List names = new ArrayList<>(notifications1.size() + notifications2.size()); + + for (StatusBarNotification notification: notifications1) { + names.add(notification.getNotification().extras.getString(EXTRA_ACCOUNT_NAME)); + } + + for (Notification notification : notifications2) { + names.add(notification.getAccount().getName()); + } + + return joinNames(context, names); } @Nullable - private static String joinNames(Context context, List notifications) { - if (notifications.size() > 3) { - int length = notifications.size(); - //notifications.get(0).getNotification().extras.getString(EXTRA_ACCOUNT_NAME); - return String.format(context.getString(R.string.notification_summary_large), - wrapItemAt(notifications.get(length - 1)), - wrapItemAt(notifications.get(length - 2)), - wrapItemAt(notifications.get(length - 3)), - length - 3); - } else if (notifications.size() == 3) { - return String.format(context.getString(R.string.notification_summary_medium), - wrapItemAt(notifications.get(2)), - wrapItemAt(notifications.get(1)), - wrapItemAt(notifications.get(0))); - } else if (notifications.size() == 2) { - return String.format(context.getString(R.string.notification_summary_small), - wrapItemAt(notifications.get(1)), - wrapItemAt(notifications.get(0))); + private static String joinNames(Context context, List names) { + if (names.size() > 3) { + int length = names.size(); + return context.getString(R.string.notification_summary_large, + StringUtils.unicodeWrap(names.get(length - 1)), + StringUtils.unicodeWrap(names.get(length - 2)), + StringUtils.unicodeWrap(names.get(length - 3)), + length - 3 + ); + } else if (names.size() == 3) { + return context.getString(R.string.notification_summary_medium, + StringUtils.unicodeWrap(names.get(2)), + StringUtils.unicodeWrap(names.get(1)), + StringUtils.unicodeWrap(names.get(0)) + ); + } else if (names.size() == 2) { + return context.getString(R.string.notification_summary_small, + StringUtils.unicodeWrap(names.get(1)), + StringUtils.unicodeWrap(names.get(0)) + ); } return null; @@ -780,23 +761,17 @@ private static String titleForType(Context context, Notification notification, A String accountName = StringUtils.unicodeWrap(notification.getAccount().getName()); switch (notification.getType()) { case MENTION: - return String.format(context.getString(R.string.notification_mention_format), - accountName); + return context.getString(R.string.notification_mention_format, accountName); case STATUS: - return String.format(context.getString(R.string.notification_subscription_format), - accountName); + return context.getString(R.string.notification_subscription_format, accountName); case FOLLOW: - return String.format(context.getString(R.string.notification_follow_format), - accountName); + return context.getString(R.string.notification_follow_format, accountName); case FOLLOW_REQUEST: - return String.format(context.getString(R.string.notification_follow_request_format), - accountName); + return context.getString(R.string.notification_follow_request_format, accountName); case FAVOURITE: - return String.format(context.getString(R.string.notification_favourite_format), - accountName); + return context.getString(R.string.notification_favourite_format, accountName); case REBLOG: - return String.format(context.getString(R.string.notification_reblog_format), - accountName); + return context.getString(R.string.notification_reblog_format, accountName); case POLL: if(notification.getStatus().getAccount().getId().equals(account.getAccountId())) { return context.getString(R.string.poll_ended_created); @@ -804,9 +779,9 @@ private static String titleForType(Context context, Notification notification, A return context.getString(R.string.poll_ended_voted); } case SIGN_UP: - return String.format(context.getString(R.string.notification_sign_up_format), accountName); + return context.getString(R.string.notification_sign_up_format, accountName); case UPDATE: - return String.format(context.getString(R.string.notification_update_format), accountName); + return context.getString(R.string.notification_update_format, accountName); case REPORT: return context.getString(R.string.notification_report_format, account.getDomain()); } diff --git a/app/src/test/java/com/keylesspalace/tusky/MainActivityTest.kt b/app/src/test/java/com/keylesspalace/tusky/MainActivityTest.kt index 94247c2f5f..458c261ef5 100644 --- a/app/src/test/java/com/keylesspalace/tusky/MainActivityTest.kt +++ b/app/src/test/java/com/keylesspalace/tusky/MainActivityTest.kt @@ -116,8 +116,7 @@ class MainActivityTest { status = null, report = null ), - accountEntity, - true + accountEntity ) notificationManager.notify("id", 1, notification) }