zulip / zulip-flutter

Upcoming Zulip mobile apps for Android and iOS, using Flutter
Apache License 2.0
178 stars 164 forks source link

Remove notifications when messages are read, on Android #341

Closed gnprice closed 2 months ago

gnprice commented 1 year ago

We have this feature in zulip-mobile, and should match it here.

We should also eventually add this feature on iOS. We don't yet have it there in zulip-mobile (https://github.com/zulip/zulip-mobile/issues/3119), so that will be tracked separately and happen later.

gnprice commented 7 months ago

Mentioned in feedback here: https://chat.zulip.org/#narrow/stream/48-mobile/topic/notifications.20from.20cold.20start/near/1749654

(Which was the first day after Android notifications started working generally.)

SharmaDhruv2511 commented 7 months ago

I would like to contribute to it @gnprice

gnprice commented 7 months ago

@SharmaDhruv2511 Please see our guide here: https://github.com/zulip/zulip-flutter#picking-an-issue-to-work-on

gnprice commented 7 months ago

I picked this up today, and I've gotten the feature working successfully in a draft branch. There's still a fair amount of cleanup needed to turn that into something mergeable, though.

The way this feature works in zulip-mobile (which has worked quite well) is that on each notification we include in the "extras" a key lastZulipMessageId. Then when an RemoveFcmMessage comes in from the server, bearing a list of Zulip message IDs that have been read (and previously caused a notification), we look through the existing notifications in the notification UI to see which ones correspond to those Zulip message IDs and so should be removed.

To date, the way we've been interacting with the Android SDK for controlling notifications is with package:flutter_local_notifications, which has a Flutter plugin for this purpose. As discussed at #351, though, that package isn't entirely satisfactory. And implementing this feature turns up another case of that: the plugin provides a version of the getActiveNotifications method which tells us about the existing notifications in the notification UI… but it doesn't give us a way to see the extras on the notifications. So our choices are:

I decided this was a good prompt for trying out Pigeon and doing #351. It worked. Pigeon has some rough edges, but it definitely feels a lot more pleasant than trying to write by hand all the serialization, deserialization, and dispatch logic for talking between Dart and Kotlin.

My current draft branch doesn't do all of #351, in that it doesn't replace some of the other methods which aren't involved in this feature. But it already replaces by far the most complex of those methods, for showing a notification (so that we can set that value in "extras" in the first place). So I might just do #351 completely along the way to this issue.

gnprice commented 2 months ago

I picked this up today, and I've gotten the feature working successfully in a draft branch. There's still a fair amount of cleanup needed to turn that into something mergeable, though.

Parts of that draft branch turned into #592, toward #351 switching to Pigeon.

I've now taken the remainder of that draft branch, rebased it atop all the other changes that have happened in our notifications code, and sent it as a draft PR #864. So @rajveermalviya with that branch you now have all the work I've done toward this issue, and you can go from there without duplicating work.