Skip to content

Commit

Permalink
fix: Improve push and pull notification reliability (#880)
Browse files Browse the repository at this point in the history 
Clean up the notification handling code and fix a lot of bugs, hopefully
without introducing new ones in the process.

Specific bugs discovered and fixed:

- The code that tried to sync notification filtering state between the
server and Pachli could fail, leaving things in an inconsistent state,
resulting in dropped notifications. Remove that code, do filtering
client-side.

- Logging out of an account would disable push notifications for all
accounts.

- If any account did not support push notifications then push
notifications were disabled for all accounts.

- If any account did not support push notifications the user was
prompted to log out of all accounts. Drop that entirely.

- The UnifiedPush library could get to a state where configuring the
notification mechanism would silently fail,

The preferences UI now has a section for notifications, showing:

- The Unified Push distributor in use (if any)
- A mechanism to change the distributor
- Per-account configuration and notification fetch details
- Battery optimisation state

General changes:

- Update to UnifiedPush library 2.4.0.

- NotificationFetcher.fetchAndShow() can now fetch a single account's
notifications, or all accounts, depending on data passed to the worker.

- Use ApiResult for `push/subscription` responses.

- Drop the "needs migration" terminology for the more specific "has push
scope", to make it clear what the issue with the account is.
  • Loading branch information
@nikclayton
nikclayton authored Aug 18, 2024
1 parent fcbcb40 commit 8b9fe6d
Show file tree
Hide file tree
Showing 54 changed files with 772 additions and 580 deletions.
78 changes: 28 additions & 50 deletions app/lint-baseline.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="17"
line="18"
column="9"/>
</issue>

Expand All @@ -47,7 +47,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="9"/>
</issue>

Expand All @@ -58,7 +58,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="25"
line="26"
column="9"/>
</issue>

Expand All @@ -69,7 +69,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="26"
line="27"
column="9"/>
</issue>

Expand Down Expand Up @@ -694,25 +694,14 @@
column="86"/>
</issue>

<issue
id="Typos"
message="Repeated word &quot;tek&quot; in message: possible typo"
errorLine1=" &lt;string name=&quot;dialog_push_notification_migration_other_accounts&quot;>Pachli\&apos;ye bildirim aboneliği izni vermek için mevcut hesabınıza yeniden giriş yaptınız. Ancak, yine de bu şekilde geçirilmemiş başka hesaplarınız var. UnifiedPush bildirimleri desteğini etkinleştirmek için bunlara geçin ve tek tek yeniden giriş yapın.&lt;/string>"
errorLine2=" ^">
<location
file="src/main/res/values-tr/strings.xml"
line="497"
column="294"/>
</issue>

<issue
id="Typos"
message="&quot;Media&quot; is a common misspelling; did you mean &quot;Medier&quot;?"
errorLine1=" &lt;string name=&quot;hint_media_description_missing&quot;>Media bør ha en beskrivelse.&lt;/string>"
errorLine2=" ^">
<location
file="src/main/res/values-nb-rNO/strings.xml"
line="519"
line="514"
column="51"/>
</issue>

Expand All @@ -723,7 +712,7 @@
errorLine2=" ^">
<location
file="src/main/res/values-nb-rNO/strings.xml"
line="649"
line="644"
column="43"/>
</issue>

Expand Down Expand Up @@ -756,7 +745,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="99"
line="101"
column="5"/>
</issue>

Expand Down Expand Up @@ -800,7 +789,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="629"
line="626"
column="5"/>
</issue>

Expand All @@ -811,7 +800,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -822,7 +811,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -833,7 +822,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -844,7 +833,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -855,7 +844,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -866,7 +855,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -877,7 +866,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -888,7 +877,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -899,7 +888,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="24"
line="25"
column="31"/>
</issue>

Expand All @@ -910,7 +899,7 @@
errorLine2=" ~~~~~">
<location
file="src/main/AndroidManifest.xml"
line="18"
line="19"
column="30"/>
</issue>

Expand Down Expand Up @@ -1207,7 +1196,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="19"
line="21"
column="13"/>
</issue>

Expand All @@ -1218,7 +1207,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="22"
line="24"
column="13"/>
</issue>

Expand All @@ -1229,7 +1218,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="32"
line="34"
column="13"/>
</issue>

Expand All @@ -1240,7 +1229,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="74"
line="76"
column="13"/>
</issue>

Expand All @@ -1251,7 +1240,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="80"
line="82"
column="13"/>
</issue>

Expand All @@ -1262,7 +1251,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="101"
line="103"
column="13"/>
</issue>

Expand All @@ -1273,7 +1262,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="109"
line="111"
column="13"/>
</issue>

Expand All @@ -1284,7 +1273,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="143"
line="145"
column="13"/>
</issue>

Expand Down Expand Up @@ -1409,17 +1398,6 @@
column="13"/>
</issue>

<issue
id="UnusedResources"
message="The resource `R.string.restart` appears to be unused"
errorLine1=" &lt;string name=&quot;restart&quot;>Restart&lt;/string>"
errorLine2=" ~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="426"
column="13"/>
</issue>

<issue
id="UnusedResources"
message="The resource `R.string.download_failed` appears to be unused"
Expand Down Expand Up @@ -1559,7 +1537,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="621"
line="618"
column="13"/>
</issue>

Expand All @@ -1570,7 +1548,7 @@
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/res/values/strings.xml"
line="647"
line="644"
column="13"/>
</issue>

Expand Down
12 changes: 1 addition & 11 deletions app/src/main/AndroidManifest.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
<uses-permission android:name="android.permission.READ_MEDIA_AUDIO" />
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="28"/>
<uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
<uses-permission android:name="android.permission.REQUEST_IGNORE_BATTERY_OPTIMIZATIONS" />

<application
android:name=".PachliApplication"
Expand Down Expand Up @@ -177,16 +178,6 @@
<action android:name="org.unifiedpush.android.connector.REGISTRATION_REFUSED"/>
</intent-filter>
</receiver>
<receiver
android:exported="true"
android:enabled="true"
android:name=".receiver.NotificationBlockStateBroadcastReceiver"
tools:ignore="ExportedReceiver">
<intent-filter>
<action android:name="android.app.action.NOTIFICATION_CHANNEL_BLOCK_STATE_CHANGED"/>
<action android:name="android.app.action.NOTIFICATION_CHANNEL_GROUP_BLOCK_STATE_CHANGED"/>
</intent-filter>
</receiver>

<service
android:name=".service.PachliTileService"
Expand Down Expand Up @@ -221,5 +212,4 @@
tools:node="remove" />

</application>

</manifest>
23 changes: 5 additions & 18 deletions app/src/main/java/app/pachli/MainActivity.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ import app.pachli.appstore.ProfileEditedEvent
import app.pachli.components.compose.ComposeActivity.Companion.canHandleMimeType
import app.pachli.components.notifications.androidNotificationsAreEnabled
import app.pachli.components.notifications.createNotificationChannelsForAccount
import app.pachli.components.notifications.disableAllNotifications
import app.pachli.components.notifications.enablePushNotificationsWithFallback
import app.pachli.components.notifications.showMigrationNoticeIfNecessary
import app.pachli.components.notifications.enableAllNotifications
import app.pachli.core.activity.AccountSelectionListener
import app.pachli.core.activity.BottomSheetActivity
import app.pachli.core.activity.PostLookupFallbackBehavior
Expand Down Expand Up @@ -1065,21 +1063,10 @@ class MainActivity : BottomSheetActivity(), ActionButtonActivity, MenuProvider {
accountManager.updateActiveAccount(me)
createNotificationChannelsForAccount(accountManager.activeAccount!!, this)

// Setup push notifications
showMigrationNoticeIfNecessary(
this,
binding.mainCoordinatorLayout,
binding.composeButton,
accountManager,
sharedPreferencesRepository,
)
if (androidNotificationsAreEnabled(this, accountManager)) {
lifecycleScope.launch {
enablePushNotificationsWithFallback(this@MainActivity, mastodonApi, accountManager)
}
} else {
disableAllNotifications(this, accountManager)
}
// Setup notifications
// TODO: Continue to call this, as it sets properties in NotificationConfig
androidNotificationsAreEnabled(this, accountManager)
lifecycleScope.launch { enableAllNotifications(this@MainActivity, mastodonApi, accountManager) }

updateProfiles()

Expand Down
16 changes: 13 additions & 3 deletions app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import app.pachli.core.network.model.Links
import app.pachli.core.network.model.Marker
import app.pachli.core.network.model.Notification
import app.pachli.core.network.retrofit.MastodonApi
import app.pachli.worker.NotificationWorker
import com.github.michaelbull.result.Err
import com.github.michaelbull.result.Ok
import dagger.hilt.android.qualifiers.ApplicationContext
Expand All @@ -52,9 +53,18 @@ class NotificationFetcher @Inject constructor(
private val accountManager: AccountManager,
@ApplicationContext private val context: Context,
) {
suspend fun fetchAndShow() {
suspend fun fetchAndShow(accountId: Long) {
Timber.d("NotificationFetcher.fetchAndShow() started")
for (account in accountManager.getAllAccountsOrderedByActive()) {

val accounts = buildList {
if (accountId == NotificationWorker.ALL_ACCOUNTS) {
addAll(accountManager.getAllAccountsOrderedByActive())
} else {
accountManager.getAccountById(accountId)?.let { add(it) }
}
}

for (account in accounts) {
Timber.d(
"Checking %s$, notificationsEnabled = %s",
account.fullName,
Expand All @@ -66,7 +76,7 @@ class NotificationFetcher @Inject constructor(

// Create sorted list of new notifications
val notifications = fetchNewNotifications(account)
.filter { filterNotification(notificationManager, account, it) }
.filter { filterNotification(notificationManager, account, it.type) }
.sortedWith(compareBy({ it.id.length }, { it.id })) // oldest notifications first
.toMutableList()

Expand Down
Loading

0 comments on commit 8b9fe6d

Please sign in to comment.