Improve notifications fetching (#1930)

* Improve notifications fetching

 - Only fetch notifications up to the latest fetched one
 - Use timeline markers to avoid showing already seen notifications

* Apply some of the suggestions

app/src/main/java/com/keylesspalace/tusky/MainActivity.kt

@@ -450,7 +450,7 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje
                     }
             )
 
-            if(addSearchButton) {
+            if (addSearchButton) {
                 mainDrawer.addItemsAtPosition(4,
                         primaryDrawerItem {
                             nameRes = R.string.action_search
@@ -482,7 +482,7 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje
 
     private fun setupTabs(selectNotificationTab: Boolean) {
 
-        val activeTabLayout = if(preferences.getString("mainNavPosition", "top") == "bottom") {
+        val activeTabLayout = if (preferences.getString("mainNavPosition", "top") == "bottom") {
             val actionBarSize = ThemeUtils.getDimension(this, R.attr.actionBarSize)
             val fabMargin = resources.getDimensionPixelSize(R.dimen.fabMargin)
             (composeButton.layoutParams as CoordinatorLayout.LayoutParams).bottomMargin = actionBarSize + fabMargin
@@ -646,10 +646,11 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, HasAndroidInje
                 .transform(
                         RoundedCorners(resources.getDimensionPixelSize(R.dimen.avatar_radius_36dp))
                 )
-                .into(object : CustomTarget<Drawable>(){
+                .into(object : CustomTarget<Drawable>() {
                     override fun onResourceReady(resource: Drawable, transition: Transition<in Drawable>?) {
                         mainToolbar.navigationIcon = resource
                     }
+
                     override fun onLoadCleared(placeholder: Drawable?) {
                         mainToolbar.navigationIcon = placeholder
                     }

app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationFetcher.kt

@@ -0,0 +1,84 @@
+package com.keylesspalace.tusky.components.notifications
+
+import android.util.Log
+import com.keylesspalace.tusky.db.AccountEntity
+import com.keylesspalace.tusky.db.AccountManager
+import com.keylesspalace.tusky.entity.Marker
+import com.keylesspalace.tusky.entity.Notification
+import com.keylesspalace.tusky.network.MastodonApi
+import com.keylesspalace.tusky.util.isLessThan
+import javax.inject.Inject
+
+class NotificationFetcher @Inject constructor(
+        private val mastodonApi: MastodonApi,
+        private val accountManager: AccountManager,
+        private val notifier: Notifier
+) {
+    fun fetchAndShow() {
+        for (account in accountManager.getAllAccountsOrderedByActive()) {
+            if (account.notificationsEnabled) {
+                try {
+                    val notifications = fetchNotifications(account)
+                    notifications.forEachIndexed { index, notification ->
+                        notifier.show(notification, account, index == 0)
+                    }
+                    accountManager.saveAccount(account)
+                } catch (e: Exception) {
+                    Log.w(TAG, "Error while fetching notifications", e)
+                }
+            }
+        }
+    }
+
+    private fun fetchNotifications(account: AccountEntity): MutableList<Notification> {
+        val authHeader = String.format("Bearer %s", account.accessToken)
+        // We fetch marker to not load/show notifications which user has already seen
+        val marker = fetchMarker(authHeader, account)
+        if (marker != null && account.lastNotificationId.isLessThan(marker.lastReadId)) {
+            account.lastNotificationId = marker.lastReadId
+        }
+        Log.d(TAG, "getting Notifications for " + account.fullName)
+        val notifications = mastodonApi.notificationsWithAuth(
+                authHeader,
+                account.domain,
+                account.lastNotificationId,
+                true,
+                Notification.Type.asStringList
+        ).blockingGet()
+
+        val newId = account.lastNotificationId
+        var newestId = ""
+        val result = mutableListOf<Notification>()
+        for (notification in notifications.reversed()) {
+            val currentId = notification.id
+            if (newestId.isLessThan(currentId)) {
+                newestId = currentId
+                account.lastNotificationId = currentId
+            }
+            if (newId.isLessThan(currentId)) {
+                result.add(notification)
+            }
+        }
+        return result
+    }
+
+    private fun fetchMarker(authHeader: String, account: AccountEntity): Marker? {
+        return try {
+            val allMarkers = mastodonApi.markersWithAuth(
+                    authHeader,
+                    account.domain,
+                    listOf("notifications")
+            ).blockingGet()
+            val notificationMarker = allMarkers["notifications"]
+            Log.d(TAG, "Fetched marker: $notificationMarker")
+            notificationMarker
+        } catch (e: Exception) {
+            Log.e(TAG, "Failed to fetch marker", e)
+            null
+        }
+    }
+
+    companion object {
+        const val TAG = "NotificationFetcher"
+    }
+}

app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationWorker.kt

@@ -16,83 +16,35 @@
 package com.keylesspalace.tusky.components.notifications
 
 import android.content.Context
-import android.util.Log
 import androidx.work.ListenableWorker
 import androidx.work.Worker
 import androidx.work.WorkerFactory
 import androidx.work.WorkerParameters
-import com.keylesspalace.tusky.db.AccountEntity
-import com.keylesspalace.tusky.db.AccountManager
-import com.keylesspalace.tusky.entity.Notification
-import com.keylesspalace.tusky.network.MastodonApi
-import com.keylesspalace.tusky.util.isLessThan
-import java.io.IOException
 import javax.inject.Inject
 
 class NotificationWorker(
-        private val context: Context,
+        context: Context,
         params: WorkerParameters,
-        private val mastodonApi: MastodonApi,
-        private val accountManager: AccountManager
+        private val notificationsFetcher: NotificationFetcher
 ) : Worker(context, params) {
 
     override fun doWork(): Result {
-        val accountList = accountManager.getAllAccountsOrderedByActive()
-        for (account in accountList) {
-            if (account.notificationsEnabled) {
-                try {
-                    Log.d(TAG, "getting Notifications for " + account.fullName)
-                    // don't care about withMuted because they are always silently ignored
-                    val notificationsResponse = mastodonApi.notificationsWithAuth(
-                            String.format("Bearer %s", account.accessToken),
-                            account.domain, true,
-                            Notification.Type.asStringList).execute()
-                    val notifications = notificationsResponse.body()
-                    if (notificationsResponse.isSuccessful && notifications != null) {
-                        onNotificationsReceived(account, notifications)
-                    } else {
-                        Log.w(TAG, "error receiving notifications")
-                    }
-                } catch (e: IOException) {
-                    Log.w(TAG, "error receiving notifications", e)
-                }
-            }
-        }
+        notificationsFetcher.fetchAndShow()
         return Result.success()
     }
-
-    private fun onNotificationsReceived(account: AccountEntity, notificationList: List<Notification>) {
-        val newId = account.lastNotificationId
-        var newestId = ""
-        var isFirstOfBatch = true
-        notificationList.reversed().forEach { notification ->
-            val currentId = notification.id
-            if (newestId.isLessThan(currentId)) {
-                newestId = currentId
-            }
-            if (newId.isLessThan(currentId)) {
-                NotificationHelper.make(context, notification, account, isFirstOfBatch)
-                isFirstOfBatch = false
-            }
-        }
-        account.lastNotificationId = newestId
-        accountManager.saveAccount(account)
-    }
-
-    companion object {
-        private const val TAG = "NotificationWorker"
-    }
-
 }
 
 class NotificationWorkerFactory @Inject constructor(
-        val api: MastodonApi,
-        val accountManager: AccountManager
-): WorkerFactory() {
-
-    override fun createWorker(appContext: Context, workerClassName: String, workerParameters: WorkerParameters): ListenableWorker? {
-        if(workerClassName == NotificationWorker::class.java.name) {
-            return NotificationWorker(appContext, workerParameters, api, accountManager)
+        private val notificationsFetcher: NotificationFetcher
+) : WorkerFactory() {
+
+    override fun createWorker(
+            appContext: Context,
+            workerClassName: String,
+            workerParameters: WorkerParameters
+    ): ListenableWorker? {
+        if (workerClassName == NotificationWorker::class.java.name) {
+            return NotificationWorker(appContext, workerParameters, notificationsFetcher)
         }
         return null
     }

app/src/main/java/com/keylesspalace/tusky/components/notifications/Notifier.kt

@@ -0,0 +1,20 @@
+package com.keylesspalace.tusky.components.notifications
+
+import android.content.Context
+import com.keylesspalace.tusky.db.AccountEntity
+import com.keylesspalace.tusky.entity.Notification
+
+/**
+ * Shows notifications.
+ */
+interface Notifier {
+    fun show(notification: Notification, account: AccountEntity, isFirstInBatch: Boolean)
+}
+
+class SystemNotifier(
+        private val context: Context
+) : Notifier {
+    override fun show(notification: Notification, account: AccountEntity, isFirstInBatch: Boolean) {
+        NotificationHelper.make(context, notification, account, isFirstInBatch)
+    }
+}

app/src/main/java/com/keylesspalace/tusky/di/AppModule.kt

@@ -25,6 +25,8 @@ import androidx.room.Room
 import com.keylesspalace.tusky.TuskyApplication
 import com.keylesspalace.tusky.appstore.EventHub
 import com.keylesspalace.tusky.appstore.EventHubImpl
+import com.keylesspalace.tusky.components.notifications.Notifier
+import com.keylesspalace.tusky.components.notifications.SystemNotifier
 import com.keylesspalace.tusky.db.AppDatabase
 import com.keylesspalace.tusky.network.MastodonApi
 import com.keylesspalace.tusky.network.TimelineCases
@@ -78,7 +80,11 @@ class AppModule {
                         AppDatabase.MIGRATION_13_14, AppDatabase.MIGRATION_14_15, AppDatabase.MIGRATION_15_16,
                         AppDatabase.MIGRATION_16_17, AppDatabase.MIGRATION_17_18, AppDatabase.MIGRATION_18_19,
                         AppDatabase.MIGRATION_19_20, AppDatabase.MIGRATION_20_21, AppDatabase.MIGRATION_21_22,
-						AppDatabase.MIGRATION_22_23, AppDatabase.MIGRATION_23_24, AppDatabase.MIGRATION_24_25)
+                        AppDatabase.MIGRATION_22_23, AppDatabase.MIGRATION_23_24, AppDatabase.MIGRATION_24_25)
                 .build()
     }
+
+    @Provides
+    @Singleton
+    fun notifier(context: Context): Notifier = SystemNotifier(context)
 }

app/src/main/java/com/keylesspalace/tusky/entity/Marker.kt

@@ -0,0 +1,15 @@
+package com.keylesspalace.tusky.entity
+
+import com.google.gson.annotations.SerializedName
+import java.util.*
+
+/**
+ * API type for saving the scroll position of a timeline.
+ */
+data class Marker(
+        @SerializedName("last_read_id")
+        val lastReadId: String,
+        val version: Int,
+        @SerializedName("updated_at")
+        val updatedAt: Date
+)

app/src/main/java/com/keylesspalace/tusky/network/MastodonApi.kt

@@ -105,10 +105,18 @@ interface MastodonApi {
             @Query("with_muted") withMuted: Boolean?
     ): Call<List<Notification>>
 
+    @GET("api/v1/markers")
+    fun markersWithAuth(
+            @Header("Authorization") auth: String,
+            @Header(DOMAIN_HEADER) domain: String,
+            @Query("timeline[]") timelines: List<String>
+    ): Single<Map<String, Marker>>
+
     @GET("api/v1/notifications")
     fun notificationsWithAuth(
             @Header("Authorization") auth: String,
             @Header(DOMAIN_HEADER) domain: String,
+            @Query("since_id") sinceId: String?,
             @Query("with_muted") withMuted: Boolean?,
             @Query("include_types[]") includeTypes: List<String>?
     ): Call<List<Notification>>