Skip to content

Commit

Permalink
Properly summarize all notifications (#4848)
Browse files Browse the repository at this point in the history 
Do summary notifications like the Api defines it:
* Schedule and summarize without delay (in order for summerization to
work)
* Always have a summary notification: simplify code with this and make
more reliable
* Do not care about single notification count (the system already does
that as well)
* **Bugfix: Schedule summary first: This avoids a rate limit problem
that (then) not groups at all**

Testing this is probably the most difficult part.
For example I couldn't get any notification to ring with older Api
versions in the debugger. (Same as for current develop)
However one hack to always get notifications: Fix "minId" in
"fetchNewNotifications()" to a somewhat older value.


Next possible step: Have only one summary notification at all (for all
channels/notification types). You can still configure single channels
differently.
Or: For very many notifications: Only use a true summary one (something
like "you have 28 favorites and 7 boosts").

Generally: The notification timeline must be improved now. Because that
must be the go-to solution for any large number of notifications. It
must be easy to read. E. g. with grouping per post.
  • Loading branch information
@Lakoja
Lakoja authored Jan 12, 2025
1 parent 5741d57 commit 6cbcf3e
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 196 deletions.
96 changes: 32 additions & 64 deletions ...c/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationFetcher.kt
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package com.keylesspalace.tusky.components.systemnotifications

import android.Manifest
import android.app.NotificationManager
import android.content.Context
import android.content.pm.PackageManager
import android.util.Log
import androidx.annotation.WorkerThread
import androidx.core.app.ActivityCompat
import androidx.core.app.NotificationManagerCompat
import com.keylesspalace.tusky.appstore.EventHub
import com.keylesspalace.tusky.appstore.NewNotificationsEvent
import com.keylesspalace.tusky.components.systemnotifications.NotificationHelper.filterNotification
Expand All @@ -16,9 +20,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

/** Models next/prev links from the "Links" header in an API response */
data class Links(val next: String?, val prev: String?) {
Expand Down Expand Up @@ -53,87 +54,61 @@ class NotificationFetcher @Inject constructor(
private val eventHub: EventHub
) {
suspend fun fetchAndShow() {
if (ActivityCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) != PackageManager.PERMISSION_GRANTED) {
return
}

for (account in accountManager.getAllAccountsOrderedByActive()) {
if (account.notificationsEnabled) {
try {
val notificationManager = context.getSystemService(
Context.NOTIFICATION_SERVICE
) as NotificationManager

val notificationManagerCompat = NotificationManagerCompat.from(context)

// Create sorted list of new notifications
val notifications = fetchNewNotifications(account)
.filter { filterNotification(notificationManager, account, it) }
.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)
)
.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.
val newNotifications = ArrayList<NotificationManagerCompat.NotificationWithIdAndTag>()

notificationsByType.forEach { notificationsGroup ->
notificationsGroup.value.forEach { notification ->
val androidNotification = NotificationHelper.make(
val notificationsByType: Map<Notification.Type, List<Notification>> = notifications.groupBy { it.type }
notificationsByType.forEach { notificationsGroup: Map.Entry<Notification.Type, List<Notification>> ->
// NOTE Enqueue the summary first: Needed to avoid rate limit problems:
// ie. single notification is enqueued but that later summary one is filtered and thus no grouping
// takes place.
newNotifications.add(
NotificationHelper.makeSummaryNotification(
context,
notificationManager,
notification,
account,
notificationsGroup.value.size == 1
)
notificationManager.notify(
notification.id,
account.id.toInt(),
androidNotification
notificationsGroup.key,
notificationsGroup.value
)
)

// 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)
notificationsGroup.value.forEach { notification ->
newNotifications.add(
NotificationHelper.make(
context,
notificationManager,
notification,
account
)
)
}
}

NotificationHelper.updateSummaryNotifications(
context,
notificationManager,
account
)
// NOTE having multiple summary notifications this here should still collapse them in only one occurrence
notificationManagerCompat.notify(newNotifications)

accountManager.saveAccount(account)
} catch (e: Exception) {
Expand Down Expand Up @@ -246,12 +221,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
}
}
Loading

0 comments on commit 6cbcf3e

Please sign in to comment.