Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove notifications when messages are read, on Android #341

Closed
gnprice opened this issue Oct 26, 2023 · 5 comments · Fixed by #878
Closed

Remove notifications when messages are read, on Android #341

gnprice opened this issue Oct 26, 2023 · 5 comments · Fixed by #878
Assignees
Labels
a-Android Issues specific to Android, or requiring Android-specific work a-notifications beta feedback Things beta users have specifically asked for

Comments

@gnprice
Copy link
Member

gnprice commented Oct 26, 2023

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 (zulip/zulip-mobile#3119), so that will be tracked separately and happen later.

@gnprice gnprice added a-Android Issues specific to Android, or requiring Android-specific work a-notifications labels Oct 26, 2023
@gnprice gnprice added this to the Launch milestone Oct 26, 2023
@gnprice gnprice added the beta feedback Things beta users have specifically asked for label Mar 1, 2024
@gnprice gnprice modified the milestones: Launch, Beta 2 Mar 1, 2024
@gnprice
Copy link
Member Author

gnprice commented Mar 1, 2024

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

This comment was marked as off-topic.

@gnprice

This comment was marked as off-topic.

@gnprice gnprice self-assigned this Mar 21, 2024
@gnprice
Copy link
Member Author

gnprice commented Mar 22, 2024

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:

  • Find some other way of implementing the feature which works with the existing package:flutter_local_notifications API. There may not exist a clean way to do this, though we can likely find some kind of hack.
  • Patch package:flutter_local_notifications to provide those extras. Do so locally at first, and try to upstream it.
  • Replace flutter_local_notifications with a thin API binding using Pigeon #351.

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
Copy link
Member Author

gnprice commented Aug 2, 2024

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.

@gnprice gnprice removed their assignment Aug 2, 2024
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 11, 2024
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 11, 2024
Fixes zulip#341
Co-authored-by: Greg Price <greg@zulip.com>
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 11, 2024
Fixes: zulip#341
Co-authored-by: Greg Price <greg@zulip.com>
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 11, 2024
Fixes: zulip#341
Co-authored-by: Greg Price <greg@zulip.com>
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 15, 2024
Fixes: zulip#341
Co-authored-by: Greg Price <greg@zulip.com>
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 15, 2024
Fixes: zulip#341
Co-authored-by: Greg Price <greg@zulip.com>
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 16, 2024
Fixes: zulip#341
Co-authored-by: Greg Price <greg@zulip.com>
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 16, 2024
gnprice added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 20, 2024
rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this issue Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Android Issues specific to Android, or requiring Android-specific work a-notifications beta feedback Things beta users have specifically asked for
Projects
Status: Done
3 participants