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

[iOS] Clear notifications when messages are read on desktop/web #3119

Open
gnprice opened this issue Nov 7, 2018 · 9 comments
Open

[iOS] Clear notifications when messages are read on desktop/web #3119

gnprice opened this issue Nov 7, 2018 · 9 comments

Comments

@gnprice
Copy link
Member

gnprice commented Nov 7, 2018

This is a twin of #2634 . That issue covers the remaining steps to get this feature to Android users. All the essential logic for that is in server release 1.9 plus mobile's master branch (and so the next release), and what's left is a matter of release management.

For iOS, the situation is a bit different. @borisyankov and I spent some time the other day looking through both our code, and Apple's APIs and docs, so we understand this all more concretely than before. To do this effectively, we'll need to make some changes on both the client and the server, though on the server side the core machinery we need is the same that we've already added in zulip/zulip#10094 (and the 1.9 release).

@gnprice gnprice added this to Visible Issues in Next via automation Nov 7, 2018
@gnprice
Copy link
Member Author

gnprice commented Nov 7, 2018

Here's some notes:

  • We don't currently have any client-side code of our own (as we do on Android) to manage notifications in the UI. Instead, they're generated 1:1 by the platform from APNs messages ("push notifications" in Apple's terms, but they use that term ambiguously for the resulting bits of UI too) as dictated by the Zulip server.
  • So, each Zulip message that causes a notification corresponds directly to a notification in the UI.
  • To cancel one of these, we'll want to have a bit of client-side code (in Objective-C, or better yet Swift) that calls removeDeliveredNotifications.
    • The one piece of information that removeDeliveredNotifications call will need is the identifier of the notification. We can control this by setting apns-collapse-id in the APNs headers when the notification is first created.
    • We can just set these to something like msg-${message_id}. Then at remove time, this code just needs to learn the message ID or message IDs.
  • Most straightforwardly, we'd invoke this from a didReceiveRemoteNotification handler, in response to the server sending a "silent notification" very much like the GCM messages we're sending Android clients.
  • More thought required to design an effective solution.
  • While running up against this rule is annoying, I can't really complain -- it's Apple holding us to a higher standard in respecting the user's battery. Once we work out our solution for iOS, we should apply it back to Android too for the benefit of our Android users.

@borisyankov
Copy link
Contributor

That is an interesting challenge.
We do not need to have 'one silent notification per message read'.
We can batch a lot of message ids into one of these. That will lead to quite a delay but would still be useful.

Dismissing notifications even 10-20 minutes after being delivered is still good, as a phone sitting idly with notifications growing will just pleasantly dismiss most of these before the user has even seen them.

That, in addition to actively dismissing notifications from within a running app sounds like a pretty good solution.

@gnprice
Copy link
Member Author

gnprice commented Nov 7, 2018

Yep, I think that's basically the kind of behavior we need -- batched updates limited to 2-3/hr, and actively updating when the app is in the foreground. It'll just require some more wrinkles to implement.

(For example, say we send a "silent notification" that things have been read, and then a minute later the user gets another notification and promptly reads that message too. We don't want to send another silent notification right then... but then say they get no more notification-causing messages for hours. At some point we want to send another "silent notification" to say that one was read -- which means we're triggering it from some kind of timer, rather than directly from a request.)

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Feb 24, 2020

Does this cover the following issue from #2891? That is, does the title mean to say "...when messages are read on desktop/web/mobile?" I've marked that this is the case, but I may be wrong.

When I get a notification from Zulip of a message and go into the app and read it all the notifications stay on my iphone notifications instead of being cleared when I open the app and read them as they should.

If we have to clear notifications in batches of 2-3 per hour, as Greg mentions, this issue might not be satisfied; the user might like to see them cleared immediately, whereas some delay is probably acceptable if messages are marked as read on desktop/web.

@gnprice
Copy link
Member Author

gnprice commented Feb 26, 2020

Does this cover the following issue from #2891? That is, does the title mean to say "...when messages are read on desktop/web/mobile?"

Hmm, good question!

Definitely agree that we want to clear notifications when messages are read within the app, too. And then yeah, because the mechanism for doing so over APNs may be in infrequent batches, we'll want a different mechanism for the app to do so directly when it's in the foreground.

There was a bit of discussion of that latter in the thread above, as "actively dismissing notifications from within a running app" / "actively updating when the app is in the foreground". But... because it will be a separate mechanism, it may involve a separate line of work to make that happen, so it may not get resolved at the same time. For that reason I think it'd be good to have a separate issue for that -- also P1, and with a link to this one.

@chrisbobbe
Copy link
Contributor

Since this issue has the labels a-IOS and a-notifications, I'm marking it as blocked by #4115, so we only have to implement a solution once. The PR #4163 is currently open for that issue.

@chrisbobbe chrisbobbe added the blocked on other work To come back to after another related PR, or some other task. label Nov 10, 2020
@gnprice gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Jun 18, 2021
@gnprice
Copy link
Member Author

gnprice commented Jun 18, 2021

#4163 is merged!

@gnprice
Copy link
Member Author

gnprice commented Nov 2, 2021

When we implement this, we'll also want to clear old notifications whenever we reach over 400 200 outstanding notifications. Rationale in this chat thread:
https://chat.zulip.org/#narrow/stream/378-api-design/topic/truncating.20.60remove.60.20push.20notifications/near/1230596

(I'm not sure the OS will even let us get to that many in the first place; and if it does, it's not going to be a good user experience anyway, and you're likely to clear the whole stack of them at once without going through them. So this should rarely be visible in practice. But it lets us be sure we don't get into a particular kind of broken-looking state, while reducing large bursts of data we'd otherwise try to send through APNs.)

@gnprice
Copy link
Member Author

gnprice commented Dec 4, 2021

I'm not sure the OS will even let us get to that many in the first place

In fact, it appears the OS only keeps our latest 100 notifications in the first place!

I recently turned on a notifications firehose for myself by enabling notifications on all my (unmuted) messages in chat.zulip.org, partly in order to better exercise our new Android notifications experience that's set for the next release. As a result I have lots of notifications... particularly on iOS devices (where, per this very issue, we don't remove notifications when the message is read). And just now when coming back to an iPhone 7 running iOS 14.8, I found that I had a suspiciously round number of notifications for Zulip: 100 of them.

On an iPad mini 4 running iOS 15.0.2, it doesn't tell me how many notifications I have… but if I tap to expand the list of them and then count them, screenful by screenful, I also find I have 100.

As more notifications arrive, the oldest ones disappear off the end.

And on the other hand if I dismiss a couple of notifications in the middle of the list, the oldest ones don't reappear to bring the total back up to 100 -- so it's not like there are 100 in view plus more in the wings. When those oldest notifications get evicted to keep it to just 100, they really are gone.

So. There's actually nothing for us to do for that point -- we'll have only at most 100 notifications in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Next
  
In Progress
Development

No branches or pull requests

3 participants