Page MenuHomePhabricator

Implement thick thread badge updates and rescinds handling in native notifs code
ClosedPublic

Authored by marcin on Aug 26 2024, 5:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 28, 12:40 PM
Unknown Object (File)
Sat, Dec 28, 5:51 AM
Unknown Object (File)
Sat, Dec 28, 5:51 AM
Unknown Object (File)
Sat, Dec 28, 5:51 AM
Unknown Object (File)
Sat, Dec 28, 5:51 AM
Unknown Object (File)
Fri, Dec 20, 10:21 PM
Unknown Object (File)
Fri, Dec 20, 10:18 PM
Unknown Object (File)
Fri, Dec 20, 10:00 PM
Subscribers

Details

Summary

This differential implements badge count and rescind handling in the NSE/CommNotificationsHandler.

Notes on rescinds:

  1. The difference between thick thread rescind and thin thread rescind is presence of notificationId/rescindID property.
  2. Thin thread rescinds remove single notification while thick thread rescinds remove all notification for particular therad.
  3. In future we want thin thread rescinds work like thick thread rescinds.

Notes on badge count:

  1. Badge count value is calculated based on the sum of unread counts for each keyserver and the size of unread thick threads set.
  2. Visual notifs and badge updates add relevant threadID to the unread thick threads set. Rescinds remove threadID from unread thick threads set.
Test Plan
  1. Apply this patch: https://gist.github.com/marcinwasowicz/3e766542268c0840626ece5af61cc5db
  2. Build native app, log in to two different users on native and web, background native app.
  3. Send text message from web to native.
  4. Ensure the following happens:
    1. Keyserver based notif is delivered, badge count is 1.
    2. Thread creation and text message thick thread notifs are delivered, badge count is 2.
    3. After a little while thick thread notifs are removed, keyserver notif stays and badge count is 1 again.

Backgrounding the app is necessary since the JS code cannot update badge count with thick threads yet. This will be the matter of next diffs.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-8284-new-tmp
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Use getStringSet instead of getStringSetSize

tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
251–255 ↗(On Diff #43726)

Is there a more direct way of distinguishing these? E.g. can we introduce a flag or something?

native/ios/NotificationService/NotificationService.mm
180 ↗(On Diff #43726)

Why do we remove this condition?

211–217 ↗(On Diff #43726)

Can we introduce a const for notificationId?

This revision is now accepted and ready to land.Aug 29 2024, 5:58 AM
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
251–255 ↗(On Diff #43726)

Those conditions look complex but the difference is straightforward. All rescinds have rescind flag and thin thread rescinds additionally have rescindID property since they work with per-notification precision. I would like to keep it that way for two reasons:

  1. This is only temporary. Our goal for future is to make all rescinds work with per-thread precision regardless of thread type. Rescinds with per-notification precision will be used for spacial cases like removing reaction for a message. Therefore all rescind will be notifs with rescind flag and those carrying rescindID will be a special case of more generic notification type.
  2. As a rule of thumb notifications shouldn't carry redundant data since they have limited size.
native/ios/NotificationService/NotificationService.mm
180 ↗(On Diff #43726)

Thick thread notifications do not contain badge property but they can still update badge count since they add new entry to the list of unread threads.

Extract notificationId string into a constant