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

Total unread message count in push notifications #221

Closed
or-else opened this issue Feb 19, 2019 · 19 comments
Closed

Total unread message count in push notifications #221

or-else opened this issue Feb 19, 2019 · 19 comments

Comments

@or-else
Copy link
Contributor

or-else commented Feb 19, 2019

In order to display icon badges with unread count iOS requires the value to be sent by the server #154 @ozobken:

Yes - IOS push notifications can set the "badge" on the app icon but it has to send an absolute "total" number in each notification - So the code in push_fcm.go needs to be able to compute the total and send it along with the message in the JSON payload as a "badge" value
See:
https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification

@ozobken
Copy link

ozobken commented Feb 21, 2019

Not really worth a PR, but I did a quick check with sending a badge using FCM after line 181 in push_fcm.go:

if d.Platform == "ios" {
                                        msg.Notification = &fcm.Notification {
                                                 Title: "New Message",
                                                 Body:  data["content"],
                                        }
                                        // Testing for https://github.com/tinode/chat/issues/221
                                        // Placeholder for actual value, to be calculated for the user
                                        badgeval := int(1)

                                        msg.APNS = &fcm.APNSConfig{}
                                        msg.APNS.Payload = &fcm.APNSPayload{}
                                        msg.APNS.Payload.Aps = &fcm.Aps{
                                                 Badge: &badgeval,
                                        }
                                }

@or-else
Copy link
Contributor Author

or-else commented Feb 21, 2019

The main problem is in counting the total unread messages for a given user. The server has the value per subscription (user-topic), but not the total. The count is not stored directly either. It's calculated as lastID - readID, i.e. the maximum sequential message ID in the topic minus the ID of the message marked as read by the client.

This is further complicated by the asynchronous nature of the server and the fact that the server may not have the data of a specific user. User's data is stored in the 'me' topic but it's loaded into memory only when the given user is online and subscribed to 'me'. So, even if I have the unread count available in a topic, generally there is no central place where it can be summed up for the given user. This place has to be created. It's not too hard, but has to be done and debugged.

Unfortunately it gets more complicated as some topics are muted, some archived, some banned. In those cases the user does not get notifications of new messages. The unread messages in those topics probably should not be counted towards the total. Otherwise it would be viewed by users as an annoying bug: the badge shows some non-zero count while none of the topics in the UI have unread messages.

It's certainly possible to account for all these nuances but it's a lot of work, it may get computationally heavy and/or may require changes to the storage schema, and it's going to be brittle.

One solution, rather unorthodox, is to ask the client to report the number of unread messages back to the server. Currently the user reports the read ID in {note topic="usr123abc" what="read" seqId=123}. Change it to either (a) or (b):

  • (a) {note topic="usr123abc" what="read" seq=123 unread=2}.
  • (b) {note topic="me" what="unread" seq=2}.

The (a) would still require the "central repository of user data", the (b) would not.

@ozobken
Copy link

ozobken commented Feb 21, 2019

Yep - I know it's a bit fraught - and depending on the schema of synchronisation to the IOS client, there may be sync issues (if you're caching messages, for example) with what has been cached, but not read.
Your options would help this, as the client can report, as you say - but if the client app is not currently running, but we still want to use badging as a means of letting people know they have unread (as they can choose to not be sent pop-ups), then we're stuck with trying to compute on the back end.
It would, of course, be much nicer if Apple allowed you to just increment the badge, but they don't.

@or-else
Copy link
Contributor Author

or-else commented Feb 21, 2019

with what has been cached, but not read.

That's fine. Clients already explicitly and separately report received (recv) and read (read) messages. The server sends the message but makes no assumption if it was delivered.

but if the client app is not currently running, but we still want to use badging as a means of letting people know they have unread

Sure, but that's a simpler problem to solve. The server just increments the counter as new messages are being sent to the client until the client comes online and explicitly resets the unread counter. This way there is no issue with the unread counter on a badge being out of whack with the unread counts shown in the app.

There is a very annoying bug in FB Messenger when the unread count gets stuck at 1 or 2. Mine has been showing "1 unread message" for the past 6 months with no ability to reset it. I would really prefer not to have anything like that in Tinode.

or-else added a commit that referenced this issue Mar 11, 2019
@or-else
Copy link
Contributor Author

or-else commented Mar 13, 2019

@ozobken Hi Ken.

Please take a look at https://github.com/tinode/chat/tree/ios-unread. It has the feature you requested. I've done some basic testing, but I don't have an iOS client. This version does not require any changes to the client. Could you please check if it actually works as expected. Thanks!

@ozobken
Copy link

ozobken commented Mar 14, 2019

Awesome - I'll have a play - hopefully later today, otherwise it'll be early next week

@ozobken
Copy link

ozobken commented Mar 19, 2019

I got the branch installed, and updated my client to more reliably send {note topic="usr123abc" what="read" seqId=n} messages, and now it always sends a badge value of 0.
I did initially see it send a larger value, but after I started sending the {note} messages, it's not reporting the correct number of unread in the new FCM messages

@ozobken
Copy link

ozobken commented Mar 19, 2019

Just had a look at the commit - Does this only get updated if I specifically supply an "unread" {note}?

@or-else
Copy link
Contributor Author

or-else commented Mar 19, 2019

Although I did add the option for the client to report the unread count it's currently not used by the server at all. Everything should work without any charges to the client: the unread message count is incremented with every new {data} message, it's decreased by new_read - old_read with every {note read} sent by the client.

@or-else
Copy link
Contributor Author

or-else commented Mar 19, 2019

Can you show a session log with {data} and {note} messages and what unread values are being reported in pushes after every {data}?
Keep in mind that the unread count is per-user, not per-device.

@ozobken
Copy link

ozobken commented Mar 19, 2019

Here's a server log for a cycle of connect/sub/read

2019/03/19 12:01:07 grpc in: hi:<id:"12345" user_agent:"ObjcTinode/0.0.1 (iOS/12.1.4)" ver:"0.15.10" device_id:"**deleted**" lang:"EN" platform:"ios" >  4D4a2yYeGnc
2019/03/19 12:01:07 grpc in: login:<id:"12346" scheme:"token" secret:"**deleted**" >  4D4a2yYeGnc
2019/03/19 12:01:07 grpc in: sub:<id:"12348" topic:"usrRImzgIpC1GY" get_query:<what:"data" data:<since_id:31 > > >  4D4a2yYeGnc
2019/03/19 12:01:07 grpc in: sub:<id:"12348" topic:"usrKpZ8EnJ9U0s" get_query:<what:"data" data:<since_id:10 > > >  4D4a2yYeGnc
2019/03/19 12:01:10 grpc in: note:<topic:"usrRImzgIpC1GY" seq_id:30 >  4D4a2yYeGnc
2019/03/19 12:01:13 grpc in: note:<topic:"usrKpZ8EnJ9U0s" seq_id:9 >  4D4a2yYeGnc

I do note that the server is not logging the InfoNote type, but I assume that's arriving ok

Here's a subsequent send to one of the subbed channels from another client - I added a debugging log.Println just after the badge := rcpt.To[uid].Unread on line 191 of push_fcm.go:

2019/03/19 12:03:50 grpc in: pub:<id:"14372" topic:"usrcA-tzbwTMgY" no_echo:true content:"TestMsg" >  4d2VeW08W-s
2019/03/19 12:03:50 fcm ios push: badge value 0

@or-else
Copy link
Contributor Author

or-else commented Mar 19, 2019

I'll try to reproduce later today.

@or-else
Copy link
Contributor Author

or-else commented Mar 19, 2019

It's probably fixed: a0b891f

The issue was with the unread message count being reset to 0 on the first message. All subsequent messages generated pushes with incremented badge value.

@ozobken
Copy link

ozobken commented Mar 19, 2019

Thx will test

@ozobken
Copy link

ozobken commented Mar 21, 2019

Looking good so far

2019/03/21 16:00:13 grpc in: pub:<id:"16173" topic:"usrcA-tzbwTMgY" no_echo:true content:"Test1" >  t2eFpcIGL9k
2019/03/21 16:00:13 fcm ios push: badge value 1
2019/03/21 16:00:22 grpc in: pub:<id:"15400" topic:"usrcA-tzbwTMgY" no_echo:true content:"Test2" >  t2eFpcIGL9k
2019/03/21 16:00:23 fcm ios push: badge value 2

@or-else
Copy link
Contributor Author

or-else commented Apr 10, 2019

@ozobken Hi Ken,

Would you mind testing the latest from https://github.com/tinode/chat/tree/ios-unread ?

It has bug fixes from devel as well as measures to prevent unbounded cache growth. Thanks!

@ozobken
Copy link

ozobken commented May 28, 2019

Sorry, @or-else Gene, I missed this - do you still need specific testing on the branch, or is this merged now?

@or-else
Copy link
Contributor Author

or-else commented May 28, 2019

The branch is merged but it would be nice to test the feature some more. My iOS client is not yet handling push notifications.

Thanks!

@or-else
Copy link
Contributor Author

or-else commented Jan 7, 2020

It's been released a while ago and seems to work correctly.

@or-else or-else closed this as completed Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants