Skip to content
This repository was archived by the owner on May 23, 2025. It is now read-only.

Improve push notifications #4896

Merged
merged 12 commits into from
Feb 20, 2025
Merged

Conversation

Lakoja
Copy link
Collaborator

@Lakoja Lakoja commented Jan 26, 2025

Besides the refactoring these improvements:

  • Track last push distributor and reset settings and subscription on any incompatible change (ie. uninstall)
  • Only update (push) notification settings on server if needed
  • Allow to only fetch notifications for one account (the one for which a push message was received)

This is (also) the revival of #3642

It's not really well tested so far. (Ie. with two or more accounts or two or more push providers.)

@Lakoja Lakoja mentioned this pull request Jan 26, 2025
Copy link
Collaborator

@connyduck connyduck left a comment

Choose a reason for hiding this comment

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

Where to show the current push distributor information?

I think we need a new Account Preference for that

Can we have a rate-limit? At least on sounds? If you are a popular account only Android can (and will) silence Tusky.

Yeah but if you are a popular account you probably disable notifications anyway. Or at least don't have sound on.
I think it would be ideal if it would alert only once per channel, surely there must be a way in Android to do that?


// reset all accounts
accountManager.accounts.forEach {
// TODO this would like to call reset, but that is suspending; why would DB access be suspending?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course db access is suspending, it is very slow compared to other operations and should run on a background thread.
You can inject the Application Scope and use that to launch a coroutine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If this runs in the background there is a great (?) chance that the real initialization (with setupNotifications()) which comes slightly afterwards will not find updated account data. Ie. with push fields reset when there is a push provider change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that could be a race condition. The solution probably is to have only one place where all initialization is done, so that the different steps can be executed in the correct order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is now reordered.

UnifiedPush.unregisterApp(context, account.id.toString())
}

private fun buildSubscriptionData(account: AccountEntity): Map<String, Boolean> =
fun fetchNotificationsOnPushMessage(account: AccountEntity) {
// TODO should there be a rate limit here? Ie. we could be silent (can we?) for another notification in a short timeframe.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah some kind of throttling would be good so we don't spam the server with requests on a lot of pushes. But doesn't the WorkManager do that automatically? Or maybe it can be configured to do so.

Comment on lines 791 to 792
// Otherwise the pull worker is still present after starting push notifications (and blocks the one time worker there).
disablePullNotifications()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be really good for reliability if we had pull notifications on all the time? Surely there is a way around the one time worker conflict? Usind a different worker or something.

Copy link
Collaborator Author

@Lakoja Lakoja Jan 29, 2025

Choose a reason for hiding this comment

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

Maybe. :-)
Pull notifications are probably at least as unreliable as push notifications. E. g. with deep sleep no (background) worker ever is executed or only very seldomly. Also I have the feeling that Tusky often not recovers well enough when the device is again active.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use WorkManager, that should be as reliable as it gets. If we use it correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pull notifications (worker) is now a fallback anytime.

@Lakoja
Copy link
Collaborator Author

Lakoja commented Jan 29, 2025

Where to show the current push distributor information?

I think we need a new Account Preference for that

What would it switch? A preference only for displaying something?

Of course there might be potential for that: e. g. reset things and/or choose a provider. But that would be more complex and a later step.
For the moment I think I will remove the display changes in the PR (so it stays with "no information for users").

@connyduck
Copy link
Collaborator

What would it switch? A preference only for displaying something?

It would open a new screen or dialog with the info

@p1gp1g
Copy link

p1gp1g commented Feb 5, 2025

Starting with version 4.4.0, Mastodon will support the standard webpush: mastodon/mastodon#33528, so you can get the content of the event from the push messages directly, see the PR for moshidon: LucasGGamerM/moshidon#588

@connyduck
Copy link
Collaborator

@Lakoja Whats your plan for this? Will you add more improvements or are you looking to get it merged like this?

@Lakoja
Copy link
Collaborator Author

Lakoja commented Feb 17, 2025

@Lakoja Whats your plan for this? Will you add more improvements or are you looking to get it merged like this?

Should be good to go now.

Any other improvements can be a followup.

@@ -407,6 +407,8 @@
<string name="about_device_info">%1$s %2$s\nAndroid version: %3$s\nSDK version: %4$d</string>
<string name="about_account_info_title">Your account</string>
<string name="about_account_info">\@%1$s\@%2$s\nVersion: %3$s</string>
<string name="about_push_notification_info_title">Push notifications</string>
<string name="about_push_notification_info">Provider: %1$s</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are unused

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

@connyduck connyduck merged commit 6450af6 into tuskyapp:develop Feb 20, 2025
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants